diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fa77fdecc..1eaae2807 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -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: diff --git a/BACKLOG.md b/BACKLOG.md index 86165e337..9ddf44f20 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -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 diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterminismDiffReportTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterminismDiffReportTest.java new file mode 100644 index 000000000..7aaa1dc2d --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterminismDiffReportTest.java @@ -0,0 +1,88 @@ +package tests.wurstscript.tests; + +import org.testng.annotations.Test; + +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertTrue; + +/** + * Covers the summary the determinism test prints when it fails. + *
+ * The failure it describes has happened once and is not reproducible on demand, so the summary is + * the only thing that will be read when it happens again. That makes it worth testing on its own: + * a description which misreports what changed is worse than none, because it sends the next person + * looking in the wrong place. + */ +public class DeterminismDiffReportTest { + + /** + * A block appearing in one script and not the other reads as one addition. Comparing equal + * indexes instead would report every line after the insertion as differing, which is the case + * emission-order nondeterminism actually produces. + */ + @Test + public void anInsertedBlockIsOneAdditionRatherThanEverythingAfterIt() { + String first = "a\nb\nc\nd\ne\nf\ng\nh\n"; + String second = "a\nb\nINSERTED\nc\nd\ne\nf\ng\nh\n"; + + String report = LuaTranslationTests.describeFirstDifferences(first, second); + + assertTrue(report.contains("0 line(s) only in the first, 1 only in the second"), + "one inserted line should read as one addition:\n" + report); + assertTrue(report.contains("INSERTED"), "the added line should be named:\n" + report); + assertTrue(report.contains("line 3"), "the added line's number should be given:\n" + report); + } + + /** A block moved rather than inserted reads as one removal and one addition, not a cascade. */ + @Test + public void aMovedLineIsOneRemovalAndOneAddition() { + String first = "one\nmoved\ntwo\nthree\nfour\n"; + String second = "one\ntwo\nthree\nmoved\nfour\n"; + + String report = LuaTranslationTests.describeFirstDifferences(first, second); + + assertTrue(report.contains("1 line(s) only in the first, 1 only in the second"), + "a moved line should read as one removal and one addition:\n" + report); + } + + /** Replacing a line in place is a removal and an addition at the same position. */ + @Test + public void aReplacedLineNamesBothVersions() { + String first = "x\nbefore\nz\n"; + String second = "x\nafter\nz\n"; + + String report = LuaTranslationTests.describeFirstDifferences(first, second); + + assertTrue(report.contains("before"), "the first version should be named:\n" + report); + assertTrue(report.contains("after"), "the second version should be named:\n" + report); + } + + /** + * Two scripts whose lines all match but which are not equal differ in how the lines end. The + * caller only asks after finding them unequal, so passing identical strings would test a state + * production never reaches — this passes CRLF against LF, which it can. + */ + @Test + public void differingOnlyInLineEndingsSaysSoRatherThanListingEveryLine() { + String crlf = "alpha\r\nbeta\r\ngamma\r\n"; + String lf = "alpha\nbeta\ngamma\n"; + assertNotEquals(crlf, lf, "the two inputs must be unequal for this to mean anything"); + + String report = LuaTranslationTests.describeFirstDifferences(crlf, lf); + + assertTrue(report.contains("line terminators"), + "line endings should be named rather than every line reported as changed:\n" + report); + assertFalse(report.contains("only in the first"), + "no line should be reported as removed:\n" + report); + } + + /** Bytes after the last line reach the same branch: every line matches, the scripts do not. */ + @Test + public void trailingBytesReachTheSameCase() { + String report = LuaTranslationTests.describeFirstDifferences("a\nb\n", "a\nb"); + + assertTrue(report.contains("line terminators") || report.contains("only in the"), + "a trailing difference should be described one way or the other:\n" + report); + } +} diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java index 014a3d9ce..c64ac4144 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java @@ -1418,7 +1418,102 @@ 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); + fail("the same program compiled to different Lua twice in one run." + + "\n" + describeFirstDifferences(first, second) + + "\nboth kept at " + firstFile.getPath() + " and " + secondFile.getPath()); + } + } + + /** + * What changed between the two scripts, aligned rather than compared line by line. + *
+ * Comparing equal indexes is not a diff: emission order moving a block shifts every line after + * it, so the count becomes "everything from here down" and the listed pairs are unrelated. That + * is the shape this diagnostic exists to investigate, so it is the shape it has to describe. + * Aligned on the longest common subsequence, a moved block reads as one removal and one addition. + *
+ * 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 totals stated either way.
+ */
+ /**
+ * Splits on either terminator, so a CRLF script aligns against an LF one line for line. Splitting
+ * on "\n" alone leaves the carriage return in the line text, which makes every line of a CRLF
+ * script differ from its LF counterpart and buries the actual difference.
+ */
+ private static final String NEWLINE_RE = "\r?\n";
+ private static final char NEWLINE = '\n';
+
+ static String describeFirstDifferences(String first, String second) {
+ final int reportLimit = 40;
+ String[] a = first.split(NEWLINE_RE, -1);
+ String[] b = second.split(NEWLINE_RE, -1);
+
+ // O(n*m) in memory, so a pathological pair falls back to reporting the sizes rather than
+ // exhausting the worker. The scripts this compares are a few hundred lines.
+ if ((long) a.length * b.length > 4_000_000L) {
+ return " too large to align: " + a.length + " lines against " + b.length;
+ }
+
+ int[][] common = new int[a.length + 1][b.length + 1];
+ for (int i = a.length - 1; i >= 0; i--) {
+ for (int j = b.length - 1; j >= 0; j--) {
+ common[i][j] = a[i].equals(b[j])
+ ? common[i + 1][j + 1] + 1
+ : Math.max(common[i + 1][j], common[i][j + 1]);
+ }
+ }
+
+ List