Repository navigation
test(wasm): verify acceptance child runner argument order - #2718
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Small, well-scoped fix. fullChildCommand now appends --browser-only for the raw J32-GoJS profile (p.Target == "" && p.GOOS == "js"), placed immediately after the runner path and before the artifact. This matches:
- the
emscripten-runner.mjscontract, which only parses--browser-onlywhen it isargv[2](before the module), and - how
llgo testruns raw GoJS output (internal/build/build.go:1318→node runner.mjs --browser-only {module}), so the added comment's "same browser mode as llgo test" is accurate.
The guard correctly excludes the Emscripten JS profiles (which set Target) and GoJS-reference (handled by the earlier Reference branch). The new slices.Equal assertion is a genuine strengthening — the previous Contains-based checks could not have caught a misordered --browser-only.
Correctness, security, performance, and the new code comment all check out. One minor, optional maintainability note is left inline. Nothing blocking.
| if name == "J32-GoJS" { | ||
| want = append(want, "--browser-only") | ||
| } else if name == "J64-Emscripten" { | ||
| want[3] = filepath.Join("/repo", "targets", "emscripten-memory64-runner.mjs") |
Contributor
There was a problem hiding this comment.
Minor maintainability nit (non-blocking): want[3] mutates a positional slot whose correctness silently depends on the fixed timeout prefix length (--kill-after=10s, 30s, node, <runner>) built in fullChildCommand. The literal 3 is a magic index — if that prefix ever changes, this would substitute the wrong element rather than fail loudly. Consider locating the runner slot by searching for the .mjs entry (or building want per-profile from a shared base) so the assertion documents intent. Current value is correct.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/fix-wasm-std-issue28-20261003
branch
from
October 4, 2026 22:56
b6422e9 to
cbfbe1e
Compare
cpunion
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The original GoJS child-loader fix is already on main through #2730. Rebased onto
ec9c2488band retained only the stronger regression test; this PR now changes no production code.Check the complete child argument list for J32-GoJS, J32-Emscripten and J64-Emscripten, instead of checking only whether
--browser-onlyis present. This also verifies the correct runner, browser flag placement, artifact and selector order:The existing timeout wrapper remains part of the exact assertion. This protects the host-side print/panic/finalizer/Goexit acceptance checks from invoking raw GoJS glue with the wrong loader mode.
Refs llgo-compat/ci#28. The remaining diff is one test file (+9/-2).
Validation after rebase:
go test -race ./dev/wasmstdlib -count=1passed (90.5s).git diff --checkpassed.