diff --git a/packages/studio/AGENTS.md b/packages/studio/AGENTS.md index 35ee67faab..325b5c9aef 100644 --- a/packages/studio/AGENTS.md +++ b/packages/studio/AGENTS.md @@ -149,7 +149,7 @@ bun run --cwd packages/studio test:edit-accuracy -- --grid pr --filter '^resize- - **Before and After captures.** A PR that changes code under `packages/studio` or `packages/player` needs `## Before` and `## After` sections in its description, each with an image or video. The exemptions (a small change with - no visible effect, Markdown) are defined in `scripts/check-pr-captures.mjs`. + no visible effect, Markdown, tests) are defined in `scripts/check-pr-captures.mjs`. ## Traps worth knowing diff --git a/scripts/check-pr-captures.mjs b/scripts/check-pr-captures.mjs index 9e6ab31420..31c941cd46 100644 --- a/scripts/check-pr-captures.mjs +++ b/scripts/check-pr-captures.mjs @@ -1,6 +1,6 @@ #!/usr/bin/env node // Fail a PR touching packages/studio or packages/player unless its body has Before and After sections with media. -// Markdown under them is docs, not behaviour, so it is not watched. +// Markdown and tests under them are not behaviour a user sees, so they are not watched. // usage: node scripts/check-pr-captures.mjs --base origin/main --head ; the body arrives in the env (see main). import { execFileSync } from "node:child_process"; @@ -35,8 +35,6 @@ function isMediaUrl(raw) { return url !== null && (isAttachmentUrl(url) || MEDIA_PATH.test(url.pathname)); } -const TEST_FILE = /\.(?:test|spec)\.[jt]sx?$/; - const CAPTURE_TITLES = { before: /^before(?:\s*[:(].*|\s+[-–—]\s.*)?$/i, after: /^after(?:\s*[:(].*|\s+[-–—]\s.*)?$/i, @@ -133,8 +131,12 @@ function parseRecord(record) { } const DOC_FILE = /\.mdx?$/i; +const TEST_PATH = /\/(?:tests|__tests__)\/|\.(?:test|spec)\./; const isWatched = (path) => - path !== "" && WATCHED_PREFIXES.some((prefix) => path.startsWith(prefix)) && !DOC_FILE.test(path); + path !== "" && + WATCHED_PREFIXES.some((prefix) => path.startsWith(prefix)) && + !DOC_FILE.test(path) && + !TEST_PATH.test(path); /** Parse `git diff --numstat -z --no-renames`, keeping watched paths. A binary file counts as a full budget. */ export const parseNumstat = (numstat) => @@ -143,8 +145,7 @@ export const parseNumstat = (numstat) => .map(parseRecord) .filter((file) => isWatched(file.path)); -const isVisualFile = (path) => - VISUAL_EXTENSIONS.some((ext) => path.endsWith(ext)) && !TEST_FILE.test(path); +const isVisualFile = (path) => VISUAL_EXTENSIONS.some((ext) => path.endsWith(ext)); /** Why a "No visible change" declaration does not hold for this diff; empty means it holds. */ export function noVisibleChangeFailures(files) { @@ -468,7 +469,7 @@ function printFailure(problems, prNumber) { "Edit the body text first: gh pr edit --body-file replaces the body and drops attachments.", ); console.error( - `A change with no visible effect (under ${NO_VISIBLE_CHANGE_MAX_LINES} lines, no .tsx/.css/.html) may instead add a '## No visible change' section.`, + `A change with no visible effect (under ${NO_VISIBLE_CHANGE_MAX_LINES} lines, no .tsx/.css/.html; tests and Markdown are not counted) may instead add a '## No visible change' section.`, ); } diff --git a/scripts/check-pr-captures.test.mjs b/scripts/check-pr-captures.test.mjs index 610c898c89..4fcfd62968 100644 --- a/scripts/check-pr-captures.test.mjs +++ b/scripts/check-pr-captures.test.mjs @@ -116,6 +116,22 @@ test("markdown under a watched package needs no capture", () => { assert.equal(evaluate({ body: "", files }).ok, true); }); +test("test files under a watched package need no capture", () => { + const files = parseNumstat( + "28\t0\tpackages/studio/tests/e2e/edit-accuracy/server.test.mjs\0" + + "12\t4\tpackages/studio/tests/e2e/edit-accuracy/case.mjs\0" + + "6\t1\tpackages/player/src/a.spec.tsx\0" + + "40\t0\tpackages/studio/src/b.test.ts\0", + ); + assert.deepEqual(files, []); + assert.equal(evaluate({ body: "", files }).ok, true); + const mixed = parseNumstat( + "3\t0\tpackages/studio/src/a.ts\0" + "40\t0\tpackages/studio/src/a.test.ts\0", + ); + assert.deepEqual(mixed, [studio("packages/studio/src/a.ts", 3)]); + assert.equal(evaluate({ body: "", files: mixed }).ok, false); +}); + test("a binary file spends the whole no-visible-change budget", () => { const [file] = parseNumstat("-\t-\tpackages/studio/public/logo.png\0"); assert.equal(file.lines, 20); @@ -191,9 +207,8 @@ test("comment fragments cannot rebuild a comment or hide a section from the read ); }); -test("No visible change accepts a test-only .tsx change but not a component change", () => { +test("No visible change does not cover a component change", () => { const body = "## No visible change\nx"; - assert.equal(evaluate({ body, files: [studio("packages/studio/src/A.test.tsx", 5)] }).ok, true); assert.equal(evaluate({ body, files: [studio("packages/studio/src/A.tsx", 5)] }).ok, false); });