diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ed4568a034..e2e88936f56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3292,11 +3292,16 @@ jobs: # vitest root, so the suite must run with cwd = ui/ (it does, below); run # from this repo's root every key misses and every file weighs the median. # - # Every event runs the whole suite. objectui's affected-test selector - # (ui/scripts/select-affected-tests.mjs) reads `git diff --name-only`, which - # prints paths relative to the GIT root — `ui/packages/...` here — against a - # package graph spelled relative to the objectui root, so it is not wired - # until objectui's repo-root lookups are migrated (sub-issue 2 of the epic). + # ── Test UI: the two speeds (#25868) ──────────────────────────────────── + # `push`, `schedule` and `workflow_dispatch` run the whole suite: the full + # battery. `pull_request` and `merge_group` run the files + # ui/scripts/select-affected-tests.mjs selects -- the ui packages the change + # sits in and their dependents, plus the tests the package graph cannot + # place, which run on every change -- or the whole suite when the change + # reaches something every ui test runs through, outside ui/ included (a + # non-documentation root file, `patches/`, one of the seven packages the + # `ui_consumers` filter names). The selector's header states the rules and + # what it does not see; the full battery on `main` is the backstop. test-ui: name: Test UI (${{ matrix.shard }}/16) needs: filter @@ -3313,6 +3318,10 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@v7 + with: + # The select step below diffs against the base commit, which needs + # the base's history in the clone. + fetch-depth: 0 - name: Setup Node.js uses: actions/setup-node@v7 @@ -3363,13 +3372,85 @@ jobs: --filter=@objectstack/sdui-parser... \ --filter=@objectstack/types... + # Fails OPEN: a selector that exits non-zero, prints no verdict, or has + # no base to diff against means the whole suite, never a skip. The exit + # status is captured beside the output, not through a command + # substitution. A pull request diffs from merge-base(origin/, + # HEAD), never the payload's `base.sha`, which is frozen when the PR is + # opened: everything `main` gained since would be charged to this PR, + # and a lockfile change among it would make every long-open PR a full + # run (Test Core's "WHERE THE AFFECTED DIFF STARTS" above is the same + # fact). A merge group diffs from its `base_sha`. Every shard computes + # the same selection from the same checkout, so `--shard` partitions + # one set. + - name: Select the tests this change affects + id: select + env: + EVENT: ${{ github.event_name }} + PR_BASE_REF: ${{ github.event.pull_request.base.ref }} + QUEUE_BASE: ${{ github.event.merge_group.base_sha }} + run: | + base='' + case "$EVENT" in + pull_request) + if [ -n "$PR_BASE_REF" ]; then + base=$(git merge-base "refs/remotes/origin/$PR_BASE_REF" HEAD) || base='' + fi + ;; + merge_group) base="$QUEUE_BASE" ;; + *) + echo 'mode=full' >> "$GITHUB_OUTPUT" + echo "A $EVENT run is the full battery: every ui test file, sixteen shards." + exit 0 + ;; + esac + status=0 + if [ -z "$base" ]; then + echo "::warning::no diff base on this $EVENT event; running the full suite rather than guessing." + status=2 + else + echo "Diff base: $base" + node ui/scripts/select-affected-tests.mjs --base "$base" --out "$RUNNER_TEMP/affected-tests.txt" \ + > "$RUNNER_TEMP/select.out" || status=$? + cat "$RUNNER_TEMP/select.out" + fi + verdict='' + if [ "$status" -eq 0 ]; then + verdict=$(tail -n 1 "$RUNNER_TEMP/select.out") + fi + case "$verdict" in + mode=affected | mode=full) ;; + *) + echo "::warning::the affected-test selector reached no verdict (exit $status); running the full suite rather than skipping silently." + verdict='mode=full' + ;; + esac + echo "$verdict" >> "$GITHUB_OUTPUT" + + # `$MODE` is the VALUE of the select step's output `mode`: its + # `mode=affected` line is GitHub's `key=value` form, so what arrives here + # is `affected` or `full`, never the line itself. + # ui/scripts/__tests__/select-affected-tests-wiring.test.ts runs both + # steps as written, so neither end can be renamed alone. The filters are + # paths relative to ui/, the vitest root, passed as positionals with no + # `--`, the only spelling ui/scripts/vitest-invocation-guard.mjs admits. + # # objectui's full-battery runs also pass its per-file timings reporter # and upload the result for the weekly dataset refresh. Both belong to # that refresh workflow, which is in the governance-workflow audit # (sub-issue 4 of the epic); until it is ported the committed dataset # is what the sequencer reads. - name: Run the ui tests (shard ${{ matrix.shard }}/16) - run: pnpm --dir ui run test --shard=${{ matrix.shard }}/16 --reporter=default --reporter=github-actions + env: + MODE: ${{ steps.select.outputs.mode }} + run: | + if [ "$MODE" = 'affected' ]; then + mapfile -t filters < "$RUNNER_TEMP/affected-tests.txt" + echo "Affected speed: ${#filters[@]} vitest filter(s), shard ${{ matrix.shard }} of 16." + pnpm --dir ui exec vitest run --shard=${{ matrix.shard }}/16 --reporter=default --reporter=github-actions "${filters[@]}" + else + pnpm --dir ui run test --shard=${{ matrix.shard }}/16 --reporter=default --reporter=github-actions + fi - name: Save Turbo cache (main only) if: always() && github.event_name == 'push' && matrix.shard == 1 diff --git a/ui/scripts/__tests__/helpers/root-ci.ts b/ui/scripts/__tests__/helpers/root-ci.ts index eeba4613d51..41ae14fff2c 100644 --- a/ui/scripts/__tests__/helpers/root-ci.ts +++ b/ui/scripts/__tests__/helpers/root-ci.ts @@ -11,9 +11,11 @@ * this module. It never reads a copy under `ui/.github/`: nothing runs one. * * What the port did not carry (the coverage lane, the docs job, the shard - * aggregate, the affected-test and type-check-scope selectors, the timings - * upload) has no job here, and the pins on those retired with a disposition - * beside them rather than being pointed at a job that does something else. + * aggregate, the type-check-scope selector, the timings upload) has no job + * here, and the pins on those retired with a disposition beside them rather + * than being pointed at a job that does something else. The affected-test + * selector was wired into `test-ui` later (objectstack#25868), and its pins + * read that job. */ import fs from 'node:fs'; import path from 'node:path'; diff --git a/ui/scripts/__tests__/select-affected-tests-wiring.test.ts b/ui/scripts/__tests__/select-affected-tests-wiring.test.ts index e0c57e73e56..d4ed1181882 100644 --- a/ui/scripts/__tests__/select-affected-tests-wiring.test.ts +++ b/ui/scripts/__tests__/select-affected-tests-wiring.test.ts @@ -5,13 +5,17 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { workspaceRoot } from '../workspace-root.mjs'; + /** - * objectui#13350 — the two steps of the `test` job that make the affected - * speed, held together by RUNNING them. + * objectui#13350 — the two steps of the `Test UI` job that make the affected + * speed, held together by RUNNING them. Since objectstack#25868 the job is + * `test-ui` in the objectstack monorepo's root `ci.yml`, the workflow that + * runs this tree's suite; the objectui copy under `ui/.github/` runs nowhere. * * `Select the tests this change affects` (id `select`) appends the selector's - * verdict line to `$GITHUB_OUTPUT`; `Run tests (shard N/16)` reads one output of - * that step into an env var and branches on it. The runner splits each appended + * verdict line to `$GITHUB_OUTPUT`; `Run the ui tests (shard N/16)` reads one + * output of that step into an env var and branches on it. The runner splits each appended * line at its first `=`, so the line `mode=affected` is the output `mode` with * the value `affected`. From objectui#12214 until this card the run step * compared its env var with the whole line -- never true -- and every pull @@ -22,27 +26,32 @@ import { fileURLToPath } from 'node:url'; * compares with. * * So this file restates neither end. It extracts both steps' `env:` and `run:` - * from `ci.yml`, runs the select step with bash against this checkout -- the - * real selector, based on HEAD -- parses what it appended the way the runner - * does, hands the run step whatever output its `env:` names, and reads which - * command the run step chose off a `pnpm` stand-in that records its argv. + * from `ci.yml`, runs the select step with bash at the workflow's working + * directory (the monorepo root) against this checkout -- the real selector, + * with the merge base a `git` stand-in answers as HEAD -- parses what it + * appended the way the runner does, hands the run step whatever output its + * `env:` names, and reads which command the run step chose off a `pnpm` + * stand-in that records its argv. * Textual rather than YAML-parsed, for the reason `markdown-test-inputs.test.ts` * gives: what is under test is the bytes CI runs. * * affected a `pull_request` and a `merge_group` event hand vitest exactly the * filters the selector wrote. - * full a `push` event, and a pull request with no base sha (the - * fail-open path), run the whole suite. + * full a `push` event, a pull request with no base ref and one whose + * merge base cannot be computed (the fail-open paths), run the + * whole suite. * controls the run step's comparison reverted to the whole line, and the * select step's output written under another key or value, each * ALONE, put a pull request on the full suite -- so this file * cannot pass while the two ends disagree. */ const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); -const CI_YML = path.join(repoRoot, '.github', 'workflows', 'ci.yml'); +/** Where the workflow runs: its steps' working directory, and the home of `.github/`. */ +const workflowRoot = workspaceRoot(repoRoot); +const CI_YML = path.join(workflowRoot, '.github', 'workflows', 'ci.yml'); const SELECT_HEADER = ' - name: Select the tests this change affects\n'; -const RUN_HEADER = ' - name: Run tests (shard ${{ matrix.shard }}/16)\n'; +const RUN_HEADER = ' - name: Run the ui tests (shard ${{ matrix.shard }}/16)\n'; /** The output reference a run step's `env:` value is, when it reads the select step. */ const SELECT_OUTPUT = /^\$\{\{ steps\.select\.outputs\.([A-Za-z0-9_-]+) \}\}$/; @@ -98,6 +107,7 @@ function modeBinding(): { variable: string; key: string } { /* ── the runner, as far as these two steps need it ──────────────────────── */ +/** `base` is the pull request's base REF, or the merge group's base sha. */ type Event = { name: 'pull_request' | 'merge_group' | 'push'; base: string }; /** `${{ … }}` resolved the way the runner would for this event, shard 1. */ @@ -108,7 +118,7 @@ function expand(text: string, event: Event, temp: string, outputs: Record; log: string }; /** Run the select step, with `bash -e` as the runner does, in a fresh `RUNNER_TEMP`. */ -function runSelect(event: Event, temp: string, bin?: string): Selected { +function runSelect(event: Event, temp: string, bin?: string, extraEnv: Record = {}): Selected { const env = Object.fromEntries(SELECT.env.map(([name, value]) => [name, expand(value, event, temp)])); const script = path.join(temp, 'select.sh'); fs.writeFileSync(script, expand(SELECT.run, event, temp)); const githubOutput = path.join(temp, 'github-output-select'); fs.writeFileSync(githubOutput, ''); const run = spawnSync('bash', ['-e', script], { - cwd: repoRoot, + cwd: workflowRoot, encoding: 'utf8', env: { ...process.env, ...env, + ...extraEnv, RUNNER_TEMP: temp, GITHUB_OUTPUT: githubOutput, - PATH: [bin, nodeDir, process.env.PATH].filter(Boolean).join(path.delimiter), + PATH: [bin, gitBin, nodeDir, process.env.PATH].filter(Boolean).join(path.delimiter), }, }); if (run.status !== 0) throw new Error(`the select step exited ${run.status}:\n${run.stdout}${run.stderr}`); @@ -170,6 +181,10 @@ function runSelect(event: Event, temp: string, bin?: string): Selected { type Shard = { argv: string[]; log: string }; +/** How the run step's `pnpm` argv starts on each speed: the suite runs from ui/, the vitest root. */ +const FULL_ARGV = ['--dir', 'ui', 'run', 'test']; +const AFFECTED_ARGV = ['--dir', 'ui', 'exec', 'vitest', 'run']; + /** Run the shard step with the outputs the select step left, `pnpm` recording its argv. */ function runShard(event: Event, temp: string, outputs: Record, run = RUN.run): Shard { const env = Object.fromEntries(RUN.env.map(([name, value]) => [name, expand(value, event, temp, outputs)])); @@ -178,7 +193,7 @@ function runShard(event: Event, temp: string, outputs: Record, r const argvFile = path.join(temp, 'pnpm-argv'); const bin = standIn('pnpm', `printf '%s\\n' "$@" > ${JSON.stringify(argvFile)}\n`); const result = spawnSync('bash', ['-e', script], { - cwd: repoRoot, + cwd: workflowRoot, encoding: 'utf8', env: { ...process.env, ...env, RUNNER_TEMP: temp, PATH: `${bin}${path.delimiter}${process.env.PATH}` }, }); @@ -189,6 +204,11 @@ function runShard(event: Event, temp: string, outputs: Record, r /* ── one real selector run, replayed where only the wiring is in question ── */ let head = ''; +/** The base ref every pull_request leg names; the `git` stand-in answers its merge base as HEAD. */ +const BASE_REF = 'main'; +/** A `git` that answers `merge-base` with HEAD and records what it was asked; every other call is the real git. */ +let gitBin = ''; +let mergeBaseArgv = ''; /** The real selector's answer on this checkout, taken through the real select step. */ let real: Selected & { filters: string[]; temp: string } = { outputs: {}, log: '', filters: [], temp: '' }; /** A `node` that replays `real` for the select step, so each leg does not re-walk the tree. */ @@ -201,11 +221,25 @@ function freshTemp(leg: string): string { beforeAll(() => { scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'select-affected-wiring-13350-')); head = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot, encoding: 'utf8' }).trim(); + const realGit = execFileSync('bash', ['-c', 'command -v git'], { encoding: 'utf8' }).trim(); + mergeBaseArgv = path.join(scratch, 'merge-base-argv'); + gitBin = standIn( + 'git', + [ + 'if [ "$1" = merge-base ]; then', + ` printf '%s\\n' "$@" > ${JSON.stringify(mergeBaseArgv)}`, + ' if [ -n "$OS_TEST_MERGE_BASE_FAILS" ]; then exit 1; fi', + ` exec ${JSON.stringify(realGit)} rev-parse HEAD`, + 'fi', + `exec ${JSON.stringify(realGit)} "$@"`, + '', + ].join('\n'), + ); const temp = freshTemp('real'); // The select step itself names these two files under `$RUNNER_TEMP`. const filters = path.join(temp, 'affected-tests.txt'); const stdout = path.join(temp, 'select.out'); - real = { ...runSelect({ name: 'pull_request', base: head }, temp), filters: [], temp }; + real = { ...runSelect({ name: 'pull_request', base: BASE_REF }, temp), filters: [], temp }; real.filters = fs.readFileSync(filters, 'utf8').split('\n').filter(Boolean); replay = standIn( 'node', @@ -238,10 +272,21 @@ describe('the affected speed, both steps run as ci.yml writes them (objectui#133 expect(modeBinding().key).not.toBe(''); }); + it('a pull_request diffs from the merge base of origin/ and HEAD, never the frozen payload sha', () => { + expect(fs.readFileSync(mergeBaseArgv, 'utf8').split('\n').filter(Boolean)).toEqual([ + 'merge-base', + `refs/remotes/origin/${BASE_REF}`, + 'HEAD', + ]); + expect(real.log).toContain(`Diff base: ${head}`); + expect(SELECT.env.map(([name]) => name)).not.toContain('PR_BASE'); + }); + it('a pull_request hands vitest exactly the filters the real selector wrote', () => { expect(real.filters.length).toBeGreaterThan(0); // The same `RUNNER_TEMP` the select step wrote its filters into, as one job's steps share it. - const shard = runShard({ name: 'pull_request', base: head }, real.temp, real.outputs); + const shard = runShard({ name: 'pull_request', base: BASE_REF }, real.temp, real.outputs); + expect(shard.argv.slice(0, AFFECTED_ARGV.length)).toEqual(AFFECTED_ARGV); expect(shard.argv.slice(-real.filters.length)).toEqual(real.filters); expect(shard.argv).toContain('--shard=1/16'); expect(shard.log).toContain(`Affected speed: ${real.filters.length} vitest filter(s), shard 1 of 16.`); @@ -260,18 +305,28 @@ describe('the affected speed, both steps run as ci.yml writes them (objectui#133 const temp = freshTemp('push'); const event: Event = { name: 'push', base: '' }; const shard = runShard(event, temp, runSelect(event, temp, replay).outputs); - expect(shard.argv[0]).toBe('test'); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); expect(shard.argv).toContain('--shard=1/16'); expect(shard.argv.filter((arg) => real.filters.includes(arg))).toEqual([]); }); - it('a pull request with no base sha runs the whole suite (the step fails open)', () => { + it('a pull request with no base ref runs the whole suite (the step fails open)', () => { const temp = freshTemp('no-base'); const event: Event = { name: 'pull_request', base: '' }; const selected = runSelect(event, temp, replay); expect(selected.log).toContain('running the full suite rather than guessing'); const shard = runShard(event, temp, selected.outputs); - expect(shard.argv[0]).toBe('test'); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); + }); + + it('a pull request whose merge base cannot be computed runs the whole suite (the step fails open)', () => { + const temp = freshTemp('no-merge-base'); + const event: Event = { name: 'pull_request', base: BASE_REF }; + const selected = runSelect(event, temp, replay, { OS_TEST_MERGE_BASE_FAILS: '1' }); + expect(selected.log).toContain('running the full suite rather than guessing'); + expect(selected.outputs[modeBinding().key]).toBe('full'); + const shard = runShard(event, temp, selected.outputs); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); }); }); @@ -283,21 +338,21 @@ describe('controls: either end edited alone is a full run, so the legs above can expect(found, `the run step no longer compares $${variable} with a quoted literal; rewrite this control`).not.toBeNull(); const reverted = RUN.run.replace(comparison, `[ "$${variable}" = '${key}=${found![1]}' ]`); expect(reverted).not.toBe(RUN.run); - const shard = runShard({ name: 'pull_request', base: head }, freshTemp('reverted'), real.outputs, reverted); - expect(shard.argv[0]).toBe('test'); + const shard = runShard({ name: 'pull_request', base: BASE_REF }, freshTemp('reverted'), real.outputs, reverted); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); }); it('the select step writing its verdict under another key runs the whole suite', () => { const renamed = Object.fromEntries(Object.entries(real.outputs).map(([key, value]) => [`${key}_renamed`, value])); - const shard = runShard({ name: 'pull_request', base: head }, freshTemp('renamed-key'), renamed); - expect(shard.argv[0]).toBe('test'); + const shard = runShard({ name: 'pull_request', base: BASE_REF }, freshTemp('renamed-key'), renamed); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); }); it('the select step writing another value under the same key runs the whole suite', () => { const { key } = modeBinding(); expect(real.outputs[key], `the select step wrote no \`${key}\` output`).toBeDefined(); const changed = { ...real.outputs, [key]: `${real.outputs[key]}-renamed` }; - const shard = runShard({ name: 'pull_request', base: head }, freshTemp('renamed-value'), changed); - expect(shard.argv[0]).toBe('test'); + const shard = runShard({ name: 'pull_request', base: BASE_REF }, freshTemp('renamed-value'), changed); + expect(shard.argv.slice(0, FULL_ARGV.length)).toEqual(FULL_ARGV); }); }); diff --git a/ui/scripts/__tests__/select-affected-tests.test.ts b/ui/scripts/__tests__/select-affected-tests.test.ts index ce6887e98a5..b232983397c 100644 --- a/ui/scripts/__tests__/select-affected-tests.test.ts +++ b/ui/scripts/__tests__/select-affected-tests.test.ts @@ -6,16 +6,20 @@ import { execFileSync, spawnSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import { + changedFiles, + changedOutsideTree, closureSummaries, dedupeFilters, listWorkspacePackages, moduleSpecifiers, + outsideTreeReasons, packageClosures, parseWorkspaceGlobs, readHeavyProject, selectAffectedTests, trackedFiles, } from '../select-affected-tests.mjs'; +import { jobBlock, rootCi, triggerBlock, UI_JOBS, uiRun, withoutComments } from './helpers/root-ci'; /** * objectui#12214 — the affected-test selector behind the two-speed `Test` lane. @@ -222,6 +226,102 @@ describe('select-affected-tests — the heavy DOM setup', () => { }); }); +/** + * objectstack#25868 — the tree nested as `ui/` of a monorepo, the layout the + * root `ci.yml` runs it in. The tree's own files are the fixture above; the + * parent holds the one workspace file, the lockfile and its own packages: + * `@os/base` <- `@os/mid` (runtime) <- the tree's `@fx/a` (dev), `@os/mid` + * dev-depends on `@os/tooling`, and `@os/other` is unrelated. + */ +function nestedWorkspace() { + const tree = workspace(); + const files: Record = { + 'pnpm-workspace.yaml': "packages:\n - 'packages/*'\n - ui\n - 'ui/packages/*'\n", + 'package.json': manifest('monorepo'), + 'pnpm-lock.yaml': 'lockfileVersion: 9.0\n', + 'README.md': '# monorepo\n', + 'packages/base/package.json': manifest('@os/base'), + 'packages/base/src/index.ts': 'export const base = 1;\n', + 'packages/mid/package.json': JSON.stringify({ + name: '@os/mid', + version: '0.0.0', + dependencies: { '@os/base': 'workspace:*' }, + devDependencies: { '@os/tooling': 'workspace:*' }, + }), + 'packages/tooling/package.json': manifest('@os/tooling'), + 'packages/tooling/src/index.ts': 'export const t = 1;\n', + 'packages/other/package.json': manifest('@os/other'), + 'packages/other/src/index.ts': 'export const o = 1;\n', + }; + for (const rel of tree.tracked) { + if (rel === 'pnpm-workspace.yaml') continue; // a nested workspace root is ignored; the parent's governs + const text = fs.readFileSync(path.join(tree.root, rel), 'utf8'); + files[`ui/${rel}`] = + rel === 'packages/a/package.json' + ? JSON.stringify({ name: '@fx/a', version: '0.0.0', devDependencies: { '@os/mid': 'workspace:*' } }) + : text; + } + const { root } = fixture(files); + const ui = path.join(root, 'ui'); + return { monorepo: root, root: ui, tracked: tree.tracked.filter((f) => f !== 'pnpm-workspace.yaml') }; +} + +describe('select-affected-tests — nested as ui/ of a monorepo (objectstack#25868)', () => { + it('a change outside the tree that every test installs or builds from is a full run', () => { + const { root } = nestedWorkspace(); + const reasons = (outside: string[]) => outsideTreeReasons({ root, outside }); + expect(reasons(['pnpm-lock.yaml'])[0]).toMatch(/^pnpm-lock\.yaml: a workspace-root file that is not documentation/); + expect(reasons(['package.json'])).toHaveLength(1); + expect(reasons(['patches/x@1.0.0.patch'])[0]).toMatch(/a dependency patch/); + // A package the tree depends on, and what that package depends on at run time. + expect(reasons(['packages/mid/src/index.ts'])[0]).toMatch(/@os\/mid changed, .* \(through @fx\/a\)$/); + expect(reasons(['packages/base/src/index.ts'])[0]).toMatch(/@os\/base changed/); + }); + + it('a change outside the tree that no test runs through is not', () => { + const { root } = nestedWorkspace(); + // An outer package's devDependency is its own test tooling, not its built output. + for (const rel of [ + 'packages/tooling/src/index.ts', + 'packages/other/src/index.ts', + 'README.md', + 'docs/guide.md', + '.github/workflows/ci.yml', + 'scripts/gate.mjs', + ]) { + expect(outsideTreeReasons({ root, outside: [rel] }), rel).toEqual([]); + } + }); + + it('reads the tree as before when nothing outside it changed, and turns full when the lockfile did', () => { + const { root, tracked } = nestedWorkspace(); + expect(listWorkspacePackages(root).map((p) => p.dir)).toContain('packages/a'); + const v = affected(root, tracked, ['packages/c/src/index.ts']); + expect(v.affectedPackages).toEqual(['@fx/c']); + const outside = ['docs/guide.md', 'packages/other/src/index.ts']; + expect(selectAffectedTests({ root, tracked, changed: ['packages/c/src/index.ts'], outside }).mode).toBe('affected'); + const verdict = selectAffectedTests({ root, tracked, changed: ['packages/c/src/index.ts'], outside: ['pnpm-lock.yaml'] }); + expect(verdict.mode).toBe('full'); + }); + + it('git hands the tree its own changes and the monorepo its own, both through --relative', () => { + const { monorepo, root } = nestedWorkspace(); + const git = (...args: string[]) => + execFileSync('git', ['-c', 'user.name=fixture', '-c', 'user.email=fixture@example.invalid', ...args], { + cwd: monorepo, + encoding: 'utf8', + }); + git('init', '-q'); + git('add', '-A'); + git('commit', '-q', '-m', 'base'); + fs.appendFileSync(path.join(root, 'packages/c/src/index.ts'), 'export const d = 2;\n'); + fs.appendFileSync(path.join(monorepo, 'pnpm-lock.yaml'), '# moved\n'); + git('commit', '-q', '-am', 'change'); + expect(changedFiles(root, 'HEAD~1')).toEqual(['packages/c/src/index.ts']); + expect(changedOutsideTree(root, 'HEAD~1')).toEqual(['pnpm-lock.yaml']); + }); +}); + describe('select-affected-tests — readers', () => { it('drops a filter another one already matches, as the vitest invocation guard requires (objectui#7814)', () => { expect(dedupeFilters(['packages/c/src/plain.test.tsx', 'packages/c/src/plain.test.ts', 'packages/c/'])).toEqual([ @@ -316,14 +416,40 @@ describe('select-affected-tests — the live tree', () => { }); }); -// `ci.yml wires the selector fail-open (objectui#12214)` retired with -// objectstack#25712. It held objectui ci.yml's `test` job to this selector: -// the hourly full battery, the `Select the tests this change affects` step's -// two selecting events and its fail-open verdict, and the full-suite command -// beside the filtered one. That file runs nowhere under the monorepo. The -// root `test-ui` job that replaced it runs the whole suite on every event and -// does not wire the selector (its comment says why: `git diff --name-only` -// prints `ui/...` paths against a graph spelled relative to the objectui -// root), as row 7 `Test (shard n/16)` of the objectstack#25697 workflow roster -// records. Wiring it there brings these pins back against `test-ui` -// (`helpers/root-ci.ts`). +// objectui ci.yml's `test` job wired this selector; objectstack#25712 retired +// these pins with that file, and objectstack#25868 wired the selector into the +// root `test-ui` job that replaced it, so they read that job now. +describe("the root ci.yml's Test UI job wires the selector fail-open (objectui#12214, objectstack#25868)", () => { + const code = withoutComments(rootCi()); + const job = jobBlock(code, UI_JOBS.test); + + it('runs the full battery hourly on main, beside push', () => { + expect(triggerBlock(code)).toMatch(/^ {2}schedule:\n {4}- cron: '[^']+'$/m); + }); + + it('selects only on pull_request and merge_group, and treats every other event as full', () => { + const step = job.slice(job.indexOf('- name: Select the tests this change affects')); + expect(step).toMatch(/pull_request\)\n\s+if \[ -n "\$PR_BASE_REF" \]; then\n\s+base=\$\(git merge-base "refs\/remotes\/origin\/\$PR_BASE_REF" HEAD\) \|\| base=''/); + expect(step).toMatch(/merge_group\) base="\$QUEUE_BASE"/); + expect(step).toMatch(/\*\)\n\s+echo 'mode=full' >> "\$GITHUB_OUTPUT"/); + }); + + it('turns a selector that cannot decide into a full run, not a skip', () => { + const step = job.slice(job.indexOf('- name: Select the tests this change affects')); + expect(step).toContain('|| status=$?'); + expect(step).toMatch(/verdict='mode=full'/); + // objectui#6221: the status must not be swallowed by a command substitution. + expect(step).not.toMatch(/\$\(node ui\/scripts\/select-affected-tests\.mjs/); + }); + + it('checks out the history the diff base needs', () => { + expect(job).toMatch(/uses: actions\/checkout@\S+\n\s+with:\n\s+fetch-depth: 0\n/); + }); + + it('keeps the matrix width and the full-suite command for the full battery', () => { + const run = job.slice(job.indexOf('- name: Run the ui tests (shard ${{ matrix.shard }}/16)')); + expect(job).toContain('shard: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]'); + expect(run).toContain(`${uiRun('test')} --shard=` + '${{ matrix.shard }}/16'); + expect(run).toContain('pnpm --dir ui exec vitest run --shard=${{ matrix.shard }}/16 --reporter=default --reporter=github-actions "${filters[@]}"'); + }); +}); diff --git a/ui/scripts/select-affected-tests.mjs b/ui/scripts/select-affected-tests.mjs index 7270814406d..d57fc0e8e22 100644 --- a/ui/scripts/select-affected-tests.mjs +++ b/ui/scripts/select-affected-tests.mjs @@ -68,7 +68,11 @@ * one); * - an affected package that a BROAD root setup file imports by name: * the light DOM setup installs the i18n catalogues into every `dom` - * test, an edge no test file states. + * test, an edge no test file states; + * - nested as `ui/` of the objectstack monorepo, a change OUTSIDE the + * tree that its tests install or build from: a non-documentation + * file at the workspace root, `patches/`, or a monorepo package the + * tree depends on (`outsideTreeReasons` says which). * * 4. THE HEAVY SETUP. The setup of the `dom-heavy` project registers four * packages' components into every test of that project and of each @@ -110,7 +114,13 @@ import { execFileSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import { isEntrypoint } from './invoked-as.mjs'; -import { parseWorkspaceGlobs, workspaceGlobs } from './workspace-root.mjs'; +import { + nestedPrefix, + parseWorkspaceGlobs, + workspaceGlobs, + workspaceManifestPath, + workspaceRoot, +} from './workspace-root.mjs'; export const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -141,6 +151,9 @@ const DISK_SPECIFIERS = new Set([ 'tinyglobby', ]); +const DEPENDENCY_FIELDS = ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies']; +const RUNTIME_DEPENDENCY_FIELDS = DEPENDENCY_FIELDS.filter((field) => field !== 'devDependencies'); + const RESOLVE_EXTENSIONS = ['', '.ts', '.tsx', '.mts', '.cts', '.js', '.jsx', '.mjs', '.cjs', '.json']; const toPosix = (p) => p.split(path.sep).join('/'); @@ -154,21 +167,19 @@ const toPosix = (p) => p.split(path.sep).join('/'); export { parseWorkspaceGlobs }; /** - * Every workspace package: its name, its repository-relative directory and the - * workspace packages it names in any dependency field. + * The named manifests under `globs` (`dir` or `dir/*`, already parsed), + * relative to `base`. * - * @param {string} root - * @returns {Array<{ name: string, dir: string, deps: string[] }>} + * @param {string} base + * @param {string[]} globs + * @returns {Array<{ name: string, dir: string, pkg: Record }>} */ -export function listWorkspacePackages(root = REPO_ROOT) { - // Relative to `root` in both layouts: the objectui root's own file, or the - // monorepo root's `ui/...` globs with the prefix stripped (objectstack#25723). - const globs = workspaceGlobs(root); +function readManifests(base, globs) { const dirs = []; for (const glob of globs) { if (glob.endsWith('/*')) { const parent = glob.slice(0, -2); - const abs = path.join(root, parent); + const abs = path.join(base, parent); if (!fs.existsSync(abs)) continue; for (const entry of fs.readdirSync(abs, { withFileTypes: true })) { if (entry.isDirectory()) dirs.push(`${parent}/${entry.name}`); @@ -179,16 +190,30 @@ export function listWorkspacePackages(root = REPO_ROOT) { } const manifests = []; for (const dir of dirs.sort()) { - const file = path.join(root, dir, 'package.json'); + const file = path.join(base, dir, 'package.json'); if (!fs.existsSync(file)) continue; const pkg = JSON.parse(fs.readFileSync(file, 'utf8')); if (typeof pkg.name !== 'string') continue; manifests.push({ name: pkg.name, dir, pkg }); } + return manifests; +} + +/** + * Every workspace package: its name, its repository-relative directory and the + * workspace packages it names in any dependency field. + * + * @param {string} root + * @returns {Array<{ name: string, dir: string, deps: string[] }>} + */ +export function listWorkspacePackages(root = REPO_ROOT) { + // Relative to `root` in both layouts: the objectui root's own file, or the + // monorepo root's `ui/...` globs with the prefix stripped (objectstack#25723). + const manifests = readManifests(root, workspaceGlobs(root)); const names = new Set(manifests.map((m) => m.name)); return manifests.map(({ name, dir, pkg }) => { const deps = new Set(); - for (const field of ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies']) { + for (const field of DEPENDENCY_FIELDS) { for (const dep of Object.keys(pkg[field] ?? {})) if (names.has(dep) && dep !== name) deps.add(dep); } return { name, dir, deps: [...deps].sort() }; @@ -441,6 +466,100 @@ export function changedFiles(root, base, head = 'HEAD') { .filter(Boolean); } +/** + * The changed files OUTSIDE this tree, relative to the workspace root -- the + * monorepo's own files when the tree is nested (`ui/` of objectstack), and + * nothing standalone, where the tree is the whole repository. `changedFiles` + * leaves them out by construction (`--relative`), so this is the second half + * of the same diff: git runs at the workspace root with `--relative` again, + * and the tree's own prefix is dropped from what it prints. + */ +export function changedOutsideTree(root, base, head = 'HEAD') { + const prefix = nestedPrefix(root); + if (!prefix) return []; + return changedFiles(workspaceRoot(root), base, head).filter((f) => !f.startsWith(prefix)); +} + +/** + * Why a change outside this tree reaches every test in it, one reason per + * file that does (an empty list means none does). The tree's tests install + * from the workspace root, so its install and toolchain inputs are this tree's + * too, and they import the monorepo's own packages the tree depends on from + * their built output (the CI job builds them first): + * + * - a workspace-root file that is not documentation (the one lockfile, the + * workspace file, the root manifest, `turbo.json`, a root `tsconfig`), or + * anything under `patches/` -- the rule item 3 states for this tree's own + * root, applied to the root it installs from; + * - a file in a workspace package outside this tree that a package of this + * tree depends on (any dependency field), or that such a package depends + * on at run time (`dependencies`, `peerDependencies`, + * `optionalDependencies`; an outer package's devDependencies are its own + * test tooling, not its built output). On objectstack that is the seven + * packages the `ui_consumers` filter of `ci.yml` names. Which of this + * tree's tests read the changed export is not something the package graph + * can say, so every test runs. + * + * Every other path -- another package of the monorepo, its docs, scripts and + * workflows -- reaches this tree's tests only through the always-on set, as a + * file outside every package does inside it. + * + * @param {{ root?: string, outside: string[] }} input `outside` relative to the workspace root + * @returns {string[]} + */ +export function outsideTreeReasons({ root = REPO_ROOT, outside }) { + if (outside.length === 0) return []; + const prefix = nestedPrefix(root); + const wsRoot = workspaceRoot(root); + const own = new Set(listWorkspacePackages(root).map((p) => p.name)); + const globs = parseWorkspaceGlobs(fs.readFileSync(workspaceManifestPath(root), 'utf8')).filter( + (g) => !g.startsWith(prefix) && `${g}/` !== prefix, + ); + const outer = readManifests(wsRoot, globs).filter((m) => !own.has(m.name)); + const outerNames = new Set(outer.map((m) => m.name)); + const depsOf = (pkg, fields = DEPENDENCY_FIELDS) => fields.flatMap((field) => Object.keys(pkg[field] ?? {})); + + // The outer packages this tree reaches, each with the tree package that + // reaches it first (named in the reason). + const reachedBy = new Map(); + const queue = []; + for (const m of readManifests(root, workspaceGlobs(root))) { + for (const dep of depsOf(m.pkg)) { + if (outerNames.has(dep) && !reachedBy.has(dep)) { + reachedBy.set(dep, m.name); + queue.push(dep); + } + } + } + const byName = new Map(outer.map((m) => [m.name, m])); + while (queue.length) { + const name = queue.shift(); + for (const dep of depsOf(byName.get(name).pkg, RUNTIME_DEPENDENCY_FIELDS)) { + if (outerNames.has(dep) && !reachedBy.has(dep)) { + reachedBy.set(dep, reachedBy.get(name)); + queue.push(dep); + } + } + } + + const reasons = []; + for (const rel of outside) { + if (!rel.includes('/') && !ROOT_DOC_FILE.test(rel)) { + reasons.push(`${rel}: a workspace-root file that is not documentation (install, toolchain or config input of this tree)`); + continue; + } + if (rel.startsWith('patches/')) { + reasons.push(`${rel}: a dependency patch changes what is installed for every test`); + continue; + } + const owner = owningPackage(rel, outer); + if (owner && reachedBy.has(owner.name)) { + reasons.push(`${rel}: ${owner.name} changed, and this tree's tests read its built output (through ${reachedBy.get(owner.name)})`); + } + } + return reasons; +} + /** The root vitest config and every `vitest.setup*` file beside it. */ export function rootTestConfigFiles(tracked) { return tracked.filter((f) => ROOT_TEST_CONFIG.includes(f) || /^vitest\.setup[^/]*\.[cm]?[jt]sx?$/.test(f)).sort(); @@ -474,10 +593,12 @@ export function readHeavyProject(configText) { /** * The verdict for one change. * - * @param {{ root?: string, changed: string[], tracked?: string[] }} input + * @param {{ root?: string, changed: string[], outside?: string[], tracked?: string[] }} input + * `changed` relative to `root`; `outside` the changed files outside the tree, + * relative to the workspace root (`changedOutsideTree`) * @returns {{ mode: 'full', reasons: string[] } | { mode: 'affected', changedPackages: string[], affectedPackages: string[], heavyEdge: string[], alwaysOn: Array<{ file: string, why: string }>, filters: string[], testFileCount: number, selectedTestFileCount: number }} */ -export function selectAffectedTests({ root = REPO_ROOT, changed, tracked = trackedFiles(root) }) { +export function selectAffectedTests({ root = REPO_ROOT, changed, outside = [], tracked = trackedFiles(root) }) { const packages = listWorkspacePackages(root); const names = new Set(packages.map((p) => p.name)); const { dependencyClosure, dependentClosure } = packageClosures(packages); @@ -537,7 +658,7 @@ export function selectAffectedTests({ root = REPO_ROOT, changed, tracked = track }) .map((f) => `${path.posix.dirname(f)}/`); - const reasons = []; + const reasons = outsideTreeReasons({ root, outside }); const changedPackages = new Set(); for (const rel of changed) { const base = rel.split('/').pop() ?? rel; @@ -658,8 +779,10 @@ function main(argv) { return 2; } const changed = changedFiles(REPO_ROOT, base); + const outside = changedOutsideTree(REPO_ROOT, base); console.log(`changed files against the merge base of ${base}: ${changed.length}`); - const verdict = selectAffectedTests({ changed }); + if (nestedPrefix(REPO_ROOT)) console.log(`changed files outside ${nestedPrefix(REPO_ROOT)}: ${outside.length}`); + const verdict = selectAffectedTests({ changed, outside }); if (verdict.mode === 'full') { console.log('FULL RUN — this change reaches something every test runs through:'); for (const r of verdict.reasons) console.log(` - ${r}`);