ci: route Maestro e2e selection through sniffler impact analysis#7476
ci: route Maestro e2e selection through sniffler impact analysis#7476diegolmello wants to merge 3 commits into
Conversation
Regular dev PRs (and local runs) now execute only the Maestro flows sniffler flags as impacted; release-cut PRs still run the full 14-shard suite. Under-selection is impossible — every uncertainty falls back to the full suite. CI (build-pr.yml): - e2e-shards runs first and narrows the shard matrix via select-impacted-shards.sh (release-lane / no-base / merge-base-fail / sniffler-error / bad-JSON / run-all all fall back to full 14). - A confident zero (no impacted flow, e.g. docs-only) sets should_run=false, skipping the whole e2e stage (no approval, no native build, no flows). - New always() e2e-result job is the single required e2e check: green on a confident-zero skip, green iff both run jobs pass, red otherwise — never hangs "Waiting for status". Local: pnpm e2e:changed <android|ios> runs the impacted flows against a booted device, working-tree-aware, mirroring CI's tag excludes. Coverage: .sniffler config maps runtime-decoupled files the import graph can't trace — domain sagas anchored to their flows, global sagas / routers / nav stacks / root container / store / root reducer in runAllWhenChanged. Freshness: validate-test-map.js (wired into eslint.yml) fails a PR on orphan flows, dangling globs, stale globals, or any uncovered saga/stack (decoupled gap); warns on uncovered views. NOTE for whoever merges: branch-protection must be reconfigured to require the e2e-result check and de-require the per-shard + build contexts, or a confident-zero PR hangs on missing contexts.
WalkthroughAdds Sniffler-based impacted-test selection for local e2e runs and CI, plus validation of the test-map data and workflow gating for conditional Maestro execution. ChangesImpacted E2E selection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant build_pr as build-pr.yml
participant select as select-impacted-shards.sh
participant sniffler as sniffler impact
participant hold as e2e-hold
participant e2e as e2e build/run jobs
participant result as e2e-result
build_pr->>select: run e2e-shards preflight
select->>sniffler: pnpm exec sniffler impact
sniffler-->>select: recommendedTests JSON
alt should_run=true
select-->>build_pr: shards + should_run=true
build_pr->>hold: request approval gate
hold->>e2e: run Android/iOS build and run jobs
e2e-->>result: shard outcomes
else should_run=false
select-->>build_pr: shards=[] + should_run=false
build_pr->>result: skip E2E stage
end
result-->>build_pr: aggregate status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/scripts/select-impacted-shards.sh:
- Around line 46-62: The `select-impacted-shards.sh` logic only checks that
`recommendedTests` exists, so a `null` value can slip through and make the
`.recommendedTests[]` queries in the run-all and paths handling behave like “no
tests,” causing an incorrect confident-zero skip. Update the validation around
the `recommendedTests` check in this script to require that `recommendedTests`
is an array before proceeding, and keep malformed payloads on the
full/emit-false path rather than letting `paths` become empty. Use the existing
`recommendedTests` handling block and the `paths`/`emit` flow to place the type
guard so under-selection cannot happen.
In @.github/scripts/validate-test-map.js:
- Around line 69-70: The uncovered-view check in validate-test-map.js is using a
prefix match that can falsely treat sibling directories as covered. Update the
uncovered calculation around allDependsOn and viewDirs so it only matches an
exact view anchor or a true descendant path, not any string that merely starts
with the directory name. Keep the fix localized to the coverage filter logic and
preserve the intended warning behavior for real uncovered view directories.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a0aea5b-415f-4839-88ba-cb5624c68c9d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/scripts/e2e-changed.sh.github/scripts/select-impacted-shards.sh.github/scripts/validate-test-map.js.github/workflows/build-pr.yml.github/workflows/eslint.yml.gitignore.sniffler/config.json.sniffler/test-map.jsonpackage.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: E2E Shard Preflight
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2026-05-07T17:47:14.516Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7303
File: package.json:5-5
Timestamp: 2026-05-07T17:47:14.516Z
Learning: When reviewing pnpm `packageManager` version pins in any `package.json` (e.g., `"packageManager": "pnpm@<version>"`), don’t rely solely on web-search results to determine whether a version exists. For very recently published versions, cross-check the target version against the official pnpm release page (https://github.com/pnpm/pnpm/releases) and the npm registry page for pnpm (https://www.npmjs.com/package/pnpm) before flagging the pinned version as non-existent.
Applied to files:
package.json
🪛 ast-grep (0.44.1)
.github/scripts/validate-test-map.js
[warning] 34-34: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(TEST_MAP_PATH, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 35-35: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(CONFIG_PATH, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 40-40: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(f, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🪛 zizmor (1.26.1)
.github/workflows/build-pr.yml
[warning] 74-77: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 107-107: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
[warning] 127-127: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
🔇 Additional comments (8)
.github/scripts/e2e-changed.sh (1)
1-54: LGTM!package.json (2)
22-22: LGTM!
216-220: Checksniffler's published version
package.jsonpinssnifflerto^0.4.0, and the lockfile currently resolvessniffler@0.4.0. Keep this only if that release is the intended published version..sniffler/config.json (1)
1-33: LGTM!.sniffler/test-map.json (1)
1-336: LGTM!.gitignore (1)
98-98: LGTM!.github/workflows/build-pr.yml (1)
63-177: LGTM!.github/workflows/eslint.yml (1)
22-24: LGTM!
- Add unit tests for select-impacted-shards.sh, e2e-changed.sh and validate-test-map.js: per-branch fixture maps plus a shell harness that stubs external deps (sniffler/git/maestro) on $PATH. They wire into the existing jest run, so CI's ESLint-and-test job exercises the selection tooling the PR previously only verified by hand. - Fix an uncovered-view false negative in validate-test-map.js: the prefix match let a view dir cover-match a longer dir (SomeView vs SomeViewExtra/**); anchor it on a trailing slash. Add a fixture that proves it. - Add a TESTMAP_ROOT override so the validator can run against fixtures. - Pin sniffler to an exact version: it gates what e2e runs, so range drift should be an explicit reviewed change (matches fast-glob's pin). - Update .github/README.md for the new e2e-shards preflight, the e2e-result required check, and the conditional approve_e2e_testing gate.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/scripts/testlib/runScript.js (1)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider escaping the
keybefore interpolating into the RegExp.The
getfunction interpolateskeydirectly into aRegExpconstructor. Current callers only passshardsandshould_run(no metacharacters), so this is safe today. Adding a simple escape would future-proof the helper against accidental misuse.♻️ Optional refactor
const get = key => { - const m = output.match(new RegExp(`^${key}=(.*)$`, 'm')); + const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const m = output.match(new RegExp(`^${escaped}=(.*)$`, 'm')); return m ? m[1] : undefined; };🤖 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/scripts/testlib/runScript.js around lines 45 - 48, The get helper in runScript.js interpolates key directly into the RegExp pattern, so update that lookup logic to escape any regex metacharacters in key before building the expression. Keep the change scoped to the get function so callers like the shards and should_run lookups continue to work, but future keys cannot accidentally change the match behavior. Ensure the escaped key is used in the RegExp constructor and the rest of the parsing logic stays the same.
🤖 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/scripts/testlib/runScript.js:
- Around line 45-48: The get helper in runScript.js interpolates key directly
into the RegExp pattern, so update that lookup logic to escape any regex
metacharacters in key before building the expression. Keep the change scoped to
the get function so callers like the shards and should_run lookups continue to
work, but future keys cannot accidentally change the match behavior. Ensure the
escaped key is used in the RegExp constructor and the rest of the parsing logic
stays the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0bdd7dc-adbf-47db-b80f-ed91a78bf69f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.github/README.md.github/scripts/__tests__/changed-runner.test.js.github/scripts/__tests__/coverage.test.js.github/scripts/__tests__/fixtures/flows/no-tag.yaml.github/scripts/__tests__/fixtures/scenario-catalog.json.github/scripts/__tests__/select-impacted-shards.test.js.github/scripts/__tests__/validate-test-map.test.js.github/scripts/testlib/fixtures/maps/clean-pass/.maestro/tests/foo.yaml.github/scripts/testlib/fixtures/maps/clean-pass/.sniffler/config.json.github/scripts/testlib/fixtures/maps/clean-pass/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/clean-pass/app/sagas/foo.js.github/scripts/testlib/fixtures/maps/clean-pass/app/stacks/bar.tsx.github/scripts/testlib/fixtures/maps/clean-pass/app/views/Foo/placeholder.txt.github/scripts/testlib/fixtures/maps/dangling-glob/.maestro/tests/.gitkeep.github/scripts/testlib/fixtures/maps/dangling-glob/.sniffler/config.json.github/scripts/testlib/fixtures/maps/dangling-glob/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/dangling-glob/app/views/.gitkeep.github/scripts/testlib/fixtures/maps/decoupled-gap/.maestro/tests/.gitkeep.github/scripts/testlib/fixtures/maps/decoupled-gap/.sniffler/config.json.github/scripts/testlib/fixtures/maps/decoupled-gap/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/decoupled-gap/app/sagas/foo.js.github/scripts/testlib/fixtures/maps/decoupled-gap/app/views/.gitkeep.github/scripts/testlib/fixtures/maps/orphan-flow/.maestro/tests/orphan.yaml.github/scripts/testlib/fixtures/maps/orphan-flow/.sniffler/config.json.github/scripts/testlib/fixtures/maps/orphan-flow/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/orphan-flow/app/views/.gitkeep.github/scripts/testlib/fixtures/maps/prefix-collision/.maestro/tests/some.yaml.github/scripts/testlib/fixtures/maps/prefix-collision/.sniffler/config.json.github/scripts/testlib/fixtures/maps/prefix-collision/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/prefix-collision/app/views/SomeView/placeholder.txt.github/scripts/testlib/fixtures/maps/prefix-collision/app/views/SomeViewExtra/placeholder.txt.github/scripts/testlib/fixtures/maps/stale-global/.maestro/tests/.gitkeep.github/scripts/testlib/fixtures/maps/stale-global/.sniffler/config.json.github/scripts/testlib/fixtures/maps/stale-global/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/stale-global/app/views/.gitkeep.github/scripts/testlib/fixtures/maps/uncovered-view/.maestro/tests/.gitkeep.github/scripts/testlib/fixtures/maps/uncovered-view/.sniffler/config.json.github/scripts/testlib/fixtures/maps/uncovered-view/.sniffler/test-map.json.github/scripts/testlib/fixtures/maps/uncovered-view/app/views/SomeView/placeholder.txt.github/scripts/testlib/runScript.js.github/scripts/validate-test-map.jspackage.json
✅ Files skipped from review due to trivial changes (18)
- .github/scripts/testlib/fixtures/maps/decoupled-gap/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/stale-global/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/uncovered-view/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/prefix-collision/.maestro/tests/some.yaml
- .github/scripts/testlib/fixtures/maps/clean-pass/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/clean-pass/app/stacks/bar.tsx
- .github/scripts/testlib/fixtures/maps/uncovered-view/app/views/SomeView/placeholder.txt
- .github/scripts/testlib/fixtures/maps/clean-pass/app/sagas/foo.js
- .github/scripts/testlib/fixtures/maps/prefix-collision/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/clean-pass/app/views/Foo/placeholder.txt
- .github/scripts/testlib/fixtures/maps/stale-global/.sniffler/test-map.json
- .github/scripts/testlib/fixtures/maps/decoupled-gap/.sniffler/test-map.json
- .github/scripts/testlib/fixtures/maps/clean-pass/.maestro/tests/foo.yaml
- .github/scripts/testlib/fixtures/maps/dangling-glob/.sniffler/test-map.json
- .github/scripts/testlib/fixtures/maps/orphan-flow/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/dangling-glob/.sniffler/config.json
- .github/scripts/testlib/fixtures/maps/orphan-flow/.maestro/tests/orphan.yaml
- .github/scripts/testlib/fixtures/maps/decoupled-gap/app/sagas/foo.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .github/scripts/validate-test-map.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: E2E Shard Preflight
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (12)
.github/scripts/__tests__/fixtures/flows/no-tag.yaml (1)
1-5: LGTM!.github/scripts/__tests__/fixtures/scenario-catalog.json (1)
1-164: LGTM!.github/scripts/testlib/fixtures/maps/clean-pass/.sniffler/test-map.json (1)
1-7: LGTM!.github/scripts/__tests__/select-impacted-shards.test.js (1)
1-187: LGTM!.github/scripts/__tests__/coverage.test.js (1)
1-93: LGTM!.github/scripts/__tests__/validate-test-map.test.js (1)
1-74: LGTM!.github/scripts/testlib/fixtures/maps/orphan-flow/.sniffler/test-map.json (1)
1-1: LGTM!.github/scripts/testlib/fixtures/maps/prefix-collision/.sniffler/test-map.json (1)
1-6: LGTM!.github/scripts/testlib/fixtures/maps/uncovered-view/.sniffler/test-map.json (1)
1-1: LGTM!.github/scripts/__tests__/changed-runner.test.js (1)
1-198: LGTM!.github/README.md (2)
9-9: 📐 Maintainability & Code QualityDocument the release-cut full-suite bypass. The CI docs should mention the release-cut path (
master/releaselabel) runs the full 14-shard suite instead of the sniffler-selected subset.
48-63: Outdated E2E job names in the diagram. The current workflow usese2e-run-androidande2e-run-ios, note2e-result, so the missing-edge concern no longer applies.> Likely an incorrect or invalid review comment.
Proposed changes
E2E test selection now runs through sniffler impact analysis, so a regular dev PR only executes the Maestro flows its diff actually touches instead of the full 14-shard suite every time. Release-cut PRs (base
masteror areleaselabel) still run all 14. The design's hard rule is that under-selection is impossible — every uncertainty (no base ref, shallow clone, sniffler error, unparseable output, arunAllWhenChangedhit, a release lane) falls back to the full suite; the optimization only ever narrows, never risks missing coverage.CI (
build-pr.yml)e2e-shardsnow runs first and narrows the shard matrix viaselect-impacted-shards.sh. All six fail-safe guards fall back to the full 14.should_run=falseand skips the whole e2e stage: no manual approval, no native build, no flows.always()e2e-resultjob is the single required e2e check: green on a confident-zero skip, green when both run jobs pass, red otherwise. It never hangs "Waiting for status".Local
pnpm e2e:changed <android|ios>runs the impacted flows against a booted device with the app installed. It is working-tree-aware (committed + uncommitted + untracked) and mirrors CI'sutil/ wrong-platform tag excludes.Coverage + freshness
.sniffler/config.json+test-map.jsonmap the runtime-decoupled surface the import graph can't trace: domain sagas anchored to their flows; global sagas, notification/stream routers, navigation stacks, the root container, the store, and the root reducer inrunAllWhenChanged.validate-test-map.js(wired intoeslint.yml) fails a PR on orphan flows, dangling globs, stale globals, or any uncovered saga/stack, and warns on uncovered views — so the map can't silently rot.Tests
select-impacted-shards.sh,e2e-changed.sh, andvalidate-test-map.js— so the logic that gates e2e is exercised on everyESLint and Testrun, not just verified by hand. A shell harness stubs the external deps (sniffler/git/maestro) on$PATH; per-branch fixture maps drive each validator check.Other
sniffleris pinned to an exact version — it decides what e2e runs, so range drift should be an explicit reviewed change.validate-test-map.js: the prefix match let a view dir cover-match a longer sibling (e.g.SomeViewvsSomeViewExtra/**); it now anchors on a trailing slash, with a fixture proving it..github/README.mdupdated for the newe2e-shardspreflight, thee2e-resultrequired check, and the now-conditionalapprove_e2e_testinggate.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1405
How to test or reproduce
CI evidence
Real
build-pr.ymlruns, exercised on a throwaway branch stacked on this PR (opened/closed as #7477 — do not look for it, it's gone):e2e-shardsdecisionapp/views/RoomView/index.tsxshould_run=true, matrix narrowed to shards[3,7,9,11,12,13,14](7 of 14) on both Android + iOS;E2E Holdapproved and greenREADME.md)should_run=false;E2E Holdand the whole e2e stage skipped — no approval, no build, no flowspackage.json(this PR's own run)should_run=true, full 14-shard suite queued behind the manualapprove_e2e_testinggateThe narrow run's shard list is visible directly in the
E2E Run Android/iOS (N)matrix job names; the skip run showsE2E Holdand downstream e2e jobs as skipped.Local verification
Against the live
sniffler@0.4.0binary (exact-restore, nothing left in the tree):IS_RELEASE_LANE=true→ all 14, short-circuits before sniffler.pnpm e2e:changed androidon the same RoomView edit → the exact same flows the dev-PR shards cover, no under-selection.sniffler impactonAppContainer.tsx, each nav stack,lib/store/index.ts,reducers/index.js→ all select the full 55 flows.node .github/scripts/validate-test-map.js→ exit 0 (0 errors, 8 uncovered-view warnings); trips to exit 1 on a synthetic orphan / dangling glob / decoupled gap.TZ=UTC pnpm test --testPathPattern='.github/scripts/__tests__'→ all pass; runs as part of the normalESLint and Testjob.Screenshots
N/A — CI + tooling only, no user-facing UI.
Types of changes
Checklist
Further comments
Branch protection: no change required on merge.
develop's required checks arelicense/cla,ESLint and Test, andCodeRabbit— no per-shard or native-build context is required, and e2e gates through a manual approval environment rather than a required status check. So a confident-zero PR can't hang "Waiting for status". Optionally, to make e2e a hard merge gate, addE2E Resultto the required checks — it's safe with confident-zero because the job isalways()and reports green on a skip. Do not add the per-shard (E2E Run Android (N)…) or native-build contexts: they zero out on a narrowed run. That optional follow-up (to be done only after merge) is tracked in https://rocketchat.atlassian.net/browse/NATIVE-1408.Seam choice: shard-level narrowing (not per-flow) in CI keeps
run-maestro.sh, bothmaestro-*.yml, and the retry loop untouched — over-selection at shard granularity is acceptable since selection is a dev-speed optimization, never a correctness gate.Summary by CodeRabbit