Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/build.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -127,6 +127,21 @@ jobs:
check_name: Test Results (${{ matrix.os }})
fail_on_failure: false

# luaOutputIsDeterministicForGenericOverrideSlots compiles one program twice and keeps both
# scripts when they differ. They are written to the runner's filesystem, so without this the
# evidence goes with the runner and the failure is as unactionable as it was before it kept
# anything. Most failures are not this test, hence if-no-files-found: ignore.
- name: Upload determinism scripts (on failure)
if: failure()
uses: actions/upload-artifact@v4
with:
name: determinism-scripts-${{ matrix.os }}
path: |
de.peeeq.wurstscript/test-output/determinism-first.lua
de.peeeq.wurstscript/test-output/determinism-second.lua
if-no-files-found: ignore
retention-days: 14

- name: Upload packaged artifact (per-OS)
uses: actions/upload-artifact@v4
with:
Expand Down
31 changes: 18 additions & 13 deletions BACKLOG.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -125,22 +125,27 @@ itself, and one gap in what the suite can see.
this stops being dead weight, because the cost of getting it wrong is a real mis-binding while
the cost of leaving it is one unused table key per specialised class.

24. **`luaOutputIsDeterministicForGenericOverrideSlots` fails intermittently.** It failed once on
Windows CI and passed on a re-run of the same commit, having blocked an unrelated pull request
24. **`luaOutputIsDeterministicForGenericOverrideSlots` failed once and has not since.** It failed
on Windows CI and passed on a re-run of the same commit, having blocked an unrelated pull request
in between.

Do not weaken the assertion. It compiles one repro twice and compares the output byte for byte,
so an intermittent mismatch is evidence of intermittent nondeterminism in Lua emission, which is
exactly what it exists to catch — a re-run passing says the nondeterminism is intermittent, not
that the test is at fault. Calling it flaky was too quick.

Diagnose it instead: capture both outputs on a failing run and diff them, and rule out harness
interference rather than assuming it. The two compiles do start from the same cache state, but
that comes from two separate resets: `WurstScriptTest`'s `@BeforeMethod` clears before the
first, and the explicit `GlobalCaches.clearAll()` between the compilations inside the test
clears before the second. Both are load bearing — remove either and the comparison stops being
between equal starting states, which would invalidate the conclusion rather than explain the
failure.
so a mismatch is evidence of nondeterminism in Lua emission, which is what it exists to catch.

**Not reproduced.** 250 compiles of that repro in one JVM, caches cleared between each, came out
byte-identical. Sources of hash-ordered iteration in the emission path were read rather than
guessed at: `TypeId.calculate` sorts by name and package, `createMethods` groups through a
`TreeMap`, `assignDispatchAliases` collects into a `TreeSet` and iterates a list, and
`collectSuperClasses` uses its set only to mark what it has seen. None of those can vary.

So whatever differs is either rarer than one in 250, or comes from something the local run does
not vary — a different core count changing the fork layout, memory pressure, or the interpreter
build on that runner.

What changed meanwhile is that the failure now carries evidence: both scripts are written beside
the test output and the first differing lines are named with their numbers. The one occurrence so
far produced nothing to work from, which is why it cost a re-run and no diagnosis. The next one
will say what differed.

12. **Standing item, never finished.** When nothing above is left, find the next thing worth
doing and add it here rather than stopping. Good sources, in order: a test that would have
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -1418,7 +1418,59 @@ public void luaOutputIsDeterministicForGenericOverrideSlots() throws IOException
test().testLua(true).compilationUnits(genericOverrideReproUnits());
String second = Files.toString(new File("test-output/lua/LuaTranslationTests_luaOutputIsDeterministicForGenericOverrideSlots.lua"), Charsets.UTF_8);

assertEquals(first, second);
if (!first.equals(second)) {
// This has failed once on CI and not since, and 250 compiles in one JVM did not
// reproduce it. A bare "expected X but got Y" over two whole scripts is unreadable and
// the run's output is gone by the time anyone looks, so the failure carries what it
// takes to act on: both scripts kept beside the test output, and the differing lines
// named. Without this the next occurrence is as unactionable as the first.
File firstFile = new File(TEST_OUTPUT_PATH, "determinism-first.lua");
File secondFile = new File(TEST_OUTPUT_PATH, "determinism-second.lua");
Files.write(first.getBytes(Charsets.UTF_8), firstFile);
Files.write(second.getBytes(Charsets.UTF_8), secondFile);
Comment on lines +1427 to +1430

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Upload the preserved scripts from failing CI runs

When this intermittent test fails in CI, these files are written only to the ephemeral runner filesystem, so the promised full evidence is still gone after the job finishes. The checked workflow uploads only release archives in .github/workflows/build.yml lines 130–137—and that step does not use if: always()—while the JUnit report retains only the five-line summary. Add a failure-time artifact upload for these scripts, or include the complete diff in the test result.

Useful? React with 👍 / 👎.

fail("the same program compiled to different Lua twice in one run."
+ "\n" + describeFirstDifferences(first, second)
+ "\nboth kept at " + firstFile.getPath() + " and " + secondFile.getPath());
}
}

/**
* Every differing line, with its number, so the failure itself says what changed.
* <p>
* The scripts are also uploaded as an artifact on a failing CI run, but the message has to stand
* on its own: an artifact needs fetching, and the check is what gets read first. Bounded so a
* wholesale difference does not bury the report, with the total stated either way.
*/
private static String describeFirstDifferences(String first, String second) {
final int reportLimit = 40;
String[] a = first.split("\n", -1);
String[] b = second.split("\n", -1);
StringBuilder sb = new StringBuilder();
int differing = 0;
for (int i = 0; i < Math.max(a.length, b.length); i++) {
String lineA = i < a.length ? a[i] : "<missing>";
String lineB = i < b.length ? b[i] : "<missing>";
if (!lineA.equals(lineB)) {
differing++;
Comment on lines +1450 to +1454

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Align lines before summarizing differences

When nondeterminism inserts, removes, or reorders a generated function or block, comparing only equal line indexes shifts every subsequent comparison, so the reported total is inflated and the first forty entries can be unrelated line pairs rather than the actual changes. This is especially relevant for emission-order nondeterminism, the scenario this diagnostic is intended to investigate; use a line-diff/LCS-style alignment before counting and reporting additions, removals, and replacements.

Useful? React with 👍 / 👎.

}
}
sb.append(" ").append(differing).append(" line(s) differ")
.append(differing > reportLimit ? ", first " + reportLimit + ":\n" : ":\n");
int reported = 0;
for (int i = 0; i < Math.max(a.length, b.length) && reported < reportLimit; i++) {
String lineA = i < a.length ? a[i] : "<missing>";
String lineB = i < b.length ? b[i] : "<missing>";
if (!lineA.equals(lineB)) {
sb.append(" line ").append(i + 1).append(":\n")
.append(" first: ").append(lineA).append('\n')
.append(" second: ").append(lineB).append('\n');
reported++;
}
}
if (reported == 0) {
sb.append(" the scripts differ but no line does, so it is line endings or trailing bytes");
}
return sb.toString();
}

@Test
Expand Down
Loading