-
-
Notifications
You must be signed in to change notification settings - Fork 841
Description
Description
Some tests below were found to be potentially flaky with research tool NonDex, which explores non-determinism in tests. These tests can fail due to different iteration orders under different JVMs, hash seeds, etc. I am posting this issue before I submit PR's to make sure you would like these kind of fixes, given that they don't usually fail if tests are run under fixed JVM version, etc. Therefore the fix is more like "precaution" if implementation for these kinds of non-deterministic methods ever changes in the future.
- net.bytebuddy.ClassFileVersionOtherTest#testLatestVersion
- net.bytebuddy.description.annotation.AnnotationDescriptionForLoadedAnnotationTest#testEquals
- net.bytebuddy.description.method.MethodDescriptionForLoadedTest#testExceptions
- net.bytebuddy.description.method.MethodDescriptionForLoadedTest#testToString
- ... and more
I have identified root cause for the test net.bytebuddy.description.annotation.AnnotationDescriptionForLoadedAnnotationTest#testEquals and propose my fix as below so you can have an idea of what my PRs would be. I plan to resolve all such flaky tests if you would like it. They shouldn't affect any production logic, and can make the tests more robust.
Example
In /src/test/java/net/bytebuddy/description/annotation/AbstractAnnotationDescriptionTest.java, the line:
assertThat(describe(first), not(equalFirstNameOnly));in test testEquals() implicitly assumes that the first annotation properties being compared must be primitive or primitive-array annotation but not object-array properties. In the case where object-array properties like Annotation[] or Class<?>[] is compared first, the comparison is delegated to equals() in ForDescriptionArray class and the line:
if (!value.getClass().isArray())will crash with NPE because our mocked annotatedValue always returns null with the line:
when(annotationValue.resolve()).thenReturn(null);This test is fine if we assume that the getDeclaredMethods() in equals() for AnnotationDescription returns fields in some fixed order, but we should not rely on these non-deterministic methods for best practices. My proposed fix is quite simple as below.
Example fix
In /src/test/java/net/bytebuddy/description/annotation/AbstractAnnotationDescriptionTest.java, we can make the mocked annotatedValue always return an empty array instead of null, by replacing the line:
when(annotationValue.resolve()).thenReturn(null);with
when(annotationValue.resolve()).thenReturn(new Object[0]);, now for both primitive and object-array properties the comparison is legal.
Failure Reproduction
Run tests with research tool NonDex under Java17:
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -pl byte-buddy-dep \
-Djacoco.skip -Drat.skip -Dpmd.skip -Denforcer.skip \
-Dtest=AnnotationDescriptionLatentTest#testEquals -DnondexRuns=20
Please let me know if you would accept these kinds of fix for flaky tests, and I am ready to submit the PR at least for this test above.