From 0ae0b6b3c2a8b768ffdc5deec827cacbe732cb1c Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Fri, 2 Oct 2026 11:35:51 +0000 Subject: [PATCH] ci: Exclude test files from required reviews (no-changelog) (#40164) --- .github/WORKFLOWS.md | 6 ++-- .github/scripts/owners/owners.mjs | 6 +++- .github/scripts/owners/owners.test.mjs | 16 ++++++++++ .github/scripts/owners/required-reviews.mjs | 2 +- .github/scripts/quality/check-pr-size.mjs | 30 +++--------------- .github/scripts/test-files.mjs | 34 +++++++++++++++++++++ OWNERS | 4 +-- 7 files changed, 66 insertions(+), 32 deletions(-) create mode 100644 .github/scripts/test-files.mjs diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index 68003bcd501..d57a43f6b76 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -884,8 +884,10 @@ The file drives four workflows: ### Required reviews An entry with the `required` option makes team approval mandatory: when a PR -changes a file whose winning entry carries `required`, a member of each listed -team must approve the PR. `ci-owners-required-reviews.yml` evaluates this on +changes a non-test file whose winning entry carries `required`, a member of each +listed team must approve the PR. Test files match the shared patterns in +`test-files.mjs` and do not trigger required reviews. +`ci-owners-required-reviews.yml` evaluates this on PR changes and review events, and reports a commit status named **Required Reviews** on the head SHA. A missing approval reports `pending` ("Waiting for approval from: …"), not `failure`, so an unreviewed PR diff --git a/.github/scripts/owners/owners.mjs b/.github/scripts/owners/owners.mjs index 219391da390..1ebbcda25ad 100644 --- a/.github/scripts/owners/owners.mjs +++ b/.github/scripts/owners/owners.mjs @@ -1,6 +1,8 @@ import { readFileSync, statSync } from "node:fs"; import { join } from "node:path"; +import { isTestFile } from "../test-files.mjs"; + /** * @typedef OwnersEntry * @property { string } pattern @@ -236,7 +238,7 @@ export function assignOwnership(files, entries) { /** * Determine which teams must approve the changeset: a team is required when - * its `required` entry wins (last-match) for a changed file. + * its `required` entry wins (last-match) for a changed non-test file. * * @param { Set } files * @param { OwnersEntry[] } entries @@ -247,6 +249,8 @@ export function resolveRequiredTeams(files, entries) { const teamToFiles = new Map(); for (const file of [...files].sort()) { + if (isTestFile(file)) continue; + const entry = findOwningEntry(file, entries); if (!entry?.required) continue; diff --git a/.github/scripts/owners/owners.test.mjs b/.github/scripts/owners/owners.test.mjs index 0e7bc7e7b5c..02e267238c0 100644 --- a/.github/scripts/owners/owners.test.mjs +++ b/.github/scripts/owners/owners.test.mjs @@ -293,6 +293,22 @@ describe('resolveRequiredTeams', () => { assert.equal(result.size, 0); }); + it('ignores test files whose winning entry is required', () => { + const result = resolveRequiredTeams( + new Set([ + '.github/scripts/check.test.mjs', + '.github/workflows/__tests__/check.ts', + '.github/workflows/ci.yml', + ]), + [ + entry({ pattern: '.github/scripts/', team: '@n8n-io/qa-dx', required: true }), + entry({ pattern: '.github/workflows/', team: '@n8n-io/qa-dx', required: true }), + ], + ); + + assert.deepEqual(result.get('@n8n-io/qa-dx'), ['.github/workflows/ci.yml']); + }); + it('a later non-required entry overrides an earlier required one', () => { const result = resolveRequiredTeams(new Set(['db/migrations/1-init.ts']), [ entry({ pattern: 'db/', team: '@n8n-io/migrations-review', required: true }), diff --git a/.github/scripts/owners/required-reviews.mjs b/.github/scripts/owners/required-reviews.mjs index 6c1cc114ca9..d7bf939f2b8 100644 --- a/.github/scripts/owners/required-reviews.mjs +++ b/.github/scripts/owners/required-reviews.mjs @@ -1,7 +1,7 @@ /** * Required-review enforcement for OWNERS entries marked `required`. * - * Computes the teams whose approval the changeset needs (per OWNERS) + * Computes the teams whose approval the non-test changes need (per OWNERS) * and reports the verdict as a "Required Reviews" commit status on the PR * head SHA. A ruleset that lists this status as a required check blocks the * merge until a member of each required team has approved the PR. Merge-queue diff --git a/.github/scripts/quality/check-pr-size.mjs b/.github/scripts/quality/check-pr-size.mjs index 89cb6dd6a51..48966f45a82 100644 --- a/.github/scripts/quality/check-pr-size.mjs +++ b/.github/scripts/quality/check-pr-size.mjs @@ -14,35 +14,13 @@ import { initGithub, getEventFromGithubEventPath } from '../github-helpers.mjs'; import { matchesGlob } from '../glob.mjs'; +import { isTestFile, TEST_PATTERNS } from '../test-files.mjs'; + +export { isTestFile, TEST_PATTERNS }; export const SIZE_LIMIT = 1000; export const OVERRIDE_COMMAND = '/size-limit-override'; -export const TEST_PATTERNS = [ - // Test files (by extension) - '**/*.test.ts', - '**/*.test.js', - '**/*.test.mjs', - '**/*.spec.ts', - '**/*.spec.js', - '**/*.spec.mjs', - // Test directories - '**/test/**', - '**/tests/**', - '**/__tests__/**', - // Snapshots - '**/__snapshots__/**', - '**/*.snap', - // Fixtures and mocks - '**/fixtures/**', - '**/__mocks__/**', - // Former packages/testing workspaces - 'packages/quality/policy/**', - 'packages/quality/testing/**', - 'packages/quality/environments/**', - 'packages/quality/efficiency/microbenchmarks/**', -]; - export const MISC_PATTERNS = [ // Lock file (can produce massive diffs on dependency changes) 'pnpm-lock.yaml', @@ -59,7 +37,7 @@ export const EXCLUDE_PATTERNS = [...TEST_PATTERNS, ...MISC_PATTERNS]; * @returns { 'testFiles' | 'misc' | 'sourceCode' } */ export function categorizeFile(filename) { - if (TEST_PATTERNS.some((pattern) => matchesGlob(filename, pattern))) return 'testFiles'; + if (isTestFile(filename)) return 'testFiles'; if (MISC_PATTERNS.some((pattern) => matchesGlob(filename, pattern))) return 'misc'; return 'sourceCode'; } diff --git a/.github/scripts/test-files.mjs b/.github/scripts/test-files.mjs new file mode 100644 index 00000000000..0bb01b1fa14 --- /dev/null +++ b/.github/scripts/test-files.mjs @@ -0,0 +1,34 @@ +import { matchesGlob } from './glob.mjs'; + +export const TEST_PATTERNS = [ + // Test files (by extension) + '**/*.test.ts', + '**/*.test.js', + '**/*.test.mjs', + '**/*.spec.ts', + '**/*.spec.js', + '**/*.spec.mjs', + // Test directories + '**/test/**', + '**/tests/**', + '**/__tests__/**', + // Snapshots + '**/__snapshots__/**', + '**/*.snap', + // Fixtures and mocks + '**/fixtures/**', + '**/__mocks__/**', + // Former packages/testing workspaces + 'packages/quality/policy/**', + 'packages/quality/testing/**', + 'packages/quality/environments/**', + 'packages/quality/efficiency/microbenchmarks/**', +]; + +/** + * @param { string } filename + * @returns { boolean } + */ +export function isTestFile(filename) { + return TEST_PATTERNS.some((pattern) => matchesGlob(filename, pattern)); +} diff --git a/OWNERS b/OWNERS index 6f4c97c95bb..74ec4cdb22f 100644 --- a/OWNERS +++ b/OWNERS @@ -8,8 +8,8 @@ # # Last-match-wins: specific rules MUST come AFTER general rules. # -# The `required` option makes team approval mandatory: a member of the team -# must approve the PR before merge. This is enforced by the +# The `required` option makes team approval mandatory for non-test files: a +# member of the team must approve the PR before merge. This is enforced by the # "Required Reviews" commit status (ci-owners-required-reviews.yml). # All other entries are advisory (reviewer recommendations and opt-in # reviewer auto-assignment).