Change wasm-tests CI to run Node harness instead of Pascal harness#61
Conversation
📝 WalkthroughWalkthroughThis pull request replaces the Pascal-based WASM integration test harness with a Node.js ES module implementation. The changes include removing FPC installation steps from CI workflows, updating test invocations to use the new Node.js runner, removing the old Pascal script, adding the new Node.js module, and updating documentation references. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant ScriptLoader as ScriptLoader Binary
participant TempDir as Temp Directory
participant Fixture as Test Fixture
participant Host as Host Script
participant Node as Node.js Runtime
Runner->>Runner: Resolve ScriptLoader & Host paths
Runner->>Runner: Create temp working directory
loop For each fixture
Runner->>Fixture: Read fixture file
alt Skip directive detected
Runner->>Runner: Mark as SKIPPED
else Fixture valid
Runner->>ScriptLoader: Invoke --emit=wasm<br/>(fixture → temp wasm)
alt Compilation succeeds
ScriptLoader-->>TempDir: WASM file created
Runner->>Host: Execute node ./host<br/>(with WASM file)
Host->>Node: Run in Node.js runtime
alt Execution succeeds
Node-->>Host: stdout output
Runner->>Fixture: Parse // Expected: lines
Runner->>Runner: Compare actual vs expected
alt Output matches
Runner->>Runner: Mark as PASS
else Output mismatch
Runner->>Runner: Mark as FAIL
end
else Execution fails
Runner->>Runner: Mark as FAIL<br/>(node error/exit code)
end
else Compilation fails
Runner->>Runner: Mark as FAIL<br/>(compile error)
end
end
end
Runner->>TempDir: Clean up
Runner->>Runner: Output final summary<br/>(passed/failed/skipped counts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchmark Results254 benchmarks Interpreted: 🟢 2 improved · 🔴 1 regressed · 251 unchanged · avg +0.3% arraybuffer.js — Interp: 14 unch. · avg -1.1% · Bytecode: 14 unch. · avg -0.9%
arrays.js — Interp: 🟢 1, 18 unch. · avg +1.9% · Bytecode: 🟢 1, 🔴 2, 16 unch. · avg -1.5%
async-await.js — Interp: 6 unch. · avg +0.3% · Bytecode: 6 unch. · avg +1.5%
classes.js — Interp: 31 unch. · avg +0.4% · Bytecode: 31 unch. · avg +0.6%
closures.js — Interp: 🔴 1, 10 unch. · avg -1.6% · Bytecode: 11 unch. · avg +0.0%
collections.js — Interp: 12 unch. · avg +0.4% · Bytecode: 12 unch. · avg -0.5%
destructuring.js — Interp: 22 unch. · avg +0.4% · Bytecode: 🔴 2, 20 unch. · avg -2.0%
fibonacci.js — Interp: 8 unch. · avg -0.2% · Bytecode: 🟢 2, 6 unch. · avg +1.2%
for-of.js — Interp: 7 unch. · avg +0.4% · Bytecode: 🟢 1, 6 unch. · avg +1.5%
iterators.js — Interp: 20 unch. · avg +0.2% · Bytecode: 🔴 1, 19 unch. · avg +0.3%
json.js — Interp: 20 unch. · avg +0.5% · Bytecode: 🟢 1, 🔴 1, 18 unch. · avg -0.6%
jsx.jsx — Interp: 21 unch. · avg +0.5% · Bytecode: 🟢 1, 🔴 1, 19 unch. · avg +0.1%
numbers.js — Interp: 11 unch. · avg -1.0% · Bytecode: 11 unch. · avg -0.4%
objects.js — Interp: 7 unch. · avg +3.3% · Bytecode: 🔴 1, 6 unch. · avg -0.4%
promises.js — Interp: 🟢 1, 11 unch. · avg +1.4% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg -1.0%
strings.js — Interp: 11 unch. · avg -1.1% · Bytecode: 11 unch. · avg +1.1%
typed-arrays.js — Interp: 22 unch. · avg -0.1% · Bytecode: 22 unch. · avg -0.5%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-wasm/run-wasm-tests.mjs (1)
52-75: Consider caching file content to avoid double reads.Both
parseExpectedLinesandisSkippedcallreadFileSyncon the same fixture file. InrunFixture, if the file isn't skipped,parseExpectedLinesreads it again. For a small test suite this is fine, but you could optimize by reading once and passing the content to both functions.♻️ Optional: Single-read approach
+function readFixtureSource(fileName) { + return readFileSync(fileName, "utf8"); +} + -function parseExpectedLines(fileName) { +function parseExpectedLines(source) { const expectedPrefix = "// Expected: "; - const source = readFileSync(fileName, "utf8"); return source .split(/\r?\n/) .filter((line) => line.startsWith(expectedPrefix)) .map((line) => line.slice(expectedPrefix.length)); } -function isSkipped(fileName) { - const source = readFileSync(fileName, "utf8"); +function isSkipped(source) { for (const rawLine of source.split(/\r?\n/)) {Then in
runFixture:const source = readFixtureSource(fixtureFile); if (isSkipped(source)) { ... } // later: const expected = parseExpectedLines(source);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-wasm/run-wasm-tests.mjs` around lines 52 - 75, Both parseExpectedLines and isSkipped independently call readFileSync causing duplicate reads; change the API to accept the file content string instead of reading inside those functions. Update parseExpectedLines(source) and isSkipped(source) to take a source string (remove internal readFileSync usage), add a helper like readFixtureSource in runFixture to read the file once, and pass that source to isSkipped and parseExpectedLines. Ensure all call sites that previously passed fileName are updated to pass the cached source string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests-wasm/run-wasm-tests.mjs`:
- Around line 52-75: Both parseExpectedLines and isSkipped independently call
readFileSync causing duplicate reads; change the API to accept the file content
string instead of reading inside those functions. Update
parseExpectedLines(source) and isSkipped(source) to take a source string (remove
internal readFileSync usage), add a helper like readFixtureSource in runFixture
to read the file once, and pass that source to isSkipped and parseExpectedLines.
Ensure all call sites that previously passed fileName are updated to pass the
cached source string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d855643-0108-4bc6-997b-802ff932808a
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/pr.ymlAGENTS.mddocs/wasm-backend.mdtests-wasm/run-wasm-tests.mjstests-wasm/run-wasm-tests.pas
💤 Files with no reviewable changes (1)
- tests-wasm/run-wasm-tests.pas
Summary by CodeRabbit
Tests
Documentation