test(e2e): wait for portable gateway serve record - #9379
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughWalkthroughGateway tests now poll asynchronous command logs for expected ChangesGateway log readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes the gateway regression test wait for the asynchronous serve record before validating process and cleanup behavior; no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit abaf903 in the TypeScript / code-coverage/cliThe overall coverage in commit abaf903 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
jyaunches
left a comment
There was a problem hiding this comment.
Could this race fix also remove the repeated gateway-command parser? This file reads, trims, splits, and parses scope.gatewayCommandLog at lines 658, 753, and 814 of the latest PR commit. Extracting readGatewayCommands and a waitForGatewayCommands helper would let this test receive the parsed generate-certs and serve sequence without the outer mutable commands variable. Reusing the helper at the other two call sites should make the fix neutral or negative in lines of code while keeping the same assertions.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review\n\nRequesting changes because repeats the same JSONL parser at three call sites. Extract and , reuse them for each assertion, and remove the mutable outer variable. The fix can preserve its behavior with less repeated test machinery.
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
test/e2e/live/openclaw-portable-lifecycle.test.tsrepeats the samegatewayCommandLogJSONL parser at three call sites.- The wait path also relies on a mutable outer variable.
Refactor direction
- Extract
readGatewayCommandsfor parsing. - Extract
waitForGatewayCommandsfor the polling contract. - Reuse both helpers for all three assertions.
Expected result
- Preserve the race fix while removing repeated parsing and mutable test state.
- Bring the change toward neutral or negative test LOC.
Superseded by the correctly rendered structured LOC Reduction / Codebase Simplicity Review.
Resolved by commit 4aaa3ff; the repeated JSONL parser and mutable outer state were replaced with shared bounded read/wait helpers.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Re-reviewed latest PR commit 4aaa3ff95ab29b8d8e705bea242815b701c9ed5f. The revision resolves my prior change request.
test/e2e/support/portable-profile-systemctl-shim.test.ts:363-380 now owns the JSONL parser and bounded wait in readGatewayCommands and waitForGatewayCommands. The three assertions reuse those helpers at lines 677, 764, and 817. The repeated parsers and mutable outer command state are gone, and the complete PR is +22/-18.
I found no new blocking LOC or codebase-simplicity issue. This comment closes only the prior simplicity review; it is not an approval of other review dimensions.
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact commit 4aaa3ff95ab29b8d8e705bea242815b701c9ed5f; all 36 focused E2E-support tests passed locally.
No actionable findings. The bounded vi.waitFor now waits for the complete ordered command sequence and returns the parsed records, while the existing launch-record PID, live-process, failure, and cleanup assertions remain intact.
GitHub does not permit an author to approve their own pull request, so this is a completed review comment rather than an approval.
Summary
An injected gateway record failure can return before the asynchronous gateway process records its
servecommand. The regression test now waits for that required command. It still validates the launch-record PID, active process, and cleanup behavior.Related Issue
Related to #9208.
Changes
generate-certsandservesequence.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project e2e-support test/e2e/support/portable-profile-systemctl-shim.test.ts --reporter=verbosepassed 36/36 onf33c18283.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit