Uh oh!
There was an error while loading. Please reload this page.
Make the determinism failure say what differed - #1245
Conversation
It compiled the repro twice and compared the two whole scripts with assertEquals, so its one failure on CI printed an unreadable dump, left nothing behind, and cost a re-run to dismiss without a diagnosis. The assertion is unchanged - still byte for byte on two compiles in one run - but a failure now writes both scripts beside the test output and names the first differing lines with their numbers. Not reproduced while doing this: 250 compiles of that repro in one JVM, caches cleared between each, came out identical, and the hash-ordered iteration in the emission path is sorted where it reaches the output. Whatever differs is rarer than that or depends on something a local run does not vary. The backlog item records both the negative result and where not to look again.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8416af0518
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
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 👍 / 👎.
luaOutputIsDeterministicForGenericOverrideSlotscompiles one repro twice and compares the output byte for byte. It failed once on Windows CI, passed on a re-run of the same commit, and blocked an unrelated pull request in between — producing nothing anyone could act on, because comparing two whole scripts withassertEqualsprints an unreadable dump and the outputs are gone by the time the failure is read.Not reproduced
Worth saying first, since it shapes what this change is.
250 compiles of that repro in one JVM, caches cleared between each, came out byte-identical. I also read the places in the emission path where hash-ordered iteration could reach the output, rather than guessing at them:
TypeId.calculatesorts by class name and packagecreateMethodsgroups through aTreeMapassignDispatchAliasescollects into aTreeSetand iterates a listcollectSuperClassesuses its set only to mark what it has already seenNone of those can vary between runs. So whatever differs is either rarer than one in 250, or comes from something a local run does not vary — a different core count changing the fork layout, memory pressure, or that runner's interpreter build.
What changed
The assertion is untouched. It still compiles twice and still compares byte for byte, because an intermittent mismatch is evidence of nondeterminism in Lua emission and that is what it exists to catch.
What is new is that a failure carries evidence: both scripts are written beside the test output, and the first differing lines are named with their numbers. If the scripts differ but no line does, it says that too, since then it is line endings or trailing bytes rather than emission.
The next occurrence will say what differed instead of costing a re-run to dismiss.
Backlog item 24 records the negative result and where not to look again, so this is not re-derived from scratch next time.
Full suite green, 6m14s.