testing: align CI gates and cleanup with the testing strategy#368
Merged
RtlZeroMemory merged 2 commits intomainfrom Apr 14, 2026
Merged
testing: align CI gates and cleanup with the testing strategy#368RtlZeroMemory merged 2 commits intomainfrom
RtlZeroMemory merged 2 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughUpdates CI to explicit fast/full/nightly/release gates, adds a new release-critical job surfaced across workflows, consolidates numerous constraint tests into in-file Node Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions\n(workflow)"
participant Repo as "Repo (checkout)"
participant Node as "Node/npm\n(scripts/testing-slices.mjs)"
participant Tests as "Test Suites / Native Build"
GH->>Repo: checkout (no submodules)
GH->>Node: setup Node / npm
Node->>Repo: read scripts/testing-slices.mjs & package.json
GH->>Node: run `npm run test:progress` (append to $GITHUB_STEP_SUMMARY)
GH->>Node: run `npm run test:release-critical`
Node->>Tests: run package slices (node --test / npm scripts)
Node->>Tests: run terminal/native slices (build native, terminal e2e)
Tests-->>Node: exit status
Node-->>GH: propagate exit status (fail/pass)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Apr 14, 2026
![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: a72bcbd700
ℹ️ 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".
Comment on lines
+251
to
+252
| needs: [quality-gates, template-smoke, matrix-config] | ||
| name: node ${{ matrix.node-version }} / ${{ matrix.os }} | ||
| name: full gate / node ${{ matrix.node-version }} / ${{ matrix.os }} |
There was a problem hiding this comment.
In .github/workflows/ci.yml, the new release-critical-suites job is labeled as part of the fast gate, but the expensive full-gate jobs (template-smoke, ci, and bun) still only depend on quality-gates. That means the matrix and Bun runs can start (and consume runner time) even when npm run test:release-critical is already failing, so the new fast gate cannot short-circuit the slower fan-out path as intended.
Useful? React with 👍 / 👎.
Comment on lines
145
to
146
| name: release gate / node ${{ matrix.node-version }} / ${{ matrix.os }} | ||
| needs: [quality-preflight] |
There was a problem hiding this comment.
In .github/workflows/release.yml, release-ci still depends only on quality-preflight even after adding release-critical-suites. As a result, the 3x3 release matrix can run in parallel with (or before completion of) the release-critical slice instead of being gated by it, which defeats the stated “release-critical first” sequencing and wastes release runner minutes when the slice fails.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/core/src/__tests__/constraints/graph.test.ts (1)
9-37: Extract the runtime fixture builders once.
runtimeNodeandboxWithIdare now duplicated here and inpackages/core/src/__tests__/constraints/resolver.test.ts, and both hand-shape internalRuntimeInstance/VNodeobjects. A shared helper would reduce drift the next time those internals move.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/constraints/graph.test.ts` around lines 9 - 37, The tests duplicate internal fixture builders runtimeNode and boxWithId; extract these into a single shared test helper module and update both tests to import and use them to avoid drift: create a new helper file exporting runtimeNode and boxWithId (preserving their signatures and how they construct RuntimeInstance/VNode), replace the local definitions in graph.test.ts and resolver.test.ts with imports from that module, and ensure any types (RuntimeInstance, VNode, InstanceId) are imported or re-exported so both test files compile unchanged.scripts/testing-slices.mjs (1)
152-170: Reject unexpected arguments instead of failing open.
summaryreturns before validating trailing args, andruncurrently ignores arbitrary--flagextras. That makes script typos succeed silently in CI/npm entrypoints instead of surfacing as configuration errors.♻️ One way to tighten the parser
function parseArgs(argv) { const [command, value, extra] = argv; const json = argv.includes("--json"); if (command === "summary") { + const unexpected = argv.slice(1).filter((arg) => arg !== "--json"); + if (unexpected.length > 0) { + throw new Error(`testing-slices: unexpected extra argument: ${unexpected[0]}`); + } return { command, json }; } if (command === "run") { if (typeof value !== "string" || value.length === 0) { throw new Error("testing-slices: run requires a slice name"); } - if (typeof extra === "string" && !extra.startsWith("--")) { + if (typeof extra === "string") { throw new Error(`testing-slices: unexpected extra argument: ${extra}`); } return { command, slice: value }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/testing-slices.mjs` around lines 152 - 170, The parseArgs function currently returns early for "summary" and doesn't validate trailing args, and "run" ignores arbitrary extra flags; update parseArgs to validate the extra token (extra) and --json handling (json) for both branches: for "summary" ensure no unexpected extra arguments (allow only an optional "--json"), set json accordingly, and throw on any other extra; for "run" ensure extra is either undefined or the "--json" flag (set json in the returned object) and throw for any other extra (use the existing variables command, value, extra, json and return objects like { command, json } or { command, slice: value, json }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 183-208: The fast gate job release-critical-suites isn't being
waited on by the downstream jobs template-smoke, ci, and bun (they only list
quality-gates), so failures in release-critical-suites won't block the full-gate
fan-out; update each downstream job (template-smoke, ci, bun and the other
occurrences mentioned) to include release-critical-suites in their needs array
(or alternatively add a new aggregate fast-gate job and have those jobs depend
on that aggregate) so the fast/full split is enforced and those jobs only start
after release-critical-suites completes.
In `@docs/dev/testing.md`:
- Around line 88-115: Update the "CI Integration" section to match the gate
model described in "CI Gates": replace references to the old single-flow
pipeline and any mention of "npm install" with the current multi-gate flow and
the use of "npm ci" everywhere, and add a clear note about the separate
release-critical job (fast/full/nightly/release split) and how it maps to the
named release-critical suites; ensure "CI Integration" explicitly documents the
split gate behavior and the existence of the release-critical job so the two
sections are consistent.
In `@scripts/__tests__/testing-slices.test.mjs`:
- Around line 1-2: The import statements are out of the repository's configured
order; swap them so they are sorted as the formatter expects (place the
"node:assert" import before "node:child_process"), i.e., import { strict as
assert } from "node:assert" then import { execFileSync } from
"node:child_process" so the symbols assert and execFileSync appear in the
correct order for the formatter.
In `@scripts/__tests__/testing-workflows.release-critical.test.mjs`:
- Around line 18-20: The three asserts use loose regexes that can match
unintended lines (e.g., test:release-critical:terminal) and anywhere in the
workflow; update the checks in
scripts/__tests__/testing-workflows.release-critical.test.mjs to first extract
or narrow to the release-critical-suites job/block (the
'release-critical-suites' block) and then use anchored full-line regexes (start
^ and end $) for the exact command lines (e.g., the npm run
test:release-critical and npm run test:progress >> "$GITHUB_STEP_SUMMARY") so
they only match the intended lines inside that block; apply the same tightening
to the other occurrences referenced (the asserts at 26-28 and 34-36).
In `@scripts/testing-slices.mjs`:
- Line 4: Run the project formatter/import organizer on
scripts/testing-slices.mjs to fix style and import ordering errors; specifically
normalize the import statement that currently uses "import { existsSync,
readdirSync, readFileSync } from \"node:fs\";" and run the repository's
formatter (e.g., prettier/organize-imports or the repo's npm/yarn format script)
so the sections around the other reported ranges (uses of existsSync,
readdirSync, readFileSync at the other spots) are adjusted to match repository
rules and clear CI.
---
Nitpick comments:
In `@packages/core/src/__tests__/constraints/graph.test.ts`:
- Around line 9-37: The tests duplicate internal fixture builders runtimeNode
and boxWithId; extract these into a single shared test helper module and update
both tests to import and use them to avoid drift: create a new helper file
exporting runtimeNode and boxWithId (preserving their signatures and how they
construct RuntimeInstance/VNode), replace the local definitions in graph.test.ts
and resolver.test.ts with imports from that module, and ensure any types
(RuntimeInstance, VNode, InstanceId) are imported or re-exported so both test
files compile unchanged.
In `@scripts/testing-slices.mjs`:
- Around line 152-170: The parseArgs function currently returns early for
"summary" and doesn't validate trailing args, and "run" ignores arbitrary extra
flags; update parseArgs to validate the extra token (extra) and --json handling
(json) for both branches: for "summary" ensure no unexpected extra arguments
(allow only an optional "--json"), set json accordingly, and throw on any other
extra; for "run" ensure extra is either undefined or the "--json" flag (set json
in the returned object) and throw for any other extra (use the existing
variables command, value, extra, json and return objects like { command, json }
or { command, slice: value, json }).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac741ff5-cb06-4723-a723-6dc9af17662d
📒 Files selected for processing (18)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.ymldocs/dev/testing.mdpackage.jsonpackages/core/src/__tests__/constraints/graph.test.impl.tspackages/core/src/__tests__/constraints/graph.test.tspackages/core/src/__tests__/constraints/helpers.test.impl.tspackages/core/src/__tests__/constraints/helpers.test.tspackages/core/src/__tests__/constraints/integration.test.impl.tspackages/core/src/__tests__/constraints/integration.test.tspackages/core/src/__tests__/constraints/parser.test.impl.tspackages/core/src/__tests__/constraints/parser.test.tspackages/core/src/__tests__/constraints/resolver.test.impl.tspackages/core/src/__tests__/constraints/resolver.test.tsscripts/__tests__/testing-slices.test.mjsscripts/__tests__/testing-workflows.release-critical.test.mjsscripts/testing-slices.mjs
💤 Files with no reviewable changes (5)
- packages/core/src/tests/constraints/helpers.test.impl.ts
- packages/core/src/tests/constraints/parser.test.impl.ts
- packages/core/src/tests/constraints/resolver.test.impl.ts
- packages/core/src/tests/constraints/integration.test.impl.ts
- packages/core/src/tests/constraints/graph.test.impl.ts
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.
Scope
docs/dev/testing.mdrequiring new tests to declare contract source and fidelityCI gate changes
ci.ymlnow names the fast gate (fast gate / quality,fast gate / release-critical suites) and the full gate (full gate / create-rezi smoke,full gate / node <version> / <os>,full gate / bun / ubuntu-latest)nightly.ymlruns the scheduled nightly gate with release-critical package and terminal slices, template smoke, the 3x3 Node/OS matrix, and Bunrelease.ymlnow repeats the release-critical package and terminal slices before the release matrixRelease-critical visibility changes
scripts/testing-slices.mjsas the shared release-critical suite catalog and progress-summary source$GITHUB_STEP_SUMMARYTests retired
.test.tsnames:Working-agreement location
docs/dev/testing.mdTest commands run
node --test scripts/__tests__/testing-slices.test.mjs scripts/__tests__/testing-workflows.release-critical.test.mjs scripts/__tests__/ci-workflow-linux-reduced-e2e.test.mjsnpm cinpm run buildnpm run test:progressnpm run test:release-criticalnpm run build:nativenpm run test:release-critical:terminalKnown limitations
Closes #355
Closes #356
Closes #357
Closes #358
Closes #359
Closes #325
#Closes #355
#Closes #356
#Closes #357
#Closes #358
#Closes #359
#Closes #325
Summary by CodeRabbit
New Features
Documentation
Tests
Chores