Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Generator.java to fix the flaky errors #727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

njain2208
Copy link

PR Overview:


This PR fixes the flaky/non-deterministic behavior of the following test because it assumes the ordering of methods returned by getDeclaredMethods()

org.bytedeco.javacpp.BufferTest

org.bytedeco.javacpp.EnumTest

org.bytedeco.javacpp.PointerTest

Test Overview:


In all of the above tests, the tests call the setUpClass() method where the problem arises. The setUpClass() makes an internal to the Generator class’s object where the problem resides.

You can reproduce the issue by running the following commands (the below command is for BufferTest, please change class name for targeting other tests) -

mvn install -pl . -am -DskipTests
mvn test  -Dtest=org.bytedeco.javacpp.BufferTest
mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.bytedeco.javacpp.BufferTest

Root Cause


Method[] methods = cls.getDeclaredMethods();

In the Generator class, the cls.getDeclaredMethods() method returns an array with values in a non-deterministic order, leading to the core of the issue. Within the functionMethods() method of the Generator class, there's a call to cls.getDeclaredMethods() used to populate the callbackAllocators[] array. Additionally, the same cls.getDeclaredMethods() method is invoked in the methods() method of the Generator class, which utilizes the callbackAllocators array. However, due to the differing order of results produced by cls.getDeclaredMethods(), it results in a test failure.
This flakiness was identified by the nondex tool created by the researchers of UIUC

Fix:


To fix the issue I decided to make the output of callbackAllocators array deterministic by ordering the cls.getDeclaredMethods() result both in methods() method and functionMethods() method.

https://github.com/njain2208/javacpp/blob/4c62303548be1b9edc3e22d23d3674c397ff9829/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L2085-L2091

https://github.com/njain2208/javacpp/blob/4c62303548be1b9edc3e22d23d3674c397ff9829/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L3643-L3649

@saudet
Copy link
Member

saudet commented Nov 26, 2023

I've never encountered any problems with that, and I will not merge something like this that needs sorting, but I would be inclined to merge a solution that calls getDeclaredMethods() only once, if you can provide it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants