chore(e2e): delete deprecated bash entrypoints and orphan helper libs#4996
Merged
Conversation
PR #4380 made the TypeScript runner (test/e2e-scenario/scenarios/run.ts) the canonical scenario executor, collapsed run-scenario.sh and run-suites.sh to fail-fast deprecation stubs, and deleted the runtime/resolver/ tree. Several bash files were left behind with no production callers. This PR removes them. Tombstone bash entrypoints (no callers; bodies were just exit-2 stubs): - test/e2e-scenario/runtime/run-scenario.sh - test/e2e-scenario/runtime/run-suites.sh Orphan helper libs (no production callers): - test/e2e-scenario/runtime/lib/artifacts.sh - test/e2e-scenario/runtime/lib/cleanup.sh - test/e2e-scenario/runtime/lib/negative.sh - test/e2e-scenario/runtime/lib/port-holder.sh Test cleanup riding along with the deletions: - Drop the 'artifact_helper_should_collect_known_logs_*' test in e2e-lib-helpers.test.ts: it was the only caller of artifacts.sh. - Drop 'gateway_helper_should_report_unhealthy_gateway_clearly' in e2e-lib-helpers.test.ts: it sourced ${RUNTIME_LIB}/gateway.sh, which does not exist on main and was passing only by accident (the missing-file stderr happens to match /gateway/i). - Drop the dead E2E_DRY_RUN: '1' env arg in 'older_base_image_should_emit_dockerfile_pointing_at_tagged_base' in e2e-lib-helpers.test.ts: PR #4380 deleted the dry-run short-circuit from older-base-image.sh, so the env var is now unread test scaffolding. Comment hygiene: - sandbox-absent.sh: drop the 'Typed replacement for the legacy run-scenario.sh inline check' note now that the legacy file is deleted. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR removes deprecated Bash E2E runtime helpers (artifact collection, cleanup, negative output validation, port management, and script runners) and adjusts corresponding test fixtures to no longer depend on those utilities. ChangesE2E Test Infrastructure Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
cv
approved these changes
Jun 9, 2026
15 tasks
This was referenced Jun 9, 2026
cv
pushed a commit
that referenced
this pull request
Jun 9, 2026
## Summary Follow-up to #4996 addressing the PR review advisor's findings. Two items: (1) the e2e scenario docs still pointed users at runner paths that #4996 deleted, and (2) the gateway helper had no real negative-path test coverage after the accidentally-passing test in #4996 was removed. Both are fixed here. ## Related Issue Follow-up to #4996. ## Changes **Docs no longer point at deleted runner paths** - `test/e2e-scenario/docs/README.md` — replaced the "How to run" block. It listed `bash test/e2e-scenario/runtime/run-scenario.sh ...` and `run-suites.sh` with `--plan-only`, `--dry-run`, and `--validate-only` flags, all of which were either deleted in #4380 or stubbed out in #4996. Rewritten to use only the canonical TypeScript runner (`--list`, `--emit-matrix`, `--scenarios <id[,id...]>`, and `--plan-only` for local debug only). Repository-layout block under `runtime/` no longer claims to contain `run-scenario.sh`, `run-suites.sh`, `coverage-report.sh`, or `resolver/`. - `test/e2e-scenario/docs/MIGRATION.md` — dropped the YAML/shell resolver block and the `coverage-report.sh` reference (deleted in #4380), and stopped advertising `--dry-run` on the typed runner. Reworded the "leave legacy executable scripts in place" line so it no longer implies the bash entrypoints still exist. **Replaced the accidentally-passing gateway test with a real negative-path test** - `test/e2e-scenario/framework-tests/e2e-lib-helpers.test.ts` — added `gateway helper should fail clearly when URL is unreachable`. It sources the supported helper at `validation_suites/assert/gateway-alive.sh`, calls `e2e_gateway_assert_healthy` against an unbound port, and asserts the helper exits non-zero with stderr that names the gateway and the URL/port so on-call engineers can grep for it. The test deleted in #4996 was sourcing a nonexistent `runtime/lib/gateway.sh` and was passing only because the missing-file stderr happened to match `/gateway/i`. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Notes on verification: - Targeted vitest run is green: `npx vitest run test/e2e-scenario/framework-tests/e2e-lib-helpers.test.ts -t "gateway helper should fail"` passes; `npx vitest run test/e2e-scenario/` passes except the same `e2e-fixture-context.test.ts` timing flake that fails on a clean `origin/main` checkout under parallel load and passes on isolated re-run. Pre-existing — not caused by this PR. - Local `npx prek run --all-files` reports the same `Test (CLI)` flakes that exist on a clean `origin/main`. Leaving the verification box unchecked because the local prek run wasn't fully clean even though the issue is unrelated. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated E2E scenario runner documentation to reflect TypeScript as the canonical entrypoint and primary execution workflow. * Removed legacy bash and YAML resolver runner command examples and guidance. * Clarified runtime context configuration and command examples. * **Tests** * Added test coverage for gateway health check helper to validate failure handling on unreachable gateways. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bash is not the direction the E2E scenario stack is heading, and the vitest-runner migration underway makes that even clearer — none of these bash files have a future role. This PR sweeps out a set of inert tombstones and orphan helpers under
test/e2e-scenario/runtime/that have no production callers, so the migration lands on a smaller surface. Net: 6 file deletions + minor test trimming. ~250 lines removed. No production behavior change.Changes
Tombstone bash entrypoints (deleted; bodies were just
exit 2stubs):test/e2e-scenario/runtime/run-scenario.shtest/e2e-scenario/runtime/run-suites.shOrphan helper libs (deleted; zero callers in production):
test/e2e-scenario/runtime/lib/artifacts.sh— only caller was a self-test ine2e-lib-helpers.test.ts.test/e2e-scenario/runtime/lib/cleanup.sh— was a thin re-export ofsandbox-teardown.sh.test/e2e-scenario/runtime/lib/negative.sh— superseded byscenarios/orchestrators/negative-matcher.ts.test/e2e-scenario/runtime/lib/port-holder.sh— superseded by typed probes.Test cleanup in
test/e2e-scenario/framework-tests/e2e-lib-helpers.test.ts(rides along):artifact_helper_should_collect_known_logs_without_failing_when_missing— it was the only caller ofartifacts.sh.gateway_helper_should_report_unhealthy_gateway_clearly— it sourced${RUNTIME_LIB}/gateway.sh, which does not exist on main. The test was passing only by accident: the missing-file stderr happens to match/gateway/i.E2E_DRY_RUN: "1"env arg inolder_base_image_should_emit_dockerfile_pointing_at_tagged_base. PR test(e2e): execute real shell assertions; delete dry-run, --validate-only, and the bash runner #4380 deleted thee2e_env_is_dry_runshort-circuit fromolder-base-image.sh, so the env var is unread test scaffolding now.Comment hygiene:
nemoclaw_scenarios/probes/sandbox-absent.sh— dropped the "Typed replacement for the legacyrun-scenario.shinline check" note now that the legacy file is deleted.Intentionally untouched:
test/e2e-scenario-advisor.test.tsstill referencestest/e2e-scenario/runtime/run-scenario.shas a string fixture. The advisor never reads the file — it filters changed-file paths by shape — so the fixture continues to validate the same plumbing. Refreshing those fixtures is out of scope.test/e2e-scenario/docs/{README,MIGRATION}.mdreference the old bash entrypoints. Doc updates are out of scope; the docs already mark these paths as deprecated.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Notes on verification:
npx vitest run test/e2e-scenario/framework-tests/e2e-lib-helpers.test.ts test/e2e-scenario-advisor.test.ts test/e2e-scenario/— 3 test files, 106 tests, 0 failures.npx prek run --all-filesreports the sameTest (CLI)flakes that exist on a cleanorigin/maincheckout (timing-sensitive tests innemoclaw-start.test.ts,doctor-gateway-token.test.ts,e2e-fixture-context.test.ts,runtime-hermes-secret-boundary-behavioural.test.ts). All pass on isolated re-run; none reference any of the deleted bash files. Pre-existing — not caused by this PR. Leaving the verification box unchecked because the local prek run wasn't fully clean even though the issue is unrelated.Signed-off-by: Julie Yaunches jyaunches@nvidia.com