Skip to content

test: consolidate and rename e2e runner scripts#1698

Merged
ahg-g merged 5 commits into
llm-d:mainfrom
roytman:utests
Jun 21, 2026
Merged

test: consolidate and rename e2e runner scripts#1698
ahg-g merged 5 commits into
llm-d:mainfrom
roytman:utests

Conversation

@roytman

@roytman roytman commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind test

What this PR does / why we need it:

What

Consolidate shared logic between the two e2e runner scripts and rename them
(plus their Makefile targets) so the name states which suite each runs.

  • Extract the duplicated interrupt-cleanup trap and label-filtered Ginkgo
    runner into test/scripts/e2e-common.sh, sourced by both runners.
  • Move hack/test-e2e.sh to test/scripts/, co-located with the other runner.
  • Rename for clarity:
    • hack/test-e2e.sh -> test/scripts/test-e2e-gaie.sh (GAIE suite)
    • test/scripts/run_e2e.sh -> test/scripts/test-e2e-router.sh (router suite)
  • Rename the Makefile targets test-e2e-scheduler{,-run} to
    test-e2e-router{,-run}, aligning them with the existing e2e-router CI job
    and the "router" product name.
  • Update the CI workflow to call make test-e2e-router-run.

Why

The two scripts shared near-identical cleanup and test-invocation blocks, and
their names (test-e2e.sh, run_e2e.sh) gave no hint which suite each drives.
The Makefile also still calls the second suite "scheduler" while CI already
called it "router"; this unifies on "router".

Behavior

No change to what the suites test or how CI shards them. The GAIE and router
e2e jobs run exactly as before; only file names, the shared helper, and the target
names changed.

Which issue(s) this PR fixes:

Part of #955

Release note (write NONE if no user-facing change):

NONE

@roytman
test: consolidate and rename e2e runner scripts
9e59890 
Signed-off-by: roytman <roytman@il.ibm.com>
@roytman
roytman requested a review from a team as a code owner June 20, 2026 20:15
@roytman
roytman requested review from ahg-g, Copilot and vMaroon June 20, 2026 20:15
@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 20, 2026
Copilot AI reviewed Jun 20, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the e2e test runners by extracting shared bash helpers, renaming the runner scripts/Make targets to reflect which suite they execute (GAIE vs router), and updating CI to use the renamed router target.

Changes:

  • Add test/scripts/e2e-common.sh with shared interrupt cleanup and label-filtered Ginkgo runner logic.
  • Rename/consolidate the router and GAIE e2e runner scripts to test-e2e-router.sh and test-e2e-gaie.sh.
  • Rename Make targets from test-e2e-scheduler{,-run} to test-e2e-router{,-run} and update CI accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/scripts/test-e2e-router.sh New router e2e runner that sources shared helpers and runs the router suite.
test/scripts/test-e2e-gaie.sh Updated GAIE e2e runner to use shared helpers and new invocation pathing.
test/scripts/run_e2e.sh Removed old router runner script superseded by test-e2e-router.sh.
test/scripts/e2e-common.sh New shared bash helpers for interrupt cleanup and label-filtered Ginkgo execution.
Makefile Renamed e2e targets and updated to call the renamed runner scripts.
.github/workflows/ci-pr-checks.yaml Updated CI to call make test-e2e-router-run.
Comments suppressed due to low confidence (1)

test/scripts/test-e2e-gaie.sh:18

  • The set invocation looks malformed: -o expects an option-name argument, so set -euox pipefail can fail immediately. Since this script is now the one invoked by make test-e2e-gaie-run, it should use valid bash set syntax.

@roytman
fix comments
a2937ef 
Signed-off-by: roytman <roytman@il.ibm.com>
shmuelk
shmuelk reviewed Jun 21, 2026
View reviewed changes
Comment thread test/scripts/test-e2e-router.sh Outdated
shmuelk
shmuelk reviewed Jun 21, 2026
View reviewed changes
Comment thread test/scripts/e2e-common.sh Outdated
@shmuelk

shmuelk commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Looks great. Just two minor changes to the copyright statements

roytman and others added 2 commits June 21, 2026 11:51
@roytman @shmuelk
Update test/scripts/test-e2e-router.sh
de2b57d 
Co-authored-by: Shmuel Kallner <kallner@il.ibm.com>
Signed-off-by: Alexey Roytman <roytman@il.ibm.com>
@roytman @shmuelk
Update test/scripts/e2e-common.sh
acfd98c 
Co-authored-by: Shmuel Kallner <kallner@il.ibm.com>
Signed-off-by: Alexey Roytman <roytman@il.ibm.com>
@roytman

roytman commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Looks great. Just two minor changes to the copyright statements

@shmuelk , thank you for your review. I accepted your suggestions.

@ahg-g

ahg-g commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

The end goal is to merge into one e2e runner, right?

@ahg-g
ahg-g merged commit 2253a35 into llm-d:main Jun 21, 2026
29 checks passed
@roytman

roytman commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

The end goal is to merge into one e2e runner, right?

Indeed, that was my original plan. I initially combined them into a single file using if [ "$SUITE" = gaie ] branching, but found a few challenges:

  • The cluster-ownership logic, image sets, and env contracts genuinely differ. As a result, the merged script ended up being mostly two separate branches side-by-side, plus a few shared helpers.
  • The Makefile uses two separate targets: test-e2e-gaie-run and test-e2e-router-run.
  • In ci-pr-checks.yaml, two separate CI jobs (e2e-gaie and e2e-router) bind to those target names by string. They run as separate, parallel jobs with different inputs. Given how long these tests take to run, it is actually beneficial to keep them as separate jobs and Makefile targets.
  • Anyone running the merged script manually would still have to explicitly choose which part (or both) to execute.

@shmuelk

shmuelk commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

The end goal is to merge into one e2e runner, right?

See PR #1699 which merges the IGW tests into the llm-d-router tests. After that PR merges I will be removing the old IGW tests and combining the test utilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@shmuelk shmuelk shmuelk approved these changes
@ahg-g ahg-g ahg-g approved these changes
@vMaroon vMaroon Awaiting requested review from vMaroon vMaroon is a code owner automatically assigned from llm-d/router-maintainers

Assignees

No one assigned

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@roytman @shmuelk @ahg-g