Request to enhance some tests performance to avoid random getDeclaredFields() results

[FarmersWrap]

Gson version

2.10.1

Java / Android version

openjdk version "11.0.20.1"

Used tools

Apache Maven 3.6.3
Ubuntu 20.04.6 LTS
Linux version: 5.4.0-163-generic

Description

There are 37 tests within gson has been reported as flaky when ran with the NonDextool. The test failed because of the randomness of the function Class.getDeclaredFields(). As mentioned by eamonnmcmanus in issue:2309, the tests are fragile due to the function. I wish to improve the stability of these tests by adding more options.

Expected behavior

The test should not only have one expected json answer according to the current implementation.
The behavior of gson tests should not rely on the getDeclaredFields() function because according to Class.getDeclaredFields() "The elements in the returned array are not sorted and are not in any particular order.". The test answers can differ.

Actual behavior

Tests pass when getDeclaredFields() returns the same order of Fields as they were declared.
But if the order of getDeclaredFields() results is not the same as it was declared. Tests fail.

Reproduction steps

1. Download NonDex
2. Execute following commands

cd gson
mvn install -pl gson -am -DskipTests
mvn -pl gson test -Dtest=com.google.gson.functional.StreamingTypeAdaptersTest#testNullSafe
mvn -pl gson edu.illinois:nondex-maven-plugin:2.1.1:nondex -DnondexRuns=10 -Dtest=com.google.gson.functional.StreamingTypeAdaptersTest#testNullSafe

Full description of Nondex please refer to its doc on github

I printed out the getDeclaredFields() results. The order of the Fields was randomized by Nondex on purpose to detect implementation-dependent flakiness. You can observe the order of the Fields is not always the same.

