Fix flaky test in DescriptionGenericBuilderTest by sorting for deterministic iterations

[SaaiVenkat]

This PR fixes 1 flaky test failure in module byte-buddy-dep

1. Test that could fail

• Module: byte-buddy-dep
• Class Name: net.bytebuddy.description.type.TypeDescriptionGenericBuilderTest
• Test Name: testTypeAnnotationExceptionType

2. Why the test might fail?

• The mentioned test tests the exception types of TypeAnnotationSamples.foo:35, which are of type Generic and RuntimeException. But, it assumes the exception types to be in the declared order, ie.Generic followed by RuntimeException.
  

  byte-buddy/byte-buddy-dep/src/test/java-8/net/bytebuddy/test/precompiled/v8/TypeAnnotationSamples.java

  Lines 35 to 36 in 880027c

  	abstract <@TypeAnnotation(26) T extends @TypeAnnotation(27) Exception> @TypeAnnotation(28) int foo(@TypeAnnotation(29) T @TypeAnnotation(30) [] @TypeAnnotation(31) [] v)
  	throws @TypeAnnotation(32) T, @TypeAnnotation(33) RuntimeException;

• The exception types are returned by Method.getGenericExceptionTypes at TypeDescriptionGenericBuilderTest.java:43, but there is no official documentation that mentions this order will be same as the declared order.
  

  byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/description/type/TypeDescriptionGenericBuilderTest.java

  Line 43 in 880027c

  return describe(method.getGenericExceptionTypes()[index], new TypeDescription.Generic.AnnotationReader.Delegator.ForLoadedExecutableExceptionType(method, index))

• Since there is no guarantee in order of exception types returned, the order might change, resulting in the test failure.

3. How to reproduce?

• I used an open-source tool called NonDex for detecting the assumption by shuffling the order of returned exception types.
• Running the following commands will reproduce the flakiness

# Clone the Repo
https://github.com/raphw/byte-buddy.git

# Compile the module byte-buddy-dep
mvn clean install -pl byte-buddy-dep -am -DskipTests

# (Optional) Run the unit test
mvn -pl byte-buddy-dep test -Dtest=net.bytebuddy.description.type.TypeDescriptionGenericBuilderTest#testTypeAnnotationExceptionType

# Run the unit test using NonDex
mvn -pl byte-buddy-dep edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=net.bytebuddy.description.type.TypeDescriptionGenericBuilderTest#testTypeAnnotationExceptionType

Expected Result

• The test should have passed successfully without any errors with NonDex.

Actual Result

• Running with NonDex produced the following result

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   TypeDescriptionGenericBuilderTest>AbstractTypeDescriptionGenericTest.testTypeAnnotationExceptionType:1448
Expected: is <NON_GENERIC>
     but: was <VARIABLE>
[INFO]
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

4. How I fixed?

• This PR ensures the order of the exception types returned by Method.getGenericExceptionTypes remain consistent across different runs by sorting the array based on the type name.
• I ran all the tests for the module using the command mvn test -pl byte-buddy-dep, and all the tests passed.

[raphw]

I am curious: is this the only test that suffers from this assumption? Or is it just a random pick?

I would consider that there are many places this is assumed, as Byte Buddy often parses members and returns them in the declaration order, too, just as the reflection API.

[SaaiVenkat]

Since I picked this test from Flaky Test Dataset, I am unsure whether other tests suffer from the assumption. I have started testing the other tests too. Please give me a few days to update about them.

[raphw]

I see. Could you somehow investigate if there sre more such ordering issues? If so, I'd rsther fix them in one go or do not fix them at all.

[SaaiVenkat]

@raphw , I detected a few more tests that could fail because of the ordering issue.

1. ClassFileVersionOtherTest#testLatestVersion

• Test name: net.bytebuddy.ClassFileVersionOtherTest#testLatestVersion

• byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/ClassFileVersionOtherTest.java

  Lines 75 to 87 in 880027c

  	public void testLatestVersion() throws Exception {
  	double version = 0d;
  	int value = 0;
  	Pattern pattern = Pattern.compile("V[0-9]+(_[0-9]+)?");
  	for (Field field : Opcodes.class.getFields()) {
  	if (pattern.matcher(field.getName()).matches()) {
  	if (version < Double.parseDouble(field.getName().substring(1).replace('_', '.'))) {
  	value = field.getInt(null);
  	}
  	}
  	}
  	assertThat(ClassFileVersion.latest().getMajorVersion(), is((short) value));
  	}

