test(e2e): fix nightly fixtures and diagnose Hermes loss - #6831
Conversation
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe live E2E changes update installer tar stubbing and shields restoration, add Hermes rebuild progress tracking and validation, register related mock-parity tests, and improve sandbox phase parsing for modern output and ChangesE2E reliability and progress reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildHermesTest
participant ProgressTracker
participant LongRunningCommand
participant ResourceSampler
RebuildHermesTest->>ProgressTracker: advance phase
RebuildHermesTest->>LongRunningCommand: run command with onOutput
LongRunningCommand->>ProgressTracker: report output
ProgressTracker->>ResourceSampler: sample host resources
ResourceSampler-->>ProgressTracker: return snapshot
ProgressTracker-->>RebuildHermesTest: emit heartbeat
RebuildHermesTest->>ProgressTracker: stop during cleanup
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the Updated |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
changed the title
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron is a non-blocking second opinion. Its prose, findings, and E2E guidance do not change the primary assessment above and remain in workflow artifacts only. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output. Recommended coverage:
1 optional coverage item · 1 optional selector · 0 new-test recommendations
This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/support/rebuild-hermes-progress.test.ts (1)
39-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case for
phase()called afterstop().Coverage is solid, but the
if (stopped) return;guard inphase()(rebuild-hermes-progress.ts) is never directly exercised here — only indirectly via ordering assumptions in the live test. A short additional case (progress.stop(); progress.phase("x"); expect(state.lines).toHaveLength(<unchanged>);) would lock in that guard.🤖 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/e2e/support/rebuild-hermes-progress.test.ts` around lines 39 - 77, Add a focused test in the “Hermes rebuild live progress” suite that starts progress, calls stop(), then invokes phase() and verifies no additional log lines are emitted. Use the existing progressHarness and state.lines assertions to directly cover the stopped guard in phase().
🤖 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.
Nitpick comments:
In `@test/e2e/support/rebuild-hermes-progress.test.ts`:
- Around line 39-77: Add a focused test in the “Hermes rebuild live progress”
suite that starts progress, calls stop(), then invokes phase() and verifies no
additional log lines are emitted. Use the existing progressHarness and
state.lines assertions to directly cover the stopped guard in phase().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c84c3cbe-2c31-4c28-b9e0-6f30437e11a5
📒 Files selected for processing (4)
test/e2e/live/rebuild-hermes-progress.tstest/e2e/live/rebuild-hermes.test.tstest/e2e/mock-parity.jsontest/e2e/support/rebuild-hermes-progress.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
area: e2e
3 participants
Summary
Addresses the five release/nightly E2E blockers according to the evidence each failure left behind. The OpenShell version-pin fixture now supports the installer's archive validation calls, the Shields target restores lockdown after its intentional failure assertion, MCP patch failures preserve their real sandbox phase, and the Hermes rebuild target emits secret-safe phase and host-resource heartbeats.
The Hermes failure was not the fixture's 45-minute rebuild timeout. GitHub ended the job after about 50 minutes because the hosted runner lost communication; the rebuild command began later in the scenario and could not yet have reached its own timeout. No final logs or artifacts survived, so the underlying memory, disk, CPU, network, or runner-termination trigger cannot be recovered from that run.
Follow-up to #6744 and #6724.
The source nightly had seven red jobs. A same-SHA rerun cleared the two accepted flakes,
hermes-shields-configandgateway-guard-recovery, leaving five release blockers:openshell-version-pin,shields-config,mcp-bridge,rebuild-hermes, andchannels-stop-start (hermes). This PR fixes the two reproducible fixture defects, repairs the MCP phase evidence, and instruments the Hermes rebuild runner loss.mcp-bridgefailed when Dockerstopexceeded its 30-second client timeout after a successful image build, whilechannels-stop-start (hermes)was externally canceled before any assertion or artifact; neither has evidence for a causal retry or timeout change, so both still require a fresh green candidate run or a reproducible product defect.Changes
tar -tzf,tar -tvzf, and extraction in the hermetic OpenShell version-pin fixture so the installer can validate fake release archives before extracting them.NAME CREATED PHASEOpenShell rows with the shared sandbox-list parser so MCP Docker-patch diagnostics reportProvisioninginstead of the creation date, while retaining the canonical terminalEvictedphase.mcp-bridgeDocker timeout andchannels-stop-start (hermes)cancellation as explicit green-evidence blockers instead of masking them with retries.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable to narrow live-target fixture and diagnostic changes. An exploratory local E2E-support run passed 979 tests and hit 15 environment-specific failures (macOS Bash behavior, Node 26 warning output, and nested-run timeouts); required Linux CI remains authoritative.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
Bug Fixes
User Experience
Tests