[�[1;34mINFO�[m] Using auto detected provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m]  T E S T S
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m] Running com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
java.lang.String com.google.gson.functional.StreamingTypeAdaptersTest$Person.name
int com.google.gson.functional.StreamingTypeAdaptersTest$Person.age
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
[�[1;34mINFO�[m] �[1;32mTests run: �[0;1;32m1�[m, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.148 s -- in com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] Results:
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] �[1;32mTests run: 1, Failures: 0, Errors: 0, Skipped: 0�[m
[�[1;34mINFO�[m] 
INFO: Adding excluded groups to newly created one
INFO: Adding NonDex argLine to existing argLine specified by the project
CONFIG: nondexFilter=.*
nondexMode=FULL
nondexSeed=1181842
nondexStart=0
nondexEnd=9223372036854775807
nondexPrintstack=false
nondexDir=/home/yu129/gson/gson/.nondex
nondexJarDir=/home/yu129/gson/gson/.nondex
nondexExecid=7bDkRyFRDJglqUt8BYXyKlsd6HNdgRY9duqp6ZWyD8=
nondexLogging=CONFIG
test=
[�[1;34mINFO�[m] Using auto detected provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m]  T E S T S
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m] Running com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
java.lang.String com.google.gson.functional.StreamingTypeAdaptersTest$Person.name
int com.google.gson.functional.StreamingTypeAdaptersTest$Person.age
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
java.util.List com.google.gson.functional.StreamingTypeAdaptersTest$Truck.passengers
double com.google.gson.functional.StreamingTypeAdaptersTest$Truck.horsePower
[�[1;34mINFO�[m] �[1;32mTests run: �[0;1;32m1�[m, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.139 s -- in com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] Results:
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] �[1;32mTests run: 1, Failures: 0, Errors: 0, Skipped: 0�[m
[�[1;34mINFO�[m] 
INFO: Adding excluded groups to newly created one
INFO: Adding NonDex argLine to existing argLine specified by the project
CONFIG: nondexFilter=.*
nondexMode=FULL
nondexSeed=1223286
nondexStart=0
nondexEnd=9223372036854775807
nondexPrintstack=false
nondexDir=/home/yu129/gson/gson/.nondex
nondexJarDir=/home/yu129/gson/gson/.nondex
nondexExecid=eoE0yaR5WJj8fVYEdEVYfmhwnpWM0Gh2QuyNp9vuw=
nondexLogging=CONFIG

Failed test(There are 36, this is an example).

INFO: Adding excluded groups to newly created one
INFO: Adding NonDex argLine to existing argLine specified by the project
CONFIG: nondexFilter=.*
nondexMode=FULL
nondexSeed=1181842
nondexStart=0
nondexEnd=9223372036854775807
nondexPrintstack=false
nondexDir=/home/yu129/gson/gson/.nondex
nondexJarDir=/home/yu129/gson/gson/.nondex
nondexExecid=32QrqKafssXCuolEzFuT5jQnxBJ7+XGF3cCy2AZ8yvE=
nondexLogging=CONFIG
test=
[�[1;34mINFO�[m] Using auto detected provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[�[1;34mINFO�[m] 
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m]  T E S T S
[�[1;34mINFO�[m] -------------------------------------------------------
[�[1;34mINFO�[m] Running com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
[�[1;31mERROR�[m] �[1;31mTests �[0;1mrun: �[0;1m1�[m, �[1;31mFailures: �[0;1;31m1�[m, Errors: 0, Skipped: 0, Time elapsed: 0.162 s�[1;31m <<< FAILURE!�[m -- in com.google.gson.functional.�[1mStreamingTypeAdaptersTest�[m
[�[1;31mERROR�[m] com.google.gson.functional.StreamingTypeAdaptersTest.testNullSafe -- Time elapsed: 0.156 s <<< FAILURE!
expected: {"horsePower":1.0,"passengers":[null,"jesse,30"]}
but was : {"passengers":[null,"jesse,30"],"horsePower":1.0}
	at com.google.gson.functional.StreamingTypeAdaptersTest.testNullSafe(StreamingTypeAdaptersTest.java:204)

Flaky tests found

Flaky tests detected(idoft format):

https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.StreamingTypeAdaptersTest.testNullSafe,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.StreamingTypeAdaptersTest.testSerializeWithCustomTypeAdapter,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.StreamingTypeAdaptersTest.testSerialize,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.StreamingTypeAdaptersTest.testSerializeNullField,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ReflectionAccessFilterTest.testBlockInaccessibleJava,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ParameterizedTypesTest.testVariableTypeFieldsAndGenericArraysSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ParameterizedTypesTest.testTypesWithMultipleParametersSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ParameterizedTypesTest.testVariableTypeFieldsAndGenericArraysDeserialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.JsonAdapterSerializerDeserializerTest.testJsonAdapterNullSafe,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.CollectionTest.testWildcardCollectionField,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.CollectionTest.testObjectCollectionSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.CollectionTest.testPriorityQueue,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.CollectionTest.testCollectionOfBagOfPrimitivesSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.NamingPolicyTest.testGsonDuplicateNameUsingSerializedNameFieldNamingPolicySerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.InheritanceTest.testSubInterfacesOfCollectionSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.InheritanceTest.testSubClassSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.TypeVariableTest.testTypeVariablesViaTypeParameter,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ExposeFieldsTest.testExposeAnnotationSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ExposeFieldsTest.testArrayWithOneNullExposeFieldObjectSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testSingletonLists,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testArrayOfArraysSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testArrayOfObjectsSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testNestedSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testNullFieldsSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testBagOfPrimitiveWrappersSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.ObjectTest.testBagOfPrimitivesSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testUpperCamelCase,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testLowerCaseWithDashes,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testLowerCaseWithUnderscores,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testUpperCamelCaseWithSpaces,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testIdentity,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.FieldNamingTest.testUpperCaseWithUnderscores,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.CustomTypeAdaptersTest.testCustomNestedSerializers,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.VersioningTest.testVersionedGsonWithUnversionedClassesSerialization,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.MapTest.testInterfaceTypeMapWithSerializer,ID,,,
https://github.com/google/gson,e685705b2bf3ae174958612a185bd231c0e0c5d9,gson,com.google.gson.functional.JsonAdapterAnnotationOnFieldsTest.testExcludeDeserializePrecedence,ID,,,

Here are some possible fixes:FarmersWrap_PR.
Simran_PR.
Thanks

[eamonnmcmanus]

Thanks, this is very interesting!

There are a couple of paths forward. One possibility would be to update all the tests so they accept any field ordering. Perhaps with a helper method that takes a Map<String, String> and returns a List<String> that is all the object permutations of the given keys and values. So if given Map.of("a", "1", "b", "2.0") it would return List.of("{\"a\":1,\"b\":2.0}", "{\"b\":2.0,\"a\":1}"). For a map of size $5$ it would return a list of size $5! = 120$. That would simplify the approach you have started on in your PR.

Another possibility would be to change the logic so it is deterministic, for example by sorting the field names. We touched on some of the implications in #2309. I would be reluctant to make that change unconditionally because I'm fairly sure there are lots of tests out there that are implicitly depending on the current order, as unspecified and fragile as that is. But perhaps it could be controlled by a system property. Then we could ensure that the system property is set for the tests that depend on a specific order, and update those tests as necessary so that they expected sorted field names.

My inclination would be for the first possibility, updating the tests, but I would be interested in others' opinions.

[Marcono1234]

Personally I think another option would be to simply do nothing. This is not really a 'solution' to this problem, but on the other hand it looks like all recent JDK versions and possibly even different JDK distributions all behave the same way, and make the tests pass.

Making the tests more complex and difficult to maintain to account for different field orders is in my opinion not worth it if the tests don't actually fail¹. As soon as the tests fail for a specific JDK though this should probably be addressed. However, I assume the OpenJDK maintainers will be careful to not change the order of the fields (despite being unspecified) due to how many projects explicitly or implicitly rely on it.

It is nonetheless great that you highlighted this issue @FarmersWrap, and the NonDex tool sounds really interesting!

Otherwise, if we do want to adjust the tests, then the idea to use a system property to guarantee the order sounds good to me. But if the system property is intended to be internal, then we should clearly indicate that in the property name to avoid users relying on it.
Or if there is another way for the code to determine if it is run as part of a unit test (with Maven or the IDE), then that might be better than having to set the system property in dozens of test classes.

Footnotes

1. And also if you cannot trigger the failure locally easily or include the check in CI, it is likely that we might overlook cases where it is checking for the implicit field order, without allowing different orders. ↩

[FarmersWrap]

Hi eamonnmcmanus and Marcono1234,

I tried to fix the tests using the maps. But as Marcono1234 said, the test will become very complicated. I had a demo here
I used a recursion to do the permutation(backtracking). Put the possible Strings into a List.
But the compiler will generate a warning when I use local variables and do not return it. I had to switch the "FailOnWarning" flag off.

And, I also find when the classes are nested or there are so many fields(variables) or formats, for example, naming policies in FieldNamingTest.java. The original code was hard coded. But if we want to find all possible jsons. We need to hardcode every variable as strings in each naming policy. That is ugly. I tried to use a switch and a backtracking function to get all String.

I searched online, I think the behavior of getDeclaredFields() is nondeterministic. Since I don't have the access to the JVM implementation. I just cite someone on StackOverflow with "And it might be affected by unexpected things ... like class unload / reload, or JIT recompilation.”

I understand that you would not like to sort the fields because it is not necessary at all.(Here is 219sansim's fix by sorting)
If you want to fix these tests using maps and permutations. I can do it for you for free.
Otherwise, I think it is not a bad idea to ignore the tests when they occasionally fail.

Do you have any gradle/maven projects that you would recommend me to run nondex and debug flaky tests? I want to test them and fix them as well. Thank you very much!