choksi2212 opened a new pull request, #3547:
URL: https://github.com/apache/maven-surefire/pull/3547
Fixes #3544.
## What went wrong
`runOrder=random` on the JUnit Platform shuffles the classes twice with the
same seed, in every configuration where all classes reach Jupiter in one
discovery request - the default `forkCount=1`/`reuseForks=true` being the
common case:
- `DefaultRunOrderCalculator` sorts the classes alphabetically and shuffles
them with `new Random(runOrder.random.seed)`;
- `setupRunOrder()` configures Jupiter's `ClassOrderer$Random` and forwards
the same seed;
- the eager path in `execute()` then sends the already shuffled classes to
Jupiter in one request, and `ClassOrderer$Random` shuffles them again with the
same seed.
`Collections.shuffle` with a fresh `Random(seed)` always applies the same
permutation to a list of a given size, so the two shuffles compose to
sigma-of-sigma. Only squares of permutations can occur: with two classes the
result is always alphabetical, exactly the reproducer table in the issue shows
(`AB x20` for `runOrder=random` vs `AB x9, BA x11` for the plain Jupiter
orderer with the same seeds).
## The fix
When all classes reach Jupiter in one discovery request *and* this provider
configured the class orderer, hand the classes over **sorted, not shuffled**,
and leave the single shuffle to Jupiter
(`JUnitPlatformProvider.orderTestClasses`):
```java
if (jupiterShufflesTheClasses
&& classOrdererSetByProvider
&& !runsOneClassAtATime()
&& !isVintageEngineDetected()) {
return sortedAlphabetically(scannedClasses);
}
return parameters.getRunOrderCalculator().orderTestClasses(scannedClasses);
```
The executed order now follows from the seed alone and matches what
`ClassOrderer$Random` produces with that seed, which is also what the issue
used as its control.
The calculator keeps ordering the classes in every other case:
- `getSuites()` hands the classes over one at a time (fork distribution,
tests discovered in process), so Jupiter's orderer never sees more than one
class;
- JUnit 4-only suites run one class per request, where the surefire shuffle
is the only ordering;
- TestNG and vintage classes keep the discovery order, because the Jupiter
orderer cannot re-order them - a mixed suite keeps its current behavior;
- a class orderer configured by the user (system property,
`junit-platform.properties`, or the `configurationParameters` option) is not
necessarily `ClassOrderer$Random`, so the surefire shuffle must stay.
`setupRunOrder` now records whether the provider actually installed
`ClassOrderer$Random` (`putIfAbsent` returns the previous value, so a
user-configured orderer reports as "not ours"), which is what the guard checks.
## Verification
New `JUnitPlatformProviderRunOrderTest` (the existing provider test class is
3 lines under the checkstyle file-length limit, so the tests live in their own
class):
- `runOrderRandomShufflesTheClassesOnceWhenJupiterSeesThemAll` - 12 large
seeds through the real provider and launcher: both execution orders occur. **On
pristine master this fails with only `TestClass1` ever running first** - the
sigma-squared-always-alphabetical symptom from the issue;
- `runOrderRandomDoesNotShuffleAgainWhenJupiterSeesAllClasses` - the
calculator is not invoked in the governed path. On master the mock calculator
throws the `AssertionError` it is wired with, because the double shuffle is
live there;
- four guard tests pin that the calculator *is* still invoked for the suites
(`getSuites`), for JUnit 4-only suites, when the vintage engine is detected,
and when the user configured their own class orderer.
Full local runs (Windows, Maven 3.9.16, JDK 17): `mvn -pl
surefire-providers/surefire-junit-platform test` - **173/173 with the fix (167
pristine + the 6 new tests), 0 failures in both**, checkstyle clean. One note
for reviewers: the per-class `Tests run` numbers in this module are noisy
across runs on pristine master too (the provider tests execute real nested test
fixtures through live adapters inside the running surefire instance, so report
attribution between classes shifts); the totals are stable and every class is
green with and without the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
