fix(ci): select shards for changes outside app and honor the release label - #7555
Conversation
…label Sniffler put no file outside app/ into the import graph, and runAllWhenChanged listed only paths inside app/. A change to a runtime file elsewhere matched no Maestro flow, so select-impacted-shards.sh reported a confident-zero diff and CI skipped every shard. A patch to the DDP socket under patches/ selected nothing, though every flow depends on it. Add the native and build paths to runAllWhenChanged. Sniffler reads each item as a pattern, so validate-test-map.js must accept a glob; its existsSync check refused every glob as stale. The release lane used contains() over the label array, which is an exact item match, so it required a label named exactly "release" and never matched the decorated name. The workflow also declares no types: key, so it never starts on a label event and a re-run reads the old event data. Read the labels with gh pr view during the job instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughThe PR adds glob-aware stale-trigger validation, expands Sniffler rerun patterns, and updates E2E shard selection to detect release labels from the live pull request. ChangesTest-trigger validation
Release-lane detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant gh
participant GitHubAPI
GitHubActions->>gh: Query pull request labels
gh->>GitHubAPI: Read current pull request labels
GitHubAPI-->>gh: Return label names
gh-->>GitHubActions: Write release boolean to GITHUB_OUTPUT
GitHubActions->>GitHubActions: Set IS_RELEASE_LANE for shard selection
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
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
🧹 Nitpick comments (2)
.github/scripts/__tests__/validate-test-map.test.js (1)
55-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for dot-directory globs.
The production
.sniffler/config.jsonadds.maestro/**at Line 19, but these tests cover onlypatches/**and the unmatchednative/**. Add a hidden fixture such as.maestro/flow.yamland assert that.maestro/**is not reported stale. This protects thedot: truebehavior in.github/scripts/validate-test-map.js. (npmjs.com)🤖 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/__tests__/validate-test-map.test.js around lines 55 - 67, Add a hidden-directory fixture such as .maestro/flow.yaml to the test setup, then add coverage in the stale-global validation tests to verify the .maestro/** runAllWhenChanged glob is not reported as stale. Keep the assertion focused on preserving dot: true matching behavior in validate-test-map.js..github/scripts/validate-test-map.js (1)
79-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
fast-glob’s dynamic-pattern check forrunAllWhenChanged.
fast-globtreats extglob patterns such as@(android|ios)/**,+(android|ios)/**, and!(native)/**as dynamic patterns by default. The localisGlobpredicate only checks*,?, brackets, and braces, so those valid patterns can fall through tofs.existsSync()and be reported as stale.Suggested alignment
-const isGlob = p => /[*?[\]{}]/.test(p); +const isGlob = p => fg.isDynamicPattern(p);🤖 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/validate-test-map.js around lines 79 - 82, Update the local isGlob predicate used by staleGlobals to rely on fast-glob’s dynamic-pattern detection, so extglob patterns such as @(...), +(...), and !(...) follow fg.sync rather than fs.existsSync. Preserve the existing stale-entry behavior for both dynamic and literal paths.
🤖 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/workflows/build-pr.yml:
- Around line 108-112: Update the release-label check around the gh pr view
invocation to fail when label lookup errors, while treating a successful lookup
with no release label as non-release. Use IS_RELEASE_LANE and its
false/non-release values in the selection logic, rather than allowing every
missing output or command failure to enter the release=false branch.
- Line 108: Update the label check in the workflow’s release-lane condition to
match the exact configured release label name, rather than using the substring
pattern “release”; preserve case-insensitive matching and ensure labels such as
“release-notes” and “pre-release” do not qualify.
---
Nitpick comments:
In @.github/scripts/__tests__/validate-test-map.test.js:
- Around line 55-67: Add a hidden-directory fixture such as .maestro/flow.yaml
to the test setup, then add coverage in the stale-global validation tests to
verify the .maestro/** runAllWhenChanged glob is not reported as stale. Keep the
assertion focused on preserving dot: true matching behavior in
validate-test-map.js.
In @.github/scripts/validate-test-map.js:
- Around line 79-82: Update the local isGlob predicate used by staleGlobals to
rely on fast-glob’s dynamic-pattern detection, so extglob patterns such as
@(...), +(...), and !(...) follow fg.sync rather than fs.existsSync. Preserve
the existing stale-entry behavior for both dynamic and literal paths.
🪄 Autofix
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 Plus
Run ID: 54811d12-1799-4263-841b-3f9673792e19
📒 Files selected for processing (6)
.github/scripts/__tests__/validate-test-map.test.js.github/scripts/testlib/fixtures/maps/stale-global/.sniffler/config.json.github/scripts/testlib/fixtures/maps/stale-global/patches/some.patch.github/scripts/validate-test-map.js.github/workflows/build-pr.yml.sniffler/config.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: E2E Shard Preflight
🔇 Additional comments (5)
.github/scripts/testlib/fixtures/maps/stale-global/.sniffler/config.json (1)
3-3: LGTM!.github/scripts/testlib/fixtures/maps/stale-global/patches/some.patch (1)
1-1: LGTM!.sniffler/config.json (1)
11-19: LGTM!.github/workflows/build-pr.yml (2)
80-80: LGTM!
105-108: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSensitive Data Exposure (CWE-522): Insufficiently Protected Credentials
Reachability: External
Move the token-bearing lookup before PR code execution.
This job runs repository actions and scripts before
Detect release label. On apull_requestrun, a malicious PR can prepend a directory containing a fakeghexecutable throughGITHUB_PATH. Line 108 then runs that executable withGH_TOKEN, which can expose the token.Move label detection to a trusted step or job before checkout. Pass only the boolean result to the E2E job.
GitHub makes
GITHUB_PATHentries available to later actions,ghacceptsGH_TOKEN, and GitHub documents that runner tokens can be harvested by code executing on the runner. (docs.github.com)
db3c427 to
d9de38b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
* fix: render mentions, emojis, and inline elements inside headings (#6911) * chore: OXC (#7515) * chore: replace ESLint with Oxlint Migrate linting from ESLint 8 to Oxlint. `pnpm lint` drops from ~1min to ~0.6s and 17 eslint packages are removed from devDependencies. Config lives in `.oxlintrc.json`, generated with `@oxlint/migrate` from the old `.eslintrc.js` and then tuned: - `eslint-plugin-react-native` is loaded through `jsPlugins`, since Oxlint has no built-in equivalent. - `import/extensions` is off. Its old options (`js: 'warning'`, ...) were not valid values for the rule, so it never reported anything under ESLint. - `no-unused-vars` sets `caughtErrors: 'none'` to match the ESLint 8 default. - `import/no-cycle` and the React Compiler rules report as warnings. They surface findings ESLint never showed, so they are not gated yet. Rules with no Oxlint equivalent are dropped: `no-restricted-syntax` (the ban on `React.*` member syntax), `import/order`, `import/no-unresolved` and `import/named`. ESLint 8 skipped dot-directories, so files under `.rnstorybook/` and `.maestro/` were never linted. Oxlint does lint them, which surfaced four violations that are fixed here. The CI workflow keeps its filename and job id so branch protection checks stay valid. * chore: replace Prettier with Oxfmt Migrate formatting from Prettier 2.8.8 to Oxfmt via `oxfmt --migrate prettier`. - `.oxfmtrc.json` carries every previous Prettier option unchanged (tabs, single quotes, printWidth 130, no trailing comma, avoid arrow parens, bracketSameLine) plus the `.prettierignore` patterns as `ignorePatterns`. `sortPackageJson` is disabled to match previous behavior. - `.prettierrc.js` and `.prettierignore` removed; `prettier` dropped from devDependencies. - `prettier-lint` script renamed to `format-lint` and now runs `oxfmt`. - 44 files reformatted: Oxfmt follows Prettier 3 style, so nested ternaries and `extends`/type-argument wrapping indent differently than under Prettier 2. No semantic changes. - prettier.yml still ran `eslint --fix`, missed in the Oxlint migration; it now runs `oxfmt` and `oxlint --fix`. `react/exhaustive-deps` is allowed there because its autofix rewrites dependency arrays, which changes behavior and must not land unreviewed from CI. - Workflow filename kept as prettier.yml to avoid disturbing branch protection checks, same as eslint.yml. Verified: `pnpm lint` exit 0 (0 errors), `tsc` clean, 217/217 suites and 2032/2032 tests pass, `oxfmt --check` clean. * chore: update lockfile for oxfmt * chore: migrate typecheck to TypeScript 7.0 (#7516) * chore: bump TypeScript to 6.0 Baseline hop ahead of the TypeScript 7.0 (native compiler) migration, so the 7.0 cut is a version swap against a config that is already 7.0-shaped. TypeScript 6.0 raises both `moduleResolution: node10` and `baseUrl` as errors rather than warnings, and `ignoreDeprecations: "6.0"` stops working in 7.0, so clearing them properly is the only route: - `moduleResolution` -> `bundler` (Metro is a bundler), which requires an esnext-shaped `module`. - `baseUrl` removed. Exactly one import relied on it; it is now relative. - `types` enumerated, since a resolution mode that honours package `exports` no longer auto-includes every `@types` package. `@types/node` becomes an explicit devDependency. Honouring `exports` also stranded the bundled `.d.ts` of three dependencies whose maps expose only JavaScript. Each gets a `types` condition via patch-package; this is visible to the type checker only, as Metro ignores that condition. A `paths` mapping was tried first and rejected, because the jest-expo resolver reads `paths` and then loads those `.d.ts` files at runtime. The inherited block of commented-out option documentation is dropped. * chore: migrate typecheck to TypeScript 7.0 Replaces TypeScript 6.0.3 with the native Go compiler. The version is pinned exactly, since the platform binaries ship as version-matched optional dependencies; the lockfile records the linux-x64 target CI resolves. Typecheck wall time drops from 5.77s on 5.9.3 to ~1.0s. No configuration change was required: the 6.0 hop already left tsconfig in a 7.0-shaped state, and the default parallelism saturates without `--checkers`. `@react-navigation/core` needs a patch to type-check. TypeScript 7.0.2 resolves the mutual recursion between `StaticParamList` and `ParamListForScreens` eagerly where earlier versions defer it, reports the alias as circular, and degrades it to a non-generic symbol -- surfacing as `TS2315: Type 'StaticParamList' is not generic` at our call sites. The patch drops a `FlatType<>` wrapper from the alias, which only flattens intersections for editor display, so the type is unchanged and the misfire stops. * chore: Bump version to 4.76.0 (#7542) * chore(ci): apply least privilege permission to GitHub Actions (#7350) * chore: switch React Compiler to infer mode (#7545) * ci: route Maestro e2e selection through sniffler impact analysis (#7476) * fix(iOS): RoomItem Swipe not working after scroll (#7532) * fix(ci): select shards for changes outside app and honor the release label (#7555) * fix: delete background taller than its row on ServersHistory (#7536) * fix: delete background taller than its row on server items * chore: code improvements --------- Co-authored-by: Diego Mello <diegolmello@gmail.com> * fix: UIKit block messages rendering with smaller font size (#7531) * fix: UIKit block messages rendering with smaller font size * fix: snapshot * fix(db): move deleteMessage finds and prepares inside the writer lock (#7550) * fix: quote has no effect on older thread messages (#7535) * fix: Quote has no effect on older thread messages * fix: Quote has no effect on older thread messages * chore: e2e test * fix: e2e test * chore: format code and fix lint issues * fix(MessageComposer): resolve quoted thread messages and guard stale lookups * fix: test --------- Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> * fix(db): move persistMessage lookups and prepares inside the writer lock (#7551) * fix: test case 11 and 12 flaky tests (#7564) * fix: test * fix: room last messa test * fix: jumptomessage test * chore: remove comments * fix: jump to message e2e test iOS * remove unused comment * fix(db): move sendMessage reads and prepares inside the writer lock (#7546) * fix(db): move sendMessage reads and prepares inside the writer lock * fix: test improvements * chore: remove comments * fix: Admin Panel content hidden behind bottom navigation bar (#7538) * feat: add tabular numbers (#7568) * feat: tabular numbers across the app, upgrade Inter to 4.1 * update snapshot * chore: pin @rocket.chat/sdk to a specific commit hash (#7569) * fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE (#7554) * fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE * code improvements * removed unused comment * fix: run handleDelete finds and prepares inside the writer lock (#7552) * fix: run handleDelete finds and prepares inside the writer lock * chore: reuse mockWMDB * fix: crop screen hidden behind navigation bar on iOS 26 (#7529) * fix: re-fetch message inside the write in getThreadName (#7557) * fix: re-fetch message inside the write in getThreadName * fix: re-fetch message inside the write in getThreadName * chore: new test cases * remove unecessary async * fix(db): move decryptPendingMessages prepares inside the writer lock (#7548) * fix(db): move decryptPendingMessages prepares inside the writer lock * code improvements * chore: new test case encryption * chore: format code and fix lint issues --------- Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> * fix(android): VideoConf notification accept and decline button hidden and touch not working (#7533) * fix: in-app notification buttons ignoring taps on Android * fix: Decline and Accept invisible on the incoming call notification * fix: UIKit buttons not responding on some Android devices (#7573) * fix: force Google account chooser on OAuth login (#7572) * fix: ISO format support in markdown component (#6943) * fix: resolve deep links by room id for channels and groups (#7111) * Merge pull request #7570 from RocketChat/deeplink-saml-auth feat: SAML deeplink auth * fix: grant pull-requests write to build call sites in build-develop (#7599) The reusable workflows build-android.yml and build-ios.yml declare pull-requests: write on their upload jobs. GitHub validates these at call time regardless of job conditionals, so build-develop.yml (caller) must grant the permission or the workflow fails validation. build-pr.yml already grants it; this mirrors that. --------- Co-authored-by: Rohit Bansal <40559587+Rohit3523@users.noreply.github.com> Co-authored-by: Yasmim Nagat <117310290+yasnagat@users.noreply.github.com> Co-authored-by: Otávio Stasiak <91474186+OtavioStasiak@users.noreply.github.com> Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> Co-authored-by: Yash Rajpal <58601732+yash-rajpal@users.noreply.github.com>
* fix: render mentions, emojis, and inline elements inside headings (#6911) * chore: OXC (#7515) * chore: replace ESLint with Oxlint Migrate linting from ESLint 8 to Oxlint. `pnpm lint` drops from ~1min to ~0.6s and 17 eslint packages are removed from devDependencies. Config lives in `.oxlintrc.json`, generated with `@oxlint/migrate` from the old `.eslintrc.js` and then tuned: - `eslint-plugin-react-native` is loaded through `jsPlugins`, since Oxlint has no built-in equivalent. - `import/extensions` is off. Its old options (`js: 'warning'`, ...) were not valid values for the rule, so it never reported anything under ESLint. - `no-unused-vars` sets `caughtErrors: 'none'` to match the ESLint 8 default. - `import/no-cycle` and the React Compiler rules report as warnings. They surface findings ESLint never showed, so they are not gated yet. Rules with no Oxlint equivalent are dropped: `no-restricted-syntax` (the ban on `React.*` member syntax), `import/order`, `import/no-unresolved` and `import/named`. ESLint 8 skipped dot-directories, so files under `.rnstorybook/` and `.maestro/` were never linted. Oxlint does lint them, which surfaced four violations that are fixed here. The CI workflow keeps its filename and job id so branch protection checks stay valid. * chore: replace Prettier with Oxfmt Migrate formatting from Prettier 2.8.8 to Oxfmt via `oxfmt --migrate prettier`. - `.oxfmtrc.json` carries every previous Prettier option unchanged (tabs, single quotes, printWidth 130, no trailing comma, avoid arrow parens, bracketSameLine) plus the `.prettierignore` patterns as `ignorePatterns`. `sortPackageJson` is disabled to match previous behavior. - `.prettierrc.js` and `.prettierignore` removed; `prettier` dropped from devDependencies. - `prettier-lint` script renamed to `format-lint` and now runs `oxfmt`. - 44 files reformatted: Oxfmt follows Prettier 3 style, so nested ternaries and `extends`/type-argument wrapping indent differently than under Prettier 2. No semantic changes. - prettier.yml still ran `eslint --fix`, missed in the Oxlint migration; it now runs `oxfmt` and `oxlint --fix`. `react/exhaustive-deps` is allowed there because its autofix rewrites dependency arrays, which changes behavior and must not land unreviewed from CI. - Workflow filename kept as prettier.yml to avoid disturbing branch protection checks, same as eslint.yml. Verified: `pnpm lint` exit 0 (0 errors), `tsc` clean, 217/217 suites and 2032/2032 tests pass, `oxfmt --check` clean. * chore: update lockfile for oxfmt * chore: migrate typecheck to TypeScript 7.0 (#7516) * chore: bump TypeScript to 6.0 Baseline hop ahead of the TypeScript 7.0 (native compiler) migration, so the 7.0 cut is a version swap against a config that is already 7.0-shaped. TypeScript 6.0 raises both `moduleResolution: node10` and `baseUrl` as errors rather than warnings, and `ignoreDeprecations: "6.0"` stops working in 7.0, so clearing them properly is the only route: - `moduleResolution` -> `bundler` (Metro is a bundler), which requires an esnext-shaped `module`. - `baseUrl` removed. Exactly one import relied on it; it is now relative. - `types` enumerated, since a resolution mode that honours package `exports` no longer auto-includes every `@types` package. `@types/node` becomes an explicit devDependency. Honouring `exports` also stranded the bundled `.d.ts` of three dependencies whose maps expose only JavaScript. Each gets a `types` condition via patch-package; this is visible to the type checker only, as Metro ignores that condition. A `paths` mapping was tried first and rejected, because the jest-expo resolver reads `paths` and then loads those `.d.ts` files at runtime. The inherited block of commented-out option documentation is dropped. * chore: migrate typecheck to TypeScript 7.0 Replaces TypeScript 6.0.3 with the native Go compiler. The version is pinned exactly, since the platform binaries ship as version-matched optional dependencies; the lockfile records the linux-x64 target CI resolves. Typecheck wall time drops from 5.77s on 5.9.3 to ~1.0s. No configuration change was required: the 6.0 hop already left tsconfig in a 7.0-shaped state, and the default parallelism saturates without `--checkers`. `@react-navigation/core` needs a patch to type-check. TypeScript 7.0.2 resolves the mutual recursion between `StaticParamList` and `ParamListForScreens` eagerly where earlier versions defer it, reports the alias as circular, and degrades it to a non-generic symbol -- surfacing as `TS2315: Type 'StaticParamList' is not generic` at our call sites. The patch drops a `FlatType<>` wrapper from the alias, which only flattens intersections for editor display, so the type is unchanged and the misfire stops. * chore: Bump version to 4.76.0 (#7542) * chore(ci): apply least privilege permission to GitHub Actions (#7350) * chore: switch React Compiler to infer mode (#7545) * ci: route Maestro e2e selection through sniffler impact analysis (#7476) * fix(iOS): RoomItem Swipe not working after scroll (#7532) * fix(ci): select shards for changes outside app and honor the release label (#7555) * fix: delete background taller than its row on ServersHistory (#7536) * fix: delete background taller than its row on server items * chore: code improvements --------- Co-authored-by: Diego Mello <diegolmello@gmail.com> * fix: UIKit block messages rendering with smaller font size (#7531) * fix: UIKit block messages rendering with smaller font size * fix: snapshot * fix(db): move deleteMessage finds and prepares inside the writer lock (#7550) * fix: quote has no effect on older thread messages (#7535) * fix: Quote has no effect on older thread messages * fix: Quote has no effect on older thread messages * chore: e2e test * fix: e2e test * chore: format code and fix lint issues * fix(MessageComposer): resolve quoted thread messages and guard stale lookups * fix: test --------- Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> * fix(db): move persistMessage lookups and prepares inside the writer lock (#7551) * fix: test case 11 and 12 flaky tests (#7564) * fix: test * fix: room last messa test * fix: jumptomessage test * chore: remove comments * fix: jump to message e2e test iOS * remove unused comment * fix(db): move sendMessage reads and prepares inside the writer lock (#7546) * fix(db): move sendMessage reads and prepares inside the writer lock * fix: test improvements * chore: remove comments * fix: Admin Panel content hidden behind bottom navigation bar (#7538) * feat: add tabular numbers (#7568) * feat: tabular numbers across the app, upgrade Inter to 4.1 * update snapshot * chore: pin @rocket.chat/sdk to a specific commit hash (#7569) * fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE (#7554) * fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE * code improvements * removed unused comment * fix: run handleDelete finds and prepares inside the writer lock (#7552) * fix: run handleDelete finds and prepares inside the writer lock * chore: reuse mockWMDB * fix: crop screen hidden behind navigation bar on iOS 26 (#7529) * fix: re-fetch message inside the write in getThreadName (#7557) * fix: re-fetch message inside the write in getThreadName * fix: re-fetch message inside the write in getThreadName * chore: new test cases * remove unecessary async * fix(db): move decryptPendingMessages prepares inside the writer lock (#7548) * fix(db): move decryptPendingMessages prepares inside the writer lock * code improvements * chore: new test case encryption * chore: format code and fix lint issues --------- Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> * fix(android): VideoConf notification accept and decline button hidden and touch not working (#7533) * fix: in-app notification buttons ignoring taps on Android * fix: Decline and Accept invisible on the incoming call notification * fix: UIKit buttons not responding on some Android devices (#7573) * fix: force Google account chooser on OAuth login (#7572) * fix: ISO format support in markdown component (#6943) * fix: resolve deep links by room id for channels and groups (#7111) * Merge pull request #7570 from RocketChat/deeplink-saml-auth feat: SAML deeplink auth * fix: grant pull-requests write to build call sites in build-develop (#7599) The reusable workflows build-android.yml and build-ios.yml declare pull-requests: write on their upload jobs. GitHub validates these at call time regardless of job conditionals, so build-develop.yml (caller) must grant the permission or the workflow fails validation. build-pr.yml already grants it; this mirrors that. --------- Co-authored-by: Rohit Bansal <40559587+Rohit3523@users.noreply.github.com> Co-authored-by: Yasmim Nagat <117310290+yasnagat@users.noreply.github.com> Co-authored-by: Otávio Stasiak <91474186+OtavioStasiak@users.noreply.github.com> Co-authored-by: OtavioStasiak <OtavioStasiak@users.noreply.github.com> Co-authored-by: Yash Rajpal <58601732+yash-rajpal@users.noreply.github.com>
Proposed changes
The sniffler shard selection from #7476 has two separate faults. I found both during work on NATIVE-1461. The pull request for that task selected no shards, and CI skipped all Maestro flows.
A change outside
app/selected no shard. Thesource.rootsvalue is["app"]. Therefore no file outsideapp/enters the import graph. TherunAllWhenChangedlist held only paths insideapp/, pluspackage.jsonandpnpm-lock.yaml. A change to a runtime file outsideapp/matched no flow. Theselect-impacted-shards.shscript then reported a confident-zero diff.The diff for NATIVE-1461 changes
patches/@rocket.chat+sdk+1.3.3-mobile.patch. That patch changes the DDP socket, and every flow depends on it. Sniffler returnedrecommendedTests: []. The same fault applied toandroid/,ios/,metro.config.js,babel.config.js,app.json,index.js,react-native.config.jsand.maestro/.This change also makes
validate-test-map.jsaccept a glob. That part is necessary, not optional. Sniffler reads eachrunAllWhenChangeditem as a pattern. The earlierexistsSynccheck refuses every glob as stale and fails CI.The release label did not force the full set of shards. The gate used
contains(github.event.pull_request.labels.*.name, 'release'). Thecontains()function does an exact match on each item of an array. Therefore it needs a label with the exact namerelease. Our label has the name💎 release.The same line has a second fault. The
build-pr.ymlfile declares notypes:key. Therefore the workflow does not start when a user adds a label, and a new run reads the old event data. Even the exact name works only if the label was on the pull request before the last push. The job now reads the labels withgh pr viewduring the run. This corrects both faults. It also avoids alabeledtrigger, which starts a full Build PR each time a user changes a label.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1484
How to test or reproduce
Run sniffler against the two commits of the NATIVE-1461 pull request:
recommendedTests: []reason: run-allfrompatches/**Then run the CI script:
shards=[] should_run=falseshards=[1,2,3,4,5,6,7,8,9,10,11,12,13,14] should_run=trueTo test the label, add
💎 releaseto a pull request. TheDetect release labelstep must printrelease=true. The preflight must then select all 14 shards.Screenshots
Not applicable. This change affects only CI.
Types of changes
Checklist
Further comments
The test count in
.github/scripts/__tests__goes from 38 to 40. Both new tests can fail. I ran them against the earlier validator. The glob test failed, and the test that uses the real repository also failed, because the earlierexistsSynccheck refuses the new globs.One item is suitable for a separate task. The header comment of
select-impacted-shards.shsays that under-selection is not possible. That statement is true only if the test-map covers each runtime path. This fault shows that it did not. Either correct the comment, or let the validator check each top-level runtime path.Summary by CodeRabbit
CI Improvements
Tests