Repository navigation
test(cross-version): make the missing-package stand-in test hermetic - #25796
brennanb2025 wants to merge 1 commit into
Conversation
The end-to-end stand-in test relied on v1.4.221's @streamparser/json being absent from the current install. #23664 re-added that package as a pinned test-only dependency, so on main the import resolved for real, no stand-in was written, and the test failed for every PR that runs the cross-version job. It now extracts a synthetic release commit that imports a package no tree has, so the result no longer depends on the real package set. Co-Authored-By: Claude <noreply@anthropic.com>
|
CI status at head 1f5b8c6: the cross-version wire compatibility job, the one this PR fixes, passes. The only red is a test that is already broken on main and unrelated to this change. It is fixed by #25816.
#25816's CI shows the mirror image: it fails only on this PR's cross-version test. The two PRs can be merged in either order. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe test now creates a synthetic release commit containing a consumer that imports a package absent from the current tree. It materializes the checkout, verifies that the consumer module loads, and checks that calling its package-dependent export throws an error identifying the missing package. The commit helper uses a temporary Git index. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The replacement test covers the missing-package stand-in path without relying on a historical release’s installed dependencies. No change-specific issue is identified that should delay merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 3📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
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 |
|
Superseded by #24878, which fixed this on main (it rewrote the v1.4.221 test to verify the released parser with the pinned test dependency). |
1 participant
ELI5
The "cross-version wire compatibility" CI job is red on main again for every PR that runs it (seen on #24080). The failing test is one I added in #25731.
That test checked the harness's stand-in mechanism against the real v1.4.221 release, and it assumed the current tree does not install
@streamparser/json. Meanwhile #23664 re-added@streamparser/jsontopackage.jsonas a pinned test-only dependency. The two PRs landed in parallel and each CI run was green on its own base. Together, the real package now loads, no stand-in is written, and the test's "using it throws" assertion fails (expected [Function] to throw an error).This PR makes the test hermetic: it no longer depends on which packages happen to be installed.
What Changed
What changes for users: nothing. This is test-only, and it unblocks the cross-version job on every PR.
The test
loads release source that imports a package the current tree does not installnow builds a throwaway release commit with git plumbing: a temporary index,write-tree, thencommit-treewith a fixture author so it works on CI clones with no git identity. The commit's source importsorca-cross-version-fixture-missing-package, a name no tree will ever install.The test then runs the real
materializeReleaseCheckoutextraction on that commit. It asserts the module loads, and that using the missing package throws the stand-in's "does not install" error. The temporary ref is deleted afterwards, like the file's other synthetic refs.The real-tree check against v1.4.221 is dropped rather than kept as a log line. Whether a given real release needs a stand-in now depends on dependency choices, so it can't be a stable assertion.
Why
The mechanism itself is fine. Only the test's assumption about the installed package set broke.
A made-up package name keeps the end-to-end path covered (extraction, import scan, stand-in written, release module loads, use refuses) without coupling the test to
package.json.The synthetic-tree unit tests in
release-missing-packages.unit.test.tswere already hermetic and keep passing.Not changed: #23664's pin
This PR does not touch
package.jsonor the lockfile. The stand-in mechanism from #25731 makes #23664's pinned@streamparser/jsontest-only dependency unnecessary for the harness. With the pin, release code that uses the package runs the real parser. Without it, that code gets a stand-in that refuses on use, and no wire suite uses it today.Whether to keep or remove the pin is left to Brennan. If it's removed, the comment it added above
CHECKOUT_FORMATinrelease-checkout.tsshould go with it.Linked Issue
None. This is CI breakage on main from #25731 and #23664 landing in parallel.
Visual Proof
N/A: test-only change, nothing visible to users.
Testing
Reproduced main's failure locally with main's version of the test file and main's install:
AssertionError: expected [Function] to throw an error.With this change, all 21 files in
tests/e2e/cross-version-wire/pass locally (119 tests), named explicitly, the same set CI runs.Ablation: with no stand-ins written, the new test fails with
Cannot find package 'orca-cross-version-fixture-missing-package'.Also ran:
tc-gate pnpm tc:node, which passes. Intypecheck:e2e, the changed file has no errors; that project already has unrelated errors on main.oxlintandpnpm run check:code-quality:changedare clean.Platforms: macOS locally; CI covers Linux.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
The synthetic commit's objects are written to the local object store and become unreachable once the temporary ref is deleted; git's normal garbage collection removes them.
PR author: @BrennanKB5.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)