diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b5798f46..d70e2fde 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,20 +52,31 @@ permissions: contents: read jobs: - # Does this pull request contain anything an acceptance suite could break? + # Does this pull request contain anything any suite in here could break? # # THE EXPENSIVE HALF OF THIS WORKFLOW RAN ON MARKDOWN. Three documentation-only # PRs on 2026-08-14 each fired all 28 checks -- the full acceptance matrix, # three-OS builds, Docker and browser suites -- to validate files no job reads. # One of them was a single file. See #350. # - # WHY THIS GATES STEPS AND NOT JOBS. The acceptance suites are REQUIRED status - # checks -- twelve of them when this was written, thirteen since - # acceptance-ladder joined the matrix, and a leg's context has to be added to - # the branch-protection ruleset by hand or it reports without gating anything. - # A workflow-level `paths:` filter stops the workflow - # running at all, so a required check never reports and the pull request stays - # pending forever with no way to merge it. + # WHAT THIS GATES, and #378 is the second half of it. #351/#357 stopped the + # acceptance matrix; the three-OS test matrix, the go job and the cross-compile + # kept running, which is most of the twenty minutes #350 was actually + # complaining about. All five now read this output: + # + # go go build, vet, test + # crossplatform test: ubuntu-latest / macos-latest / windows-latest + # ui ui typecheck, lint, build + # cross cross-compile all release targets + # acceptance acceptance: + # + # Every name in the right-hand column is a required status context in the + # branch-protection ruleset. That is the whole reason for the shape below. + # + # WHY THIS GATES STEPS AND NOT JOBS. A leg's context has to be added to the + # ruleset by hand or it reports without gating anything. A workflow-level + # `paths:` filter stops the workflow running at all, so a required check never + # reports and the pull request stays pending forever with no way to merge it. # # A JOB-LEVEL `if:` ON A MATRIX IS THE SAME BUG WEARING A DISGUISE, and #351 # shipped it. A skipped ordinary job does report, and a skip does satisfy the @@ -80,6 +91,18 @@ jobs: # the WORK: each step carries the `if:`, the job costs a runner allocation and # nothing else, and the required context reports success either way. # + # ONE SHAPE FOR ALL FIVE, INCLUDING THE THREE THAT HAVE NO MATRIX. `go`, `ui` + # and `cross` are ordinary jobs today, and a job-level `if:` on them would be + # correct today: a skipped ordinary job reports skipped and satisfies its + # requirement. It is not written that way, because the difference between the + # safe spelling and the outage is one `strategy:` block that nobody would think + # to connect to branch protection while adding it -- a Go-version matrix on + # `go`, a Node-version matrix on `ui`. The rule is therefore flat, has no + # exceptions to remember, and is enforced rather than remembered: + # internal/testenv/docsgate_test.go fails if this output is ever read from a + # job-level `if:`. The price is four runner allocations that do nothing on a + # documentation-only PR, against twenty minutes of compute they replace. + # # IT FAILS TOWARD RUNNING. `code` is false only when EVERY changed path is # documentation; anything unrecognised makes it true. A new top-level directory # gets the full matrix until somebody decides otherwise, which is the right @@ -123,6 +146,10 @@ jobs: go: name: go build, vet, test + # The documentation gate, #378. On every STEP below, never on this job -- + # see the `changes` job for why that distinction is load-bearing and for + # what it cost the one time it was got wrong. + needs: changes runs-on: ubuntu-latest # Above the 15m per-package go test timeout below, so Go's diagnostic # panic wins the race against this. @@ -143,8 +170,16 @@ jobs: # change once someone has watched the three images pass with it. POLYEMESIS_REQUIRE_FFMPEG: "1" steps: + # Says out loud why a green check did no work, so a reader of the + # checks list is never left guessing whether this ran or no-opped. + - name: Documentation-only change, so this job did no work + if: needs.changes.outputs.code != 'true' + run: | + echo "::notice title=go build, vet, test::this check did no work -- every changed path was documentation. See the 'which changes' job." - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: needs.changes.outputs.code == 'true' - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + if: needs.changes.outputs.code == 'true' with: go-version-file: go.mod cache: true @@ -176,12 +211,14 @@ jobs: # # Bump the -v1 suffix to take a newer build on purpose. - name: Cache FFmpeg + if: needs.changes.outputs.code == 'true' uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: /tmp/ffcache key: ffmpeg-${{ runner.os }}-n8.1-latest-linux64-gpl-8.1-v1 - name: Install FFmpeg + if: needs.changes.outputs.code == 'true' timeout-minutes: 6 env: FFCACHE: /tmp/ffcache @@ -271,6 +308,7 @@ jobs: # now fails the step, and the emptiness of its output is a separate # question asked afterwards. - name: gofmt + if: needs.changes.outputs.code == 'true' run: | set -euo pipefail unformatted=$(gofmt -l ./cmd ./internal) @@ -280,8 +318,10 @@ jobs: exit 1 fi echo "gofmt: clean" - - run: go build ./... - - run: go vet ./... + - if: needs.changes.outputs.code == 'true' + run: go build ./... + - if: needs.changes.outputs.code == 'true' + run: go vet ./... # THE PREFLIGHT'S OWN LIVENESS, and it runs HERE because it was not # running anywhere. @@ -299,6 +339,7 @@ jobs: # about. ~4s; it runs the package three times with a filter that selects # almost nothing. - name: The route-coverage preflight is still wired + if: needs.changes.outputs.code == 'true' run: make preflight-guard # THE OTHER HALF OF THE SAME MECHANISM. #217/#223. @@ -334,6 +375,7 @@ jobs: # before this fires. Below the job's 25 either way, which is the property # being claimed. - name: internal/api coverage measures the tests, not the preflight + if: needs.changes.outputs.code == 'true' timeout-minutes: 14 run: make coverage-instrument-guard @@ -355,7 +397,8 @@ jobs: # filter matching one of the two tests and not the other would leave every # counterpart undischarged while still printing ok. The full suite runs # here with no filter, so strict mode costs nothing and closes that door. - - run: POLYEMESIS_LEDGER=strict go test -race -timeout 15m ./... + - if: needs.changes.outputs.code == 'true' + run: POLYEMESIS_LEDGER=strict go test -race -timeout 15m ./... # The acceptance suites' shared diagnostic helpers, tested here rather # than in their own job: it is pure shell, needs nothing installed and @@ -382,7 +425,8 @@ jobs: # belonging to whatever else is running on it. So these are blast-radius # ceilings, not fitted bounds: sized to be unreachable by a healthy run and # far below the job's 25, which is the only property being claimed. - - run: ./scripts/test-lib-observe.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/test-lib-observe.sh timeout-minutes: 6 # The acceptance suites' own deadline, which fires below this workflow's @@ -390,7 +434,8 @@ jobs: # Tested here for the reason above and one that is specific to it: the # watchdog is a background process holding the suite's stdout, so a bug # in it does not merely fail to report -- it becomes the hang. - - run: ./scripts/test-lib-watchdog.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/test-lib-watchdog.sh timeout-minutes: 6 # The SBOM guard, which only ever runs for real inside a release. That is @@ -399,7 +444,8 @@ jobs: # comment described, and no one could discover that without cutting a # release. Running its tests on every PR is the part that makes the fix # a fix rather than a better guess. Pure jq and shell, about a second. - - run: ./scripts/test-sbom-guard.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/test-sbom-guard.sh timeout-minutes: 5 # The termination guard, and its tests, alongside the SBOM pair above for @@ -414,9 +460,11 @@ jobs: # fixtures. The guard without its fixtures is a check nobody has watched # fail; the fixtures without the guard oblige nothing. Pure shell, under a # second each. - - run: ./scripts/termination-guard.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/termination-guard.sh timeout-minutes: 5 - - run: ./scripts/test-termination-guard.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/test-termination-guard.sh timeout-minutes: 5 # The OBS container entrypoint's stop path. #208 was FILED rather than @@ -425,7 +473,8 @@ jobs: # Both ways are about a process, not about OBS, so the stop logic moved # into scripts/obs/lib-stop.sh and this drives it with `sleep` and a # SIGTERM-deaf stand-in. No OBS, no Xvfb, no container, about six seconds. - - run: ./scripts/test-obs-stop.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/test-obs-stop.sh timeout-minutes: 6 # Outbound webhooks over a real socket. IN THIS JOB RATHER THAN THE @@ -442,7 +491,8 @@ jobs: # nothing to be flaky about. Its credentialed step skips here; no # POLY_HOOKS_URL is configured for this workflow and none is needed -- # 29 of its 31 checks run without one. - - run: ./scripts/acceptance-hooks.sh + - if: needs.changes.outputs.code == 'true' + run: ./scripts/acceptance-hooks.sh timeout-minutes: 6 # The acceptance suites' shared teardown. Same argument as the guard @@ -452,6 +502,7 @@ jobs: # harness, because a real regression gets dismissed as "that suite is # flaky". Needs lsof, which this job does not otherwise install. - name: ./scripts/test-lib-cleanup.sh + if: needs.changes.outputs.code == 'true' timeout-minutes: 10 run: | sudo apt-get install -y --no-install-recommends lsof @@ -474,6 +525,10 @@ jobs: # platforms. crossplatform: name: "test: ${{ matrix.os }}" + # The documentation gate, #378. On every STEP below, never on this job -- + # see the `changes` job for why that distinction is load-bearing and for + # what it cost the one time it was got wrong. + needs: changes runs-on: ${{ matrix.os }} # Above the 15m per-package go test timeout below. Windows needs the room: # internal/db alone has been measured at 265-300s and once past 600s. @@ -485,8 +540,16 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] steps: + # Says out loud why a green check did no work, so a reader of the + # checks list is never left guessing whether this ran or no-opped. + - name: Documentation-only change, so this job did no work + if: needs.changes.outputs.code != 'true' + run: | + echo "::notice title=test: ${{ matrix.os }}::this check did no work -- every changed path was documentation. See the 'which changes' job." - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: needs.changes.outputs.code == 'true' - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + if: needs.changes.outputs.code == 'true' with: go-version-file: go.mod cache: true @@ -515,11 +578,11 @@ jobs: # Linux only: macOS installs from brew and Windows caches its own # zip below. Without this the step runs on all three and stores an # empty directory under two keys that nothing ever reads. - if: runner.os == 'Linux' + if: runner.os == 'Linux' && needs.changes.outputs.code == 'true' - name: Install FFmpeg (Linux) timeout-minutes: 6 - if: runner.os == 'Linux' + if: runner.os == 'Linux' && needs.changes.outputs.code == 'true' env: FFCACHE: /tmp/ffcache run: | @@ -588,7 +651,7 @@ jobs: ffmpeg -hide_banner -protocols | tr ' ' '\n' | grep -qx srt ffmpeg -hide_banner -version | head -1 - name: Install FFmpeg (macOS) - if: runner.os == 'macOS' + if: runner.os == 'macOS' && needs.changes.outputs.code == 'true' # BREW CANNOT SUPPLY 8.1, AND THAT IS RECORDED RATHER THAN HIDDEN. # `ffmpeg` is 9.0 and there is no `ffmpeg@8` formula -- the versioned ones # jump from 7.1 straight to 9.0 -- and BtbN publishes no macOS build. So @@ -606,13 +669,13 @@ jobs: # is gated to runner.os == 'Linux'. - name: Cache FFmpeg (Windows) uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - if: runner.os == 'Windows' + if: runner.os == 'Windows' && needs.changes.outputs.code == 'true' with: path: ${{ runner.temp }}/ffcache key: ffmpeg-${{ runner.os }}-n8.1-latest-win64-gpl-8.1-v1 - name: Install FFmpeg (Windows) - if: runner.os == 'Windows' + if: runner.os == 'Windows' && needs.changes.outputs.code == 'true' # THE SAME BUILD THE LINUX ARM INSTALLS, not choco, for two reasons and # the second is the important one. # @@ -670,12 +733,16 @@ jobs: # ffmpeg's -- the same discarded-exit-status shape as the gofmt pipeline in # the go job. A step boundary checks each one. - name: FFmpeg version + if: needs.changes.outputs.code == 'true' run: ffmpeg -hide_banner -version - name: ffprobe version + if: needs.changes.outputs.code == 'true' run: ffprobe -hide_banner -version - - run: go build ./... - - run: go vet ./... + - if: needs.changes.outputs.code == 'true' + run: go build ./... + - if: needs.changes.outputs.code == 'true' + run: go vet ./... # Go's default timeout is 10 minutes PER PACKAGE, and internal/db lands # close enough to it on Windows that ordinary runner variance decides the # build. Measured on ONE unchanged tree: 264s, 297s, and once past 600s, @@ -712,7 +779,8 @@ jobs: # a goroutine dump naming the running test; the job timeout just kills the # runner and tells you nothing. Whichever fires first decides how much you # learn, so Go's has to. - - run: go test -timeout 15m ./... + - if: needs.changes.outputs.code == 'true' + run: go test -timeout 15m ./... # Build and RUN it. Compiling proves the code is valid for the platform; # it does not prove the process comes up. @@ -738,6 +806,7 @@ jobs: # -cover, so the smoke run MEASURES itself. See the covdata step at the # end of this job for what that is for and what it costs. - name: Build the smoke binary + if: needs.changes.outputs.code == 'true' timeout-minutes: 8 shell: bash run: go build -cover -o polyemesis-smoke ./cmd/polyemesis @@ -749,6 +818,7 @@ jobs: # :154-158 and :305-311 -- whichever timeout fires first decides how much # you learn, so the narrowest one has to. - name: Start the server and check it serves + if: needs.changes.outputs.code == 'true' timeout-minutes: 2 shell: bash run: | @@ -811,6 +881,7 @@ jobs: # it is below the job's 30 on purpose -- a step timeout names the step # that hung, a job timeout names nothing. - name: Push a broadcast through it and measure the output + if: needs.changes.outputs.code == 'true' timeout-minutes: 8 shell: bash run: | @@ -900,7 +971,7 @@ jobs: # touches little of internal/*, so that number would be low, meaningless, # and immediately mistaken for the project's coverage. - name: Report what the smoke run executed in cmd/polyemesis - if: always() + if: always() && needs.changes.outputs.code == 'true' shell: bash run: | set -uo pipefail @@ -927,7 +998,7 @@ jobs: echo "(no cmd/polyemesis rows in the profile)" - name: Upload broadcast artefacts on failure - if: failure() + if: failure() && needs.changes.outputs.code == 'true' uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: broadcast-${{ matrix.os }} @@ -939,52 +1010,86 @@ jobs: ui: name: ui typecheck, lint, build + # The documentation gate, #378. On every STEP below, never on this job -- + # see the `changes` job for why that distinction is load-bearing and for + # what it cost the one time it was got wrong. + needs: changes runs-on: ubuntu-latest timeout-minutes: 15 defaults: run: working-directory: ui steps: + # Says out loud why a green check did no work, so a reader of the + # checks list is never left guessing whether this ran or no-opped. + - name: Documentation-only change, so this job did no work + if: needs.changes.outputs.code != 'true' + working-directory: ${{ github.workspace }} + run: | + echo "::notice title=ui typecheck, lint, build::this check did no work -- every changed path was documentation. See the 'which changes' job." - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: needs.changes.outputs.code == 'true' - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + if: needs.changes.outputs.code == 'true' with: # Keep in step with the Dockerfiles' node tag and @types/node major. node-version: 24 cache: npm cache-dependency-path: ui/package-lock.json - - run: npm ci --ignore-scripts - - run: npx --no-install tsc -b --noEmit - - run: npm run lint - - run: npm run build + - if: needs.changes.outputs.code == 'true' + run: npm ci --ignore-scripts + - if: needs.changes.outputs.code == 'true' + run: npx --no-install tsc -b --noEmit + - if: needs.changes.outputs.code == 'true' + run: npm run lint + - if: needs.changes.outputs.code == 'true' + run: npm run build # Unit tests for the pure logic the browser suite cannot enumerate -- # platform link construction has five platforms times several missing-field # cases, and driving each through a real browser would cost minutes to # assert what a millisecond of vitest does. - - run: npm test + - if: needs.changes.outputs.code == 'true' + run: npm test # A vulnerable direct dependency should fail the build, not sit in a # report nobody opens. --audit-level=high so a low-severity transitive # advisory does not block a hotfix. - - run: npm audit --audit-level=high + - if: needs.changes.outputs.code == 'true' + run: npm audit --audit-level=high cross: name: cross-compile all release targets + # The documentation gate, #378. On every STEP below, never on this job -- + # see the `changes` job for why that distinction is load-bearing and for + # what it cost the one time it was got wrong. + needs: changes runs-on: ubuntu-latest timeout-minutes: 25 steps: + # Says out loud why a green check did no work, so a reader of the + # checks list is never left guessing whether this ran or no-opped. + - name: Documentation-only change, so this job did no work + if: needs.changes.outputs.code != 'true' + run: | + echo "::notice title=cross-compile all release targets::this check did no work -- every changed path was documentation. See the 'which changes' job." - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: needs.changes.outputs.code == 'true' - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + if: needs.changes.outputs.code == 'true' with: go-version-file: go.mod cache: true - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + if: needs.changes.outputs.code == 'true' with: node-version: 24 cache: npm cache-dependency-path: ui/package-lock.json # `make release` depends on the ui target, which needs the built assets # for go:embed. Building them here keeps the check honest end to end. - - run: make release - - run: ls -lh dist/ + - if: needs.changes.outputs.code == 'true' + run: make release + - if: needs.changes.outputs.code == 'true' + run: ls -lh dist/ # Which container suites this run needs. # @@ -1401,8 +1506,14 @@ jobs: # # if: failure() rather than always(): on a green run there is no process # left to signal and nothing to explain. + # + # The gate is ANDed on for completeness rather than for effect -- #357 + # missed this one step, and it was harmless only because a documentation + # -only run has no suite step to fail, so `failure()` was never true. That + # is a coincidence of this job's shape, not a property, and the rule in + # internal/testenv/docsgate_test.go found it. - name: Diagnostics (what was it waiting for) - if: failure() + if: failure() && needs.changes.outputs.code == 'true' # Never fail the job from here. This step exists to describe a failure # that already happened; a diagnostic that turns a legible suite # failure into a confusing one about ps flags is worse than none. diff --git a/internal/db/facebook_ui_drift_test.go b/internal/db/facebook_ui_drift_test.go index 1190e26e..984f1678 100644 --- a/internal/db/facebook_ui_drift_test.go +++ b/internal/db/facebook_ui_drift_test.go @@ -2,10 +2,10 @@ package db import ( "encoding/json" - "os" - "path/filepath" "strings" "testing" + + "github.com/rainmanjam/polyemesis/internal/testenv" ) /* =========================================================================== @@ -40,59 +40,13 @@ import ( =========================================================================== */ -// stripJSComments blanks out comments so a marker left behind in one cannot -// satisfy a guard that is asking whether a control renders. That is not -// hypothetical: the honest way to keep a substring guard green while deleting -// the thing it watches is to leave the words in a comment, and this branch's -// own audit records that temptation being declined by hand rather than by a -// guard. -// -// Block comments (`/* */`, and the `{/* */}` JSX form, which is a block comment -// inside an expression container) are removed outright. Line comments are -// removed only when nothing before the `//` on that line is quoted, so -// a URL in a string literal or a template literal is left alone rather than -// truncated at the scheme separator. -// -// Newlines are preserved so a line number quoted in a failure still means -// something to whoever goes to look. -func stripJSComments(src string) string { - var b strings.Builder - b.Grow(len(src)) - for i := 0; i < len(src); { - if strings.HasPrefix(src[i:], "/*") { - end := strings.Index(src[i+2:], "*/") - if end < 0 { - break // unterminated; the rest is comment - } - for _, r := range src[i : i+2+end+2] { - if r == '\n' { - b.WriteByte('\n') - } - } - i += 2 + end + 2 - continue - } - if strings.HasPrefix(src[i:], "//") && !quotedBefore(src, i) { - end := strings.IndexByte(src[i:], '\n') - if end < 0 { - break - } - i += end // leave the newline for the next iteration - continue - } - b.WriteByte(src[i]) - i++ - } - return b.String() -} - -// quotedBefore reports whether a quote character appears between the start of -// the line containing i and i itself -- the cheap test for "this `//` is inside -// a string literal or JSX attribute rather than starting a comment". -func quotedBefore(src string, i int) bool { - start := strings.LastIndexByte(src[:i], '\n') + 1 - return strings.ContainsAny(src[start:i], "\"'`") -} +// READING THE SOURCE AND BLANKING ITS COMMENTS both live in internal/testenv +// now -- testenv.ReadUI and testenv.StripJSComments, #379. They were written +// here, and they were the only copy, which is why the guards in internal/oauth +// spent their whole life defeatable by a comment: the alternative to importing +// them was pasting forty lines into a second package. The reasoning that used to +// sit here in full is in internal/testenv/uisource.go, next to the code, so +// there is one place to read it and one place to change it. // jsxBlockUnder returns the source of the subtree a JSX conditional renders, // given the WHOLE head of that conditional including its opening paren -- e.g. @@ -113,7 +67,7 @@ func jsxBlockUnder(t *testing.T, src, head, file string) string { t.Fatalf("guard bug: %q is not a whole conditional head; it must end with the "+ "opening paren so the block can be bounded", head) } - stripped := stripJSComments(src) + stripped := testenv.StripJSComments(src) switch n := strings.Count(stripped, head); { case n == 0: t.Fatalf("%s no longer contains %s\n\n"+ @@ -145,17 +99,6 @@ func jsxBlockUnder(t *testing.T, src, head, file string) string { return "" } -// readUI reads a file under ui/src, from internal/db. -func readUI(t *testing.T, parts ...string) string { - t.Helper() - path := filepath.Join(append([]string{"..", "..", "ui", "src"}, parts...)...) - raw, err := os.ReadFile(path) - if err != nil { - t.Fatalf("cannot read %s: %v", path, err) - } - return string(raw) -} - // The head of the Facebook create-time settings block. Everything the crosspost // list, the donate field and the backup toggle need is inside it, so all three // guards below bound themselves the same way. @@ -176,7 +119,7 @@ const facebookBlockHead = `{platform === "facebook" && (` // searched the whole file for "Crosspost to Pages" and `id="dest-fb-donate"`, // both of which survive that mutation untouched, and stayed green. func TestFacebookCrosspostAndDonateAreOfferedByTheDestinationEditor(t *testing.T) { - src := readUI(t, "components", "DestinationDialog.tsx") + src := testenv.ReadUI(t, "components", "DestinationDialog.tsx") block := jsxBlockUnder(t, src, facebookBlockHead, "DestinationDialog.tsx") // The key, not the English. Where the WORDS live is a separate question, @@ -212,7 +155,7 @@ func TestFacebookCrosspostAndDonateAreOfferedByTheDestinationEditor(t *testing.T // // This one was already bounded to the right span and is left as it was. func TestDestinationDialogSavePayloadCarriesTheFacebookBlock(t *testing.T) { - src := readUI(t, "components", "DestinationDialog.tsx") + src := testenv.ReadUI(t, "components", "DestinationDialog.tsx") marker := "const payload: Partial = {" start := strings.Index(src, marker) @@ -246,7 +189,7 @@ func TestDestinationDialogSavePayloadCarriesTheFacebookBlock(t *testing.T) { // whole card for the href template and stayed green through it, because the // href is still written inside the block that no longer renders. func TestTheCardLinksToTheScheduledBroadcast(t *testing.T) { - src := readUI(t, "components", "DestinationCard.tsx") + src := testenv.ReadUI(t, "components", "DestinationCard.tsx") block := jsxBlockUnder(t, src, "{dest.facebookBroadcastId && (", "DestinationCard.tsx") if !strings.Contains(block, "facebook.com/${dest.facebookBroadcastId}") { @@ -285,7 +228,7 @@ func TestTheCardLinksToTheScheduledBroadcast(t *testing.T) { // regression -- a backup that died while the primary was offline is exactly the // case the operator needs to see. func TestTheCardShowsTheBackupFeedsState(t *testing.T) { - src := readUI(t, "components", "DestinationCard.tsx") + src := testenv.ReadUI(t, "components", "DestinationCard.tsx") state := jsxBlockUnder(t, src, "{dest.backupProcess && (", "DestinationCard.tsx") if !strings.Contains(state, "dest.backupProcess.state") { @@ -312,7 +255,7 @@ func TestTheCardShowsTheBackupFeedsState(t *testing.T) { // destination beside the endpoint it gates, and a guard still spelling the old // shape would be a guard requiring the defect. func TestTheDialogOffersTheBackupIngestToggle(t *testing.T) { - src := readUI(t, "components", "DestinationDialog.tsx") + src := testenv.ReadUI(t, "components", "DestinationDialog.tsx") block := jsxBlockUnder(t, src, facebookBlockHead, "DestinationDialog.tsx") if !strings.Contains(block, "setBackupIngestWanted(e.target.checked)") { @@ -362,7 +305,7 @@ func TestTheDialogOffersTheBackupIngestToggle(t *testing.T) { // twice the upload, and will find out during a broadcast. func TestTheFacebookCopyLivesInTheCatalogue(t *testing.T) { var en map[string]string - if err := json.Unmarshal([]byte(readUI(t, "lib", "i18n", "en.json")), &en); err != nil { + if err := json.Unmarshal([]byte(testenv.ReadUI(t, "lib", "i18n", "en.json")), &en); err != nil { t.Fatalf("en.json is not a flat string map: %v", err) } diff --git a/internal/db/ingest_header_drift_test.go b/internal/db/ingest_header_drift_test.go index e0aaf2ae..5e2a52d6 100644 --- a/internal/db/ingest_header_drift_test.go +++ b/internal/db/ingest_header_drift_test.go @@ -3,6 +3,8 @@ package db import ( "strings" "testing" + + "github.com/rainmanjam/polyemesis/internal/testenv" ) // WHY THIS IS IN internal/db, stated rather than left to be discovered. @@ -13,12 +15,16 @@ import ( // owned by internal/engine (reconcileIngest, which returns early for SRT) and // internal/stats (the received-byte counter the bitrate series is sampled from). // -// It lives here to reuse readUI and stripJSComments from facebook_ui_drift_test.go, -// which is helper locality rather than a reason, and the honest consequence is -// that someone changing engine's ingest reconciliation will not see a db test in -// their package. `go test ./...` still runs it, so the guard holds; only its -// discoverability is worse. Moving it to internal/engine means copying both -// helpers there, so the fix is to promote them to a shared test helper first. +// It landed here to reuse readUI and stripJSComments from +// facebook_ui_drift_test.go, which is helper locality rather than a reason, and +// the honest consequence is that someone changing engine's ingest reconciliation +// will not see a db test in their package. `go test ./...` still runs it, so the +// guard holds; only its discoverability is worse. What blocked the move was that +// internal/engine would need its own copy of both helpers -- and #379 has now +// removed that blocker: they are testenv.ReadUI and testenv.StripJSComments, and +// any package can import them. What remains is the move itself, which is left to +// a change of its own because it changes which package a failure points at, and +// that is a decision about who gets paged rather than a tidy-up. // // The header's ingest indicator must not decide health from the ingest PROCESS. // @@ -59,7 +65,7 @@ import ( // removing the thing it watches is to leave the words behind, so the words are // not what is read. func TestTheHeaderAsksTheAppsOneQuestionAboutBeingLive(t *testing.T) { - src := stripJSComments(readUI(t, "components", "AppLayout.tsx")) + src := testenv.StripJSComments(testenv.ReadUI(t, "components", "AppLayout.tsx")) if !strings.Contains(src, "useIngestLive()") { t.Error("AppLayout no longer calls useIngestLive. An SRT source has no ingest " + @@ -83,7 +89,7 @@ func TestTheHeaderAsksTheAppsOneQuestionAboutBeingLive(t *testing.T) { // at status.ingest.progress would silently restore the original bug in a place // nobody would think to look. func TestIngestLiveIsDerivedFromArrivingBytes(t *testing.T) { - src := stripJSComments(readUI(t, "hooks", "useLiveData.ts")) + src := testenv.StripJSComments(testenv.ReadUI(t, "hooks", "useLiveData.ts")) // Not `src[strings.Index(...):]` unguarded: a rename made that a slice-bounds // panic rather than a failure anyone could read, which is the guard going diff --git a/internal/oauth/capabilities_drift_test.go b/internal/oauth/capabilities_drift_test.go index f958e4d9..3de07545 100644 --- a/internal/oauth/capabilities_drift_test.go +++ b/internal/oauth/capabilities_drift_test.go @@ -1,11 +1,11 @@ package oauth import ( - "os" - "path/filepath" "regexp" "strings" "testing" + + "github.com/rainmanjam/polyemesis/internal/testenv" ) // The capability matrix exists twice, and nothing was checking the copies agree. @@ -26,13 +26,15 @@ import ( // and each capability's support value -- and not the prose, because the reason // strings are written for two different audiences and forcing them identical // would make the guard fight the thing it protects. +// COMMENTS ARE BLANKED FIRST, #379. This guard reads the TypeScript as text, so +// until now a row deleted from the matrix and left behind as a comment satisfied +// it exactly as well as a row that renders -- and a commented-out platform is +// the single most likely way a row leaves this file. The stripper existed in +// internal/db and this package had no way to reach it; it is testenv. +// StripJSComments now. func TestTheUICapabilityMatrixAgreesWithGo(t *testing.T) { - path := filepath.Join("..", "..", "ui", "src", "lib", "capabilities.ts") - raw, err := os.ReadFile(path) - if err != nil { - t.Fatalf("cannot read %s: %v", path, err) - } - ui := parseUICapabilities(t, string(raw)) + ui := parseUICapabilities(t, testenv.StripJSComments( + testenv.ReadUI(t, "lib", "capabilities.ts"))) for _, row := range platformCapabilities { got, ok := ui[row.PresetID] @@ -100,6 +102,11 @@ var ( // which would make this guard skip itself on any machine where that is missing // -- and a guard that skips silently is worse than no guard, because the next // person reads green and believes it. +// +// src is expected to have been through testenv.StripJSComments. That is not +// merely defensive: every regex below would otherwise read a commented-out row +// as a live one, which turns "the UI still ships this platform" into "somebody +// once typed this platform". func parseUICapabilities(t *testing.T, src string) map[string]uiRow { t.Helper() diff --git a/internal/oauth/composer_tags_drift_test.go b/internal/oauth/composer_tags_drift_test.go index 8a6418f5..1cfb6e8b 100644 --- a/internal/oauth/composer_tags_drift_test.go +++ b/internal/oauth/composer_tags_drift_test.go @@ -1,12 +1,25 @@ package oauth import ( - "os" - "path/filepath" "strings" "testing" + + "github.com/rainmanjam/polyemesis/internal/testenv" ) +// dashboardSource is Dashboard.tsx with its comments blanked, #379. +// +// Both guards below are substring searches over a window of this file, and a +// substring search is satisfied by a comment. Neither of them stripped, so +// either could have been kept green by deleting the code and leaving the words +// -- and for the compliance guard, whose window is the whole file, a comment +// anywhere at all would have done it. Blanking happens here, once, so a third +// guard added to this file cannot forget. +func dashboardSource(t *testing.T) string { + t.Helper() + return testenv.StripJSComments(testenv.ReadUI(t, "pages", "Dashboard.tsx")) +} + // The composer must be able to SEND tags, not merely render them back. // // TestUITypesCanNameEveryMetadataField walks the field NAMES a push result can @@ -19,12 +32,7 @@ import ( // Dashboard.tsx for unrelated reasons, so a whole-file search would pass on a // composer that still cannot send them. func TestTheComposerCanSendFacebookTags(t *testing.T) { - path := filepath.Join("..", "..", "ui", "src", "pages", "Dashboard.tsx") - raw, err := os.ReadFile(path) - if err != nil { - t.Fatalf("cannot read %s: %v", path, err) - } - src := string(raw) + src := dashboardSource(t) body := strings.Index(src, `metaFetch("/metadata/push"`) if body < 0 { t.Fatal("cannot find the metadata push call in Dashboard.tsx; this guard " + @@ -54,12 +62,7 @@ func TestTheComposerCanSendFacebookTags(t *testing.T) { // Matches the derived list's use rather than its definition, because the name // appears at both and only the uses do anything. func TestTheComposerSaysWhenAPushCarriesStoredCompliance(t *testing.T) { - path := filepath.Join("..", "..", "ui", "src", "pages", "Dashboard.tsx") - raw, err := os.ReadFile(path) - if err != nil { - t.Fatalf("cannot read %s: %v", path, err) - } - src := string(raw) + src := dashboardSource(t) if !strings.Contains(src, "withCompliance.length > 0") { t.Error("the composer never mentions stored compliance, so a push sends a COPPA " + "declaration or a privacy setting with nothing on screen having said so") diff --git a/internal/oauth/ui_drift_test.go b/internal/oauth/ui_drift_test.go index 826b93db..b6cd78b7 100644 --- a/internal/oauth/ui_drift_test.go +++ b/internal/oauth/ui_drift_test.go @@ -1,11 +1,11 @@ package oauth import ( - "os" - "path/filepath" "regexp" "strings" "testing" + + "github.com/rainmanjam/polyemesis/internal/testenv" ) // The same guard internal/db keeps over Rendition, one layer over. @@ -23,15 +23,10 @@ import ( // or a blank -- it is a row that reports nothing, on the screen an operator is // looking at seconds before going live. func TestUITypesCanNameEveryMetadataField(t *testing.T) { - path := filepath.Join("..", "..", "ui", "src", "lib", "types.ts") - raw, err := os.ReadFile(path) - if err != nil { - t.Fatalf("cannot read %s: %v", path, err) - } - union, ok := tsUnion(string(raw), "MetaField") + union, ok := tsUnion(testenv.StripJSComments(testenv.ReadUI(t, "lib", "types.ts")), "MetaField") if !ok { - t.Fatalf("no `export type MetaField = ...` in %s. It was moved there from "+ - "Dashboard.tsx precisely so this guard has one canonical place to read", path) + t.Fatal("no `export type MetaField = ...` in ui/src/lib/types.ts. It was moved " + + "there from Dashboard.tsx precisely so this guard has one canonical place to read") } for _, f := range AllMetadataFields { @@ -100,11 +95,28 @@ func TestEveryMetadataFieldIsAdvertisedBySomePlatform(t *testing.T) { } } -// tsUnion returns the body of `export type = ...;`, comments included. +// tsUnion returns the body of `export type = ...;`. +// +// IT USED TO SAY comments were kept deliberately, on the grounds that the union +// carries a note per group about which push path produces those fields and that +// stripping them "would make the file harder to read in exchange for nothing". +// That reasoning was wrong twice over and #379 is the correction. +// +// It was not in exchange for nothing. The forward check below is +// `strings.Contains(union, "\"tags\"")`, so deleting a member and leaving +// `// "tags" -- removed, see ...` behind kept this guard green over a union that +// could no longer name the field. That is the whole failure mode this family of +// guards exists to catch, and this one was open to it. +// +// It also was not free in the other direction: the body is bounded by the first +// `;` after the type name, and a semicolon inside one of those explanatory +// comments truncates the union early, hiding every member after it from a check +// that would then fail while naming the wrong cause. // -// Comments are kept deliberately: the union carries a note per group explaining -// which push path produces those fields, and a helper that stripped them would -// make the file harder to read in exchange for nothing. +// Callers pass source that has already been through testenv.StripJSComments, so +// what is returned is the union as the compiler sees it. Nobody's reading +// experience changes -- the notes are still in types.ts, this just stops them +// counting as declarations. func tsUnion(src, name string) (string, bool) { start := strings.Index(src, "export type "+name+" =") if start < 0 { diff --git a/internal/testenv/docsgate_test.go b/internal/testenv/docsgate_test.go new file mode 100644 index 00000000..73344c83 --- /dev/null +++ b/internal/testenv/docsgate_test.go @@ -0,0 +1,244 @@ +package testenv_test + +// THE DOCUMENTATION-GATE RULE. #378, and the outage it is made of is #351. +// +// ci.yml's `changes` job computes one boolean -- `needs.changes.outputs.code`, +// false when every path a pull request touches is documentation -- and five jobs +// read it to decide whether to do any work. Every one of those jobs publishes a +// REQUIRED status context. That combination has exactly one safe spelling, and +// the unsafe one is not merely slower or noisier: it makes a pull request +// permanently unmergeable. +// +// THE MECHANISM, because "put it on the steps" is a rule nobody can check +// against a workflow they are editing: +// +// a skipped ORDINARY job reports a `skipped` conclusion, and branch protection +// accepts a skip as satisfied. Job-level `if:` is harmless there. +// +// a skipped MATRIX job never expands its matrix. `acceptance: ${{ matrix.suite }}` +// is not one context -- it is thirteen, one per leg, and a job that never +// expanded produces NONE of them. They are not skipped. They do not exist, and +// a required context that does not exist can never be satisfied. +// +// That shipped in #351 and #349 was the casualty: a documentation-only pull +// request -- the exact case the gate was built for -- sat at fifteen green +// checks with no way in, and had to be repaired in #357 by moving the condition +// onto every step. +// +// SO THE RULE IS FLAT: this output may be read from a step's `if:` and never +// from a job's. It is flat rather than "matrix jobs only" on purpose. Three of +// the five gated jobs have no matrix today, and a job-level `if:` on them would +// be correct today -- but the distance between the correct spelling and the +// outage is one `strategy:` block added by somebody thinking about Go versions, +// with no reason at all to be thinking about branch protection. A rule with an +// exception is a rule you have to re-derive at the moment you are least likely +// to. This one has none. +// +// WHAT IT DELIBERATELY DOES NOT CATCH, said out loud so a green run is not read +// as a stronger claim: it knows nothing about which contexts the ruleset +// actually requires -- that lives in GitHub's settings, not in this repository, +// and a test cannot read it without a token. It reasons about `changes` alone, +// so a second gate job invented later gets none of this protection until its +// name is added below. And it cannot tell a step that SHOULD be gated from one +// that legitimately runs on every event; rule two below takes the position that +// inside a job which asked for the gate, there are no such steps. + +import ( + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// docsGate is the expression whose placement this rule is about. +const docsGate = "needs.changes.outputs.code" + +// docsGateJob is the job that produces it. Named rather than inferred: a job +// this rule has never heard of is a job it silently permits, and saying which +// one it watches is the difference between "no findings" and "no opinion". +const docsGateJob = "changes" + +// gatedWorkflow is the slice of the schema this rule needs. Everything else is +// left to yaml.v3 to discard, so an unrelated schema change cannot break it. +// +// Needs is a yaml.Node because GitHub accepts both `needs: changes` and +// `needs: [changes, other]`, and a rule that understood only one of them would +// stop applying the day somebody added a second dependency -- quietly, which is +// the failure mode this whole file is about. +type gatedWorkflow struct { + Jobs map[string]struct { + If string `yaml:"if"` + Needs yaml.Node `yaml:"needs"` + Steps []struct { + Name string `yaml:"name"` + Uses string `yaml:"uses"` + Run string `yaml:"run"` + If string `yaml:"if"` + } `yaml:"steps"` + } `yaml:"jobs"` +} + +// gateFinding is one offending place, named the way a reader would look for it. +type gateFinding struct { + file, job, where, why string +} + +func (f gateFinding) String() string { + at := "job " + f.job + if f.where != "" { + at += " :: step " + f.where + } + return f.file + " :: " + at + " :: " + f.why +} + +// dependsOnGate reports whether a job's `needs` names the gate job, in either +// of the two spellings GitHub accepts. +func dependsOnGate(n yaml.Node) bool { + switch n.Kind { + case yaml.ScalarNode: + return n.Value == docsGateJob + case yaml.SequenceNode: + for _, c := range n.Content { + if c.Value == docsGateJob { + return true + } + } + } + return false +} + +// auditDocsGate returns every violation of both rules in dir, sorted for a +// stable failure message. +func auditDocsGate(t *testing.T, dir string) []gateFinding { + t.Helper() + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + + var out []gateFinding + var files int + for _, e := range entries { + if e.IsDir() { + continue + } + if ext := filepath.Ext(e.Name()); ext != ".yml" && ext != ".yaml" { + continue + } + files++ + + path := filepath.Join(dir, e.Name()) + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + var w gatedWorkflow + if err := yaml.Unmarshal(raw, &w); err != nil { + t.Fatalf("parse %s: %v", path, err) + } + + for name, job := range w.Jobs { + // RULE ONE: the gate may not be read from a job-level `if:`. + if strings.Contains(job.If, docsGate) { + out = append(out, gateFinding{e.Name(), name, "", + "reads " + docsGate + " from a job-level `if:`"}) + } + // RULE TWO: a job that asked for the gate must apply it to every + // step. A half-gated job is a job that still pays for most of + // itself on a documentation-only run while looking gated. + if !dependsOnGate(job.Needs) { + continue + } + for i, s := range job.Steps { + if strings.Contains(s.If, docsGate) { + continue + } + out = append(out, gateFinding{e.Name(), name, stepLabel(i, s.Name, s.Uses), + "has no " + docsGate + " in its `if:`"}) + } + } + } + if files == 0 { + t.Fatalf("no workflow files in %s; this rule would report nothing and "+ + "look like a pass", dir) + } + + sort.Slice(out, func(i, j int) bool { return out[i].String() < out[j].String() }) + return out +} + +// stepLabel names a step the way somebody scrolling the file would find it. +func stepLabel(i int, name, uses string) string { + switch { + case name != "": + return name + case uses != "": + return "uses: " + uses + default: + return "#" + strconv.Itoa(i+1) + " (unnamed)" + } +} + +// TestTheDocumentationGateIsNeverReadFromAJobLevelIf is the rule, applied to the +// real workflows. +func TestTheDocumentationGateIsNeverReadFromAJobLevelIf(t *testing.T) { + dir := filepath.Join(repoRoot(t), ".github", "workflows") + for _, f := range auditDocsGate(t, dir) { + t.Errorf("%s\n"+ + " The documentation gate belongs on every STEP of a job and never\n"+ + " on the job itself. A skipped matrix job does not expand its\n"+ + " matrix, so its per-leg required contexts are never created --\n"+ + " not skipped, absent -- and the pull request can never satisfy\n"+ + " branch protection. #351 shipped exactly that and #349 was\n"+ + " unmergeable at fifteen green checks until #357 undid it.\n"+ + " Move the condition onto the steps, and add a step that says out\n"+ + " loud why the check did no work.", f) + } +} + +// TestTheDocumentationGateRuleFlagsItsRedFixtures is the guard on the guard. +// +// Eight tests have shipped in this repository that passed for the wrong reason. +// A rule about a failure nobody can reproduce locally -- this one needs a +// required-context ruleset and a documentation-only pull request to demonstrate +// -- is the kind most likely to join them, so the fixtures are the evidence that +// it can fail. Both the count and the identity of the findings are asserted: a +// rule that flagged everything would satisfy a count-only test. +func TestTheDocumentationGateRuleFlagsItsRedFixtures(t *testing.T) { + base := filepath.Join("testdata", "docsgate") + + t.Run("red fixtures are all flagged", func(t *testing.T) { + want := []string{ + "half-gated-job.yml :: job build :: step #3 (unnamed) :: has no needs.changes.outputs.code in its `if:`", + "job-level-if-on-a-matrix.yml :: job build :: reads needs.changes.outputs.code from a job-level `if:`", + "job-level-if-on-an-ordinary-job.yml :: job build :: reads needs.changes.outputs.code from a job-level `if:`", + "needs-a-list.yml :: job build :: step uses: actions/checkout@v5 :: has no needs.changes.outputs.code in its `if:`", + } + var got []string + for _, f := range auditDocsGate(t, filepath.Join(base, "red")) { + got = append(got, f.String()) + } + if len(got) != len(want) { + t.Fatalf("flagged %d red places, want %d\n got: %v\nwant: %v", + len(got), len(want), got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("finding %d = %q, want %q", i, got[i], want[i]) + } + } + }) + + t.Run("green fixtures are not flagged", func(t *testing.T) { + if got := auditDocsGate(t, filepath.Join(base, "green")); len(got) != 0 { + t.Errorf("flagged %v; these fixtures either gate every step or do not "+ + "read the gate at all, and flagging them is the false-positive rate "+ + "that would make this rule grow an allowlist", got) + } + }) +} diff --git a/internal/testenv/testdata/docsgate/README.md b/internal/testenv/testdata/docsgate/README.md new file mode 100644 index 00000000..6b4a6ad5 --- /dev/null +++ b/internal/testenv/testdata/docsgate/README.md @@ -0,0 +1,16 @@ +# Fixtures for the documentation-gate rule + +These are NOT workflows. They live under `testdata/` so GitHub never reads them +and so the repository's own AST walkers skip them. + +`red/` holds files the rule in `../../docsgate_test.go` MUST flag, one per way +of getting it wrong. `green/` holds files it must NOT flag, one per way of +being fine — including the shape that looks wrong and is not: a job-level `if:` +reading some *other* job's output, which is what `container` legitimately does. + +The failure this rule prevents cannot be reproduced on a laptop. It needs a +branch-protection ruleset, a required matrix context and a documentation-only +pull request, and it presents as a green pull request that will not merge. So +the only evidence that the rule works is watching it fail on something, and +that is what these are. Adding a case to the rule means adding a red fixture +for it here. diff --git a/internal/testenv/testdata/docsgate/green/every-step-gated.yml b/internal/testenv/testdata/docsgate/green/every-step-gated.yml new file mode 100644 index 00000000..7b741acd --- /dev/null +++ b/internal/testenv/testdata/docsgate/green/every-step-gated.yml @@ -0,0 +1,29 @@ +# The shape ci.yml uses: the matrix always expands, every leg reports, and the +# notice step is what stops a green check that did no work from being mistaken +# for a green check that did. +# +# Note the notice step's condition is the NEGATION of the gate. It still names +# the gate, which is all this rule asks -- a step that mentions the gate has had +# a decision made about it, which is the property being enforced. +name: green - every step carries the gate +on: pull_request +jobs: + build: + name: "build: ${{ matrix.os }}" + needs: changes + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-latest, macos-latest] + steps: + - name: Documentation-only change, so this job did no work + if: needs.changes.outputs.code != 'true' + run: echo "::notice title=build::this check did no work." + - uses: actions/checkout@v5 + if: needs.changes.outputs.code == 'true' + - name: build + if: needs.changes.outputs.code == 'true' + run: make + - name: upload logs + if: always() && needs.changes.outputs.code == 'true' + run: cat log diff --git a/internal/testenv/testdata/docsgate/green/job-level-if-on-another-jobs-output.yml b/internal/testenv/testdata/docsgate/green/job-level-if-on-another-jobs-output.yml new file mode 100644 index 00000000..2771385a --- /dev/null +++ b/internal/testenv/testdata/docsgate/green/job-level-if-on-another-jobs-output.yml @@ -0,0 +1,27 @@ +# THE SHAPE THAT LOOKS WRONG AND IS NOT, which is why this fixture exists: a +# job-level `if:` on a matrix job, reading a different job's output. ci.yml's +# `container` job is exactly this, and it is legitimate because its per-leg +# contexts are NOT required -- a suite that is not wanted should not exist as a +# skipped row. +# +# A rule that flagged every job-level `if:` on a matrix would flag it, be wrong, +# and grow an allowlist within a month. This one is about one boolean. +name: green - a job-level if on a different job's output +on: pull_request +jobs: + suites: + name: which suites + runs-on: ubuntu-latest + steps: + - run: echo 'suites=["a"]' >> "$GITHUB_OUTPUT" + run-suites: + name: "suite: ${{ matrix.suite }}" + needs: suites + if: needs.suites.outputs.suites != '[]' + runs-on: ubuntu-latest + strategy: + matrix: + suite: ${{ fromJSON(needs.suites.outputs.suites) }} + steps: + - uses: actions/checkout@v5 + - run: ./scripts/${{ matrix.suite }}.sh diff --git a/internal/testenv/testdata/docsgate/green/ungated-job.yml b/internal/testenv/testdata/docsgate/green/ungated-job.yml new file mode 100644 index 00000000..b2f6842c --- /dev/null +++ b/internal/testenv/testdata/docsgate/green/ungated-job.yml @@ -0,0 +1,13 @@ +# A job that never asked for the gate. Rule two applies only to jobs that +# declare `needs: changes`, so an ordinary always-runs job -- the `changes` job +# itself, gitleaks, anything in another workflow -- is none of this rule's +# business and must not be flagged. +name: green - a job with no gate at all +on: pull_request +jobs: + always: + name: always runs + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - run: make lint diff --git a/internal/testenv/testdata/docsgate/red/half-gated-job.yml b/internal/testenv/testdata/docsgate/red/half-gated-job.yml new file mode 100644 index 00000000..76306141 --- /dev/null +++ b/internal/testenv/testdata/docsgate/red/half-gated-job.yml @@ -0,0 +1,18 @@ +# The quiet one. The job looks gated -- it depends on `changes` and two of its +# three steps carry the condition -- and a documentation-only run still pays for +# the third. Nothing reports anything unusual; the job is simply slower than the +# checks list suggests it should be. +name: red - a job that gates some of its steps +on: pull_request +jobs: + build: + name: build + needs: changes + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + if: needs.changes.outputs.code == 'true' + - name: build + if: needs.changes.outputs.code == 'true' + run: make + - run: make test diff --git a/internal/testenv/testdata/docsgate/red/job-level-if-on-a-matrix.yml b/internal/testenv/testdata/docsgate/red/job-level-if-on-a-matrix.yml new file mode 100644 index 00000000..2c0849d5 --- /dev/null +++ b/internal/testenv/testdata/docsgate/red/job-level-if-on-a-matrix.yml @@ -0,0 +1,22 @@ +# The outage itself: #351 in miniature. +# +# The matrix never expands, so `build: ubuntu-latest` and `build: macos-latest` +# are never created. If either is a required context the pull request is stuck +# for ever, showing green everywhere else. +name: red - job-level if on a matrix job +on: pull_request +jobs: + build: + name: "build: ${{ matrix.os }}" + needs: changes + if: needs.changes.outputs.code == 'true' + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-latest, macos-latest] + steps: + - uses: actions/checkout@v5 + if: needs.changes.outputs.code == 'true' + - name: build + if: needs.changes.outputs.code == 'true' + run: make diff --git a/internal/testenv/testdata/docsgate/red/job-level-if-on-an-ordinary-job.yml b/internal/testenv/testdata/docsgate/red/job-level-if-on-an-ordinary-job.yml new file mode 100644 index 00000000..eab98f6e --- /dev/null +++ b/internal/testenv/testdata/docsgate/red/job-level-if-on-an-ordinary-job.yml @@ -0,0 +1,19 @@ +# FLAGGED EVEN THOUGH IT WORKS TODAY, and that is the point of the rule being +# flat. A skipped ordinary job reports `skipped` and branch protection accepts +# it, so this file is correct right now. It becomes the file above the moment +# somebody adds a `strategy:` block while thinking about Go versions rather than +# about branch protection. +name: red - job-level if on an ordinary job +on: pull_request +jobs: + build: + name: build + needs: changes + if: needs.changes.outputs.code == 'true' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + if: needs.changes.outputs.code == 'true' + - name: build + if: needs.changes.outputs.code == 'true' + run: make diff --git a/internal/testenv/testdata/docsgate/red/needs-a-list.yml b/internal/testenv/testdata/docsgate/red/needs-a-list.yml new file mode 100644 index 00000000..60943f6e --- /dev/null +++ b/internal/testenv/testdata/docsgate/red/needs-a-list.yml @@ -0,0 +1,16 @@ +# `needs:` written as a list, which GitHub accepts and which a rule that only +# understood the scalar form would walk straight past. The ungated checkout +# below is the finding; the list is what a naive rule would fail to notice it +# through. +name: red - needs written as a list +on: pull_request +jobs: + build: + name: build + needs: [changes, container-suites] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - name: build + if: needs.changes.outputs.code == 'true' + run: make diff --git a/internal/testenv/uisource.go b/internal/testenv/uisource.go new file mode 100644 index 00000000..01e1eb55 --- /dev/null +++ b/internal/testenv/uisource.go @@ -0,0 +1,164 @@ +package testenv + +// UI SOURCE. Issue #379. +// +// A UI-drift guard reads TypeScript from disk and asserts a substring is in it. +// Two helpers make that honest, and until now only one package had them: +// +// internal/db/facebook_ui_drift_test.go readUI, stripJSComments +// +// COMMENTS ARE THE HOLE, and it is measured rather than theorised. #367: the +// real code was deleted out of AppLayout.tsx and its text left behind in a +// `// was: ...` comment, and the guard watching it stayed green. That is not a +// contrived mutation -- it is the honest way to keep a substring guard green +// while removing the thing it watches, which makes it the shape that actually +// happens. So the words a guard reads must not be the words in a comment. +// +// internal/oauth had four guards reading ui/src and no comment stripper, so all +// four were defeatable that way (probed and confirmed on #379). The repair is +// not a second forty-line copy of the stripper in that package: two copies of a +// rule about what a guard may see is two rules the day one of them is edited, +// and the guard that stops matching first is the one nobody notices. Hence one +// implementation, here, where the port helpers already live for the same reason +// (see ports.go on the four copies of freeUDPPort). +// +// WHAT THESE CANNOT DO, said out loud so a green run is not read as a stronger +// claim: reading source proves a control is WRITTEN, never that React puts it on +// screen. That needs a browser and ui/e2e/ has one. These are the cheap net +// under the class, run by `go test ./...` on every machine. + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// ReadUI reads a file under ui/src and returns its contents. +// +// THE ROOT IS FOUND, NOT ASSUMED. Every caller of this before it moved here +// spelled the path `filepath.Join("..", "..", "ui", "src", ...)`, which is +// correct only for a package exactly two directories below the module root. +// That was true of both callers and is not a property this helper can promise +// now that it is shared: a guard written in internal/api/media/ would resolve to +// a path that does not exist, and the failure -- "cannot read ..." -- reads like +// a missing UI file rather than like a helper counting wrong. Walking up to +// go.mod costs a few stats once and cannot be wrong about which tree it is in. +// +// AN EMPTY FILE IS A FAILURE, not an empty string handed to the caller. This is +// the one behaviour ReadUI takes from internal/testenv's own readRepoFile rather +// than from internal/db's readUI, which had no such check. The two had drifted, +// and the version with the check is the right one: a guard asserting a substring +// is ABSENT -- `if strings.Contains(src, "status?.ingest")` in the ingest-header +// guard is exactly that shape -- passes over an empty file while asserting +// nothing at all. Failing here names the file; passing vacuously names nothing. +func ReadUI(t *testing.T, parts ...string) string { + t.Helper() + path := filepath.Join(append([]string{moduleRoot(t), "ui", "src"}, parts...)...) + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("cannot read %s: %v", path, err) + } + if len(raw) == 0 { + t.Fatalf("%s is empty; every assertion that reads it would pass by "+ + "examining nothing", path) + } + return string(raw) +} + +// StripJSComments blanks out comments so a marker left behind in one cannot +// satisfy a guard that is asking whether a control is wired. +// +// Block comments (`/* */`, and the `{/* */}` JSX form, which is a block comment +// inside an expression container) are removed outright. Line comments are +// removed only when nothing before the `//` on that line is quoted, so a URL in +// a string literal or a template literal is left alone rather than truncated at +// the scheme separator. +// +// Newlines are preserved so a line number quoted in a failure still means +// something to whoever goes to look, and so any guard that bounds itself by +// counting lines or by searching forward from an offset sees the same shape it +// would have seen in the file. +// +// IT IS NOT A PARSER and does not pretend to be. A `/*` inside a string literal +// is read as a comment opener, which would swallow source. That has not happened +// in this tree and the alternative is a TypeScript toolchain invoked from a Go +// test, which would make every one of these guards skip itself on any machine +// missing it -- and a guard that skips silently is worse than no guard, because +// the next person reads green and believes it. The same trade is written down in +// internal/oauth's parseUICapabilities for the same reason. +// +// A PORT OF THIS EXISTS IN TYPESCRIPT, in ui/src/lib/tour-drift.test.ts, and it +// cannot be collapsed into this one: vitest cannot call Go. The two were +// measured against each other when this was promoted -- both run over all 96 +// .ts and .tsx files under ui/src, output hashed per file, byte-identical on +// every one -- so they have not drifted. Nothing enforces that, and the TS copy +// names this one as its origin so the next edit to either can be carried across +// by hand. +func StripJSComments(src string) string { + var b strings.Builder + b.Grow(len(src)) + for i := 0; i < len(src); { + if strings.HasPrefix(src[i:], "/*") { + end := strings.Index(src[i+2:], "*/") + if end < 0 { + break // unterminated; the rest is comment + } + for _, r := range src[i : i+2+end+2] { + if r == '\n' { + b.WriteByte('\n') + } + } + i += 2 + end + 2 + continue + } + if strings.HasPrefix(src[i:], "//") && !quotedBefore(src, i) { + end := strings.IndexByte(src[i:], '\n') + if end < 0 { + break + } + i += end // leave the newline for the next iteration + continue + } + b.WriteByte(src[i]) + i++ + } + return b.String() +} + +// quotedBefore reports whether a quote character appears between the start of +// the line containing i and i itself -- the cheap test for "this `//` is inside +// a string literal or JSX attribute rather than starting a comment". +func quotedBefore(src string, i int) bool { + start := strings.LastIndexByte(src[:i], '\n') + 1 + return strings.ContainsAny(src[start:i], "\"'`") +} + +// moduleRoot walks up from the test's working directory to the directory +// holding go.mod. +// +// `go test` runs each package in its own source directory, so the walk starts +// somewhere inside the tree and terminates at the module root or at the +// filesystem root. Reaching the filesystem root is a Fatal rather than a +// fallback: a helper that guessed would hand back a path that reads a file from +// some other checkout, and a drift guard measuring the wrong tree is the failure +// this whole family of tests exists to prevent. +func moduleRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatalf("cannot resolve the working directory: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatalf("no go.mod above %s; ReadUI cannot tell which tree it is "+ + "reading, and a drift guard pointed at the wrong tree is worse "+ + "than one that is absent", dir) + } + dir = parent + } +}