refactor(compliance): native TOML runner and generated JSON5 tests - #1047
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces Python-based TOML and JSON5 compliance orchestration with native TOML execution, generated JSON5 tests, pinned-suite automation, updated build and CI workflows, documentation, and stricter TOML array redefinition validation. ChangesNative compliance execution
TOML parser validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Comment |
Contributor
Web Tooling Benchmark
18 pinned Web Tooling workloads; 18 workloads produced at least one Goccia sample. Raw results from 1 sample per workload; full stdout/stderr for failures and min/max/CV stay in the |
Contributor
Suite TimingTest Runner (interpreted: 11,664 passed; bytecode: 11,664 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 439; bytecode: 439)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Boot
Empty-script ( Measured on ubuntu-latest x64. |
Contributor
Benchmark Results439 benchmarks · PR vs same-runner Interpreted: 🟢 37 improved · 🔴 23 regressed · 379 unchanged · avg +0.6% Typical per-run noise (median variance): interpreted ±2.6%, bytecode ±1.9%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +2.1% · Bytecode: 🟢 1, 🔴 2, 11 unch. · avg +0.0%
arrays.js — Interp: 🟢 2, 🔴 1, 16 unch. · avg +1.5% · Bytecode: 19 unch. · avg -0.1%
async-await.js — Interp: 🟢 2, 4 unch. · avg +2.3% · Bytecode: 6 unch. · avg -0.4%
async-generators.js — Interp: 🟢 1, 1 unch. · avg +5.5% · Bytecode: 2 unch. · avg -3.8%
atomics.js — Interp: 🔴 1, 5 unch. · avg +0.4% · Bytecode: 🟢 2, 4 unch. · avg +4.0%
base64.js — Interp: 🟢 1, 9 unch. · avg -1.1% · Bytecode: 🟢 1, 🔴 1, 8 unch. · avg +0.2%
classes.js — Interp: 🟢 1, 30 unch. · avg -1.1% · Bytecode: 🟢 3, 28 unch. · avg +1.3%
closures.js — Interp: 🔴 1, 10 unch. · avg -2.8% · Bytecode: 🟢 3, 🔴 1, 7 unch. · avg +0.3%
collections.js — Interp: 12 unch. · avg -0.2% · Bytecode: 🔴 1, 11 unch. · avg -2.8%
csv.js — Interp: 13 unch. · avg +1.2% · Bytecode: 🟢 1, 🔴 3, 9 unch. · avg -0.1%
destructuring.js — Interp: 🔴 4, 18 unch. · avg -0.7% · Bytecode: 🟢 1, 🔴 3, 18 unch. · avg -1.5%
fibonacci.js — Interp: 8 unch. · avg +0.5% · Bytecode: 🟢 1, 🔴 1, 6 unch. · avg -0.6%
float16array.js — Interp: 🟢 1, 31 unch. · avg +0.8% · Bytecode: 🟢 5, 🔴 3, 24 unch. · avg +0.4%
for-in/for-in.js — Interp: 3 unch. · avg -3.1% · Bytecode: 3 unch. · avg +3.6%
for-of.js — Interp: 🟢 1, 🔴 2, 4 unch. · avg +1.7% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg -0.5%
generators.js — Interp: 🟢 1, 3 unch. · avg +4.9% · Bytecode: 🟢 3, 1 unch. · avg +5.9%
intl.js — Interp: 6 unch. · avg -2.5% · Bytecode: 🔴 1, 5 unch. · avg -2.7%
iterators.js — Interp: 🟢 11, 31 unch. · avg +4.1% · Bytecode: 🟢 18, 24 unch. · avg +7.2%
json.js — Interp: 23 unch. · avg +0.8% · Bytecode: 🟢 2, 🔴 4, 17 unch. · avg +1.3%
jsx.jsx — Interp: 🔴 3, 18 unch. · avg -3.4% · Bytecode: 🟢 1, 🔴 3, 17 unch. · avg -5.3%
modules.js — Interp: 9 unch. · avg -0.4% · Bytecode: 9 unch. · avg +0.1%
numbers.js — Interp: 12 unch. · avg +1.9% · Bytecode: 🟢 1, 🔴 3, 8 unch. · avg -0.8%
objects.js — Interp: 7 unch. · avg +0.2% · Bytecode: 7 unch. · avg -5.7%
promises.js — Interp: 🟢 1, 🔴 2, 9 unch. · avg -1.1% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg +0.1%
property-access.js — Interp: 5 unch. · avg +0.4% · Bytecode: 5 unch. · avg +1.4%
regexp.js — Interp: 13 unch. · avg +0.9% · Bytecode: 13 unch. · avg -0.6%
strings.js — Interp: 19 unch. · avg -0.4% · Bytecode: 🔴 9, 10 unch. · avg -4.8%
temporal.js — Interp: 🔴 1, 5 unch. · avg +0.2% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg +1.0%
tsv.js — Interp: 9 unch. · avg -0.3% · Bytecode: 🟢 4, 🔴 1, 4 unch. · avg +3.6%
typed-arrays.js — Interp: 🟢 7, 🔴 4, 11 unch. · avg +5.8% · Bytecode: 🟢 3, 🔴 7, 12 unch. · avg -8.0%
uint8array-encoding.js — Interp: 🟢 1, 🔴 3, 14 unch. · avg -2.5% · Bytecode: 🟢 2, 🔴 5, 11 unch. · avg -8.8%
weak-collections.js — Interp: 🟢 6, 🔴 1, 8 unch. · avg +1.5% · Bytecode: 🟢 5, 🔴 3, 7 unch. · avg +21.3%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
JetStream 3 Performance Barometer
Geomean reference ratio: QuickJS 26.03×; Node.js 292.40×. 1.00× means aligned; values above 1.00× mean Goccia was proportionally slower after normalizing JetStream’s higher-is-better score. This is a directional barometer across runtimes with different goals, not a product ranking. Raw samples and failure details remain in the |
Contributor
AWFY Results
Geomean Ratios
14 pinned AWFY benchmarks. Medians from 5 interleaved samples per engine; raw JSON includes min/max/CV and is attached as the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+0 / -0 / timeout +2 / -2)New timeouts (2):
Resolved timeouts (2):
Steady-state failures and timeouts are non-blocking; PASS → non-timeout failure transitions fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
frostney
changed the title
frostney
marked this pull request as ready for review
July 24, 2026 18:59
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
coderabbitai
Bot
added
internal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.github/workflows/json5-test-bump.yml (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Node version used to generate the committed suite.
bunis set up explicitly, but the regeneration step runs the runner's defaultnode, so the committedupstream-parse.jsdepends on whatever Node ubuntu-latest ships. Since the generator's output (number formatting,JSON.stringifyescaping) is committed and diffed, add a pinnedactions/setup-nodestep for reproducibility.♻️ Proposed addition
- uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + + - uses: actions/setup-node@<pinned-sha> # v6 + with: + node-version: '22'Also applies to: 43-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/json5-test-bump.yml around lines 23 - 24, Add a pinned actions/setup-node step in the workflow before the regeneration commands, and configure it to use the Node version required for generating the committed suite. Keep the existing oven-sh/setup-bun step unchanged and ensure all regeneration steps use the pinned Node installation.source/app/compliance/Goccia.Compliance.pas (1)
141-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
StdErrStreamis unreachable dead state.
poStderrToOutPutmerges stderr intoOutput, soStdErrStreamis never written andResult.StdErrTextis always empty. Either drop the stream or usepoUsePipeswithoutpoStderrToOutPutand drainProcessRunner.Stderrseparately if separate capture is wanted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/app/compliance/Goccia.Compliance.pas` around lines 141 - 199, Update RunComplianceProcess to remove the unreachable StdErrStream state and its Result.StdErrText assignment, since poStderrToOutPut merges stderr into ProcessRunner.Output; alternatively, remove that option and drain ProcessRunner.Stderr separately if separate stderr capture is required. Keep the chosen stream-capture behavior consistent throughout the function.scripts/regenerate-json5-tests.js (1)
17-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUpstream code is loaded before the revision is verified.
require(path.join(suiteDir, "lib"))on Line 21 executes the checkout's module code before thegit rev-parsecheck on Line 28 can reject a wrong/unexpected revision, so the pin check provides no protection against running unintended upstream code. Verify first, then load. Validatingrevisionas 40-hex would also fail fast on a typo.♻️ Proposed reordering
const suiteDir = path.resolve(process.argv[2]); const revision = process.argv[3]; const outputPath = path.resolve(process.argv[4]); const testDir = path.join(suiteDir, "test"); -const realJSON5 = require(path.join(suiteDir, "lib")); const checkoutRevision = execFileSync( "git", ["-C", suiteDir, "rev-parse", "HEAD"], { encoding: "utf8" }, ).trim(); if (checkoutRevision !== revision) { throw new Error( `Suite revision mismatch: expected ${revision}, got ${checkoutRevision}`, ); } + +const realJSON5 = require(path.join(suiteDir, "lib"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/regenerate-json5-tests.js` around lines 17 - 32, Move the realJSON5 require in the regenerate flow to after the checkoutRevision comparison, so the expected revision is validated before any suite module code executes. Also validate that revision is a 40-character hexadecimal commit identifier before invoking git, while preserving the existing mismatch error for valid revisions..github/workflows/ci.yml (1)
204-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: guard against an empty compliance glob.
nullglobis still active from Line 165, so ifsource/app/compliance/*.dprever matches nothing this loop silently builds zero binaries — mirroring the explicit emptiness check used for./source/app/*.dprwould keep the failure at the build step rather than the staging step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 204 - 219, Add an explicit emptiness check for the compliance source glob before the build loop, matching the existing check for ./source/app/*.dpr. Ensure the workflow fails immediately when no files match, rather than silently skipping all compliance builds; keep the existing compilation loop unchanged for non-empty matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 496-507: The JSON5 revision validation in the CI step does not
affect the step status because its result is ignored before GocciaTestRunner
runs. Update the workflow around the pin check and test_runner invocation so a
failed grep for the expected revision immediately fails the step, while
preserving the existing test execution for matching pins.
- Around line 412-425: Update the “Run TOML 1.1.0 compliance suite” shell block
to enable strict error handling with set -euo pipefail before suite preparation,
so failures from git init, fetch, or checkout abort immediately and the runner
is not invoked.
In `@source/app/compliance/GocciaTOMLComplianceRunner.dpr`:
- Around line 428-438: Update the exception handling in the worker’s shown
try/except block so only EGocciaTOMLParseError writes the message and exits with
status 1; change the generic Exception handler to exit with status 2 while
preserving its error reporting. This ensures ExecuteCase classifies non-parse
failures as coInfrastructure.
- Around line 482-521: The compliance runner must fail fast when DiscoverCases
returns no cases. Immediately after discovering cases and before creating or
running TComplianceCoordinator, check whether the collection is empty, report
the discovery failure, set a nonzero Result, and exit through the existing
cleanup path without producing a successful report.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 204-219: Add an explicit emptiness check for the compliance source
glob before the build loop, matching the existing check for ./source/app/*.dpr.
Ensure the workflow fails immediately when no files match, rather than silently
skipping all compliance builds; keep the existing compilation loop unchanged for
non-empty matches.
In @.github/workflows/json5-test-bump.yml:
- Around line 23-24: Add a pinned actions/setup-node step in the workflow before
the regeneration commands, and configure it to use the Node version required for
generating the committed suite. Keep the existing oven-sh/setup-bun step
unchanged and ensure all regeneration steps use the pinned Node installation.
In `@scripts/regenerate-json5-tests.js`:
- Around line 17-32: Move the realJSON5 require in the regenerate flow to after
the checkoutRevision comparison, so the expected revision is validated before
any suite module code executes. Also validate that revision is a 40-character
hexadecimal commit identifier before invoking git, while preserving the existing
mismatch error for valid revisions.
In `@source/app/compliance/Goccia.Compliance.pas`:
- Around line 141-199: Update RunComplianceProcess to remove the unreachable
StdErrStream state and its Result.StdErrText assignment, since poStderrToOutPut
merges stderr into ProcessRunner.Output; alternatively, remove that option and
drain ProcessRunner.Stderr separately if separate stderr capture is required.
Keep the chosen stream-capture behavior consistent throughout the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 55a8bf33-e107-4d44-bd58-7033ca5dad06
📒 Files selected for processing (20)
.github/workflows/ci.yml.github/workflows/json5-test-bump.yml.github/workflows/toml-test-bump.ymlbuild.pasdocs/build-system.mddocs/built-ins-data-formats.mddocs/contributing/tooling.mddocs/interpreter.mddocs/testing.mdscripts/GocciaJSON5Check.dprscripts/GocciaTOMLCheck.dprscripts/regenerate-json5-tests.jsscripts/run_json5_test_suite.pyscripts/run_toml_test_suite.pyscripts/suite-bump-pin.tssource/app/compliance/Goccia.Compliance.passource/app/compliance/GocciaTOMLComplianceRunner.dprtests/built-ins/JSON5/upstream-parse.jstests/compliance/json5.pintests/compliance/toml-test.pin
💤 Files with no reviewable changes (4)
- scripts/GocciaJSON5Check.dpr
- scripts/GocciaTOMLCheck.dpr
- scripts/run_toml_test_suite.py
- scripts/run_json5_test_suite.py
…e-compliance-runners
Fail CI immediately on suite preparation errors, distinguish parser rejections from infrastructure failures, and reject array extension headers without crashing the TOML parser.
coderabbitai
Bot
added
spec compliance
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/built-ins/TOML/parse.js (1)
138-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover dotted-key assignment after an array.
These cases exercise regular-table and table-array parsing, but not the new
EnsureDottedKeyTablebranch insource/units/Goccia.TOML.pas(Lines [935-936]). Add a case such as:Suggested test
+ expect(() => + TOML.parse(` +items = [] +items.metadata = true +`), + ).toThrow(SyntaxError);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/built-ins/TOML/parse.js` around lines 138 - 149, Extend the TOML parser tests around the existing array/table-array cases to cover dotted-key assignment after an array, exercising the EnsureDottedKeyTable path in TOML.parse. Add a focused valid or invalid fixture matching the intended behavior, while preserving the current regular-table and table-array assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/built-ins/TOML/parse.js`:
- Around line 138-149: Extend the TOML parser tests around the existing
array/table-array cases to cover dotted-key assignment after an array,
exercising the EnsureDottedKeyTable path in TOML.parse. Add a focused valid or
invalid fixture matching the intended behavior, while preserving the current
regular-table and table-array assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af547358-3683-4844-ba5d-612f88abb0b5
📒 Files selected for processing (4)
.github/workflows/ci.ymlsource/app/compliance/GocciaTOMLComplianceRunner.dprsource/units/Goccia.TOML.pastests/built-ins/TOML/parse.js
🚧 Files skipped from review as they are similar to previous changes (2)
- source/app/compliance/GocciaTOMLComplianceRunner.dpr
- .github/workflows/ci.yml
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.
Summary
GocciaTestRunner; there is no JSON5-specific native runner or runtime wrapper.GocciaTestRunnerown JSON5 concurrency, per-file timeouts, aggregation, exit status, and JSON reporting.Closes #986
Testing
Additional verification:
./build.pas --clean tomlcompliancerunner testrunner./build/GocciaTestRunner tests --no-progress --output=/tmp/goccia-1047-review-interpreted.json— 11,664/11,664 passed./build/GocciaTestRunner tests --mode=bytecode --no-progress --output=/tmp/goccia-1047-review-bytecode.json— 11,664/11,664 passed./build/GocciaTestRunner tests/built-ins/TOML/parse.js tests/built-ins/JSON5/upstream-parse.js tests/built-ins/JSON5/stringify.js --jobs=8— 133/133 passed./build/GocciaTOMLComplianceRunner --suite-dir=<pinned-checkout> --jobs=8— 712/712 passed with zero infrastructure failures