Make targeted mutation tests faster and fail safely - #1854
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe mutation workflow now reuses disposable static-asset builds, maps direct tests to source files, rebuilds shared mutant test state for integration tests, records phase timings, and updates reporting, tests, cleanup, and documentation. ChangesMutation workflow overhaul
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 5
🤖 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 `@scripts/mutation/runner.ts`:
- Around line 412-455: Update the mutant execution flow around runTestStages to
establish a single deadline before any direct tests, createMutantTestState, or
integration tests run. Derive each stage’s AbortSignal timeout from the
remaining time until that deadline, and pass those remaining budgets to runTests
and the state-creation path so the combined stages cannot exceed
perMutantTimeout.
In `@scripts/mutation/summary.ts`:
- Around line 149-160: Update the successful summary paths that return early
when effective === 0 in both formatters to include the collected timing output:
append timingLines(s) in the text formatter and timingTable in the table
formatter. Preserve the existing all-ignored summary content while ensuring
phase timings are shown.
In `@scripts/mutation/test-map.ts`:
- Around line 14-24: Normalize path separators consistently in testPrefix and
ownsTest before comparing ownership paths, converting both slash types to “/” so
mixed-separator Windows paths match correctly. Preserve the existing prefix and
descendant checks, and add a regression test covering mixed separators such as
“test\shared/a” versus “test/shared/a.test.ts”.
In `@scripts/test-harness.ts`:
- Around line 72-76: Update the cleanup function so stripe-mock, test-state,
staticAssets.cleanup(), and generated-file removal run independently, ensuring
one failure does not skip later cleanup. In the generated loop, suppress only
Deno not-found errors and propagate permission or other I/O failures. Preserve
and report unexpected failures after all cleanup steps have been attempted,
without empty catches or catch-and-continue handling.
In `@test/scripts/mutation-summary.test.ts`:
- Around line 67-93: Split the combined test around summarize,
formatSummaryLines, and writeStepSummary into three independent test cases, each
asserting only one production function’s behavior. Preserve the shared timing
fixtures and expected aggregation data, while keeping console-format assertions
in the formatSummaryLines test and Markdown-output assertions in the
writeStepSummary test.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: af22b566-2aeb-4f16-a2c2-32dd44e7f14f
📒 Files selected for processing (19)
AGENTS.mdscripts/build-static-assets.tsscripts/edge-bundle-lib.tsscripts/mutation/assets.tsscripts/mutation/build-test-state.tsscripts/mutation/phases.tsscripts/mutation/runner.tsscripts/mutation/state-graph.tsscripts/mutation/summary.tsscripts/mutation/test-map.tsscripts/mutation/test-state.tsscripts/precommit-mutation.tsscripts/precommit/mutation-step.tsscripts/test-harness.tstest/scripts/mutation-ignore.test.tstest/scripts/mutation-phases.test.tstest/scripts/mutation-summary.test.tstest/scripts/mutation-test-map.test.tstest/scripts/mutation-test-state.test.ts
💤 Files with no reviewable changes (1)
- scripts/mutation/assets.ts
![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: 1902013226
ℹ️ 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".
stefan-burke
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/test-harness.ts (1)
68-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRollback generated assets when static-asset setup fails.
CSS and successful bundles may already be written when another build rejects. Because
setupStaticAssetsnever returns, its cleanup is not registered and those generated files remain.Wrap the build call and invoke
failAfterCleanupswithgenerated.map(removeIfPresent)on failure. Add a regression test for partial build failure.As per coding guidelines, every bug fix requires a regression test reproducing the original failure.
🤖 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/test-harness.ts` around lines 68 - 75, The setupStaticAssets build flow must clean up generated files when buildStaticAssets fails. Wrap the build call in failure handling and invoke failAfterCleanups with generated.map(removeIfPresent), while preserving normal success behavior; add a regression test covering partial asset generation followed by a rejected build.Source: Coding guidelines
test/scripts/mutation-phases.test.ts (1)
68-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the complete integration-only contract.
The callback ignores
files, and the timing assertion checks only the first phase. Capture both callback arguments and compare the complete result so incorrect file forwarding, extra timings, or a lost duration cannot survive.As per coding guidelines, tests must use strong mutation-resistant assertions.
🤖 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 `@test/scripts/mutation-phases.test.ts` around lines 68 - 77, Strengthen the test “runs integration tests directly when there are no direct tests” by capturing both callback arguments from runTestStages and asserting the files argument is the expected integration file and the phase is “integration-tests”. Compare the complete result, including status, detectedBy, and the exact single timing entry with its durationMs and phase, so extra timings or lost values are detected.Source: Coding guidelines
🤖 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 `@scripts/build-static-assets.ts`:
- Around line 16-19: Remove the StaticAssetBuild and StaticBundle re-export from
the build-static-assets module, then update all internal callers to import those
types directly from ./static-assets/session.ts. Do not add an alias,
compatibility wrapper, or replacement re-export.
In `@scripts/mutation/phases.ts`:
- Around line 31-34: Update TestStageResult and the state-creation handling in
evaluate.ts so the stage result preserves and propagates the actual detectedBy
source, including test-state, instead of relabeling it as integration-tests.
Ensure timeout outcomes report no detection rather than a detector. Add
regression tests covering both failed and timed-out state creation.
In `@scripts/mutation/test-map.ts`:
- Around line 29-32: Update isIntegrationTest and the surrounding test-map
pipeline to use the shared `#fp` curried combinators instead of native some, map,
and filter calls. Replace the mutating sort with the project’s non-mutating
ordering helper, preserving the existing filtering, mapping, and ordering
behavior.
In `@scripts/static-assets/session.ts`:
- Around line 79-94: Update the rebuild and restore handlers around builtBundle
so all selected operations settle before either handler completes: use
Promise.allSettled for context.rebuild calls and aggregate restore writes with
runCleanups or an equivalent mechanism that waits for every write while
preserving failure reporting. Add deterministic regression coverage where one
selected context fails and another completes later, verifying restoration does
not race with the late operation.
In `@scripts/test-harness.ts`:
- Around line 226-239: Update the test harness setup around STRIPE_MOCK_HOST and
STRIPE_MOCK_PORT to capture each variable’s pre-existing value before
overwriting it. Add a teardown task alongside the existing cleanup functions
that restores those values when present and deletes them when previously unset,
preserving isolation across harness runs.
In `@test/scripts/mutation-execution.test.ts`:
- Around line 45-53: Update the STRIPE_MOCK_PORT restoration logic in the test’s
finally block to distinguish an unset variable from an existing empty value.
Restore previousPort whenever it is not undefined, and delete the environment
variable only when it was originally absent.
In `@test/scripts/mutation-file-plan.test.ts`:
- Around line 18-40: The test around createFilePlan must verify that
result.assets.restore forwards the affected bundle to the rebuilder. Track
restore calls and their arguments in the rebuilder mock, invoke the restoration
hook, and add a mutation-resistant assertion confirming exactly one call with
bundle.
---
Outside diff comments:
In `@scripts/test-harness.ts`:
- Around line 68-75: The setupStaticAssets build flow must clean up generated
files when buildStaticAssets fails. Wrap the build call in failure handling and
invoke failAfterCleanups with generated.map(removeIfPresent), while preserving
normal success behavior; add a regression test covering partial asset generation
followed by a rejected build.
In `@test/scripts/mutation-phases.test.ts`:
- Around line 68-77: Strengthen the test “runs integration tests directly when
there are no direct tests” by capturing both callback arguments from
runTestStages and asserting the files argument is the expected integration file
and the phase is “integration-tests”. Compare the complete result, including
status, detectedBy, and the exact single timing entry with its durationMs and
phase, so extra timings or lost values are detected.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: f8be2d02-889f-4c6d-9178-d73baee47ca8
📒 Files selected for processing (25)
AGENTS.mdscripts/build-static-assets.tsscripts/cleanup.tsscripts/mutation/evaluate.tsscripts/mutation/execution.tsscripts/mutation/phases.tsscripts/mutation/run-file.tsscripts/mutation/runner.tsscripts/mutation/summary.tsscripts/mutation/test-map.tsscripts/mutation/test-state.tsscripts/precommit-mutation.tsscripts/precommit/mutation-step.tsscripts/static-assets/session.tsscripts/test-harness.tstest/scripts/cleanup.test.tstest/scripts/mutation-evaluate.test.tstest/scripts/mutation-execution.test.tstest/scripts/mutation-file-plan.test.tstest/scripts/mutation-phases.test.tstest/scripts/mutation-test-map.test.tstest/scripts/mutation-test-state.test.tstest/scripts/static-asset-build.test.tstest/test-utils/command-capture.tstest/test-utils/test-state.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/mutation/test-map.ts`:
- Around line 59-62: Update the ownership classification flow in the test-map
generation logic so explicit integration/e2e test paths are recognized and
placed in integrationTestFiles before the owner calculation assigns direct
ownership, including when a mirrored source path such as src/integration/app.ts
is selected. Add a regression test covering src/integration/app.ts with
test/integration/app.test.ts and verify the test is classified as integration
rather than receiving an owner.
In `@scripts/static-assets/output-rollback.ts`:
- Around line 13-23: Update the rollback flow around outputFiles and
failAfterCleanups to snapshot the contents of every pre-existing output before
task() runs, then restore those contents during failure cleanup while retaining
removal of newly created files. Add or update the regression test in the
existing static-asset build failure case to overwrite the existing asset and
verify its original contents are restored.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: bdb70d1b-e2cc-41a8-b4f5-1387da23f464
📒 Files selected for processing (18)
scripts/build-static-assets.tsscripts/mutation/evaluate.tsscripts/mutation/phases.tsscripts/mutation/runner.tsscripts/mutation/summary.tsscripts/mutation/test-map.tsscripts/static-assets/output-rollback.tsscripts/static-assets/session.tsscripts/test-environment.tsscripts/test-harness.tstest/scripts/mutation-evaluate.test.tstest/scripts/mutation-execution.test.tstest/scripts/mutation-file-plan.test.tstest/scripts/mutation-phases.test.tstest/scripts/mutation-summary.test.tstest/scripts/mutation-test-map.test.tstest/scripts/static-asset-build.test.tstest/scripts/test-environment.test.ts
Member
Author
|
Review-only comment: the integration-only phase test is strengthened in 82c32a7. It captures both callback arguments, verifies the exact integration file and phase, and compares the complete result including status, detector, and the single timing entry. |
Member
Author
|
Review-only comment: static asset setup rollback is fixed in 82c32a7 and strengthened in 51966e0. |
This was referenced Jul 18, 2026
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
Test layout
A mutable source must have a direct test at the matching path under
test/. Only files undertest/integration/andtest/e2e/may be unmatched. This makes the mutation run point out tests that need to move while their assertions are strengthened.Performance
Incremental browser rebuilds averaged about 39ms. Mutant database state builds averaged about 159ms.
Structure
The 919-line mutation runner is now split into focused files for orchestration, execution, mutant evaluation, and per-file progress. The largest is under 400 lines. Static asset sessions and cleanup are also shared production mechanisms with direct tests.
Verification
deno task test:quality-auditwas reviewed; it found no new weak-assertion warning in these tests.deno task precommitpasses, including type checking, lint, zero duplication, full coverage, all tests, copy checks, and the edge build.Summary by CodeRabbit