• In the above test, the fields returned by Opcodes.class.getFields() are assumed to be in the order of their declaration.
• However, the javadoc specifies that the order will not be maintained.
• Running with NonDex produced the following result

 [INFO] Results:
 [INFO] 
 [ERROR] Failures: 
 [ERROR]   ClassFileVersionOtherTest.testLatestVersion:86 
 Expected: is <58s>
      but: was <66s>
 [INFO] 
 [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

• To fix this, I tracked the latestVersion based on the field value and compared it with ClassFileVersion.latest().getMajorVersion()

2. MethodCallProxyTest#testNonGenericParameter

• Test name: net.bytebuddy.implementation.auxiliary.MethodCallProxyTest#testNonGenericParameter

• byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/implementation/auxiliary/MethodCallProxyTest.java

  Lines 80 to 86 in 880027c

  	public void testNonGenericParameter() throws Exception {
  	Class<?> auxiliaryType = proxyOnlyDeclaredMethodOf(GenericType.class);
  	assertThat(auxiliaryType.getTypeParameters().length, is(0));
  	assertThat(auxiliaryType.getDeclaredMethod("call").getGenericReturnType(), is((Type) Object.class));
  	assertThat(auxiliaryType.getDeclaredFields()[1].getGenericType(), is((Type) Object.class));
  	assertThat(auxiliaryType.getDeclaredFields()[2].getGenericType(), is((Type) Number.class));
  	}

• Here, the fields returned by auxiliaryType.getDeclaredFields() are assumed to be in a specific order, i.e Object.class then Number.class.
• But the javadoc mentions that the order will not be maintained.
• I fixed this by sorting the fields returned by auxiliaryType.getDeclaredFields() based on the field name. This ensures the same field will be access for a given index at any time.

3. TypeDescriptionArrayProjectionTest#testTypeAnnotationExceptionType

• Test name: net.bytebuddy.description.type.TypeDescriptionArrayProjectionTest#testTypeAnnotationExceptionType
• Similar to the original test against which the PR was created, this test fails for the same reason.

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   TypeDescriptionArrayProjectionTest>AbstractTypeDescriptionGenericTest.testTypeAnnotationExceptionType:1443 
Expected: is <VARIABLE>
     but: was <NON_GENERIC>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

• I fixed this issue by sorting the values returned by Method.getGenericEexceptionTypes(). This ensures that the same element will be accessed at all times.

[SaaiVenkat]

The following tests also fail because of flakiness, but I would like to have some more discussion on them.

Issue Details

• byte-buddy/byte-buddy-dep/src/main/java/net/bytebuddy/description/type/TypeList.java

  Line 942 in 880027c

  class OfMethodExceptionTypes extends AbstractBase {

• OfMethodExceptionTypes holds the information about a method's exception types. However, the exception type information is fetched every time for size, get, and asErasures.

  • byte-buddy/byte-buddy-dep/src/main/java/net/bytebuddy/description/type/TypeList.java

    Lines 961 to 963 in 880027c

    	public TypeDescription.Generic get(int index) {
    	return new OfMethodExceptionTypes.TypeProjection(method, index, method.getExceptionTypes());
    	}

  • byte-buddy/byte-buddy-dep/src/main/java/net/bytebuddy/description/type/TypeList.java

    Lines 968 to 970 in 880027c

    	public int size() {
    	return method.getExceptionTypes().length;
    	}

  • byte-buddy/byte-buddy-dep/src/main/java/net/bytebuddy/description/type/TypeList.java

    Lines 975 to 977 in 880027c

    	public TypeList asErasures() {
    	return new TypeList.ForLoadedTypes(method.getExceptionTypes());
    	}

• Since the size is not going to change even if the order changes, OfMethodExceptionTypes.size():969 won't cause any issue.
• But the callers of the methods asErasures() and get() might face issues because the order of method.getExceptionTypes() is not guaranteed to be maintained.

Test failures

The following tests fail because of the above mentioned issue:

1. testToString

• net.bytebuddy.description.method.MethodDescriptionLatentTest#testToString
• net.bytebuddy.description.method.MethodDescriptionForLoadedTest#testToString

• byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/description/method/AbstractMethodDescriptionTest.java

  Lines 278 to 284 in 880027c

  	public void testToString() throws Exception {
  	assertThat(describe(firstMethod).toString(), is(firstMethod.toString()));
  	assertThat(describe(secondMethod).toString(), is(secondMethod.toString()));
  	assertThat(describe(thirdMethod).toString(), is(thirdMethod.toString()));
  	assertThat(describe(firstConstructor).toString(), is(firstConstructor.toString()));
  	assertThat(describe(secondConstructor).toString(), is(secondConstructor.toString()));
  	}

• secondMethod throws two exceptions - RuntimeExceptions and IOExceptions. This order is affected because of the above issue.

2. testGenericTypeInconsistency

• net.bytebuddy.description.type.TypeDescriptionGenericBuilderTest#testGenericTypeInconsistency
• net.bytebuddy.description.type.TypeDescriptionForLoadedTypeTest#testGenericTypeInconsistency
• net.bytebuddy.description.type.TypeDescriptionArrayProjectionTest#testGenericTypeInconsistency

• byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/description/type/AbstractTypeDescriptionGenericTest.java

  Lines 1268 to 1276 in 880027c

  	assertThat(typeDescription.getDeclaredMethods().filter(named(FOO)).getOnly().getExceptionTypes().size(), is(2));
  	assertThat(typeDescription.getDeclaredMethods().filter(named(FOO)).getOnly().getExceptionTypes().get(0).getSort(),
  	is(TypeDescription.Generic.Sort.NON_GENERIC));
  	assertThat(typeDescription.getDeclaredMethods().filter(named(FOO)).getOnly().getExceptionTypes().get(0).asErasure().represents(Exception.class),
  	is(true));
  	assertThat(typeDescription.getDeclaredMethods().filter(named(FOO)).getOnly().getExceptionTypes().get(1).getSort(),
  	is(TypeDescription.Generic.Sort.NON_GENERIC));
  	assertThat(typeDescription.getDeclaredMethods().filter(named(FOO)).getOnly().getExceptionTypes().get(1).represents(RuntimeException.class),
  	is(true));

• byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/description/type/AbstractTypeDescriptionGenericTest.java

  Lines 1286 to 1294 in 880027c

  	assertThat(typeDescription.getDeclaredMethods().filter(isConstructor()).getOnly().getExceptionTypes().size(), is(2));
  	assertThat(typeDescription.getDeclaredMethods().filter(isConstructor()).getOnly().getExceptionTypes().get(0).getSort(),
  	is(TypeDescription.Generic.Sort.NON_GENERIC));
  	assertThat(typeDescription.getDeclaredMethods().filter(isConstructor()).getOnly().getExceptionTypes().get(0).asErasure().represents(Exception.class),
  	is(true));
  	assertThat(typeDescription.getDeclaredMethods().filter(isConstructor()).getOnly().getExceptionTypes().get(1).getSort(),
  	is(TypeDescription.Generic.Sort.NON_GENERIC));
  	assertThat(typeDescription.getDeclaredMethods().filter(isConstructor()).getOnly().getExceptionTypes().get(1).represents(RuntimeException.class),
  	is(true));

• All the above tests, assumes the first exception to be of Exception, and the second exception to be of RuntimeException. However, this order is also affected.

3. testExceptions

• net.bytebuddy.description.method.MethodDescriptionLatentTest#testExceptions
• net.bytebuddy.description.method.MethodDescriptionForLoadedTest#testExceptions
• net.bytebuddy.pool.TypePoolDefaultMethodDescriptionTest#testExceptions

  • byte-buddy/byte-buddy-dep/src/test/java/net/bytebuddy/description/method/AbstractMethodDescriptionTest.java

    Lines 607 to 611 in 880027c

    	public void testExceptions() throws Exception {
    	assertThat(describe(firstMethod).getExceptionTypes(),
    	is((TypeList.Generic) new TypeList.Generic.ForLoadedTypes(firstMethod.getExceptionTypes())));
    	assertThat(describe(secondMethod).getExceptionTypes(),
    	is((TypeList.Generic) new TypeList.Generic.ForLoadedTypes(secondMethod.getExceptionTypes())));

• secondMethod throws two exceptions - RuntimeExceptions and IOExceptions. Since the order of exception types might be changed, the above tests might fail.

Possible Fix

• Since multiple access of Method.getExceptionTypes() causes the issue, we could store to a field variable during instantiation, and reuse the same variable. Storing in a variable would not guarantee the order to be maintained. But this would ensure the same order to be maintained at every access. The above variable along with modifying the tests to check without order will solve the ordering issue.
• We could also sort the values returned by Method.getExceptionTypes(). However, these solutions do not completely solve the flakiness.

Issue Details Continuation

• While creating the Token of a method, the substitution of the exception types results in resolving them at TypeList.TypeProject:1014.

  • byte-buddy/byte-buddy-dep/src/main/java/net/bytebuddy/description/type/TypeList.java

    Lines 1014 to 1019 in 880027c

    	protected TypeDescription.Generic resolve() {
    	java.lang.reflect.Type[] type = method.getGenericExceptionTypes();
    	return erasure.length == type.length
    	? Sort.describe(type[index], getAnnotationReader())
    	: asRawType();
    	}

• Since this is called for each exception type thrown by the method, and method.getGenericExceptionTypes() is called for each resolution, there is a possibility for order change or even the same exception being resolved twice.
• For example, let's assume the order returned by method.getExceptionTypes() to be Exception.class, RuntimeException.class. When resolving for Exception.class, the order returned by method.getGenericExceptionTypes() at could be RuntimeException.class, Exception.class. This would result in incorrectly resolving the Type of Exception.class to RuntimeException.class. Assuming RuntimeException.class resolved to Exception.class, we would have Exception.class, RuntimeException.class -> RuntimeException.class, Exception.class . The above example shows how ordering issue would still be caused even if we apply the above possible fix.
• Let's assume the order returned by method.getExceptionTypes to be Exception.class, RuntimeException.class, and resolving of Exception.class worked correctly. Now, while resolving RuntimeException.class, method.getGenericExceptionTypes() could be Exception.class, RuntimeException.class. This would not result in ordering issue, but returning the same exception twice. Exception.class, RuntimeException.class -> Exception.class, Exception.class.
• Because of this, all the above mentioned tests might fail.

Possible Fix

• Since the variations in the ordering of TypeProjections.erasures:1009 and the ordering of TypeProjections.type:1015, the resolution might be incorrectly mapped.
• Ordering the erasures as well as the types ensures the correct resolution.
• I have created a PR against my forked repository with this proposed fix for your review - Fix flaky resolution of exception types SaaiVenkat/byte-buddy-flaky#3
• However, I am concerned about the performance issues because of the sorting.
• One way to reduce the number of sorting is to store the sorted Method.getGenericExceptionTypes() in a field variable during TypeProjection instantiation. We could reuse the same variable to resolve the Type. This would require sorting both the erasures and type only once.

Questions

• What are your thoughts on these findings? How can I further improve the proposed solution?
• I have error logs for the above mentioned cases when run through NonDex, but I haven't mentioned them to keep this comment short. Do you want me to attach those logs too?

[raphw]

Looking through this, I am not sure if I will follow this up. At some places, Byte Buddy's parser is supposed to work as the JVM parser, so it's not quite random that the order is like that. I would like to keep them aligned and in this light, for now, I can rely on the member order, also if this is property is not guaranteed.

[SaaiVenkat]

Looking through this, I am not sure if I will follow this up. At some places, Byte Buddy's parser is supposed to work as the JVM parser, so it's not quite random that the order is like that. I would like to keep them aligned and in this light, for now, I can rely on the member order, also if this is property is not guaranteed.

Before closing this PR, I would like to confirm if you had visited my above comment, explaining the issues with tests

• net.bytebuddy.ClassFileVersionOtherTest#testLatestVersion
• net.bytebuddy.implementation.auxiliary.MethodCallProxyTest#testNonGenericParameter

Even the Javadoc of Class.getFields() and Class.getDeclaredFields() confirms that the order WILL not be maintained. If needed, please let me know so that I could update the PR to fix only those tests. The issue and fix are similar to #1516

[raphw]

Thanks, reconsidering, I can merge these changes. The "latest version" test was however broken as the version field was not updated. I fixed this myself now. Can you change the PR to exclude that change?

[SaaiVenkat]

Thanks, reconsidering, I can merge these changes. The "latest version" test was however broken as the version field was not updated. I fixed this myself now. Can you change the PR to exclude that change?

@raphw, Thanks for the reconsideration.
Sure, I'll update the PR to remove the fix for net.bytebuddy.ClassFileVersionOtherTest#testLatestVersion.

Do you want me to remove the fixes for net.bytebuddy.description.type.TypeDescriptionArrayProjectionTest#testTypeAnnotationExceptionType and net.bytebuddy.description.type.TypeDescriptionGenericBuilderTest#testTypeAnnotationExceptionType as well?