diff --git a/.github/actions/test-scope/action.yml b/.github/actions/test-scope/action.yml index c3146cf56746d..53e3dce5c8e74 100644 --- a/.github/actions/test-scope/action.yml +++ b/.github/actions/test-scope/action.yml @@ -49,7 +49,8 @@ inputs: requires-head-file: description: >- Optional path the work needs at the PR head. A head cut before that file - existed, whose diff does not touch it, does not apply (with a notice). + existed, whose diff does not touch it, does not apply (with a notice), + and `head_file_present` says so whether or not `touches` holds. required: false default: '' @@ -95,6 +96,11 @@ outputs: A file listing the PR's changed paths, one per line (with `component-audit`); empty when the step could not list them. value: ${{ steps.scope.outputs.changed_files_path }} + head_file_present: + description: >- + `false` when the PR head lacks `requires-head-file` and its diff does not + touch it (a head cut before the file existed); `true` otherwise. + value: ${{ steps.scope.outputs.head_file_present }} applies: description: >- The job's work applies: not a specialized lane (spec, docsite or tooling @@ -127,6 +133,7 @@ runs: release_bump: 'false', touches: 'true', applies: 'true', + head_file_present: 'true', decided: 'true', }; // Outside a pull request, or when undecidable, every component audit @@ -240,17 +247,21 @@ runs: } const specialized = ['spec_only', 'docsite_only', 'tooling_only'].some(key => classified[key] === 'true'); let applies = String(!specialized && touches === 'true'); + // Whether the head has the file the work needs, decided whether or + // not `touches` holds; a diff that touches the file keeps it. + let headFilePresent = 'true'; const headFile = process.env.SCOPE_HEAD_FILE; - if (applies === 'true' && headFile && !files.some(f => f.filename === headFile || f.previous_filename === headFile)) { + if (!specialized && headFile && !files.some(f => f.filename === headFile || f.previous_filename === headFile)) { try { await github.rest.repos.getContent({...context.repo, path: headFile, ref: pr.head.sha}); } catch (error) { if (error.status === 404) { - core.notice(`This pull request's head has no ${headFile}; merge main to run this check.`); - applies = 'false'; + core.notice(`This pull request's head has no ${headFile}, so this job skips what needs it; merging main brings it in.`); + headFilePresent = 'false'; } } } + if (headFilePresent === 'false') applies = 'false'; let componentScope = {}; if (audit) { if (specialized) { @@ -280,4 +291,4 @@ runs: fs.writeFileSync(changedFilesPath, `${changed.join('\n')}\n`); componentScope.changed_files_path = changedFilesPath; } - report({...everything, ...classified, ...componentScope, release_bump: 'false', touches, applies}); + report({...everything, ...classified, ...componentScope, release_bump: 'false', touches, applies, head_file_present: headFilePresent}); diff --git a/.github/scripts/ci-package-size-predating-head.test.mjs b/.github/scripts/ci-package-size-predating-head.test.mjs new file mode 100644 index 0000000000000..7b50b82f233bc --- /dev/null +++ b/.github/scripts/ci-package-size-predating-head.test.mjs @@ -0,0 +1,82 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file ci-package-size-predating-head.test.mjs + * Contract: a pull request whose head predates the package-size check (the + * workflow comes from the merge ref, the checkout from the head) skips the + * check instead of failing build-storybook: the scope step's + * `requires-head-file` notes the missing script (test-scope-action.test.mjs), + * and the comparison steps after it run only when the head was measured. + */ + +import {spawnSync} from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import {afterEach, describe, expect, it} from 'vitest'; +import yaml from 'yaml'; + +const ROOT = path.resolve(import.meta.dirname, '../..'); +const ci = yaml.parse( + fs.readFileSync(path.join(ROOT, '.github/workflows/ci.yml'), 'utf8'), +); +const step = (job, name) => + ci.jobs[job].steps.find(entry => entry.name === name); +const roots = []; +afterEach(() => { + for (const root of roots.splice(0)) + fs.rmSync(root, {recursive: true, force: true}); +}); + +/** Runs a step's script in a fresh directory; returns its exit, stdout and outputs. */ +function runStep(script, setup = () => {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'predating-')); + roots.push(root); + setup(root); + const output = path.join(root, 'github-output'); + fs.writeFileSync(output, ''); + const result = spawnSync('bash', ['-e', '-c', script], { + cwd: root, + encoding: 'utf8', + env: {...process.env, GITHUB_OUTPUT: output, BASE_REF: 'main'}, + }); + return {...result, outputs: fs.readFileSync(output, 'utf8')}; +} + +describe('package size on a head that predates it', () => { + const job = ci.jobs['build-storybook']; + const measure = step('build-storybook', 'Measure published package size'); + + it("asks the scope step whether the head has the check's script", () => { + const scope = job.steps.find(entry => entry.id === 'scope'); + expect(scope.with['requires-head-file']).toBe( + '.github/scripts/package-size.mjs', + ); + expect(measure.if).toContain( + "steps.scope.outputs.head_file_present != 'false'", + ); + }); + + it('records a measurement only after measuring', () => { + const result = runStep(measure.run, root => { + fs.mkdirSync(path.join(root, '.github/scripts'), {recursive: true}); + fs.writeFileSync( + path.join(root, '.github/scripts/package-size.mjs'), + "import fs from 'node:fs';\nfs.writeFileSync('package-size-head.json', '{}');\n", + ); + }); + expect(result.status).toBe(0); + expect(result.outputs).toContain('measured=true'); + }); + + it('runs the comparison steps only after a measurement', () => { + for (const name of [ + "Download main's package size for the merge base", + 'Compare published package size', + ]) { + expect(step('build-storybook', name).if).toContain( + "steps.package-size.outputs.measured == 'true'", + ); + } + }); +}); diff --git a/.github/scripts/test-scope-action.test.mjs b/.github/scripts/test-scope-action.test.mjs index 4dd0ea4ad60f9..3e86070022f48 100644 --- a/.github/scripts/test-scope-action.test.mjs +++ b/.github/scripts/test-scope-action.test.mjs @@ -162,6 +162,7 @@ describe('per-job scope step', () => { expect(Object.keys(action.outputs).sort()).toEqual( [ 'applies', + 'head_file_present', 'changed_files_path', 'decided', 'docsite_only', @@ -410,6 +411,33 @@ describe('per-job scope step', () => { expect(outputs.applies).toBe('true'); }); + it('reports a head without the file whether or not touches holds', async () => { + for (const file of ['packages/core/src/Button/Button.tsx', 'README.md']) { + const {outputs} = await decide({files: changed(file), inputs}); + expect(outputs.head_file_present, file).toBe('false'); + } + const {outputs} = await decide({ + files: changed('README.md'), + headFiles: ['playwright.interaction.config.ts'], + inputs, + }); + expect(outputs.head_file_present).toBe('true'); + }); + + it('keeps the file present for a pull request that renames or deletes it', async () => { + for (const file of [ + { + filename: 'playwright.old-interaction.config.ts', + previous_filename: 'playwright.interaction.config.ts', + status: 'renamed', + }, + {filename: 'playwright.interaction.config.ts', status: 'removed'}, + ]) { + const {outputs} = await decide({files: [file], inputs}); + expect(outputs.head_file_present).toBe('true'); + } + }); + it('still applies to a pull request that deletes the file, so the check fails', async () => { const {outputs} = await decide({ files: [ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6f2acd9524ff..88f5897657b56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -951,6 +951,8 @@ jobs: # infrastructure, `full_suite`, runs it too); the build itself # follows the specialized-lane outputs. paths: '^(packages/|pnpm-lock\.yaml$|package\.json$|tsconfig\.json$|scripts/|\.github/workflows/ci\.yml$|\.github/scripts/package-size\.mjs$|\.github/scripts/lib/package-size\.js$|\.github/scripts/merge-base\.sh$)' + # A head cut before the package-size check has no script for it. + requires-head-file: .github/scripts/package-size.mjs - name: Skip heavy work for a specialized lane if: steps.scope.outputs.docsite_only == 'true' || steps.scope.outputs.spec_only == 'true' || steps.scope.outputs.tooling_only == 'true' @@ -1183,11 +1185,16 @@ jobs: # package or the lockfile runs it. - name: Measure published package size id: package-size - if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && github.event_name == 'pull_request' && (steps.scope.outputs.touches == 'true' || steps.scope.outputs.full_suite != 'false') - run: node .github/scripts/package-size.mjs measure --output package-size-head.json + # A head cut before the check has no script to run (the workflow comes + # from the merge ref, the checkout from the head): the scope step notes + # it, and the check skips until the branch merges main. + if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && github.event_name == 'pull_request' && steps.scope.outputs.head_file_present != 'false' && (steps.scope.outputs.touches == 'true' || steps.scope.outputs.full_suite != 'false') + run: | + node .github/scripts/package-size.mjs measure --output package-size-head.json + echo "measured=true" >> "$GITHUB_OUTPUT" - name: Download main's package size for the merge base - if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && steps.package-size.outcome == 'success' && steps.size-records.outputs.package_run_id != '' + if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && steps.package-size.outputs.measured == 'true' && steps.size-records.outputs.package_run_id != '' continue-on-error: true uses: actions/download-artifact@v8 with: @@ -1199,7 +1206,7 @@ jobs: # Advisory: a package over its budget is flagged in the job summary and # the PR report, and fails nothing. - name: Compare published package size - if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && steps.package-size.outcome == 'success' + if: steps.scope.outputs.docsite_only != 'true' && steps.scope.outputs.spec_only != 'true' && steps.scope.outputs.tooling_only != 'true' && steps.package-size.outputs.measured == 'true' env: HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_SHA: ${{ steps.merge-base.outputs.sha }}