feat(e2e): generate scenario fan-out matrix from typed registry#4359
Open
jyaunches wants to merge 1 commit into
Open
feat(e2e): generate scenario fan-out matrix from typed registry#4359jyaunches wants to merge 1 commit into
jyaunches wants to merge 1 commit into
Conversation
Replace the hand-wired job list in e2e-scenarios-all.yaml with a
generated matrix sourced from the existing typed scenario registry.
Adding a scenario in test/e2e-scenario/scenarios/scenarios/baseline.ts
now automatically produces a tile in the fan-out workflow on the next
run, with no workflow edits required. This closes the drift gap that
left ~14 of 22 registered scenarios undispatched.
Changes:
- Add --emit-matrix flag to test/e2e-scenario/scenarios/run.ts that
walks listScenarios() and prints a single-line JSON array of GHA
matrix include entries: { id, runner, label, platform, suites }.
- Extract runner-routing.ts that derives the runs-on label from
ScenarioEnvironment.platform, with a runs-on:<label> override path
via runnerRequirements. Throws on unknown platforms so missing
mappings fail loudly during matrix generation.
- Refactor .github/workflows/e2e-scenarios-all.yaml to a two-job
shape: generate-matrix emits the JSON, run-scenario fans out via
strategy.matrix.include and calls the existing e2e-scenarios.yaml
reusable workflow. Tile names come from the typed label so the
sidebar reflects what is actually being tested.
- Guard the run.ts CLI bootstrap so importing buildScenarioMatrix
from tests does not trigger the side-effecting main() path.
- Add e2e-scenario-matrix.test.ts covering matrix shape, registry
parity, runner overrides, unknown-platform errors, and CLI output
contract.
Contributor
📝 WalkthroughWalkthroughThis PR replaces static GitHub Actions job enumeration with a runtime-generated matrix by introducing typed runner resolution logic, extending the scenario script with a ChangesDynamic Scenario Matrix Generation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: 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: 1 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-scenario/scenarios/run.ts (1)
104-119: ⚡ Quick winApply the documented id sort before matrix emission.
The doc says matrix entries are sorted by id, but the implementation currently preserves registry order. Sorting here makes output deterministically diffable as intended.
Proposed fix
export function buildScenarioMatrix(): ScenarioMatrixEntry[] { - return listScenarios().map((scenario): ScenarioMatrixEntry => { + return [...listScenarios()] + .sort((a, b) => a.id.localeCompare(b.id)) + .map((scenario): ScenarioMatrixEntry => { const { runner } = resolveRunnerForScenario(scenario); return { id: scenario.id, runner, label: buildLabel(scenario), platform: scenario.environment?.platform ?? "unknown", suites: scenario.suiteIds ?? [], }; }); }🤖 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-scenario/scenarios/run.ts` around lines 104 - 119, The buildScenarioMatrix function currently maps listScenarios() without sorting, so update buildScenarioMatrix to sort the scenarios by their id before mapping (e.g., call .sort(...) on the array returned by listScenarios()) so the returned ScenarioMatrixEntry[] is deterministically ordered by scenario.id; locate buildScenarioMatrix and adjust the pipeline that uses listScenarios() and resolveRunnerForScenario(scenario) to operate on a sorted array (use a string compare/localeCompare on scenario.id).
🤖 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.
Inline comments:
In `@test/e2e-scenario/scenarios/runner-routing.ts`:
- Around line 39-42: The code currently accepts a `runs-on:` override that can
slice to an empty string; update the block that finds `explicit` from
`scenario.runnerRequirements` so it rejects empty labels — e.g., after computing
`const label = explicit.slice("runs-on:".length)` (or by ensuring `req.length >
"runs-on:".length` when matching) validate `label !== ""` and if empty throw a
clear Error like "Invalid runs-on: override: empty runner label" (or return a
failing result) instead of returning `{ runner: "", ... }`; refer to the
`explicit` variable and the `runnerRequirements` lookup when making this change.
---
Nitpick comments:
In `@test/e2e-scenario/scenarios/run.ts`:
- Around line 104-119: The buildScenarioMatrix function currently maps
listScenarios() without sorting, so update buildScenarioMatrix to sort the
scenarios by their id before mapping (e.g., call .sort(...) on the array
returned by listScenarios()) so the returned ScenarioMatrixEntry[] is
deterministically ordered by scenario.id; locate buildScenarioMatrix and adjust
the pipeline that uses listScenarios() and resolveRunnerForScenario(scenario) to
operate on a sorted array (use a string compare/localeCompare on scenario.id).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48621b06-d779-4cd5-a9ce-513bb8c854ab
📒 Files selected for processing (4)
.github/workflows/e2e-scenarios-all.yamltest/e2e-scenario/framework-tests/e2e-scenario-matrix.test.tstest/e2e-scenario/scenarios/run.tstest/e2e-scenario/scenarios/runner-routing.ts
Comment on lines
+39
to
+42
| const explicit = (scenario.runnerRequirements ?? []).find((req) => req.startsWith("runs-on:")); | ||
| if (explicit) { | ||
| return { runner: explicit.slice("runs-on:".length), reason: "runnerRequirements override" }; | ||
| } |
Contributor
There was a problem hiding this comment.
Reject empty runs-on: overrides to prevent invalid runner labels.
runs-on: with no label currently resolves to an empty string and propagates into the matrix. Fail fast here with a clear error.
Proposed fix
export function resolveRunnerForScenario(scenario: ScenarioDefinition): ResolvedRunner {
const explicit = (scenario.runnerRequirements ?? []).find((req) => req.startsWith("runs-on:"));
if (explicit) {
- return { runner: explicit.slice("runs-on:".length), reason: "runnerRequirements override" };
+ const runner = explicit.slice("runs-on:".length).trim();
+ if (!runner) {
+ throw new Error(`Cannot resolve runner for scenario '${scenario.id}': empty runs-on override.`);
+ }
+ return { runner, reason: "runnerRequirements override" };
}🤖 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-scenario/scenarios/runner-routing.ts` around lines 39 - 42, The code
currently accepts a `runs-on:` override that can slice to an empty string;
update the block that finds `explicit` from `scenario.runnerRequirements` so it
rejects empty labels — e.g., after computing `const label =
explicit.slice("runs-on:".length)` (or by ensuring `req.length >
"runs-on:".length` when matching) validate `label !== ""` and if empty throw a
clear Error like "Invalid runs-on: override: empty runner label" (or return a
failing result) instead of returning `{ runner: "", ... }`; refer to the
`explicit` variable and the `runnerRequirements` lookup when making this change.
wscurran
added
E2E
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.
Why
The
e2e-scenarios-all.yamlfan-out has 8 hand-wired jobs, but the typed scenario registry inbaseline.tsalready defines 22 scenarios. The other 14 are defined-but-not-dispatched, and every new scenario today requires a YAML edit that's easy to forget. The hand-wired job names also no longer reflect what each scenario actually tests (manifest, suites, expectedState).This PR makes the fan-out generated from the existing typed registry so the workflow can never drift from
baseline.tsagain.What changed
--emit-matrixflag onrun.tsWalks
listScenarios()(the existing registry — no new registry created) and prints a single-line JSON array of GHA matrixincludeentries:[ { "id": "ubuntu-repo-cloud-openclaw", "runner": "ubuntu-latest", "label": "ubuntu-local · ubuntu-repo-cloud-openclaw · smoke+inference+credentials", "platform": "ubuntu-local", "suites": ["smoke", "inference", "credentials"] }, ... ]runner-routing.tshelperDerives the
runs-onlabel fromScenarioEnvironment.platform, with aruns-on:<label>override path viarunnerRequirements. Throws on unknown platforms so missing mappings fail loudly during matrix generation rather than silently falling back toubuntu-latest(which used to mask routing bugs in the legacy bashROUTESmap).e2e-scenarios-all.yamlrefactorTwo-job shape:
generate-matrix— checks out, installs deps, runs--emit-matrix, and writes the JSON to$GITHUB_OUTPUT. Also renders a Markdown table of all matrix entries in the step summary so you can see the full plan at a glance.run-scenario— usesstrategy.matrix.include: ${{ fromJson(needs.generate-matrix.outputs.matrix) }}and calls the existinge2e-scenarios.yamlreusable workflow per scenario. Tile names use${{ matrix.label }}so the sidebar reflects what's actually being tested.Adding a scenario from now on
canonicalScenarioInputsinbaseline.ts.What didn't change
e2e-scenarios.yaml(the single-scenario reusable workflow) is untouched. Itsresolve-runnerjob continues to be the authoritative runner-selection path during execution; therunnerfield in the matrix is informational and used for the sidebar label.ROUTESmap ine2e-scenarios.yamlis left in place for now to keep this PR focused. It becomes effectively dead code once this lands and can be removed in a follow-up.test/e2e-scenario/scenarios/registry.ts.Verification
Unit tests in
e2e-scenario-matrix.test.tscover:runs-on:<label>override$GITHUB_OUTPUTLocal run:
Manual smoke:
Test depth
Unit tests are sufficient — this is a build/CI plumbing change, not a behavioral change to the test runner or scenarios themselves. The actual scenario execution path (
e2e-scenarios.yaml) is unchanged. After merge, the next manualworkflow_dispatchofE2E / Scenario Runner / Allwill validate the matrix end-to-end.Follow-ups (separate PRs)
ROUTESbash map ine2e-scenarios.yamland have itsresolve-runnerjob consumerunner-routing.tstoo.e2e-scenarios-all.yamlby platform once we cross ~80 scenarios (currently at 22, plenty of headroom against the 256-job GHA ceiling).Summary by CodeRabbit
Tests
Chores