review: test: Fixed nondeterministic failures in ImportTest.staticTypeAndMethodImport_importShouldAppearOnlyOnceIfTheirSimpleNamesAreEqual()

[yonghanlin]

What does this PR do?

This PR fixes a nondeterministic test failure in ImportTest.staticTypeAndMethodImport_importShouldAppearOnlyOnceIfTheirSimpleNamesAreEqual() when running with NonDex.

Problem

When two imports have the same simple name (foo), the test sorts them using:

imports.sort(Comparator.comparing(i -> i.getReference().getSimpleName()));

However, the imports is being set by JDTImportBuilder.java class where the imports variable is implemented as HashSet. Since both imports have the same simple name, the comparator treats them as equal, so the sorted result simply preserves the iteration order of the underlying list backed by this HashSet. When TYPE appears before METHOD, the test fails with:

Expected: METHOD
But was: TYPE

Reproduce Test

To reproduce the failure, run NonDex on . module using the following commands:

mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=spoon.test.imports.ImportTest#staticTypeAndMethodImport_importShouldAppearOnlyOnceIfTheirSimpleNamesAreEqual

The Fix

Use a deterministic comparator that resolves ties when two imports share the same simple name. After sorting by simple name as before, the comparator now explicitly prefers METHOD imports over TYPE imports, and finally falls back to comparing the full reference string to ensure stability.

[SirYwell]

I appreciate the efforts, but these Pull Requests are generally difficult to understand, especially whether the problem lies in the tests or if there is a deeper problem in the way spoon represents data.

For example in this case, where does the list with the two elements come from? Is it constructed in a way where it would be wrong if the order is different that currently expected and asserted?

It also doesn't help that the test contract says contract: static type and method import should appear only once if their simple names are equal which seems to conflict with the assertions unless I just misunderstand the test.

I think it would be very helpful to see where the tool changes the order of something (from my understanding, this only happens where order isn't properly defined), and track down if that part makes sense. Can NonDex provide this information?

[yonghanlin]

Thank you for the detailed comment and for raising these points. These pull requests focus on making the tests robust to nondeterministic iteration order, rather than changing internal data structures. My concern is that modifying the core data structures could have wider effects across the codebase, whereas fixing tests is local and only affects the specific contracts being checked.

In this pull request, the two imports in the test originate from Spoon’s internal collection of CtImport objects. This collection is backed by a data structure whose iteration order is not defined (e.g., a Set). Under normal JVM execution, the order often appears stable, but NonDex randomizes iteration order, which makes the nondeterminism visible. In other words, NonDex is not modifying the order itself, but instead revealing the nondeterminism that exists because the original ordering was never defined.

For the test contract, when looking at the assertions, the test keeps both imports (TYPE and METHOD) but expects a specific preferred order after sorting: METHOD → TYPE. This is why the assertions fail when the order flips. The wording in the contract comment is therefore a bit confusing and does not fully reflect what the assertions are enforcing. My change is intended to make that existing preference explicit and deterministic, rather than to change the intended behavior of the test.

I will also update the descriptions in other pull requests to make it clearer, and I’ll try to keep the changes smaller and more focused. Thank you again for the feedback!

[SirYwell]

Thanks for the explanation. It would be really good if we could find some kind of trace of where the indeterministic behavior comes from. From a quick look, the import list looks fine, so does it depend on the order in which the elements are inserted into that list? Or is the sorting algorithm not guaranteed to be stable?

[yonghanlin]

Thanks for pointing out this unclear part. The nondeterminism does not come from the sorting algorithm itself, it comes from the initial ordering of the imports before sorting. The imports is being set by JDTImportBuilder.java class where the imports variable is implemented as HashSet. The iterator() method of java.util.HashSet is documented as returning elements in no particular order, so the iteration order is not guaranteed to be stable (HashSet Javadoc). I hope this explanation makes the source of the nondeterminism clearer, and I could provide more details if needed.

[SirYwell]

Okay, this is very useful information. I'm not sure if there were discussions around that before, but maybe @monperrus, @I-Al-Istannen, or @algomaster99 remember anything regarding import order. To me it sounds like we should probably keep the order of what JDT gives us.

[I-Al-Istannen]

I vaguely recall there being some position based sorting for imports in the JDTTreeBuilder or similar, but I am not sure anymore. If JDT shuffles them non-deterministically we should impose our own ordering, otherwise keeping the JDT order is probably fine?

[algomaster99]

If JDT shuffles them non-deterministically we should impose our own ordering, otherwise keeping the JDT order is probably fine?

I agree. The way to check if JDT import order is consistent we can simply replace the Set implementation to TreeSet in JDTImportBuilder and see if this non-determinism goes away.

EDIT: Or maybe LinkedHashSet is a better choice to avoid implementing comparable ourselves.