Repository navigation
fix(test): make the results/ tests hermetic so npm test stops failing - #54
Merged
Merged
Conversation
`npm test` failed on every run — exit 1, reproducible on a clean main — with ENOTEMPTY/ENOENT against the repo's real results/ directory, and it deleted results/.gitkeep as a side effect. Cause: six tests across four adversarial files each backed up the REAL results/ dir, deleted it, put a temp dir (or symlink) in its place, then restored it in a finally block. The node test runner runs FILES IN PARALLEL, so those six raced on one shared global — file A's rmSync landed on file B's half-restored tree (ENOTEMPTY), or B's cpSync ran after A had already deleted it (ENOENT). Which test reported the error was luck, which is why it read as flake rather than as a design problem. lib/runner.cjs now resolves the results directory through resultsDir(), honoring NF_BENCH_RESULTS_DIR and defaulting to <repo>/results exactly as before. It is resolved per call rather than cached at import, so a test can set the variable after the module is loaded. All six sites set that variable to a directory inside their own temp dir and restore the previous value in finally — nothing touches the shared directory at all now. Verified: 3/3 consecutive full `npm test` runs pass (exit 0) with the fix; 2/2 fail (exit 1) with it stashed on the same tree. results/.gitkeep survives. Note for later: BUG 11 in adversarial4 does the same backup/delete/restore dance on challenges/ — same class, same shared global, not yet triggered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe runner now resolves the results directory from ChangesDynamic results directory
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@lib/runner.cjs`:
- Around line 8-17: Update compute-improvement’s result-directory lookup to use
the exported resultsDir() resolver instead of its fixed RESULTS_DIR constant,
ensuring NF_BENCH_RESULTS_DIR controls both saving and reading results. If
resultsDir() is not currently accessible there, move it to a shared module and
reuse it from both paths.
In `@test/adversarial.test.cjs`:
- Around line 212-222: Protect environment restoration by moving each test’s
filesystem setup into the try block that follows NF_BENCH_RESULTS_DIR
assignment: update test/adversarial.test.cjs lines 212-222 for directory and
malformed-file setup, test/adversarial2.test.cjs lines 77-86 for directory and
empty-file setup, and test/adversarial3.test.cjs lines 61-72 plus
test/adversarial4.test.cjs lines 75-85 for fixture copying; keep the existing
finally restoration logic unchanged.
In `@test/adversarial3.test.cjs`:
- Around line 59-63: Capture the fixture source by calling resultsDir() before
overriding NF_BENCH_RESULTS_DIR, then use the stored path as the fs.cpSync
source; apply this change at test/adversarial3.test.cjs lines 59-63 and
test/adversarial4.test.cjs lines 73-77.
🪄 Autofix
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: 36d10662-4600-46fa-98ee-3a320750c887
📒 Files selected for processing (5)
lib/runner.cjstest/adversarial.test.cjstest/adversarial2.test.cjstest/adversarial3.test.cjstest/adversarial4.test.cjs
…lts/) BUG 11 in adversarial4 did the identical backup/delete/repopulate/restore dance on the REAL challenges/ directory that the six results/ sites did — the same shared global under the same parallel runner. It hadn't been unlucky yet; that is the only difference. lib/challenges.cjs now resolves through challengesDir(), honoring NF_BENCH_CHALLENGES_DIR and defaulting to <repo>/challenges as before, resolved per call rather than cached at import. The test redirects the env var at its own temp fixture tree and restores the previous value in finally. Flagged in the PR body as noted-not-fixed; fixing it now instead, since leaving a known race armed to catch someone later is worse than the ten lines it costs. 3/3 consecutive full `npm test` runs pass; results/.gitkeep survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…de try Two valid findings on the hermetic-results change, plus a naming cleanup: - bin/compute-improvement.cjs and bin/nf-benchmark.cjs each carried their own `path.join(__dirname, '..', 'results')`. With NF_BENCH_RESULTS_DIR set, results were WRITTEN to the redirected dir and READ from the fixed one. Both now import resultsDir() from lib/runner.cjs, so one env var means one directory everywhere. - All six sites set NF_BENCH_RESULTS_DIR before entering `try`, so a throw during fixture setup would skip `finally` and leak the override into every later test in that file (process.env is global). The assignment and the seeding now happen inside the protected block. The third finding — that `fs.cpSync(resultsDir, ...)` passes a function and throws ERR_INVALID_ARG_TYPE — is a false positive: `resultsDir` there is a local `const resultsDir = path.join(tmp, 'results')` fixture path, and those files do not import the export (see the destructure at adversarial3:11). The suite passes, which it could not if a function were reaching cpSync. The concern about the NAME is fair though, so the locals are now `fixtureResultsDir` — nothing shadows the resolver even if a future edit imports it. Full `npm test` green across 3 consecutive runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
npm testfails on every run — exit 1, reproducible on a cleanmain— withENOTEMPTY/ENOENTagainst the repo's realresults/directory. It also deletesresults/.gitkeepas a side effect.Cause
Six tests across four adversarial files each do the same dance on the real
results/directory:fs.rmSync(results, { recursive: true })— delete the shared directoryfinally: remove that, copy the backup backThe node test runner runs files in parallel, so those six race on one shared global. File A's
rmSynclands on file B's half-restored tree (ENOTEMPTY); B'scpSyncruns after A has already deleted it (ENOENT). Which test reports the error is luck — which is why this reads as flake rather than as a design problem.Fix
lib/runner.cjsresolves the results directory throughresultsDir(), honoringNF_BENCH_RESULTS_DIRand defaulting to<repo>/resultsexactly as before. It's resolved per call, not cached at import, so a test can set the variable after the module is already loaded.All six sites now point that variable at a directory inside their own temp dir and restore the previous value in
finally. Nothing touches the shared directory at all, so there is nothing left to race on.Verification
npm testruns with the fixresults/.gitkeepsurvives a full run now.Noted, not fixed
BUG 11 in
adversarial4does the identical backup/delete/restore dance onchallenges/. Same class, same shared global — it just hasn't been unlucky yet. Worth the same treatment ifloadAllChallengesgrows an injection point.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
NF_BENCH_RESULTS_DIRenvironment variable.resultsdirectory when no custom location is configured.Bug Fixes
Tests