ci: run the full offline test suite and auto-rebuild stale dist before tests - #212
Conversation
📝 WalkthroughWalkthroughJest configuration now includes a global setup hook to verify the built package is fresh before test execution. A new freshness-checking script compares dist/index.cjs modification time against source and build input files, rebuilding automatically if needed. CI workflows are updated to document offline test suites and clarify the integration job name. ChangesTest Infrastructure Freshness Enforcement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6babbf7d54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let newest = 0; | ||
| for (const name of fs.readdirSync(entry)) { | ||
| newest = Math.max(newest, newestMtimeMs(path.join(entry, name))); | ||
| } | ||
| return newest; |
There was a problem hiding this comment.
When a source file is deleted or renamed without touching another input, this recursion ignores directory mtimes and only considers the remaining files. In that refactor scenario the containing directory's mtime is the only timestamp newer than dist/index.cjs, so globalSetup skips the rebuild and targeted Jest runs keep testing a bundle that still contains the removed code. Seed the directory case with stat.mtimeMs (or otherwise include directory mtimes) so deletions invalidate dist/ too.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/_ensureFreshDist.cjs (2)
24-24: 💤 Low valueRemove unnecessary
asynckeyword.The exported function is declared
asyncbut does not useawaitor return a Promise. Jest'sglobalSetupaccepts both sync and async functions, so theasynckeyword is unnecessary here.♻️ Simplify function signature
-module.exports = async () => { +module.exports = () => { const distEntry = path.join(ROOT, "dist", "index.cjs");🤖 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/_ensureFreshDist.cjs` at line 24, The exported function is declared async but never uses await or returns a Promise; remove the unnecessary async to make it a synchronous export by changing the declaration from "module.exports = async () => { ... }" to "module.exports = () => { ... }" (locate the export assigned to module.exports in the _ensureFreshDist module and update that arrow function signature).
32-32: 💤 Low valueAdd error handling for build failures.
If
npm run buildfails,execSyncwill throw but the error message may not clearly indicate that the freshness check triggered the build. Consider wrapping in try-catch to provide clearer context or allow the natural error to propagate with stdio context.♻️ Optional: add error context
if (!fs.existsSync(distEntry) || fs.statSync(distEntry).mtimeMs < newestInput) { console.log("\ndist/ is stale relative to src/ — rebuilding before tests..."); - execSync("npm run build", { stdio: "inherit", cwd: ROOT }); + try { + execSync("npm run build", { stdio: "inherit", cwd: ROOT }); + } catch (err) { + console.error("\n[_ensureFreshDist] Build failed during freshness check."); + throw err; + } }🤖 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/_ensureFreshDist.cjs` at line 32, Wrap the execSync call that runs "npm run build" (the execSync("npm run build", { stdio: "inherit", cwd: ROOT }) line) in a try-catch; on catch, either log or rethrow an error that adds clear context such as "Build failed during freshness check" while preserving the original error details/stack and keeping stdio behavior, so failures clearly indicate they were triggered by the freshness check (do this where the execSync call resides in test/_ensureFreshDist.cjs).
🤖 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 `@test/_ensureFreshDist.cjs`:
- Around line 12-22: The recursive function newestMtimeMs currently follows
symlinks and can loop on cycles; change its file checks to use fs.lstatSync
instead of fs.statSync and skip entries where lstat.isSymbolicLink() is true
before recursing, so in newestMtimeMs call lstatSync on entry to decide
directory vs file and when iterating fs.readdirSync(entry) lstatSync each child
and continue (skip) if isSymbolicLink(); keep existing mtimeMs handling for
regular files/directories.
---
Nitpick comments:
In `@test/_ensureFreshDist.cjs`:
- Line 24: The exported function is declared async but never uses await or
returns a Promise; remove the unnecessary async to make it a synchronous export
by changing the declaration from "module.exports = async () => { ... }" to
"module.exports = () => { ... }" (locate the export assigned to module.exports
in the _ensureFreshDist module and update that arrow function signature).
- Line 32: Wrap the execSync call that runs "npm run build" (the execSync("npm
run build", { stdio: "inherit", cwd: ROOT }) line) in a try-catch; on catch,
either log or rethrow an error that adds clear context such as "Build failed
during freshness check" while preserving the original error details/stack and
keeping stdio behavior, so failures clearly indicate they were triggered by the
freshness check (do this where the execSync call resides in
test/_ensureFreshDist.cjs).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2645d023-97df-4e10-a7bb-6b7250fa74d4
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/integration.ymljest.config.cjstest/_ensureFreshDist.cjs
| function newestMtimeMs(entry) { | ||
| const stat = fs.statSync(entry); | ||
| if (!stat.isDirectory()) { | ||
| return stat.mtimeMs; | ||
| } | ||
| let newest = 0; | ||
| for (const name of fs.readdirSync(entry)) { | ||
| newest = Math.max(newest, newestMtimeMs(path.join(entry, name))); | ||
| } | ||
| return newest; | ||
| } |
There was a problem hiding this comment.
Add symlink protection to prevent infinite loops.
The recursive directory traversal does not check for symbolic links before descending. If src/ or any subdirectory contains a symlink that creates a cycle, newestMtimeMs will loop infinitely and hang the test suite.
🛡️ Proposed fix to skip symlinks
function newestMtimeMs(entry) {
const stat = fs.statSync(entry);
- if (!stat.isDirectory()) {
+ if (stat.isSymbolicLink() || !stat.isDirectory()) {
return stat.mtimeMs;
}
let newest = 0;Alternatively, use lstatSync instead of statSync to avoid following symlinks:
function newestMtimeMs(entry) {
- const stat = fs.statSync(entry);
+ const stat = fs.lstatSync(entry);
- if (!stat.isDirectory()) {
+ if (stat.isSymbolicLink() || !stat.isDirectory()) {
return stat.mtimeMs;
}
let newest = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function newestMtimeMs(entry) { | |
| const stat = fs.statSync(entry); | |
| if (!stat.isDirectory()) { | |
| return stat.mtimeMs; | |
| } | |
| let newest = 0; | |
| for (const name of fs.readdirSync(entry)) { | |
| newest = Math.max(newest, newestMtimeMs(path.join(entry, name))); | |
| } | |
| return newest; | |
| } | |
| function newestMtimeMs(entry) { | |
| const stat = fs.lstatSync(entry); | |
| if (stat.isSymbolicLink() || !stat.isDirectory()) { | |
| return stat.mtimeMs; | |
| } | |
| let newest = 0; | |
| for (const name of fs.readdirSync(entry)) { | |
| newest = Math.max(newest, newestMtimeMs(path.join(entry, name))); | |
| } | |
| return newest; | |
| } |
🤖 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/_ensureFreshDist.cjs` around lines 12 - 22, The recursive function
newestMtimeMs currently follows symlinks and can loop on cycles; change its file
checks to use fs.lstatSync instead of fs.statSync and skip entries where
lstat.isSymbolicLink() is true before recursing, so in newestMtimeMs call
lstatSync on entry to decide directory vs file and when iterating
fs.readdirSync(entry) lstatSync each child and continue (skip) if
isSymbolicLink(); keep existing mtimeMs handling for regular files/directories.
2 participants
yarn test(all suites outsidetest/integration/— transport, safe, calibur, paymaster, signer, utilities; 468 tests, ~5s, all offline) instead of onlytest:signer.globalSetuprebuildsdist/automatically whensrc/(or build inputs) is newer, so any jest invocation — includingnpx jest <path>— tests current code instead of a stale build. No-op when fresh.integration/chainId→integration(it runs the whole suite, not just chainId). If branch protection listsintegration/chainIdas a required check, update it tointegration.Verified: full suite passes locally (468/468); guard rebuilds exactly once on stale dist and skips when fresh.
Summary by CodeRabbit
Chores
Tests