review: test: Fixed 4 non-deterministic flaky tests

[nikunjagarwal321]

What changes were proposed in this pull request?

Sorting the order of imports list variable and enums list variable in 4 flaky tests in order to make them deterministic. The assertions are written with an assumption that the list will always be in a particular predefined order, but that may not be the case. The PR contains fix for the below test cases to make them deterministic:

• spoon.test.imports.ImportTest#staticImports_ofNestedTypes_shouldBeRecorded
• spoon.test.imports.ImportTest#staticTypeAndMethodImport_importShouldAppearOnlyOnceIfTheirSimpleNamesAreEqual
• spoon.test.position.PositionTest#testPositionOfCtImport
• spoon.test.template.SubstitutionTest#testCreateTypeFromTemplate

Why are the changes needed?

Multiple flaky tests were detected while trying to run the tests using the nondex tool. NonDex is a tool for detecting and debugging wrong assumptions on under-determined Java APIs.

Sample ERROR logs:

Reason for Failure

As mentioned above, the assertions assume that the list will always be in a particular predefined order, but that may not be the case. For eg, in test case : staticImports_ofNestedTypes_shouldBeRecorded, we have the following test case

List<CtImport> imports = mainType.getPosition().getCompilationUnit().getImports();
assertThat(imports, hasSize(2));

CtImport import0 = imports.get(0);
assertThat(import0.getReference().getSimpleName(), is("InnerClass"));

The above code assumes that the first element of the list will be InnerClass which may not be true. This is because the imports is being set by JDTImportBuilder.java class where the imports variable is implemented as HashSet. As per the javadocs, the order of elements is not maintained in HashSet.

Hence, we sort the fields before asserting based on certain fields to make the order deterministic.

Steps to reproduce flakiness using nondex -

mvn install -DskipTests

mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=spoon.test.position.PositionTest#testPositionOfCtImport
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=spoon.test.template.SubstitutionTest#testCreateTypeFromTemplate
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=spoon.test.imports.ImportTest#staticTypeAndMethodImport_importShouldAppearOnlyOnceIfTheirSimpleNamesAreEqual
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=spoon.test.imports.ImportTest#staticImports_ofNestedTypes_shouldBeRecorded

Please let me know if you have any questions or would like to discuss this further for any clarificaitions.

[nikunjagarwal321]

Added fix for another flaky test :

• spoon.test.processing.ProcessingTest#testInitPropertiesWithWrongType

The change is to sort the results of getDeclaredFields() in RTHelper.java to make it deterministic. As per the java docs, the given method does not return the results in a deterministic order, which leads to the nondeterministic behaviour of the above test case.

Hence, we are sorting the fields in the function addAllFields() so as to make it deterministic.

[SirYwell]

Hey, thank you for your PR.

I'm not sure if the tests are at fault or if the implementations are broken.

For example the enum constants situation: the test asserts that substituting case1 with GOOD and case2 with BETTER in the enum

public enum AnEnumModel {
	case1, case2;
}

Now, the resulting enum must have GOOD, BETTER in that exact order, otherwise the substitution misbehaved (and the test should fail).

Can you point out where exactly the order is lost in the underlying code?

Similarly for the imports. There, the order isn't as important, but I think it is reasonable to expect (and we could also specify that in the Javadocs) that the order will match the order in the source code.

In that case, I'd prefer having one separate Pull Request per problem.

Regarding the getDeclaredFields() method, while it doesn't guarantee any order, I think we should not sort it in the model but be more flexible in the tests. But I'd like to know where exactly the problem occurs to understand it better.

[nikunjagarwal321]

Hi @SirYwell
Thanks for your comment and I totally agree with it.

1. Imports Ordering Issue:

Can you point out where exactly the order is lost in the underlying code?

Similarly for the imports. There, the order isn't as important, but I think it is reasonable to expect (and we could also specify that in the Javadocs) that the order will match the order in the source code.

