ci(e2e): split recovery validation from soak - #7933
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe E2E recovery workflow separates functional and soak profiles, resolves test parameters from environment variables, validates variant-specific workflow contracts, schedules weekday and Sunday runs separately, and includes the soak variant in reporting and release evidence. ChangesRecovery validation and soak scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant E2EWorkflow
participant RecoveryJob
participant SettingsResolver
participant RecoveryTest
participant ReportJob
Scheduler->>E2EWorkflow: trigger weekday or Sunday cron
E2EWorkflow->>RecoveryJob: dispatch recovery variant with environment
RecoveryJob->>SettingsResolver: resolve profile settings
SettingsResolver->>RecoveryTest: provide crash cycles and soak duration
RecoveryTest-->>ReportJob: publish test result and evidence
ReportJob->>ReportJob: include soak result in scorecard
🚥 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 267ddec in the TypeScript / code-coverage/cliThe overall coverage in commit 267ddec 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
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tools/e2e/workflow-boundary.mts (1)
450-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a short comment on the presence-based opt-out semantics.
E2E_CHANGE_FOCUSEDonly ever legally holds"0", so its presence — not its value — is what removes the job fromliveTestToJobsand therefore fromfocusedE2eJobsForChangedFiles. That inversion is easy to misread next to the adjacentE2E_DEFAULT_ENABLEDblock, which reads as a value check.🤖 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 `@tools/e2e/workflow-boundary.mts` around lines 450 - 456, In the FREE_STANDING_CHANGE_FOCUSED_MARKER handling, add a concise comment documenting that the marker’s presence opts the job out of live-test mapping, while its only valid value is "0"; keep the existing validation and mapping behavior unchanged.test/e2e/live/issue-2478-recovery-profile.ts (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing loudly on malformed numeric env, matching the profile check.
NEMOCLAW_E2E_CRASH_CYCLES=0or=abcsilently falls back to the profile default, while an invalidNEMOCLAW_E2E_RECOVERY_PROFILEthrows. Since these are operator-supplied diagnostic overrides, a silent revert can mask a mis-typed dispatch input (e.g. a soak run that the operator thought was shortened).♻️ Optional: throw on invalid override
-function positiveInteger(raw: string | undefined, fallback: number): number { - const parsed = raw ? Number(raw) : fallback; - return Number.isInteger(parsed) && parsed > 0 ? parsed : fallback; -} +function positiveInteger(name: string, raw: string | undefined, fallback: number): number { + if (!raw) return fallback; + const parsed = Number(raw); + if (!Number.isInteger(parsed) || parsed <= 0) { + throw new Error(`${name} must be a positive integer, got '${raw}'`); + } + return parsed; +}🤖 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/live/issue-2478-recovery-profile.ts` around lines 26 - 29, Update positiveInteger to throw an error when raw is provided but is not a positive integer, instead of silently returning fallback; retain fallback behavior only when raw is undefined, matching the validation behavior of NEMOCLAW_E2E_RECOVERY_PROFILE..github/workflows/e2e.yaml (1)
5597-5661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnv values line up with the validator and resolver; consider anchoring the duplicated step block.
Profile/cycle/soak values match
PROFILE_DEFAULTSintest/e2e/live/issue-2478-recovery-profile.tsand thevariantslist intools/e2e/workflow-boundary.mts, and the distinctNEMOCLAW_SANDBOX_NAMEkeeps a dispatched functional run from colliding with the Sunday soak.The soak job's ~60 lines of steps are a verbatim copy of the functional job's. This file already uses YAML anchors (
*dockerhub-auth), so the OpenShell-resolutionrun:block and the upload/cleanup steps could be anchored once and aliased, which also keeps the validator's shared step assertions honest as the two jobs evolve.🤖 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 @.github/workflows/e2e.yaml around lines 5597 - 5661, Deduplicate the identical E2E step sequence shared by the functional and soak jobs by defining YAML anchors for the OpenShell-resolution run step, artifact upload step, and Docker-auth cleanup step, then reference those anchors in both jobs. Preserve each job’s existing environment, test invocation, and always-run conditions while keeping the shared-step structure aligned with the workflow validator.
🤖 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 @.github/workflows/e2e.yaml:
- Around line 5597-5661: Deduplicate the identical E2E step sequence shared by
the functional and soak jobs by defining YAML anchors for the
OpenShell-resolution run step, artifact upload step, and Docker-auth cleanup
step, then reference those anchors in both jobs. Preserve each job’s existing
environment, test invocation, and always-run conditions while keeping the
shared-step structure aligned with the workflow validator.
In `@test/e2e/live/issue-2478-recovery-profile.ts`:
- Around line 26-29: Update positiveInteger to throw an error when raw is
provided but is not a positive integer, instead of silently returning fallback;
retain fallback behavior only when raw is undefined, matching the validation
behavior of NEMOCLAW_E2E_RECOVERY_PROFILE.
In `@tools/e2e/workflow-boundary.mts`:
- Around line 450-456: In the FREE_STANDING_CHANGE_FOCUSED_MARKER handling, add
a concise comment documenting that the marker’s presence opts the job out of
live-test mapping, while its only valid value is "0"; keep the existing
validation and mapping behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: faa230b6-04b4-4fa2-96c5-4ff90394ef5b
📒 Files selected for processing (7)
.github/workflows/e2e.yamltest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-2478-recovery-profile.tstest/e2e/support/issue-2478-recovery-profile.test.tstest/e2e/support/issue-2478-recovery-workflow-boundary.test.tstest/release-e2e-evidence.test.tstools/e2e/workflow-boundary.mts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Requesting changes for one focused-E2E coverage gap. The functional/soak split, release inclusion, secret boundaries, cleanup, and targeted tests otherwise look sound.
| errors.push(`${jobId} job ${FREE_STANDING_CHANGE_FOCUSED_MARKER} must be "0" when set`); | ||
| } | ||
| } else { | ||
| for (const file of collectLiveTestFiles(rawJob)) addMapValue(liveTestToJobs, file, jobId); |
There was a problem hiding this comment.
[P2] Map the new recovery-profile helper to the functional E2E job
collectLiveTestFiles() only discovers the .test.ts path embedded in the workflow job. As a result, focusedE2eJobsForChangedFiles(["test/e2e/live/issue-2478-recovery-profile.ts"]) returns [], while changing issue-2478-crash-loop-recovery.test.ts selects issue-2478-crash-loop-recovery. The new helper owns the functional/soak defaults and validation, so a future helper-only change will skip the functional recovery job, contrary to #7919’s requirement to run it when owning files change. Please add an explicit dependency mapping (or equivalent import-aware mapping) and a regression assertion for the helper path while continuing to exclude the soak job from change-focused dispatch.
|
I don't think moving the full recovery signal to a weekly lane is the right tradeoff. If this test is too slow or expensive to run with the changes it protects, that is a problem with the test boundary—not a reason to create another execution profile, schedule, selector, reporting path, and release-evidence contract that runs even less frequently. A weekly failure arrives late, is harder to attribute to the change that caused it, and can leave this behavior broken for days before we learn about it. The additional lane also becomes permanent CI surface that we have to maintain. We should instead make the test fast enough for the existing change-focused lane. Move the repeated recovery/state assertions into deterministic unit or integration tests that cover the functionality directly, and retain at most one small live E2E to prove the wiring that cannot be exercised below that boundary. If the live test still needs five crash cycles and a five-minute soak, we should first establish what unique defect each repetition catches and whether that evidence belongs in a benchmark or diagnostic—not solve the cost by running the same test weekly. My preference is to avoid adding the weekly soak lane and use this work to reduce or eliminate the expensive live coverage. |
4 participants
Summary
Separate the issue #2478 recovery coverage into a short functional profile and the retained full soak. Monday-through-Saturday scheduled runs use one crash cycle and a short stability sample, while Sunday and release evidence retain all five cycles and the five-minute soak.
Related Issue
Fixes #7919
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-needed.github/workflows/e2e.yaml, live-test profile, workflow-boundary, and release-evidence changes only.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 unavailabletest/release-e2e-evidence.test.ts(11 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; repository hooks and focused E2E/release suites cover the changed contracts.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Tests
Chores