test(bundler): verify no-fs-scan contract and bundle determinism#5886
Merged
test(bundler): verify no-fs-scan contract and bundle determinism#5886
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds two new Vitest test suites for egg-bundler (deterministic bundling and no-filesystem-scan) and simplifies Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces two new test suites for the egg-bundler tool: deterministic.test.ts ensures that bundling produces byte-identical artifacts across different environments, and no-filesystem-scan.test.ts verifies that the application correctly uses the StartupManifest to bypass directory scans at boot. A review comment suggests refining the file filtering logic in the deterministic tests to prevent over-broad exclusion of files starting with ".egg".
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression tests that lock in egg-bundler’s “no filesystem scan” runtime contract and verify bundle determinism, while preventing a known flake caused by .egg/ races when cloning fixtures.
Changes:
- Add a contract test suite ensuring
ManifestStore/FileLoaderavoid disk scans whenfileDiscoveryis present. - Add determinism tests asserting byte-identical artifacts across runs and across independent fixture clones, with no
outputDirpath leakage. - Avoid the deterministic test flake by filtering
.egg/(and.egg-bundle/) during fixture cloning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/egg-bundler/test/no-filesystem-scan.test.ts | Tests that bundled manifests short-circuit disk reads and prevent fallback globbing / directory scans. |
| tools/egg-bundler/test/deterministic.test.ts | Tests bundle output determinism across runs/clones and prevents fixture-copy races by excluding .egg* directories. |
Deploying egg-v3 with
|
| Latest commit: |
d57cbb1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://579112f1.egg-v3.pages.dev |
| Branch Preview URL: | https://split-15-bundler-no-fs-scan.egg-v3.pages.dev |
killagu
force-pushed
the
split/14-bundler-orchestrator
branch
from
bbf0363 to
6489611
Compare
Deploying egg with
|
| Latest commit: |
d57cbb1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://613ed572.egg-cci.pages.dev |
| Branch Preview URL: | https://split-15-bundler-no-fs-scan.egg-cci.pages.dev |
killagu
force-pushed
the
split/15-bundler-no-fs-scan-test
branch
from
19cbf72 to
674dd95
Compare
killagu
force-pushed
the
split/14-bundler-orchestrator
branch
from
6489611 to
510cde1
Compare
killagu
force-pushed
the
split/15-bundler-no-fs-scan-test
branch
from
674dd95 to
d31d8a0
Compare
killagu
force-pushed
the
split/14-bundler-orchestrator
branch
from
510cde1 to
2e3c087
Compare
killagu
force-pushed
the
split/15-bundler-no-fs-scan-test
branch
from
d31d8a0 to
bebc3f2
Compare
killagu
force-pushed
the
split/14-bundler-orchestrator
branch
from
2e3c087 to
66acb70
Compare
killagu
force-pushed
the
split/15-bundler-no-fs-scan-test
branch
from
bebc3f2 to
94cec4d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5886 +/- ##
=======================================
Coverage 85.53% 85.53%
=======================================
Files 662 662
Lines 18888 18888
Branches 3664 3664
=======================================
+ Hits 16155 16156 +1
+ Misses 2360 2359 -1
Partials 373 373 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/egg-bundler/test/deterministic.test.ts (1)
205-236: ⚡ Quick winUse
bundle()file outputs instead of top-levelreaddir()for drift checks.Line 227–235 only compares top-level files, so nested outputs could drift unnoticed. You already have
result.files+hashByOutputRel()to do a full comparison.Proposed diff
- await bundle({ + const resultA = await bundle({ baseDir: baseDirA, outputDir: outA, pack: { buildFunc: makeDeterministicMockBuild(outA) }, }); - await bundle({ + const resultB = await bundle({ baseDir: baseDirB, outputDir: outB, pack: { buildFunc: makeDeterministicMockBuild(outB) }, }); @@ - const drift: string[] = []; - const namesInA = (await fs.readdir(outA)).sort(); - const namesInB = (await fs.readdir(outB)).sort(); - expect(namesInA).toEqual(namesInB); - for (const name of namesInA) { + const drift: string[] = []; + const hashesA = await hashByOutputRel(resultA.files, outA); + const hashesB = await hashByOutputRel(resultB.files, outB); + expect(Object.keys(hashesA).sort()).toEqual(Object.keys(hashesB).sort()); + for (const name of Object.keys(hashesA)) { if (name === 'bundle-manifest.json') continue; // carries baseDir + generatedAt - const hashA = await sha256(path.join(outA, name)); - const hashB = await sha256(path.join(outB, name)); + const hashA = hashesA[name]; + const hashB = hashesB[name]; if (hashA !== hashB) drift.push(name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/deterministic.test.ts` around lines 205 - 236, The test currently only compares top-level output files by using fs.readdir(outA)/outB (namesInA/namesInB) and thus can miss nested drift; instead use the bundle() return value (result.files) and the helper hashByOutputRel() to compute and compare hashes for every output path. Update the two bundle() calls to capture their results (e.g. resultA/resultB), iterate resultA.files' relative paths, skip "bundle-manifest.json", compute hashes with hashByOutputRel(resultA, rel) and hashByOutputRel(resultB, rel) (or use result.hashByOutputRel if available), and collect any mismatches into drift, then assert drift is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/test/deterministic.test.ts`:
- Around line 189-193: The test only validates bmA.generatedAt's type and
parseability but never checks bmB.generatedAt; update the test to also assert
typeof bmB.generatedAt === 'string' and that new Date(bmB.generatedAt as
string).toString() is not 'Invalid Date' before deleting both fields so both
manifests' timestamps are validated (referencing bmA, bmB, and generatedAt in
the test).
---
Nitpick comments:
In `@tools/egg-bundler/test/deterministic.test.ts`:
- Around line 205-236: The test currently only compares top-level output files
by using fs.readdir(outA)/outB (namesInA/namesInB) and thus can miss nested
drift; instead use the bundle() return value (result.files) and the helper
hashByOutputRel() to compute and compare hashes for every output path. Update
the two bundle() calls to capture their results (e.g. resultA/resultB), iterate
resultA.files' relative paths, skip "bundle-manifest.json", compute hashes with
hashByOutputRel(resultA, rel) and hashByOutputRel(resultB, rel) (or use
result.hashByOutputRel if available), and collect any mismatches into drift,
then assert drift is empty.
🪄 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: 74d35f43-72c1-4865-8b02-502b3842bcf3
📒 Files selected for processing (1)
tools/egg-bundler/test/deterministic.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/test/deterministic.test.ts (1)
229-236: ⚡ Quick winUse
bundle()result files for recursive clone-output comparisonThis block hashes only top-level
readdir()entries. If pack later emits nested artifacts, this can fail noisily (EISDIR) and weakens drift diagnostics. Prefer reusingresult.files+hashByOutputRel()(same pattern as the first test) so comparison stays recursive and contract-aligned.Proposed diff
- await bundle({ + const resultA = await bundle({ baseDir: baseDirA, outputDir: outA, pack: { buildFunc: makeDeterministicMockBuild(outA) }, }); - await bundle({ + const resultB = await bundle({ baseDir: baseDirB, outputDir: outB, pack: { buildFunc: makeDeterministicMockBuild(outB) }, }); @@ - const drift: string[] = []; - const namesInA = (await fs.readdir(outA)).sort(); - const namesInB = (await fs.readdir(outB)).sort(); - expect(namesInA).toEqual(namesInB); - for (const name of namesInA) { + const drift: string[] = []; + const hashesA = await hashByOutputRel(resultA.files, outA); + const hashesB = await hashByOutputRel(resultB.files, outB); + const namesInA = Object.keys(hashesA).sort(); + const namesInB = Object.keys(hashesB).sort(); + expect(namesInA).toEqual(namesInB); + for (const name of namesInA) { if (name === 'bundle-manifest.json') continue; // carries baseDir + generatedAt - const hashA = await sha256(path.join(outA, name)); - const hashB = await sha256(path.join(outB, name)); + const hashA = hashesA[name]; + const hashB = hashesB[name]; if (hashA !== hashB) drift.push(name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/deterministic.test.ts` around lines 229 - 236, The test currently compares only top-level readdir() entries and calls sha256 on those paths (namesInA, namesInB, sha256), which fails for nested directories; change the comparison to reuse the pack/bundle() results (use the result.files arrays) and the hashByOutputRel() helper used in the first test to compute recursive, relative-hash maps for each output, then compare the two maps and produce drift while still ignoring "bundle-manifest.json"; locate the existing namesInA/namesInB loop and replace it with building hash maps from resultA.files and resultB.files via hashByOutputRel(result.files) and compare those maps for differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/egg-bundler/test/deterministic.test.ts`:
- Around line 229-236: The test currently compares only top-level readdir()
entries and calls sha256 on those paths (namesInA, namesInB, sha256), which
fails for nested directories; change the comparison to reuse the pack/bundle()
results (use the result.files arrays) and the hashByOutputRel() helper used in
the first test to compute recursive, relative-hash maps for each output, then
compare the two maps and produce drift while still ignoring
"bundle-manifest.json"; locate the existing namesInA/namesInB loop and replace
it with building hash maps from resultA.files and resultB.files via
hashByOutputRel(result.files) and compare those maps for differences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1be6712d-33e0-4fd5-b630-8368bb6db140
📒 Files selected for processing (1)
tools/egg-bundler/test/deterministic.test.ts
This was referenced May 1, 2026
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
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.
Adds
no-filesystem-scan.test.ts(7 tests: setBundleStore short-circuits disk, globFiles skips on cache hit, FileLoader ignores extra on-disk files) anddeterministic.test.ts(3 tests: byte-identical across runs, independent clones, no outputDir path leaks). Fixes the ~60% deterministic test flake from #5863 by filtering.egg/atfs.cptime.Part of #5863 split. Tracking: #5871.
🤖 Generated with Claude Code
Summary by CodeRabbit