emergent_testing: one normalized result for both runners - #1741
Conversation
Second step of `todo/share-browser-console-runner.md`, taken before the `sandbox` step it was originally sequenced after. Sharing `sandbox` means merging the browser's ~150 lines of `Symbol.species` machinery with `fjs t`, which has no equivalent -- that answers the cross-realm question `todo/imports-promises-realms.md` marks as open investigation, and a port is the wrong place to answer it. This step needs no such decision. `TestResult` and `testResult` normalize what a leaf is called, whether it passed, and how long it took. The browser derived all three inline at four sites; `fjs t` derived them again on its way to a printed line. The throw expectation is now applied through the same `invert` in both, so "did this leaf pass" is decided in one place rather than three. A thrown *value* is deliberately not in the record. Describing one needs the value, a serializable report cannot carry one, and the two hosts describe it differently for good reasons -- `fjs t` prints it to a terminal, the browser reads `message`/`stack` for a wire. So the description stays with each host, which is the shape of an extension point rather than an omission, and the type says so. Two differences that shows up are recorded in the todo, not fixed here: `fjs t` keeps no stack for a failure, and for a `throw` proof that returns cleanly the two report different messages while agreeing on the status. `fjs t`'s output is byte-identical. Proofs: `testResult.*` for the rule itself, and `normalizedResultMatchesTheSharedOne` / `expectedThrowStatusMatchesTheSharedOne` comparing real browser results against the shared function rather than against literals. Both mutants are killed -- forcing every status to `passed` fails 14, mangling the name fails 8. Changelog: - `emergent_testing`: `TestResult` and `testResult` normalize one leaf's outcome -- its identity, whether it passed once the `throw` expectation has been applied, and how long it took -- so `fjs t` and the browser runner decide those the same way. The browser's result type is now `TestResult` plus its own `message`/`stack`; `fjs t`'s output is unchanged
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb8913d331
ℹ️ 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".
| * todo tracks rather than in this change. | ||
| * | ||
| * @typedef {{ readonly module: string, readonly path: string, readonly name: string, readonly status: string, readonly duration: number, readonly message?: string, readonly stack?: string }} _BrowserTestResult | ||
| * @typedef {TestResult & { readonly message?: string, readonly stack?: string }} _BrowserTestResult |
There was a problem hiding this comment.
When a source fails to link or its proof export cannot be enumerated, moduleFailure still inserts an infrastructure/module-level record into BrowserTestReport.results, but this typedef now promises that every entry is a TestResult—documented as a leaf whose name comes from fmtImport and whose duration is that leaf's runtime. Those records instead use the raw source as name and, during loading failures, the whole loading duration, so consumers cannot safely rely on the new public invariants. Model module failures as a separate union variant rather than intersecting every browser result with TestResult.
AGENTS.md reference: AGENTS.md:L23-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right about the defect, and it is one this branch introduced — but I have fixed the documentation rather than the shape, because the shape is a decision that is not mine to take here.
The over-promise is real and worse than one field. I wrote that name is "the identity fmtImport gives the leaf" and path is "the key chain within that module's proof export", while moduleFailure produces a TestResult whose name is the raw source, whose path is empty, and whose duration is whatever was being measured when the failure happened. Three fields meant less than the comment claimed, and the claim is new in this branch. TestResult now says what is actually true: name is what ran, not which leaf ran; path is empty when the outcome is not a leaf's; duration is the leaf's execution or whatever the runner was measuring.
On the union itself: a module failure is in results deliberately, and the existing comment above the rejection path says why — a report whose totals disagreed with its results would tell an automated consumer the suite was empty rather than broken. Splitting it into a variant changes the public report and that totals story, so it is a report-shape change, not a typing cleanup. This PR shares one decision (what a leaf's status and identity are) and deliberately touches nothing else; the last attempt at this issue was reverted for bundling exactly this kind of adjacent improvement into a port.
So it is recorded instead. The report shape now has three questions open at once — whether path survives now that name exists, whether a report declares the root its module keys are relative to, and whether a module-level failure belongs in a variant of its own — and todo/share-browser-console-runner.md now says they should be settled together. Each is small alone; answering one without the others is how a report shape ends up carrying three half-decisions.
Generated by Claude Code
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 94a1f00 | Commit Preview URL Branch Preview URL |
Aug 27 2026, 05:41 PM |
The new type's documentation over-promised. It described `name` as the identity `fmtImport` gives a leaf and `path` as the key chain within a module's `proof` export, while `moduleFailure` -- a source that will not link, or a `proof` export that cannot be enumerated -- produces a `TestResult` whose name is the raw source, whose path is empty and whose duration is whatever was being measured when it failed. Three fields meant less than the comment claimed, and the claim was written in this branch. Fixes the documentation rather than the shape. A module-level failure is counted deliberately: a report whose totals disagreed with its `results` would tell an automated consumer the suite was empty rather than broken. Turning it into a separate variant changes the public report and the totals story, which is a report-shape decision -- and the report shape now has three of those open at once (`path`'s redundancy, which root a report declares, and this), which the todo now says should be settled together. No behaviour change; the suite and `fjs t`'s output are unchanged.
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. Verified at cb8913d33: npm test 3477/3477 exit 0, tsc --noEmit exit 0 (main 8609666ea: 3472/3472). Despite -49 lines, the leaf diff is +5 and 0 removed — nothing lost.
Both runners really do produce the shared shape rather than converting at a boundary: defaultReporter.result calls testResult(file, path, r) in module.f.mjs, and browser.mjs's runOne → leaf() calls the same imported testResult. Mutation-proven both ways — having the browser drift (name: 'WRONG-NAME-' + t.module) is KILLED by normalizedResultMatchesTheSharedOne and nameMatchesTheConsoleRunner, and forcing testResult status to 'passed' is KILLED with 14 failures, reproducing the table in the PR body independently.
Given the #1737 revert I checked this is not the same machinery returning: no effects/common, no second interpreter, no shared skeleton — runModuleMap predates the revert. This is a narrower mechanism, one shared normalization function.
types.ts is purely additive (TestStatus, TestResult), so not marking it breaking is right; Changelog: is literal and byte-identical to the entry file; and share-browser-console-runner.md frames the reverted attempt as history rather than live code, with only step 2 checked.
One caveat worth recording, and it is the PR's own documented one rather than a defect: under the status-forcing mutant the per-line terminal output still prints ok, because the label derives from testResult.status while the aggregate count and exit code come from the raw sandbox tuple in runModule's one(). The gate is sound; only the per-line label would mislead.
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approval still stands. Verified at 39c9c9d6b: npm test 3477/3477 exit 0, tsc --noEmit exit 0 (main 8609666ea: 3472/3472). browser.mjs +8 is comment-only, and types.ts +25/-3 changes no field and narrows no union — doc rewording plus a new block on duration, so still not breaking, and the changelog still matches the body verbatim.
I tested the new promise rather than reading it: driving runBrowserProofs with a proof export whose getter throws on enumeration, using a source with separators and spaces (a/nested::name with spaces.mjs), gives exactly {module: <source>, path: '', name: <source>, status: 'failed'} — the documented "named by whatever it does know, which for a module is its source" and "path empty when the outcome is not a leaf's" both hold.
One note for a future reader, pre-existing and not caused by this diff: the two runners are not symmetric for non-leaf faults. The same unenumerable proof export that the browser reports as a TestResult makes fjs t throw synchronously, and a genuinely unimportable module aborts the whole console run (exit 1, no TestResults emitted at all, including for modules that would have passed). The new doc is correctly hedged — "a runner may also report…" — but a consumer reading only the TestResult doc could reasonably infer a symmetry that does not exist. Worth a sentence naming the console runner's behaviour, whenever that area is next touched.
`TestResult.name` documents that a runner may report an outcome with no leaf behind it, which is true of the browser and reads as though it were true of both. It is not. Measured with two modules in the tree and only the first hostile: the browser records one failed result and runs the rest, while `fjs t` exits on an uncaught throw and the second module's passing proofs are never reported at all. A consumer reading only the type could reasonably infer a symmetry that does not exist, so the type now says which runner tolerates this and points at `todo/hostile-proof-values.md`, where closing the gap needs an operation the shared traversal does not have. That todo gains the measurement. Comment-only; no field changes, no behaviour change.
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approval still stands, and the asymmetry is now stated accurately — I re-measured rather than reading it. With two synthetic modules, a hostile-getter root plus a passing sibling: fjs t throws uncaught in collectTests → Object.entries, prints a raw stack, emits zero TestResults and no summary, and the sibling's passing proofs are never touched; runBrowserProofs on the same tree gives {tests: 2, passed: 1, failed: 1}. That matches the new text in both places, including the part that is easy to leave out — that the console run takes down modules that would have passed.
Placement is right too: the types.ts sentence stands on its own for a consumer importing by path (it names the mechanism, not just the fact), and hostile-proof-values.md keeps Status: open and frames the paragraph as measured evidence for open work rather than an accepted end state.
Gates at 94a1f0003: npm test 3477/3477 exit 0, tsc --noEmit exit 0 (main 8609666ea: 3472/3472). The types.ts +9 is entirely inside the existing JSDoc block — no signature or type changed — so the changelog is unaffected and still matches the body item-for-item.
3 participants
Step 2 of
emergent_testing/todo/share-browser-console-runner.md, after#1738.
Why this and not
sandboxThe plan had "one
sandbox" second. That ordering was wrong and the step listis corrected here. Sharing
sandboxmeans merging the browser's ~150 lines ofSymbol.speciesmachinery (runPromise,subscribe,speciesFails) withfjs t, which has no equivalent — so it answers the cross-realm promisequestion that
todo/imports-promises-realms.mdmarks as open investigation.That is a decision to take deliberately, not a side effect of moving code; it is
how the reverted attempt lost a defence nobody chose to lose. It is now step 3
and marked blocked-on-a-decision.
This step needs no such decision.
The change
TestResultandtestResultnormalize one leaf's outcome: its identity,whether it passed, and how long it took.
The browser derived all three inline at four sites.
fjs tderived them againon its way to a printed line (
s === 'ok' ? …). The throw expectation is nowapplied through the same
invertin both, so "did this leaf pass" is decidedin one place instead of three.
The browser's result type is now
TestResult & { message?, stack? }.What is deliberately not in it
A thrown value. Describing one needs the value; a serializable report cannot
carry one; and the two hosts describe it differently for reasons their hosts
impose —
fjs tprints it to a terminal, the browser readsmessage/stackfor a wire. So the description stays with each host. That is the shape of an
extension point rather than an omission, and the type says so where a reader
will look.
What this revealed, recorded rather than fixed
With the status shared, two description differences became visible. Both are in
the todo, neither is touched here:
fjs tkeeps no stack for a failure; the browser keeps one.throwproof that returns cleanly,fjs treports the returned valueas the error while the browser reports the fixed string
Expected the proof to throw. They agree on the status — which is what this step shares — and differin the message.
One more thing worth knowing:
testResultnow sits insidefjs t's ownreporting path, so a defect in it can mislabel the very failures it causes. The
pass/fail counts come from the walk's state rather than from the reporter, so
they stay honest and the summary still reports the failures — but the per-test
labels do not. Noted in the todo.
Verification
npx tscclean,fjs t3477/3477,npm run cov100% lines, branches andfunctions.
fjs t's output is byte-identical.Five new proofs:
testResult.passes/.fails/.namesTheLeaffor the rule,and
normalizedResultMatchesTheSharedOne/expectedThrowStatusMatchesTheSharedOnecomparing real browser results againstthe shared function rather than against literals. Mutation-checked:
statusforced to'passed'namemangledChangelog:
emergent_testing:TestResultandtestResultnormalize one leaf'soutcome — its identity, whether it passed once the
throwexpectation hasbeen applied, and how long it took — so
fjs tand the browser runner decidethose the same way. The browser's result type is now
TestResultplus its ownmessage/stack;fjs t's output is unchanged🤖 Generated with Claude Code
https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
Generated by Claude Code