refactor(sandbox): give rebuild test harnesses one shared core - #8581
refactor(sandbox): give rebuild test harnesses one shared core#8581laitingsheng wants to merge 1 commit into
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe rebuild-flow test infrastructure now has a shared harness core with separate generic and DCode profiles. Test suites use the new profile modules and centralized lifecycle hooks. The obsolete test harness is removed from the ChangesRebuild harness refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
refactor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 80e6b12 in the TypeScript / code-coverage/cliThe overall coverage in commit 80e6b12 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/helpers/rebuild-flow-harness.ts (1)
107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd mock-history cleanup to the harness.
If Vitest does not enable
clearMocks, callvi.clearAllMocks()beforevi.restoreAllMocks()so each test starts with empty mock history.🤖 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/helpers/rebuild-flow-harness.ts` around lines 107 - 109, Update the afterEach cleanup in the rebuild-flow harness to call vi.clearAllMocks() before vi.restoreAllMocks(), ensuring mock history is reset between tests while preserving the existing purgeRebuildModule() cleanup.Source: Coding guidelines
test/helpers/rebuild-flow-dcode-harness.ts (1)
171-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCentralize common mock setup in
rebuild-flow-harness.ts.The shared core defines no direct spies. The generic and DCode harnesses independently define the same console, session, registry, Docker, and lifecycle spies. This duplication can cause divergent test behavior.
🤖 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/helpers/rebuild-flow-dcode-harness.ts` around lines 171 - 720, Centralize the duplicated console, session, registry, Docker, and lifecycle spy setup in the shared rebuild-flow harness used by createRebuildFlowHarness. Update the generic and DCode harnesses to reuse those shared spies and return values instead of defining independent mocks, while preserving existing override behavior and spy handles.
🤖 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 `@test/helpers/rebuild-flow-dcode-harness.ts`:
- Around line 171-720: Centralize the duplicated console, session, registry,
Docker, and lifecycle spy setup in the shared rebuild-flow harness used by
createRebuildFlowHarness. Update the generic and DCode harnesses to reuse those
shared spies and return values instead of defining independent mocks, while
preserving existing override behavior and spy handles.
In `@test/helpers/rebuild-flow-harness.ts`:
- Around line 107-109: Update the afterEach cleanup in the rebuild-flow harness
to call vi.clearAllMocks() before vi.restoreAllMocks(), ensuring mock history is
reset between tests while preserving the existing purgeRebuildModule() cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89684682-10c1-4ddb-805e-667e52d52a62
📒 Files selected for processing (23)
scripts/checks/test-create-require-budget.mtssrc/lib/actions/sandbox/rebuild-base-image-resolution-flow.test.tssrc/lib/actions/sandbox/rebuild-dcode-artifact-drift.test.tssrc/lib/actions/sandbox/rebuild-dcode-base-image-lease.test.tssrc/lib/actions/sandbox/rebuild-dcode-mutation-edge.test.tssrc/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.tssrc/lib/actions/sandbox/rebuild-dcode-preflight.test.tssrc/lib/actions/sandbox/rebuild-dcode-recovered-provider.test.tssrc/lib/actions/sandbox/rebuild-dcode-recovery.test.tssrc/lib/actions/sandbox/rebuild-flow-credential-preflight.test.tssrc/lib/actions/sandbox/rebuild-flow-lifecycle.test.tssrc/lib/actions/sandbox/rebuild-flow-recovery.test.tssrc/lib/actions/sandbox/rebuild-flow-target-credentials.test.tssrc/lib/actions/sandbox/rebuild-flow-target-image.test.tssrc/lib/actions/sandbox/rebuild-flow-target-session.test.tssrc/lib/actions/sandbox/rebuild-hermes-post-restore.test.tssrc/lib/actions/sandbox/rebuild-local-provider-recreate.test.tssrc/lib/actions/sandbox/rebuild-prepared-recovery.test.tstest/helpers/rebuild-dcode-flow-helpers.tstest/helpers/rebuild-flow-dcode-harness.tstest/helpers/rebuild-flow-generic-harness.tstest/helpers/rebuild-flow-harness.tstest/rebuild-stale-recovery.test.ts
💤 Files with no reviewable changes (1)
- scripts/checks/test-create-require-budget.mts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
I reviewed the two automated maintainability suggestions against the repository’s current Vitest configuration and the profile-specific fixtures.
Focused verification is green: stale-recovery 5/5, DCode preflight 15/15 with the repository’s slow-environment timeout override, and the generic target-session case 1/1. All required GitHub gates currently pass; the remaining Nemotron failure is advisory-service behavior, not a product-test failure. The branch still needs a current-main refresh and independent approval before merge. |
2 participants
Summary
Two rebuild test harnesses each carried their own CommonJS loader seam, module handles, session factory, and mock lifecycle, so the same setup existed twice and the two suites drifted apart. One shared core now owns that machinery, and two named profiles hold only the mocks that distinguish them. Every rebuild suite selects its profile through the import path.
Related Issue
Fixes #8359
Parent epic: #8345
Changes
test/helpers/rebuild-flow-harness.tsbecomes the shared core, 848 lines to 115. It owns the singlecreateRequireseam, the 39 dependency module handles both profiles spy on,purgeRebuildModule,loadRebuildSandbox,sourceSandboxGateway, a temp-directory registry, and the mock lifecycle. It re-exports the type and session layer thatrebuild-flow-test-support.tsalready owned, so profiles import from one place.test/helpers/rebuild-flow-generic-harness.ts, renamed fromrebuild-flow-test-harness.ts, holds the generic profile at 660 lines.test/helpers/rebuild-flow-dcode-harness.tsholds the Deep Agents Code profile at 721 lines, split out of the old harness. Neither declarescreateRequire. Deep Agents Code route and image mocks stay out of the generic profile, and the generic credential, messaging, and prepared-image mocks are not mandatory Deep Agents Code setup.RebuildFlowStep,RebuildFlowSession,createStep,createRebuildFlowSession,installTerminalStepFailureMock, orsourceSandboxGateway. Each copy was byte-identical to the shared one.installRebuildFlowTestHooks(options). The Deep Agents Code suites keep theirNEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWAREsetting through{ acceptThirdPartySoftware: true }, so the generic suites do not inherit it. Temp-directory cleanup and module-cache purging now run for both profiles.createRequireonnew URL("../../src/lib/actions/sandbox/rebuild.ts", import.meta.url). The old harness anchored onpath.join(process.cwd(), "src/lib/actions/sandbox/rebuild-flow-harness.ts"), a path absent from the tree, which made module resolution depend on the process working directory.scripts/checks/test-create-require-budget.mtsdrops the retiredrebuild-flow-test-harness.tspath.TEST_SUPPORT_CREATE_REQUIRE_FILESgoes from 8 entries to 7 and gains none, so the seam that the retired harness held is not reused elsewhere.Type of Change
Quality Gates
test/helpers/**, the suites that consume those helpers, and one repository check;test/is absent from thefileslist inpackage.json, and no page underdocs/references these helpers.Documentation Writer Review
no-docs-neededWRITING.mdand the controlled word list. No documentation update is required: the diff changes onlyscripts/checks/test-create-require-budget.mtsand four modules undertest/helpers/, plus 18 consumer suites whose edits are imports and a hook swap; a-U0diff over those suites shows zero added or removed test titles and no assertion change. Nosrc/libmodule, CLI command, configuration key, default, workflow, or error string changed, so theCONTRIBUTING.mddocumentation trigger is not met. Thefileslist inpackage.jsonexcludestest/, so the helpers are not published, and a repository-wide search for the six affected helper module names returns no hit underdocs/,fern/,.agents/skills/,CONTRIBUTING.md,AGENTS.md, orCLAUDE.md. The three retained comments were checked against source and hold. No issue number, PR number, ticket ID, or author name appears in any changed file. No blocking finding was raised; one non-blocking naming suggestion was recorded and left for the maintainer.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/rebuild— 61 files, 657 passed, matching the pre-change baseline on the same worktree.npx vitest run --project integration test/rebuild-stale-recovery.test.ts— 1 file, 5 passed.npm run typecheck:cli,npm run checks:repository,npm run test:titles:check, andnpm run test:projects:checkall passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit