From 05ea6414ea79813558c5404584e8d6bfde970ab7 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Wed, 28 Oct 2026 04:24:40 -0700 Subject: [PATCH] ci: run the full matrix once per PR, in the merge queue (#420) Pull requests now run a fast subset: Python 3.12 on ubuntu-latest, the coverage gate, black/flake8, frontend modules, bandit and the Chromium/touch frontend coverage shards plus merge. The merge queue, pushes to main and manual runs keep the full matrix: Linux/Windows/macOS x Python 3.12-3.14, real ComfyUI compatibility and the Firefox/WebKit page workflows. A dropped matrix leg never reports a check, so the new required "all tests" gate requires every test.yml job to succeed outside pull requests and real ComfyUI to be skipped only on pull requests; viewerjs does the same for the browser shards. branch-protection.json drops the eight legs a pull request no longer runs and adds "all tests". --- .github/branch-protection.json | 9 +- .github/workflows/test.yml | 41 ++++++++- .github/workflows/viewerjs-check.yml | 13 ++- TESTING.md | 60 ++++++++++--- tests/vendor/ci-workflow.test.mjs | 129 ++++++++++++++++++++++----- 5 files changed, 202 insertions(+), 50 deletions(-) diff --git a/.github/branch-protection.json b/.github/branch-protection.json index 985d4116..4aca6925 100644 --- a/.github/branch-protection.json +++ b/.github/branch-protection.json @@ -3,17 +3,10 @@ "strict": false, "contexts": [ "py3.12 on ubuntu-latest", - "py3.13 on ubuntu-latest", - "py3.14 on ubuntu-latest", - "py3.12 on windows-latest", - "py3.13 on windows-latest", - "py3.14 on windows-latest", - "py3.12 on macos-latest", - "py3.13 on macos-latest", - "py3.14 on macos-latest", "coverage gate (85%)", "black and flake8", "frontend modules", + "all tests", "viewerjs", "real ComfyUI compatibility", "bandit (high severity)" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 43bf3d99..7b1fe8bf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -32,6 +32,11 @@ env: # coverage gate (85%): 7m26s / 7m22s / 7m54s -> 15 min # perf budget 30s / 29s / 27s, black and flake8 39s / 37s / 39s, # frontend modules 23s / 19s / 29s -> 5 min (the floor) +# all tests: a result check without setup, 5 min (the floor) +# +# Pull requests run a fast subset (#420): Python 3.12 on ubuntu-latest, coverage, lint and frontend +# modules. The merge queue (merge_group), pushes to main and manual runs take the full matrix and the +# real ComfyUI job. "all tests" is the required gate that holds the queue to that full set. jobs: test: name: py${{ matrix.python-version }} on ${{ matrix.os }} @@ -40,8 +45,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] - python-version: ["3.12", "3.13", "3.14"] + os: ${{ github.event_name == 'pull_request' && fromJSON('["ubuntu-latest"]') || fromJSON('["ubuntu-latest", "windows-latest", "macos-latest"]') }} + python-version: ${{ github.event_name == 'pull_request' && fromJSON('["3.12"]') || fromJSON('["3.12", "3.13", "3.14"]') }} steps: - uses: actions/checkout@v7 @@ -63,6 +68,9 @@ jobs: comfyui: name: real ComfyUI compatibility + # Merge queue, main and manual runs only. A job skipped by its condition reports Success, so this + # required context never blocks a pull request; "all tests" fails if it is skipped anywhere else. + if: github.event_name != 'pull_request' runs-on: ubuntu-latest # Chromium and its system libraries come from the pinned Playwright image (tag = @playwright/test in # tools/vendor/package-lock.json; tests/vendor/browser-image.test.mjs checks it). The runner install @@ -253,3 +261,32 @@ jobs: - name: Run JS tests run: node --test tests/js/*.test.js + + # The required context "all tests". A dropped matrix leg never reports a check at all, so the eight + # legs that pull requests skip cannot be required by name; this gate requires them instead. Every + # job must succeed; real ComfyUI must be skipped on a pull request and succeed on every other event. + all-tests: + name: all tests + needs: [test, comfyui, coverage, lint, js-test] + if: always() + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Require every test job for this event + env: + EVENT_NAME: ${{ github.event_name }} + TEST_RESULT: ${{ needs.test.result }} + COMFYUI_RESULT: ${{ needs.comfyui.result }} + COVERAGE_RESULT: ${{ needs.coverage.result }} + LINT_RESULT: ${{ needs.lint.result }} + JS_RESULT: ${{ needs.js-test.result }} + run: | + queue_only=success + if [ "$EVENT_NAME" = pull_request ]; then queue_only=skipped; fi + echo "event: $EVENT_NAME (real ComfyUI must be $queue_only)" + echo "test matrix: $TEST_RESULT" + echo "comfyui: $COMFYUI_RESULT" + echo "coverage: $COVERAGE_RESULT" + echo "lint: $LINT_RESULT" + echo "js-test: $JS_RESULT" + test "$TEST_RESULT" = success && test "$COVERAGE_RESULT" = success && test "$LINT_RESULT" = success && test "$JS_RESULT" = success && test "$COMFYUI_RESULT" = "$queue_only" diff --git a/.github/workflows/viewerjs-check.yml b/.github/workflows/viewerjs-check.yml index 8582a67d..783e1b92 100644 --- a/.github/workflows/viewerjs-check.yml +++ b/.github/workflows/viewerjs-check.yml @@ -165,6 +165,8 @@ jobs: browser-workflows: name: page workflows (${{ matrix.browser }}, shard ${{ matrix.shard }}/2) + # Merge queue, main and manual runs only (#420); pull requests run the Chromium/touch coverage shards. + if: github.event_name != 'pull_request' runs-on: ubuntu-latest # Layout comparisons depend on the fonts each browser falls back to, so browser jobs run in the # same pinned Playwright image as local runs (tag = @playwright/test in tools/vendor/package-lock.json; @@ -219,8 +221,9 @@ jobs: retention-days: 14 # Branch protection requires the exact context "viewerjs". This job keeps that - # name and fails unless every frontend job above succeeded (every shard, the - # coverage merge and every browser shard), including when one was cancelled or skipped. + # name and fails unless every coverage shard and the coverage merge succeeded and + # the browser shards succeeded, or, on a pull request only, were skipped (#420). + # Any failure, cancellation or other skip fails it. viewerjs: needs: [frontend-coverage, frontend-coverage-merge, browser-workflows] if: always() @@ -229,11 +232,15 @@ jobs: steps: - name: Require every frontend job to succeed env: + EVENT_NAME: ${{ github.event_name }} COVERAGE_RESULT: ${{ needs.frontend-coverage.result }} MERGE_RESULT: ${{ needs.frontend-coverage-merge.result }} WORKFLOWS_RESULT: ${{ needs.browser-workflows.result }} run: | + queue_only=success + if [ "$EVENT_NAME" = pull_request ]; then queue_only=skipped; fi + echo "event: $EVENT_NAME (browser-workflows must be $queue_only)" echo "frontend-coverage: $COVERAGE_RESULT" echo "frontend-coverage-merge: $MERGE_RESULT" echo "browser-workflows: $WORKFLOWS_RESULT" - test "$COVERAGE_RESULT" = success && test "$MERGE_RESULT" = success && test "$WORKFLOWS_RESULT" = success + test "$COVERAGE_RESULT" = success && test "$MERGE_RESULT" = success && test "$WORKFLOWS_RESULT" = "$queue_only" diff --git a/TESTING.md b/TESTING.md index 67df06a6..91d31c3c 100644 --- a/TESTING.md +++ b/TESTING.md @@ -181,10 +181,12 @@ check, so run it before pushing a new or deleted frontend file instead of waiting for the `frontend coverage` jobs. The `viewerjs` check runs on every PR, including backend-only changes. It is an -aggregate job that passes only when every frontend job succeeds: the three -`frontend coverage (shard N/3)` jobs, `frontend coverage (merge)`, and the four -`page workflows (firefox|webkit, shard N/2)` jobs. A failed, cancelled or skipped -job fails it. Every browser job runs with `PM_TEST_WORKERS: 3` (the public +aggregate job. It requires the three `frontend coverage (shard N/3)` jobs and +`frontend coverage (merge)` to succeed on every event. The four +`page workflows (firefox|webkit, shard N/2)` jobs run only in the merge queue, +on `main` and on `workflow_dispatch` (see "Pull requests and the merge queue"). +`viewerjs` requires them to be skipped on a pull request and to succeed +everywhere else. Any other failed, cancelled or skipped job fails it. Every browser job runs with `PM_TEST_WORKERS: 3` (the public runners have 4 vCPUs). Each coverage shard runs `test:coverage:shard -- N/3` (shard 1 also runs the node units and the viewer suite, shard 3 the vendor pins and tooling tests) and uploads its raw V8 files as `coverage-raw-N` (kept 3 @@ -519,28 +521,60 @@ Before changing paths, SQLite, streams, restore or cleanup: open with `FirstWriterHold`) and give peers that must all succeed the ordinary busy timeout; cross-thread handoffs wait on events with generous ceilings, never a short wall clock (#242). -- Run native Windows CI early for these changes and include a Windows impact and - evidence note in the PR. All twelve OS/Python combinations and every applicable +- Run native Windows CI early for these changes: pull requests run only Linux, so + start the full matrix on the branch with `gh workflow run test.yml --ref `. + Include a Windows impact and evidence note in the PR. All nine OS/Python + combinations (they run again in the merge queue) and every applicable exact-head check must pass before merge. Portable `ntpath`/`PureWindowsPath` controls do not establish native filesystem, locking or cleanup success. +## Pull requests and the merge queue + +Each PR runs the full matrix once, in the merge queue (#420). The event decides +what runs: + +| Job | `pull_request` | `merge_group`, push to `main`, `workflow_dispatch` | +| --- | --- | --- | +| `py3.12 on ubuntu-latest` | runs | runs | +| other Python/OS legs (3.12-3.14 x Linux/Windows/macOS) | not created | run | +| `real ComfyUI compatibility` | skipped | runs | +| `coverage gate (85%)`, `black and flake8`, `frontend modules`, `bandit (high severity)` | run | run | +| `frontend coverage (shard N/3)`, `frontend coverage (merge)` (vendor pins and tooling tests on shard 3) | run | run | +| `page workflows (firefox\|webkit, shard N/2)` | skipped | run | +| `all tests`, `viewerjs` (gates) | run | run | +| `perf budget` | skipped | push and dispatch only (not in the queue) | + +GitHub has one required set for pull requests and the queue. A job skipped by +its `if:` reports Success. A dropped matrix leg never reports, so it would stay +pending. That is why `all tests` (in `test.yml`) and `viewerjs` carry the +guarantee: +- On a pull request they require the subset to succeed and the queue-only jobs + to be skipped. +- On every other event they require every job to succeed. + +So a merge group cannot turn green unless all nine Python/OS legs, real ComfyUI +and the four browser shards passed. `tests/vendor/ci-workflow.test.mjs` pins the +event split and runs both gate scripts for each event. + ## Required merge checks -`.github/branch-protection.json` records the required checks: all twelve Python/OS -combinations, Python coverage, formatting, frontend modules, `viewerjs`, `real ComfyUI compatibility`, -and `bandit (high severity)`. -The branch must be current with main, and administrators follow the same checks. +`.github/branch-protection.json` records the required checks: +`py3.12 on ubuntu-latest`, `coverage gate (85%)`, `black and flake8`, +`frontend modules`, `all tests`, `viewerjs`, `real ComfyUI compatibility` and +`bandit (high severity)`. Each one reports on a pull request. +The live set is in the `main` ruleset (25960526), together with the merge queue. It does not require an additional human approval. A repository administrator applies the configuration after the new workflow is passing, then verifies it: ```sh -gh api --method PUT repos/ComfyAssets/ComfyUI_PromptManager/branches/main/protection \ - --input .github/branch-protection.json -gh api repos/ComfyAssets/ComfyUI_PromptManager/branches/main/protection/required_status_checks +gh api repos/ComfyAssets/ComfyUI_PromptManager/rulesets/25960526 \ + | jq '.rules[] | select(.type == "required_status_checks") | [.parameters.required_status_checks[].context]' ``` +To change the set, PUT the ruleset's `rules` back with the edited +`required_status_checks` (`gh api --method PUT .../rulesets/25960526 --input `). Update the manifest whenever required job names change. Keep the frontend check free of path filters so a required check is never silently skipped. diff --git a/tests/vendor/ci-workflow.test.mjs b/tests/vendor/ci-workflow.test.mjs index cfafcaae..e9dde1a4 100644 --- a/tests/vendor/ci-workflow.test.mjs +++ b/tests/vendor/ci-workflow.test.mjs @@ -67,6 +67,45 @@ const browserJobIds = [...allJobs].filter(([, body]) => /npm run --prefix tools\ const mergeJob = () => allJobs.get('frontend-coverage-merge'); const verifyStep = () => steps(mergeJob()).find(step => /name: Verify every shard's raw coverage arrived/.test(step)); +// Jobs that run only in the merge queue, on main and on demand (#420): skipped on pull_request. +const QUEUE_ONLY_IF = /^ {4}if: github\.event_name != 'pull_request'$/m; +const EVENTS = ['pull_request', 'merge_group', 'push', 'workflow_dispatch']; + +function gateVariables(jobText) { + return new Map([...jobText.matchAll(/^ {10}([A-Z_]+): \$\{\{ needs\.([\w-]+)\.result \}\}$/gm)] + .map(match => [match[2], match[1]])); +} + +function runGate(jobText, env) { + const script = runScript(steps(jobText).find(step => /run: \|/.test(step))); + try { + execFileSync('bash', ['--noprofile', '--norc', '-eo', 'pipefail', '-c', script], { env: { PATH: process.env.PATH, ...env }, stdio: 'pipe' }); + return true; + } catch { + return false; + } +} + +// A gate passes only when every always-on job succeeded and every queue-only job is skipped on a +// pull request and succeeded on every other event. Anything else fails it. +function assertGateBehaviour(jobText, queueOnly) { + assert.match(jobText, /^ {10}EVENT_NAME: \$\{\{ github\.event_name \}\}$/m, 'the gate knows the event'); + const variables = gateVariables(jobText); + for (const id of queueOnly) assert.ok(variables.has(id), `${id} reaches the gate`); + for (const event of EVENTS) { + const expected = id => (queueOnly.includes(id) && event === 'pull_request' ? 'skipped' : 'success'); + const green = Object.fromEntries([...variables].map(([id, name]) => [name, expected(id)])); + assert.equal(runGate(jobText, { ...green, EVENT_NAME: event }), true, `${event}: the expected results pass`); + for (const [id, name] of variables) { + for (const result of ['success', 'failure', 'cancelled', 'skipped']) { + if (result === expected(id)) continue; + assert.equal(runGate(jobText, { ...green, EVENT_NAME: event, [name]: result }), false, + `${event}: ${id}=${result} must fail the gate`); + } + } + } +} + test('viewerjs keeps its name and needs every frontend job', () => { const viewerjs = allJobs.get('viewerjs'); assert.ok(viewerjs, 'the required context is the job id viewerjs'); @@ -76,32 +115,17 @@ test('viewerjs keeps its name and needs every frontend job', () => { assert.deepEqual([...needs].sort(), [...others].sort(), 'viewerjs needs every other job in the workflow'); assert.deepEqual([...needs].sort(), ['browser-workflows', 'frontend-coverage', 'frontend-coverage-merge']); assert.match(viewerjs, /^ {4}if: always\(\)$/m, 'viewerjs runs (and fails) when a needed job failed or was skipped'); - const script = runScript(steps(viewerjs).find(step => /run: \|/.test(step))); - for (const id of needs) { - const variable = new RegExp(`^ {10}([A-Z_]+): \\$\\{\\{ needs\\.${id}\\.result \\}\\}$`, 'm').exec(viewerjs); - assert.ok(variable, `${id}'s result reaches the check`); - assert.match(script, new RegExp(`test "\\$${variable[1]}" = success`), `${id} must be exactly success`); - } + assert.deepEqual([...gateVariables(viewerjs).keys()].sort(), [...needs].sort(), 'every needed result reaches the check'); }); -test('viewerjs fails unless every needed job succeeded', () => { - const viewerjs = allJobs.get('viewerjs'); - const script = runScript(steps(viewerjs).find(step => /run: \|/.test(step))); - const variables = [...viewerjs.matchAll(/^ {10}([A-Z_]+): \$\{\{ needs\.[\w-]+\.result \}\}$/gm)].map(match => match[1]); - const run = env => { - try { - execFileSync('bash', ['--noprofile', '--norc', '-eo', 'pipefail', '-c', script], { env: { PATH: process.env.PATH, ...env }, stdio: 'pipe' }); - return true; - } catch { - return false; - } - }; - const allSuccess = Object.fromEntries(variables.map(name => [name, 'success'])); - assert.equal(run(allSuccess), true); - for (const name of variables) { - for (const result of ['failure', 'cancelled', 'skipped']) { - assert.equal(run({ ...allSuccess, [name]: result }), false, `${name}=${result} must fail viewerjs`); - } +test('viewerjs requires the browser shards everywhere but on a pull request', () => { + assertGateBehaviour(allJobs.get('viewerjs'), ['browser-workflows']); +}); + +test('Firefox/WebKit page workflows run only outside pull requests; coverage runs on every event', () => { + assert.match(allJobs.get('browser-workflows'), QUEUE_ONLY_IF); + for (const id of ['frontend-coverage', 'frontend-coverage-merge']) { + assert.doesNotMatch(allJobs.get(id), /^ {4}if:/m, `${id} runs on every event`); } }); @@ -276,3 +300,60 @@ test('the bandit pre-commit hook fails at the severity CI enforces and passes ev assert.doesNotMatch(hook, /true|exit-zero|bash -c/, 'nothing masks the exit code or drops a file'); assert.doesNotMatch(hook, /pass_filenames: false/, 'the hook scans the staged files'); }); + +// Event split (#420): pull requests run a fast subset, the merge queue and main run everything. +const testJobs = () => jobs(readWorkflow('test.yml')); + +test('the Python matrix is one leg on a pull request and the full matrix on every other event', () => { + const body = testJobs().get('test'); + const lists = key => { + const match = new RegExp(`^ {8}${key}: \\$\\{\\{ github\\.event_name == 'pull_request' && fromJSON\\('(\\[[^\\]]*\\])'\\) \\|\\| fromJSON\\('(\\[[^\\]]*\\])'\\) \\}\\}$`, 'm').exec(body); + assert.ok(match, `${key} is chosen by event`); + return { pr: JSON.parse(match[1]), other: JSON.parse(match[2]) }; + }; + assert.deepEqual(lists('os'), { pr: ['ubuntu-latest'], other: ['ubuntu-latest', 'windows-latest', 'macos-latest'] }); + assert.deepEqual(lists('python-version'), { pr: ['3.12'], other: ['3.12', '3.13', '3.14'] }); + assert.match(body, /^ {6}fail-fast: false$/m); + assert.match(body, /^ {4}name: py\$\{\{ matrix\.python-version \}\} on \$\{\{ matrix\.os \}\}$/m); +}); + +test('real ComfyUI runs only outside pull requests; the other test.yml checks run on every event', () => { + const found = testJobs(); + assert.match(found.get('comfyui'), QUEUE_ONLY_IF); + for (const id of ['test', 'coverage', 'lint', 'js-test']) { + assert.doesNotMatch(found.get(id), /^ {4}if:/m, `${id} runs on every event`); + } +}); + +test('all tests needs every gating test.yml job and requires the full set outside pull requests', () => { + const found = testJobs(); + const gate = found.get('all-tests'); + assert.ok(gate, 'the aggregate gate exists'); + assert.match(gate, /^ {4}name: all tests$/m, 'the required context is "all tests"'); + assert.match(gate, /^ {4}if: always\(\)$/m); + const gating = [...found.keys()].filter(id => !['all-tests', 'perf-budget'].includes(id)); + assert.deepEqual([...needsOf(gate)].sort(), gating.sort(), 'the gate needs every job but the non-gating perf budget'); + assert.deepEqual([...gateVariables(gate).keys()].sort(), gating.sort(), 'every needed result reaches the gate'); + assertGateBehaviour(gate, ['comfyui']); +}); + +test('the CI workflows run on pull_request, merge_group and push without path filters', () => { + for (const name of BOUNDED_WORKFLOWS) { + const text = readWorkflow(name); + const triggers = text.slice(text.indexOf('\non:'), text.indexOf('\n', text.indexOf('\non:') + 1) + 400); + for (const event of ['merge_group', 'pull_request', 'push']) { + assert.match(triggers, new RegExp(`^ {2}${event}:`, 'm'), `${name} runs on ${event}`); + } + assert.doesNotMatch(text, /^ {4}paths(-ignore)?:/m, `${name} has no path filter that would leave a required check pending`); + } +}); + +test('every required check reports on a pull request and the gates carry the full matrix', () => { + const contexts = JSON.parse(readRepoFile('.github/branch-protection.json')).required_status_checks.contexts; + assert.deepEqual([...contexts].sort(), [ + 'all tests', 'bandit (high severity)', 'black and flake8', 'coverage gate (85%)', 'frontend modules', + 'py3.12 on ubuntu-latest', 'real ComfyUI compatibility', 'viewerjs', + ].sort()); + assert.equal(contexts.filter(context => /^py/.test(context)).length, 1, + 'a dropped matrix leg never reports on a pull request, so it cannot be required'); +});