Uh oh!
There was an error while loading. Please reload this page.
Run a standard library program on Lua - #1242
Conversation
Nothing did. Every test putting the library on that target compiled only, because three packages could not initialise: Colors, Vectors and GameTimer each call a native the runtime shim did not define. StringCase made a fourth once the program itself ran. That is how the empty allocation in #1239 survived - the paths which would have caught it were compiled and never executed. The four natives are byte-accurate rather than approximations, since a test comparing targets is worth nothing if they disagree by construction: StringHash is the same lookup2 as Wc3StringHash and pinned against it for a fixed corpus, StringCase folds only ascii. A timer keeps what it was started with and never fires: a test drives its own program rather than waiting on game time, and a package which starts one at init needs the call to succeed. Success is read off stdout, and a program with the library gets the library's own testSuccess, which is empty - so such a test could only ever be reported as not having succeeded, whatever it did. The harness now installs one which prints, as the generated fallback does without the library. FastHashMap against the standard library runs on Lua as a result, rather than being compiled and assumed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:9bbc4f2650
ℹ️ 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".
| // standard library gets a generated fallback which does; one with the library gets | ||
| // the library's own, which is empty - so without this, a test on that target can | ||
| // only ever be reported as not having succeeded, whatever it did. | ||
| chunk.append("testSuccess = function() print('testSuccess') end;"); |
There was a problem hiding this comment.
Include os.exit() after printing, as the existing Lua native in LuaNatives does and the interpreter's OutputProvider.testSuccess() does by throwing its success sentinel. With the unconditional replacement here, any Lua test that calls testSuccess() before code that fails or loops will now fail or time out after already succeeding, creating backend-specific test behavior.
AGENTS.md reference: AGENTS.md:L217-L221
Useful? React with 👍 / 👎.
| assertEquals(Wc3StringHash.hash(ILconstString.fromText("abc").getVal()), 1043745117); | ||
| assertEquals(Wc3StringHash.hash(ILconstString.fromText("Hello World").getVal()), -1563733934); | ||
| assertEquals(Wc3StringHash.hash(ILconstString.fromText("Units\\Human\\Footman.mdx").getVal()), 166547459); | ||
| assertEquals(Wc3StringHash.hash(ILconstString.fromText("ä").getVal()), 1899444195); | ||
| assertEquals(Wc3StringHash.hash(ILconstString.fromText("abcdefghijklmnop").getVal()), 190888648); |
There was a problem hiding this comment.
Run these inputs through wc3shim.lua rather than comparing only Wc3StringHash.hash(...) with literals. Every assertion here executes the Java implementation, so a change that breaks only the new Lua StringHash leaves this purported drift test green and removes the intended cross-backend validation.
AGENTS.md reference: AGENTS.md:L217-L221
Useful? React with 👍 / 👎.
The injected testSuccess dropped the os.exit() the generated native does, so a program calling it before something which fails or loops would newly fail or time out after having already succeeded - a difference between the targets rather than a fix to one. The parity test compared the Java hash against numbers written down from a Lua run, which is not a parity test: a change breaking only the Lua side left it green. It runs the inputs through the shim now and compares the two, with the input escaped byte by byte so what Lua hashes is what Java was handed.
Frotty
commented
Aug 16, 2026
Both fixed. Preserve termination in the injected success native — right, and it is a real behaviour difference rather than a cosmetic one. Exercise the Lua hash in the parity test — also right, and the test was misnamed for what it did: every assertion ran the Java implementation against numbers I had pasted in from a Lua run, so a change breaking only the Lua side left it green. That is the case worth catching, and it was the one case it could not catch. It now runs each input through Full suite green. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:db333697af
ℹ️ 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".
| } | ||
| String script = "dofile('src/test/resources/luaruntime/wc3shim.lua') " | ||
| + "print(StringHash('" + literal + "'))"; | ||
| Process p = new ProcessBuilder(getLuaExecutable(), "-e", script) |
There was a problem hiding this comment.
On hosts where the bundled interpreter is incompatible or lacks required shared libraries and no working lua53/lua exists on PATH, getLuaExecutable() throws IllegalStateException here and fails the entire test class. The normal Lua execution path catches this condition and raises SkipException, and the repository explicitly requires Lua tests to skip visibly when no interpreter is found; apply the same handling to this direct invocation.
AGENTS.md reference: AGENTS.md:L106-L110
Useful? React with 👍 / 👎.
The item said the caches being cleared around every test method is what gives the two compilations the same starting state. That is only half of it: the before-method reset covers the first compile, and the explicit clearAll between them covers the second. Naming the wrong one matters when diagnosing, because removing the in-test reset would invalidate the conclusion rather than explain the failure.
Frotty
commented
Aug 16, 2026
Fixed. One thing to flag about the history rather than leave it to be noticed: commit f78ca81 carries the wrong message. Its content is this skip handling; the message belongs to a change on the backlog branch, and a |
Frotty
commented
Aug 16, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f78ca81671
ℹ️ 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".
| String out; | ||
| try (java.io.BufferedReader r = new java.io.BufferedReader( | ||
| new java.io.InputStreamReader(p.getInputStream(), java.nio.charset.StandardCharsets.UTF_8))) { | ||
| out = r.lines().collect(java.util.stream.Collectors.joining("\n")).trim(); |
There was a problem hiding this comment.
If the shim or StringHash hangs, r.lines().collect(...) blocks waiting for EOF before execution reaches the 30-second waitFor, so this parity test can hang the test suite indefinitely—the precise failure mode its timeout appears intended to bound. Drain the stream asynchronously (as the main Lua harness does), wait with the timeout, and forcibly terminate the process when it expires.
Useful? React with 👍 / 👎.
It read the process output to the end before waiting with a timeout, so a shim or a hash which hung never reached the wait: the read blocks until the stream closes, and a hung process does not close it. The timeout could only have fired after the hang had already stopped the suite. Drained on a thread of its own now, waited for with the deadline, and killed when it expires - the same shape as the main Lua harness, which had this same bug and this same fix.
Frotty
commented
Aug 16, 2026
Right, and it is the same bug the main Lua harness had — which I fixed earlier in this work and then reintroduced here, in a helper small enough that it did not look like it needed the same care.
Now: drained on a thread of its own, waited for with the deadline, Parity test still passes on all eight inputs. @codex review |
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Uh oh!
There was an error while loading. Please reload this page.
Nothing in this repository has ever executed a standard library program on Lua. Every test putting the library on that target compiles only — which is why
fastHashMapAgainstTheStandardLibraryLuaasserted on emitted shape rather than on a result, and it is how the empty-allocation bug in #1239 survived: the paths which would have caught it are compiled and never run.It turned out to be four natives.
What was missing
Three packages could not initialise, each on one undefined global:
StringHashLocationTimerStartStringCasemade a fourth once the program itself ran. The wrapped message (Could not initialize package GameTimer.) hides this; the underlying error is on the line above it, which is where I should have looked the first time.The implementations
Byte-accurate rather than approximations, since a test comparing two targets is worth nothing if they disagree by construction:
StringHashis the same Bob Jenkins lookup2 asWc3StringHash, over bytes, with the same normalisation. It is a second transcription of one algorithm, which can drift, soagreesWithTheLuaRuntimeImplementationpins both against a fixed corpus — ascii, a path with backslashes, a multibyte character and the empty string. A change parting the two fails there.StringCasefolds only ascii letters, matching the interpreter: the bytes of a multibyte character are not letters, and folding one rewrites the character.Locationis a plain pair; nothing in a test reads terrain from one.TimerStartkeeps what it was started with and never fires. A test drives its own program rather than waiting on game time, and a package which starts a timer at init only needs the call to succeed. Making timers actually fire needs a model of game time, which is a design question rather than more stubs — worth saying plainly, because this is the one stub that is not equivalent to the real thing.The harness
Success is read off stdout. Without the library, a program gets the generated fallback for
testSuccess, which prints. With the library it gets the library's own, which is empty — so a test on that target could only ever be reported as not having succeeded, whatever it actually did. The harness now installs a printing one after loading the script.This is the part that matters beyond the natives: it was not possible for a standard library Lua test to pass.
Result
fastHashMapAgainstTheStandardLibraryLuaexecutes now instead of being compiled and assumed, and the container works there. Full suite green.Closes the executed-on-Lua half of backlog item 16 (#1241).