chore(e2e): refresh docs and add real gateway negative-path test#5042
Merged
Conversation
Address PR review advisor follow-ups on the bash cleanup:
1. Docs still pointed users at deleted runner paths.
- test/e2e-scenario/docs/README.md 'How to run' listed the deleted
run-scenario.sh and run-suites.sh entrypoints with --plan-only,
--dry-run, and --validate-only flags. Replaced with the
TypeScript runner usage that survives on main: --list,
--emit-matrix, --scenarios <id[,id...]>, and --plan-only as a
local-debug-only mode.
- Repository-layout block under runtime/ no longer claims to
contain run-scenario.sh, run-suites.sh, coverage-report.sh, or
resolver/. It now just lists lib/ for the shared helper libs
that validation_suites/ source.
- test/e2e-scenario/docs/MIGRATION.md 'Useful commands' dropped
the YAML/shell resolver block (paths deleted) and the
coverage-report.sh reference (deleted in #4380), and stopped
advertising --dry-run on the typed runner. The 'leave legacy
executable scripts in place' line was reworded so it no longer
implies the bash entrypoints still exist.
2. Replace the accidentally-passing gateway helper test with a real
negative-path test that sources the supported helper.
The test deleted in the prior commit
(gateway_helper_should_report_unhealthy_gateway_clearly) sourced a
nonexistent runtime/lib/gateway.sh and was passing only because
the missing-file stderr happened to match /gateway/i.
validation_suites/assert/gateway-alive.sh is the real, supported
helper and had no negative-path coverage. The new test
gateway_helper_should_fail_clearly_when_url_is_unreachable sources
that file, points e2e_gateway_assert_healthy at an unbound port,
and asserts the helper exits non-zero with a stderr that names
the gateway and the URL/port so on-call engineers can grep for
it.
Verification:
- npx vitest run test/e2e-scenario/framework-tests/e2e-lib-helpers.test.ts
test/e2e-scenario-advisor.test.ts test/e2e-scenario/ -> all green
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.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
|
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 (3)
📝 WalkthroughWalkthroughThis PR consolidates E2E scenario runner documentation to reflect the completed migration to TypeScript as the sole canonical executor. Migration and usage docs are updated to remove legacy bash/YAML runner references, and a new test validates shell helper failure handling on unreachable gateways. ChangesE2E Scenario Runner Consolidation
🎯 2 (Simple) | ⏱️ ~8 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
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. |
wscurran
added
area: docs
cv
approved these changes
Jun 9, 2026
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
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 listedbash test/e2e-scenario/runtime/run-scenario.sh ...andrun-suites.shwith--plan-only,--dry-run, and--validate-onlyflags, all of which were either deleted in test(e2e): execute real shell assertions; delete dry-run, --validate-only, and the bash runner #4380 or stubbed out in chore(e2e): delete deprecated bash entrypoints and orphan helper libs #4996. Rewritten to use only the canonical TypeScript runner (--list,--emit-matrix,--scenarios <id[,id...]>, and--plan-onlyfor local debug only). Repository-layout block underruntime/no longer claims to containrun-scenario.sh,run-suites.sh,coverage-report.sh, orresolver/.test/e2e-scenario/docs/MIGRATION.md— dropped the YAML/shell resolver block and thecoverage-report.shreference (deleted in test(e2e): execute real shell assertions; delete dry-run, --validate-only, and the bash runner #4380), and stopped advertising--dry-runon 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— addedgateway helper should fail clearly when URL is unreachable. It sources the supported helper atvalidation_suites/assert/gateway-alive.sh, callse2e_gateway_assert_healthyagainst 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 chore(e2e): delete deprecated bash entrypoints and orphan helper libs #4996 was sourcing a nonexistentruntime/lib/gateway.shand was passing only because the missing-file stderr happened to match/gateway/i.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 -t "gateway helper should fail"passes;npx vitest run test/e2e-scenario/passes except the samee2e-fixture-context.test.tstiming flake that fails on a cleanorigin/maincheckout under parallel load and passes on isolated re-run. Pre-existing — not caused by this PR.npx prek run --all-filesreports the sameTest (CLI)flakes that exist on a cleanorigin/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
Summary by CodeRabbit
Documentation
Tests