From 753d40d1b42f290f543f42cce1ba5669cf071709 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 1 Sep 2026 15:21:43 -0700 Subject: [PATCH] fix(ci): stop fanning out the whole cockpit e2e matrix for unrelated changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #932 changed three files under apps/website/src and ran 41 `Cockpit — e2e` lanes. Two independent scoping defects stacked up. 1. apps/cockpit carried `scope:cockpit-e2e`. The cockpit-e2e matrix dispatches `nx e2e` for the standalone Angular cap apps under cockpit/**; none of the 40 caps depends on the apps/cockpit Next.js shell, and no workflow runs the shell's own `e2e` target. The tag could never select real work — only over-select. And it did: apps/cockpit statically depends on apps/website, so a website-only PR made the shell nx-affected and flipped cockpit_e2e true. 2. cockpit-matrix matched affected projects on `cap.angular` only. A cap is two unlinked nx projects (Angular app + python backend), so a python-only cap change attributed nothing, which main() reads as "nx found no cap" and answers with the full fleet — 40 lanes to cover one cap. Measured on this checkout, before → after: website-only (PR #932 base..head) 41 lanes → 0 (job gate is now false) python-only cap change 40 lanes → 1 (the cap that changed) libs/chat change 40 lanes → 40 (unchanged; no coverage lost) The python sibling's nx name is read from its own project.json rather than derived from the path, because a wrong guess fails open to the full fleet without ever reporting that it guessed. Co-Authored-By: Claude Opus 5 --- apps/cockpit/project.json | 1 - scripts/ci-scope.spec.mjs | 49 ++++++++++++++++++ scripts/cockpit-matrix.mjs | 59 ++++++++++++++++++--- scripts/cockpit-matrix.spec.mjs | 91 +++++++++++++++++++++++++++++---- 4 files changed, 181 insertions(+), 19 deletions(-) diff --git a/apps/cockpit/project.json b/apps/cockpit/project.json index d16a34802..885ae6fc7 100644 --- a/apps/cockpit/project.json +++ b/apps/cockpit/project.json @@ -6,7 +6,6 @@ "tags": [ "scope:cockpit", "scope:cockpit-deploy-smoke", - "scope:cockpit-e2e", "scope:cockpit-examples", "type:app" ], diff --git a/scripts/ci-scope.spec.mjs b/scripts/ci-scope.spec.mjs index c6f056b73..3da77cacb 100644 --- a/scripts/ci-scope.spec.mjs +++ b/scripts/ci-scope.spec.mjs @@ -413,3 +413,52 @@ describe('SCOPE_KEYS export', () => { ]); }); }); + +describe('classifyFromAffected — cockpit shell does not own the e2e matrix', () => { + // The cockpit-e2e matrix dispatches `nx e2e` for the standalone Angular cap + // apps under cockpit/**; none of them depends on the apps/cockpit Next.js + // shell, and no workflow runs the shell's own `e2e` target. A + // `scope:cockpit-e2e` tag on the shell therefore cannot select any real + // work — it can only over-select. It used to: apps/cockpit imports from + // apps/website, so a website-only PR made the shell nx-affected, flipped + // cockpit_e2e true, and (with no cap affected) hit the dispatcher's + // full-fleet fallback. PR #932 changed three apps/website/src files and ran + // the whole cap matrix. + it('apps/cockpit is not tagged scope:cockpit-e2e', async () => { + const project = JSON.parse( + await readFile('apps/cockpit/project.json', 'utf8') + ); + + assert.ok( + !project.tags.includes('scope:cockpit-e2e'), + 'the cockpit shell must not select the cockpit-e2e cap matrix' + ); + }); + + it('a website-only change leaves cockpit_e2e false', async () => { + const cockpit = JSON.parse( + await readFile('apps/cockpit/project.json', 'utf8') + ); + const website = JSON.parse( + await readFile('apps/website/project.json', 'utf8') + ); + + // The real nx-affected set for PR #932 was [website, cockpit, scripts]: + // apps/cockpit statically depends on apps/website. + const scope = classifyFromAffected( + [ + 'apps/website/src/app/layout.tsx', + 'apps/website/src/components/shared/SiteFooter.tsx', + ], + [ + { name: 'website', tags: website.tags }, + { name: 'cockpit', tags: cockpit.tags }, + ] + ); + + assert.equal(scope.cockpit_e2e, false); + // The shell still builds and tests — it consumes the changed website code. + assert.equal(scope.cockpit, true); + assert.equal(scope.website, true); + }); +}); diff --git a/scripts/cockpit-matrix.mjs b/scripts/cockpit-matrix.mjs index 29386ba36..dfc9c0041 100644 --- a/scripts/cockpit-matrix.mjs +++ b/scripts/cockpit-matrix.mjs @@ -1,13 +1,32 @@ #!/usr/bin/env node // SPDX-License-Identifier: MIT +/** + * Does this cap own one of the nx-affected projects? + * + * A cap is two independent nx projects — the Angular app that owns the `e2e` + * target and the python backend it talks to — with no edge between them in the + * project graph. Matching only on the Angular name made a python-only change + * look unattributed, which main() answers with the full fleet. + * + * @param {{angular: string, pythonName?: string}} cap + * @param {Set} affectedNames + * @returns {boolean} + */ +export function isCapAffected(cap, affectedNames) { + if (affectedNames.has(cap.angular)) return true; + // Guard the falsy case: caps with a Node-hosted backend carry '' here, and + // `affectedNames` must never be probed with an empty key. + return Boolean(cap.pythonName) && affectedNames.has(cap.pythonName); +} + /** * Pure-function classifier for the cockpit-e2e matrix. * - * @param {Array<{angular: string, python: string}>} allCockpitCaps + * @param {Array<{angular: string, python: string, pythonName: string}>} allCockpitCaps * All cockpit angular projects with an e2e target, paired with - * their python sibling path. Derived from the project graph by - * the CLI wrapper (or hard-coded in tests). + * their python sibling path and project name. Derived from the + * project graph by the CLI wrapper (or hard-coded in tests). * @param {Set} affectedNames * Set of project names nx-affected returned for this diff. * @param {{fullFleet: boolean}} opts @@ -19,7 +38,7 @@ */ export function selectCockpitCaps(allCockpitCaps, affectedNames, { fullFleet }) { if (fullFleet) return allCockpitCaps; - return allCockpitCaps.filter((cap) => affectedNames.has(cap.angular)); + return allCockpitCaps.filter((cap) => isCapAffected(cap, affectedNames)); } // ── CLI wrapper ──────────────────────────────────────────────────────────── @@ -100,7 +119,29 @@ function deriveCockpitCaps() { return false; } })(); - caps.push({ angular: angularName, python: hasPython ? relPython : '' }); + // Read the python sibling's own project.json for its nx name rather + // than deriving one from the path — the name is what nx-affected + // reports, and a convention-derived guess would fail open (no match + // → full fleet) without ever saying so. + const pythonName = (() => { + if (!hasPython) return ''; + try { + const sibling = JSON.parse( + readFileSync( + path.join(repoRoot, relPython, 'project.json'), + 'utf8', + ), + ); + return typeof sibling.name === 'string' ? sibling.name : ''; + } catch { + return ''; + } + })(); + caps.push({ + angular: angularName, + python: hasPython ? relPython : '', + pythonName, + }); } catch { // No project.json or invalid JSON — skip silently. } @@ -135,14 +176,18 @@ function main() { // Empty-affected fallback: when scope says e2e is required but nx // didn't attribute any cap (lib fanout), run all caps. - const haveAnyCockpitAffected = allCaps.some((c) => affected.has(c.angular)); + const haveAnyCockpitAffected = allCaps.some((c) => isCapAffected(c, affected)); const effectiveFullFleet = args.fullFleet || !haveAnyCockpitAffected; const selected = selectCockpitCaps(allCaps, affected, { fullFleet: effectiveFullFleet, }); - const json = JSON.stringify(selected); + // ci.yml reads matrix.cap.angular / matrix.cap.python; pythonName is an + // internal attribution detail, so keep it out of the emitted matrix. + const json = JSON.stringify( + selected.map(({ angular, python }) => ({ angular, python })), + ); const ghOutput = process.env.GITHUB_OUTPUT; if (ghOutput) { diff --git a/scripts/cockpit-matrix.spec.mjs b/scripts/cockpit-matrix.spec.mjs index 9b06ec9c0..9756cde71 100644 --- a/scripts/cockpit-matrix.spec.mjs +++ b/scripts/cockpit-matrix.spec.mjs @@ -1,11 +1,25 @@ import { test, describe } from 'node:test'; import assert from 'node:assert/strict'; -import { selectCockpitCaps } from './cockpit-matrix.mjs'; +import { isCapAffected, selectCockpitCaps } from './cockpit-matrix.mjs'; const ALL_CAPS = [ - { angular: 'cockpit-chat-messages-angular', python: 'cockpit/chat/messages/python' }, - { angular: 'cockpit-chat-input-angular', python: 'cockpit/chat/input/python' }, - { angular: 'cockpit-langgraph-streaming-angular', python: 'cockpit/langgraph/streaming/python' }, + { + angular: 'cockpit-chat-messages-angular', + python: 'cockpit/chat/messages/python', + pythonName: 'cockpit-chat-messages-python', + }, + { + angular: 'cockpit-chat-input-angular', + python: 'cockpit/chat/input/python', + pythonName: 'cockpit-chat-input-python', + }, + { + angular: 'cockpit-langgraph-streaming-angular', + python: 'cockpit/langgraph/streaming/python', + pythonName: 'cockpit-langgraph-streaming-python', + }, + // Node-hosted backend: no python sibling on disk. + { angular: 'cockpit-runtimes-mastra-angular', python: '', pythonName: '' }, ]; describe('selectCockpitCaps', () => { @@ -15,9 +29,7 @@ describe('selectCockpitCaps', () => { new Set(['cockpit-chat-messages-angular']), { fullFleet: false }, ); - assert.deepEqual(result, [ - { angular: 'cockpit-chat-messages-angular', python: 'cockpit/chat/messages/python' }, - ]); + assert.deepEqual(result, [ALL_CAPS[0]]); }); test('returns multiple affected caps preserving input order', () => { @@ -26,10 +38,7 @@ describe('selectCockpitCaps', () => { new Set(['cockpit-langgraph-streaming-angular', 'cockpit-chat-messages-angular']), { fullFleet: false }, ); - assert.deepEqual(result, [ - { angular: 'cockpit-chat-messages-angular', python: 'cockpit/chat/messages/python' }, - { angular: 'cockpit-langgraph-streaming-angular', python: 'cockpit/langgraph/streaming/python' }, - ]); + assert.deepEqual(result, [ALL_CAPS[0], ALL_CAPS[2]]); }); test('returns all caps when fullFleet=true regardless of affected', () => { @@ -69,3 +78,63 @@ describe('selectCockpitCaps', () => { assert.deepEqual(JSON.parse(JSON.stringify(result)), result); }); }); + +describe('selectCockpitCaps — python sibling attribution', () => { + // A cap's python project is a separate nx project from its Angular app, and + // the two are not linked in the project graph. Matching only on `cap.angular` + // meant a python-only cap change produced an empty selection, which + // cockpit-matrix's main() reads as "nx attributed nothing" and answers with + // the full fleet — ~40 lanes to cover a one-cap change. + test('selects the cap when only its python project is affected', () => { + const result = selectCockpitCaps( + ALL_CAPS, + new Set(['cockpit-chat-messages-python']), + { fullFleet: false }, + ); + assert.deepEqual(result, [ALL_CAPS[0]]); + }); + + test('does not double-select when both siblings are affected', () => { + const result = selectCockpitCaps( + ALL_CAPS, + new Set(['cockpit-chat-messages-python', 'cockpit-chat-messages-angular']), + { fullFleet: false }, + ); + assert.deepEqual(result, [ALL_CAPS[0]]); + }); + + test('mixes angular- and python-attributed caps', () => { + const result = selectCockpitCaps( + ALL_CAPS, + new Set(['cockpit-langgraph-streaming-python', 'cockpit-chat-input-angular']), + { fullFleet: false }, + ); + assert.deepEqual(result, [ALL_CAPS[1], ALL_CAPS[2]]); + }); +}); + +describe('isCapAffected', () => { + test('matches on the angular project name', () => { + assert.equal( + isCapAffected(ALL_CAPS[0], new Set(['cockpit-chat-messages-angular'])), + true, + ); + }); + + test('matches on the python project name', () => { + assert.equal( + isCapAffected(ALL_CAPS[0], new Set(['cockpit-chat-messages-python'])), + true, + ); + }); + + test('an empty pythonName never matches an empty-string entry', () => { + // cockpit-runtimes-mastra has no python sibling; a falsy pythonName must + // not turn `affectedNames.has('')` into a match. + assert.equal(isCapAffected(ALL_CAPS[3], new Set([''])), false); + }); + + test('unrelated affected names do not match', () => { + assert.equal(isCapAffected(ALL_CAPS[0], new Set(['chat', 'website'])), false); + }); +});