Conversation
WalkthroughUnified test-runner completion handling and Docker invocation tweaks in Changes
Sequence Diagram(s)sequenceDiagram
participant Child as Child Process
participant Handler as handleCompletion
participant FS as File System
participant Promise as Test Promise
rect rgb(245,250,245)
Note over Handler: hasCompleted = false\nhandleCompletion enforces single completion
end
alt Child emits error
Child->>Handler: error(err)
Handler->>Handler: if not hasCompleted\n set hasCompleted = true
Handler->>Promise: reject(err)
else Child closes/exits
Child->>Handler: close(code, signal)
Handler->>Handler: if not hasCompleted\n set hasCompleted = true
Handler->>FS: read(outputFile)
FS-->>Handler: file contents
Handler->>Handler: parse results, assert failures === 0
alt assertions pass
Handler->>Promise: resolve(results)
else
Handler->>Promise: reject(assertionError)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code graph analysis (1)test/runTests.test.js (1)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (1)
test/runTests.test.js (1)
59-90:hasExitedguard correctly fixes double‑rejection; optionally improve exit diagnosticsThe
hasExitedpattern around the"error"and"exit"handlers looks solid and should prevent the Promise from being rejected more than once, even if both events fire.As an optional refinement, you might improve the
"exit"handler’s diagnostics for cases where the process terminates via signal (i.e.,code === null) instead of relying on the later JSON read to fail:- runTests.on("exit", (code) => { + runTests.on("exit", (code, signal) => { if (hasExited) return; hasExited = true; - - console.log(`Child process exited with code ${code}`); - - if (code !== null && code !== 0) { - reject(new Error(`Docker process exited with code ${code}`)); - return; - } + + if (code === null) { + reject(new Error(`Docker process exited due to signal ${signal || "unknown"}`)); + return; + } + + console.log(`Child process exited with code ${code}`); + + if (code !== 0) { + reject(new Error(`Docker process exited with code ${code}`)); + return; + }Not required for correctness, but it will produce clearer failures when Docker is killed by a signal rather than exiting with a numeric code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/runTests.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
|
@copilot Could you have a go at this? It worked locally. |
|
@hawkeyexl I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
Fix test race condition with close event
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/runTests.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
test/runTests.test.js (3)
55-55: LGTM: Typo fixed.The test description typo has been corrected.
58-66: LGTM: Completion guard prevents double resolution.The
handleCompletionguard is a solid pattern that prevents race conditions between theerrorandcloseevents, ensuring the promise resolves or rejects exactly once.
80-83: LGTM: Error handler properly guarded.The error handler correctly uses
handleCompletionto prevent double rejection.
|
@copilot In test/runTests.test.js around lines 85 to 106, the close handler only accepts Also update the test workflow to run in any pr. |
|
@hawkeyexl I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
Fix signal handling in test close handler, enable workflow for all PRs, and add resource limits
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.