test: extract shared mocked MCP and Nock harness - #625
test: extract shared mocked MCP and Nock harness#625charliecreates[bot] wants to merge 4 commits into
Conversation
|
Queued — the merge queue status continues in this comment ↓. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
==========================================
+ Coverage 92.94% 93.56% +0.61%
==========================================
Files 35 37 +2
Lines 1559 1709 +150
Branches 392 432 +40
==========================================
+ Hits 1449 1599 +150
Misses 48 48
Partials 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Unit Test Results 1 files 34 suites 3s ⏱️ Results for commit c421ae0. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
✨ PR Review
The PR is a well-structured test infrastructure refactor that centralises lifecycle management, fixture factories, and network isolation into typed, reusable helpers. The migration is clean and the new support files are clearly tested. One non-obvious state management concern and one workflow reliability gap warrant attention.
3 issues detected:
🐞 Bug - The `restorePolicy` argument passed by a second caller at depth > 0 is silently discarded; if two callers intend different post-run network states, only the first caller's intent is satisfied.
Details: disableMockedMcpExternalNetworking only stores restorePolicy when networkIsolationDepth === 0. If a second test suite (different describe block in the same worker, or a re-entrant call) calls this function while depth is already > 0, its restore policy is silently dropped. When the outermost caller eventually releases, it runs the first suite's restore callback, not the most-recently-provided one. If test suites are ordered such that the outer caller passes nock.enableNetConnect but the inner caller passed nock.disableNetConnect, the final state will be wrong and undetected by the harness.
File: tests/support/mocked-mcp.ts (107-127)
🐞 Bug - The `upload-coverage` job has no guard condition and will fail with a misleading artifact-not-found error whenever the Node 24 build leg is cancelled or its coverage steps do not produce output files.
Details: The upload-coverage job unconditionally downloads the coverage-reports-node-24 artifact and uses if-no-files-found: error on the upload step inside build. If the Node 24 matrix leg is cancelled or if either coverage run (tests/integration/mocked or the unit suite) fails before writing lcov output, the artifact is never created. The subsequent upload-coverage job will then fail at actions/download-artifact, blocking the entire workflow result and masking the original test failure.
File: .github/workflows/build-and-test.yml (119-143)
🐞 Bug - Pending-mocks are snapshotted before leaked harnesses are force-closed, so interceptors that would be consumed by those harnesses are incorrectly flagged as unused.
Details: In cleanupMockedMcpTestState, scope.pendingMocks() is evaluated before leaked harnesses are force-closed. If a leaked harness owns interceptors that it would have consumed on close, those interceptors are counted as unused and reported as failures—even though the subsequent harness.close() call a few lines later might have legitimately consumed them. This produces a spurious "Unused Nock interceptors" error alongside the legitimate "Unclosed mocked MCP harnesses" error, inflating the AggregateError with a misleading extra entry.
File: tests/support/mocked-mcp.ts (247-286)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
Covered the missed fixture-default and mocked MCP lifecycle paths reported by — charlied/pr-check-repair |
Merge Queue Status
This pull request spent 3 minutes 30 seconds in the queue, with no time running CI. ReasonThe pull request conflicts with pull requests ahead in queue: #626, #623, #624 HintThere is nothing you can do for now. If the pull request ahead in the queue is merged, this pull request will become conflicting and you'll have to update it. Tick the box to put this pull request back in the merge queue (same as
|
|
I stopped the conflict repair because — charlied/pr-merge-conflict-repair |
|
I stopped automatic conflict repair.
Please recreate or rebase this branch from current — charlied/pr-merge-conflict-repair |
Summary
Safety and behavior
tests/; no production package exports or generated files change.Validation
Validated with Node 24 and
HEVY_API_KEYunset:npx vitest run tests/support tests/integration/mocked— 4 files, 36 tests passednpx vitest run tests/integration/mocked— 2 files, 16 tests passednpx vitest run --exclude 'tests/integration/**'— 34 files, 538 tests passednpm run checknpm run check:typesnpm run buildnpm run check:changeset.github/workflows/*.yml— 9/9 workflows passednpm pack --dry-run --ignore-scripts— no test support includedgit diff --check origin/main...HEADGemini feedback was attempted as required, but the CLI stopped before review because no authentication method was configured; no Gemini review is claimed.
Changeset
Empty changeset: internal test/CI refactoring only; no package version bump.
Resolves #606
Refs #601
Strategy: #604
✨ PR Description
Purpose: Extract shared mocked MCP and Nock test harnesses into reusable support modules while removing live credential dependencies from CI workflow.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how