test: harden and slim local test workflow - #51
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR separates CI build into its own job, implements workspace-aware test discovery and sharding, expands Vitest multi-project configs and computed worker pooling, adds integration and browser-smoke tests, increases unit/test-infra coverage, standardizes git test setup, and updates testing documentation and inventories. ChangesTesting Suite Infrastructure & CI Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/test-changed.mjs (1)
61-106: ⚡ Quick winSwitch workspace discovery to a real YAML+glob parser to handle pnpm workspace patterns correctly.
readWorkspacePatterns()/expandWorkspacePattern()only handle bare paths and the/*suffix. pnpm-workspace.yaml supports negation patterns (prefixed with!, e.g.,'!/test/') and broader glob patterns (e.g.,'components/'matching nested subdirectories), which this implementation silently skips. This can cause packages to be unexpectedly excluded from both changed-test selection and CI sharding.🤖 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-changed.mjs` around lines 61 - 106, readWorkspacePatterns and expandWorkspacePattern are too naive; replace the ad-hoc parsing with a YAML loader and a glob resolver so pnpm-workspace.yaml patterns (including negations like '!/test/' and nested globs like 'components/**') are handled correctly. In readWorkspacePatterns, read and parse pnpm-workspace.yaml using a YAML parser (e.g., js-yaml) and return the raw patterns array from the packages key; in expandWorkspacePattern, use a glob library (e.g., fast-glob) rooted at projectRoot to resolve each pattern to package.json paths (respecting negations and nested matches) instead of only handling literal paths and trailing "/*". Ensure functions named readWorkspacePatterns, expandWorkspacePattern and variables projectRoot/pattern are updated to use these libraries and preserve the final result as an array of existing package.json file paths.
🤖 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 `@packages/dashboard/scripts/browser-layout-smoke.mjs`:
- Around line 25-43: The current loadDashboardCss() function rebuilds the
stylesheet cascade by alphabetizing files from componentCssRoot which can
diverge from runtime import order; instead have loadDashboardCss() read the
emitted client CSS (or otherwise reuse the app's existing single source of truth
that preserves runtime order) rather than sorting componentEntries — locate
loadDashboardCss, stop sorting componentEntries (or remove the componentCssRoot
walk entirely) and instead load the compiled/emitted CSS bundle (or the build
artifact that preserves import order) using appRoot/dashboardRoot references so
the smoke test uses the actual runtime cascade.
- Around line 280-321: The launchBrowser() DevTools-handshake promise can reject
and leave the spawned Chrome process and temporary userDataDir (and the fixture
HTTP server started in main()) running; update launchBrowser() to ensure any
error path cleans up by killing the spawned browser process and removing the
userDataDir before rejecting (e.g., install a rejection/cleanup handler that
calls browser.kill() and removes the temp dir), and update main() so it either
awaits launchBrowser() before starting the fixture server or registers a cleanup
path that will stop the server if launchBrowser() later fails; reference
launchBrowser, the spawned browser variable (browser), and userDataDir to locate
where to add try/catch/finally or promise.catch cleanup.
In `@scripts/__tests__/test-governance.test.mjs`:
- Around line 22-29: readWorkspacePackageDirs() currently only extracts
double-quoted workspace patterns via the regex in the patterns assignment, which
misses single-quoted or unquoted entries; update the extraction to accept
double-quoted, single-quoted, or unquoted YAML list items (or better, parse the
YAML with a YAML parser) so patterns like '- "pkg/*"', "- 'pkg/*'", and '-
pkg/*' are all captured; specifically modify the lines that build patterns (the
matchAll regex) to handle the three cases and normalize to the same capture (or
replace the regex with a YAML parse) so the later logic that iterates patterns
and expands "/*" directories will include all workspace packages.
---
Nitpick comments:
In `@scripts/test-changed.mjs`:
- Around line 61-106: readWorkspacePatterns and expandWorkspacePattern are too
naive; replace the ad-hoc parsing with a YAML loader and a glob resolver so
pnpm-workspace.yaml patterns (including negations like '!/test/' and nested
globs like 'components/**') are handled correctly. In readWorkspacePatterns,
read and parse pnpm-workspace.yaml using a YAML parser (e.g., js-yaml) and
return the raw patterns array from the packages key; in expandWorkspacePattern,
use a glob library (e.g., fast-glob) rooted at projectRoot to resolve each
pattern to package.json paths (respecting negations and nested matches) instead
of only handling literal paths and trailing "/*". Ensure functions named
readWorkspacePatterns, expandWorkspacePattern and variables projectRoot/pattern
are updated to use these libraries and preserve the final result as an array of
existing package.json file paths.
🪄 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 Plus
Run ID: 285b019d-a8f0-4bb1-bc11-f8d0424faf89
📒 Files selected for processing (36)
.github/workflows/pr-checks.ymlAGENTS.mddocs/bugs/testing-suite-hardening.mddocs/contributing.mddocs/skipped-test-inventory.mddocs/testing-suite-quality-prd.mdpackages/cli/package.jsonpackages/cli/src/__tests__/bundle-output-helpers.tspackages/cli/src/__tests__/bundle-output.test.tspackages/cli/src/__tests__/ci-workflow.test.tspackages/cli/src/__tests__/extension-integration.test.tspackages/cli/src/__tests__/extension.test.tspackages/cli/src/__tests__/package-config.test.tspackages/core/src/__test-utils__/vitest-setup.tspackages/dashboard/app/__tests__/agent-css-classes.test.tspackages/dashboard/app/test/cssFixture.tspackages/dashboard/package.jsonpackages/dashboard/scripts/browser-layout-smoke.mjspackages/dashboard/src/__tests__/debug-execfile.test.tspackages/dashboard/src/__tests__/project-routes.test.tspackages/dashboard/src/__tests__/routes-git.test.tspackages/dashboard/src/__tests__/routes-github.test.tspackages/dashboard/src/__tests__/routes-system.test.tspackages/dashboard/vitest.config.tspackages/dashboard/vitest.setup.tspackages/droid-cli/vitest.config.tspackages/engine/src/__tests__/custom-providers.test.tspackages/engine/src/__tests__/task-completion.test.tspackages/engine/src/__tests__/verification-utils.test.tspackages/engine/src/custom-providers.tspackages/pi-claude-cli/vitest.config.tspackages/pi-llama-cpp/vitest.config.tsscripts/__tests__/test-changed.test.mjsscripts/__tests__/test-governance.test.mjsscripts/ci-test-shard.mjsscripts/test-changed.mjs
|
Much needed!! Thank you!! |
|
Addressed the remaining review-summary nitpick in e6c676d as well: scripts/test-changed.mjs now parses pnpm-workspace.yaml through yaml, resolves workspace package.json files with fast-glob, and includes regression coverage for quoted, bare, nested, and negated workspace patterns. I also fixed the local CI shard precondition by having scripts/ci-test-shard.mjs build plugin packages before running shard tests. |
e6c676d to
06d0226
Compare
|
Rebased onto main and force-pushed the review fixes as 06d0226. All three review threads remain resolved; local verification passed including the isolated CI shard rerun. Monitoring CodeRabbit/checks now. |
Greptile SummaryThis PR hardens the local/PR test workflow by fixing the two previously identified bugs in
Confidence Score: 5/5Safe to merge. Both previously-flagged bugs in Both bugs from previous review rounds are fully addressed:
Important Files Changed
Reviews (5): Last reviewed commit: "test(FN-000): include reverse dependents..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/cli/src/__tests__/ci-workflow.test.ts`:
- Around line 186-189: The negative assertion in the test uses strict equality
to detect a pnpm build step (step.run === "pnpm build"), which misses
multiline/block-run values like "pnpm build\n"; update the check to use
substring matching (e.g., step.run?.includes("pnpm build")) consistent with the
existing findBuildStepByRun helper, and make the analogous change in the related
test at the earlier "keeps build coverage" assertion so both positive and
negative checks use .includes() to robustly detect the build command.
In `@packages/cli/src/__tests__/package-config.test.ts`:
- Around line 251-256: The current assertions only check inclusion and can miss
accidental overlaps between curated and deep lanes; update tests around
dashboardPkg.scripts to use boundary-aware matches (e.g., toMatch with
word-boundary regex for "--project dashboard-app" and "--project dashboard-api")
and add negative assertions so the default test script
(dashboardPkg.scripts?.test) explicitly does NOT contain deep projects
("--project dashboard-app" or "--project dashboard-api"), while the deep lane
(dashboardPkg.scripts?.["test:deep"]) asserts it DOES contain the deep projects
and DOES NOT contain the quality projects ("--project dashboard-app-quality" /
"--project dashboard-api-quality").
In `@packages/core/src/__test-utils__/vitest-setup.ts`:
- Around line 41-60: The file contains a leftover inline override of
process.emitWarning that double-wraps the filter installed by
installWarningFilter(); remove the stale inline reassignment block (the old
wrapper that captures originalEmitWarning and reassigns process.emitWarning) so
only installWarningFilter(), its EmitWarningArgs/EmitWarningRestArgs types, and
its own originalEmitWarning binding remain and are invoked; ensure no other
direct process.emitWarning reassignments remain so the warningState guard in
installWarningFilter and its filtering logic are the single active
implementation.
🪄 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 Plus
Run ID: 28cc95ea-3cab-4995-bb12-31379d44214c
📒 Files selected for processing (14)
.github/workflows/pr-checks.ymlAGENTS.mddocs/bugs/testing-suite-hardening.mddocs/contributing.mddocs/skipped-test-inventory.mddocs/testing-suite-quality-prd.mdpackage.jsonpackages/cli/package.jsonpackages/cli/src/__tests__/ci-workflow.test.tspackages/cli/src/__tests__/extension-integration.test.tspackages/cli/src/__tests__/extension.test.tspackages/cli/src/__tests__/package-config.test.tspackages/core/src/__test-utils__/vitest-setup.tspackages/dashboard/app/__tests__/agent-css-classes.test.ts
✅ Files skipped from review due to trivial changes (4)
- packages/cli/package.json
- package.json
- docs/bugs/testing-suite-hardening.md
- packages/cli/src/tests/extension-integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-checks.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/fusion-plugin-cursor-runtime/vitest.config.ts`:
- Around line 19-21: Remove the deprecated poolOptions configuration: delete the
entire poolOptions object (the nested threads/minThreads/maxThreads structure)
and keep only the top-level pool: "threads" and maxWorkers entries; update any
references to poolOptions in this module (e.g., in the vitest config object) so
the config conforms to Vitest 4's API.
🪄 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 Plus
Run ID: 8b38d235-c1cb-4256-8fd2-b6eb7ae99964
📒 Files selected for processing (1)
plugins/fusion-plugin-cursor-runtime/vitest.config.ts
|
Follow-up review pass is complete on 2bc0f53. Replied to and resolved all currently open review threads; thread-aware scrape now reports 0 unresolved / 8 resolved. Latest PR checks are all green: Build, Lint, Typecheck, Test shards 1/3, 2/3, 3/3, and CodeRabbit. |
|
Addressed Greptile's P1 outside-diff finding in 409d4ef. Local verification passed: |
|
Addressed Greptile's reverse-dependency expansion P1 in b3e5932. Local verification passed: |
|
Greptile has re-reviewed the latest head b3e5932 and now reports Confidence Score 5/5, Safe to merge. The two Greptile-flagged |
test: harden and slim local test workflow
Summary
This PR turns the testing-suite audit into implementation work and makes the local/PR test workflow faster without dropping the old exhaustive dashboard coverage.
What changed
docs/testing-suite-quality-prd.mdwith the testing-suite assessment and implementation plan.environmentMatchGlobs.pnpm --filter @fusion/dashboard test:deep, plustest:appandtest:api.AGENTS.mdwith current project and agent test-running instructions.Runtime impact
Measured locally on this branch:
pnpm test:full:744.95s/ about12m25s.pnpm test:full:308.34s/ about5m08s.148files,3,642tests,real 91.73sstandalone.419files,10,747tests,real 335.47s, still passing and opt-in.Dashboard test policy
Default local/PR dashboard coverage now runs the curated quality projects:
pnpm --filter @fusion/dashboard testRun the exhaustive dashboard sweep for broad dashboard architecture, shared modal/view infrastructure, route registration, or curated-selection-risk changes:
Additional explicit lanes remain:
Bugs tracked/fixed
The living log is in
docs/bugs/testing-suite-hardening.mdand includes TSH-001 through TSH-013, including dashboard runtime/noise, PR action minutes, dashboard lane script selection, temporary PR build coverage loss, warning filtering cleanup, and dashboard default runtime.Verification
Passed locally:
Notes
@runfusion/fusionpackage behavior.Summary by CodeRabbit
Chores
Tests
Documentation