I debugged further and found that for imports, the order is changing from JDTImportBuilder.java

Root Cause : In JDTImportBuilder, we are using HashMap which does not guarantee the order of elements.

If the order of imports is expected to be in a particular order, we may need to change the code in the JDTImportBuilder here. Otherwise, if we are okay with a nondeterministic order of the function, we can improve the test case as done in the PR.

Please let me know what is the expected behaviour here so that I can proceed with that fix.

---

2. getDeclaredFields() Ordering Issue:

Regarding the getDeclaredFields() method, while it doesn't guarantee any order, I think we should not sort it in the model but be more flexible in the tests. But I'd like to know where exactly the problem occurs to understand it better.

The given fix is for spoon.test.processing.ProcessingTest#testInitPropertiesWithWrongType test, where we are setting the fields through ProcessorProperties and then asserting on the fields.

		props.set("aString", "foo");
		props.set("anObject", "foo");
		props.set("anInt", "foo");

		try {
			ProcessorUtils.initProperties(p, props);
			fail();
		} catch (SpoonException e) {
			assertTrue(e.getMessage().contains("anInt"));
		}


		assertEquals("foo", p.aString);
		assertEquals(0, p.anInt);
		assertNull(p.anObject);

In the given test, the assertions are expecting that the value of attribute aString will be set first, then the value of anInt will be set, which will throw SpoonException.
But this may not be the case. It will fail in the following scenarios:

• If initProperties tries to set the value of anInt first, then the value of aString will still remain NULL and the assertion will fail.
• Similarly, if the value of anInt is set after setting anObject, that will also fail, since the value of anObject will be set as "foo".

Hence, to fix this test case, we may have to remove the assertion on aString and anObject. Please let me know your views on it or if I can provide any more details. Post that, I'll proceed with the change in a separate PR as you suggested.

---

3. Enums Ordering Issue:

Now, the resulting enum must have GOOD, BETTER in that exact order, otherwise the substitution misbehaved (and the test should fail).

For the enumValues, I am still investigating. I have narrowed it down to function generateTargets in DefaultGenerator.java and am looking into it. Will update you once I find the exact implementation which affects this.

[nikunjagarwal321]

Hi @SirYwell ,
Please let me know if the above changes make sense. I can open separate PRs for the changes or make change in the same one as you say.

[SirYwell]

Hi @SirYwell , Please let me know if the above changes make sense. I can open separate PRs for the changes or make change in the same one as you say.

Hi, if you found the root cause for the enum value problem, this seems to be the most relevant one.
For the import order, a separate PR would be appreciated.
For the reflective getDeclaredField call, I'm not sure what's sensible.

[nikunjagarwal321]

Hi, if you found the root cause for the enum value problem, this seems to be the most relevant one. For the import order, a separate PR would be appreciated. For the reflective getDeclaredField call, I'm not sure what's sensible.

Hi,
Sure.

1. For enum value problem, I'll check this again. Earlier, I was able to narrow it down to generateTargets in DefaultGenerator.java.

2. For import order flaky tests, I have raised a new PR: review: test: Fixed 3 non-deterministic flaky tests  #6123

3. For the reflective getDeclaredField call, may be we can remove the following 2 assertions from ProcessingTest.java as they depend on the order of getDeclaredFields()

		assertEquals("foo", p.aString);
		assertNull(p.anObject);

Or maybe we can change the code to:

		props.set("aString", "foo");
		props.set("anObject", "foo");
		ProcessorUtils.initProperties(p, props);
		try {
			props.set("anInt", "foo");
			ProcessorUtils.initProperties(p, props);
			fail();
		} catch (SpoonException e) {
			assertTrue(e.getMessage().contains("anInt"));
		}


		assertEquals("foo", p.aString);
		assertEquals(0, p.anInt);
		assertEquals("foo", p.anObject);

This will test the behaviour of wrongDataType correctly irrespective of the order the fields are returned. Let me know your thoughts on this.