diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index bcc5ed391..a69fc4251 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -47,7 +47,8 @@ gitlab-mcp-server/ │ ├── bench_resources/ # What the server costs to run, and the charts the docs publish │ ├── godoc_tool/ # Godoc auditor + fixer (audit/fix; was audit_godocs + add_docs; make check-godocs) │ ├── format_md_tables/ # Markdown pipe-table normalizer -│ ├── measure_conditions/ # Plan and job summary of the on-demand condition coverage workflow (conditions.yml, dispatched by hand) +│ ├── measure_conditions/ # Plan, measuring loop and job summary of the on-demand condition and branch coverage workflow (conditions.yml, dispatched by hand), and the whole-tree figures (make coverage-conditions-tree) +│ ├── fuzz_targets/ # The fuzz targets the weekly fuzzing workflow (fuzz.yml) runs with go test -fuzz │ ├── gen_action_catalog_manifest/ # ActionSpec group-builder manifest │ ├── gen_brand/ # Every vector brand asset from one parametric geometry │ ├── gen_icon_webp/ # Light/dark WebP icon fallbacks from icons.go diff --git a/.github/workflows/conditions.yml b/.github/workflows/conditions.yml index b3fe0d4e2..1a131262e 100644 --- a/.github/workflows/conditions.yml +++ b/.github/workflows/conditions.yml @@ -12,6 +12,16 @@ # dispatch names, so every figure is the one the gate would compute there: # measured on that system, never inferred from another. # +# It also takes the figure for the whole tree: with tree set, every package +# below cmd/ and internal/ is measured, in shards, and the summary adds up +# each system's total. mode chooses what gobco counts: condition, its +# default, counts every operand of && and || and every comparison; branch is +# gobco's -branch, which counts the whole condition of each if and for and +# each switch case, the branches the OpenSSF Best Practices criterion +# test_branch_coverage80 asks about. docs/development/testing/README.md +# records the figures taken in both, and the command that takes them on any +# machine. +# # Dispatch only. A push or a pull request never runs it: the answer is wanted # when somebody asks the question, and a package can take the better part of # an hour on Windows. Nothing here is a gate. No figure fails it, whatever the @@ -20,15 +30,21 @@ # job, and says why. # # Shape: plan validates the inputs and, for an empty package list, finds the -# packages carrying a GOOS- or GOARCH-constrained file; measure fans out over -# package x system; summary joins the records the runs uploaded into one table -# in the job summary. The plan and the table are cmd/measure_conditions, whose -# tests hold this file to the record it reads. +# packages carrying a GOOS- or GOARCH-constrained file, or with tree lists +# the tree and deals it into shards; measure fans out over job x system, each +# job measuring its packages one after another; summary joins the records the +# runs uploaded into one table, with the totals, in the job summary. The plan, +# the measuring loop and the table are cmd/measure_conditions, whose tests +# hold this file to the record it reads. # # Dispatch with: # # gh workflow run conditions.yml -f packages="./cmd/server ./internal/toolutil" \ # -f systems="windows-latest macos-latest ubuntu-latest" -f gate=all +# +# gh workflow run conditions.yml -f tree=true -f systems=ubuntu-latest -f gate=all +# +# gh workflow run conditions.yml -f tree=true -f systems=ubuntu-latest -f gate=all -f mode=branch name: Condition Coverage @@ -50,6 +66,17 @@ on: description: "Runners to measure on, separated by spaces or commas: windows-latest, macos-latest, ubuntu-latest (the comparison)." default: "windows-latest macos-latest ubuntu-latest" type: string + tree: + description: "Measure every package below cmd/ and internal/ instead, in shards, with each system's total. Leave packages empty." + default: false + type: boolean + mode: + description: "What gobco counts. condition, its default, counts every operand of && and || and every comparison; branch (gobco -branch) counts the whole condition of each if and for and each switch case." + default: condition + type: choice + options: + - condition + - branch # Read only. Nothing here writes to the repository, comments, or mints an # identity: the default token reads the checkout and the artifacts of this run. @@ -79,7 +106,10 @@ jobs: # the expression syntax inside the script, so a value is data whatever # it holds. The command refuses a package, a runner or a gate it would # not hand on, and for an empty package list lists the module under - # every release target to find the packages whose files differ. + # every release target to find the packages whose files differ. With + # tree it lists the packages below cmd/ and internal/ instead, and + # refuses a package list beside it. The mode is refused here too when + # the script would refuse it, before any runner starts. - name: Plan the runs id: plan shell: bash @@ -87,17 +117,22 @@ jobs: PACKAGES: ${{ inputs.packages }} SYSTEMS: ${{ inputs.systems }} GATE: ${{ inputs.gate }} - run: go run ./cmd/measure_conditions plan -packages "$PACKAGES" -systems "$SYSTEMS" -gate "$GATE" >> "$GITHUB_OUTPUT" + MODE: ${{ inputs.mode }} + TREE: ${{ inputs.tree }} + run: go run ./cmd/measure_conditions plan -packages "$PACKAGES" -systems "$SYSTEMS" -gate "$GATE" -mode "$MODE" -tree="$TREE" >> "$GITHUB_OUTPUT" measure: - name: "🔬 ${{ matrix.package }} on ${{ matrix.system }}" + name: "🔬 ${{ matrix.name }} on ${{ matrix.system }}" needs: plan runs-on: ${{ matrix.system }} # gobco's go test is bounded at 30 minutes below, the bound ci.yml gives - # cmd/server on Windows; the job bound leaves room for the toolchain and - # the staged copy around it, and ends a hang on a runner billed at a - # multiple of the Linux rate. - timeout-minutes: 60 + # cmd/server on Windows. A job of named packages measures one, and keeps + # the hour it had before the tree could be measured; a shard of the tree + # measures fifteen or so one after another, and is given two. Each bound + # leaves room for the toolchain and the staged copies around the + # measuring, and ends a hang on a runner billed at a multiple of the Linux + # rate. + timeout-minutes: ${{ inputs.tree && 120 || 60 }} strategy: # Every run reports. Stopping at the first that could not measure would # hide every figure the others took. @@ -112,20 +147,26 @@ jobs: go-version-file: go.mod - run: go mod download # The script, through bash, exactly as ci.yml's legs run it and as - # `make coverage-conditions` runs it, with the gate the dispatch chose. - # Both streams go to the log and to the raw output file. + # `make coverage-conditions` runs it, with the gate and the mode the + # dispatch chose, over each package of the job one after another. The + # measure subcommand runs it and writes each package's record: both + # streams of the script go to the log and to the package's raw output + # file, beside a run.env naming the package, the runner, the platform + # and the script's status, in a directory of its own named after the + # package. # - # The record is written outside the checkout: the script copies the + # The records are written outside the checkout: the script copies the # module into a staged directory for a package with a platform file, - # and a file this step is still writing inside the tree would be copied - # mid-write. RUNNER_TEMP is a Windows path on Windows, written with - # slashes here, which Git Bash and go both read, as the script does with - # go list's directories. + # and a file written inside the tree would be copied with it. + # RUNNER_TEMP is a Windows path on Windows, written with slashes here, + # which Git Bash and go both read, as the script does with go list's + # directories. # - # The exit status is read for one thing: 0 and 3 are a measurement (3 is - # the gate holding a condition, which the script uses for nothing else), - # and anything else means nothing was measured. That fails the job with - # the reason; a figure never does. + # The script's status is read for one thing: 0 and 3 are a measurement + # (3 is the gate holding a condition, which the script uses for nothing + # else), and anything else means nothing was measured. A package not + # measured fails the job with the reason once the job's other packages + # are measured; a figure never does. # # setup-go registers a problem matcher, owner go, that files every line # shaped like file.go:line:col: text as an error annotation, and every @@ -137,32 +178,21 @@ jobs: - name: Measure shell: bash env: - PACKAGE: ${{ matrix.package }} + PACKAGES: ${{ matrix.packages }} SYSTEM: ${{ matrix.system }} ARTIFACT: ${{ matrix.artifact }} GOBCO_GATE: ${{ inputs.gate }} + GOBCO_MODE: ${{ inputs.mode }} GOBCO_TEST_FLAGS: -timeout=30m run: | echo "::remove-matcher owner=go::" - record="${RUNNER_TEMP//\\//}/$ARTIFACT" - mkdir -p "$record" - set +e - bash scripts/coverage-conditions.sh "$PACKAGE" 2>&1 | tee "$record/gobco.txt" - status=${PIPESTATUS[0]} - set -e - { - echo "package=$PACKAGE" - echo "system=$SYSTEM" - echo "platform=$(go env GOOS)/$(go env GOARCH)" - echo "status=$status" - } >"$record/run.env" - case "$status" in - 0|3) echo "Measured. The figure is in the summary job's table." ;; - *) - echo "::error title=Not measured::scripts/coverage-conditions.sh exited $status for $PACKAGE on $SYSTEM, so nothing was measured; the gobco: lines above say why, and the raw output is the artifact $ARTIFACT" - exit "$status" - ;; - esac + status=0 + go run ./cmd/measure_conditions measure -packages "$PACKAGES" -system "$SYSTEM" -records "${RUNNER_TEMP//\\//}/$ARTIFACT" || status=$? + if [ "$status" -ne 0 ]; then + echo "::error title=Not measured::a package of this job was not measured on $SYSTEM (measure_conditions exited $status); the measure_conditions: and gobco: lines above say which and why, and the raw output is in the artifact $ARTIFACT" + exit "$status" + fi + echo "Measured. The figures are in the summary job's table." # Uploaded whatever became of the measurement, since a run that could # not measure is the one whose output somebody will want to read. A run # that never reached the step above has nothing to upload, which the @@ -202,13 +232,15 @@ jobs: with: pattern: gobco-* path: ${{ runner.temp }}/records - # The table goes to the job summary and to the log. The command exits - # non-zero only when a record cannot be read or disagrees with itself, - # never on a figure. + # The table goes to the job summary and to the log, headed by the mode + # the runs measured in, whose figure it reads each record's in. The + # command exits non-zero only when a record cannot be read or disagrees + # with itself, never on a figure. - name: Write the job summary shell: bash env: MATRIX: ${{ needs.plan.outputs.matrix }} GATE: ${{ inputs.gate }} + MODE: ${{ inputs.mode }} COMMIT: ${{ github.sha }} - run: go run ./cmd/measure_conditions summarize -plan "$MATRIX" -records "$RUNNER_TEMP/records" -gate "$GATE" -commit "$COMMIT" | tee -a "$GITHUB_STEP_SUMMARY" + run: go run ./cmd/measure_conditions summarize -plan "$MATRIX" -records "$RUNNER_TEMP/records" -gate "$GATE" -mode "$MODE" -commit "$COMMIT" | tee -a "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml new file mode 100644 index 000000000..94578de63 --- /dev/null +++ b/.github/workflows/fuzz.yml @@ -0,0 +1,116 @@ +# Fuzzing for gitlab-mcp-server +# +# The module carries fuzz targets (FuzzMeterFor, FuzzEvaluate_IsSoundAgainstGitLab, +# FuzzRefuseUnreadable and the rest), and go test runs each one's seed corpus +# as one more test: it varies no input unless it is given -fuzz, and until this +# workflow nothing here gave it. This runs every target with go test -fuzz for +# a fixed time, so the inputs the targets exist to vary are varied. +# +# Where it runs, and where it does not: +# +# * once a week on main, on Wednesdays, which no other weekly workflow +# runs on; +# * when dispatched, from the Actions tab or with `gh workflow run fuzz.yml`; +# * on a pull request that changes this file or cmd/fuzz_targets, and on no +# other, with one minute per target rather than ten. GitHub offers a +# workflow for dispatch only once its file is on the default branch, so +# the pull request that adds this file can run it through this trigger +# alone; a later change could be dispatched from its branch, and this +# runs it without anybody having to. +# +# It is a workflow of its own for the reason race.yml is: ci.yml converges on +# one job, "CI", the single check branch protection requires, and a job added +# there would gate every pull request. A failure here is a defect a fuzzer +# found, to be fixed, and is not wired into the release. +# +# Shape: plan lists the targets with cmd/fuzz_targets, which reads the test +# files the go command builds and prints one matrix cell per target, so a +# target added to any package is fuzzed from the next run on without an edit +# here, and a plan that found none fails rather than passing with nothing +# fuzzed. fuzz runs each cell. A failing input is what the go command writes +# under the package's testdata/fuzz/; it is uploaded as the cell's +# artifact, and copying it into that directory of a checkout makes it a seed +# that `go test` replays from then on (docs/development/static-analysis.md, +# Fuzzing). The cmd/fuzz_targets tests hold this file to the matrix it prints. + +name: Fuzz + +on: + schedule: + # Wednesdays, 05:43 UTC: no other weekly workflow runs on that day (the + # one every day is gitlab-mirror.yml, every six hours at 17 past), and off + # the hour for the reason recorded in e2e.yml, since GitHub delays runs + # that pile up on round times. + - cron: "43 5 * * 3" + workflow_dispatch: + pull_request: + paths: + - .github/workflows/fuzz.yml + - cmd/fuzz_targets/** + +# Read only. Nothing here writes to the repository, comments, or mints an +# identity: the default token reads the checkout and this run's artifacts. +permissions: + contents: read + +jobs: + plan: + name: "📋 Plan" + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + matrix: ${{ steps.plan.outputs.matrix }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + with: + go-version-file: go.mod + - name: List the fuzz targets + id: plan + run: go run ./cmd/fuzz_targets >> "$GITHUB_OUTPUT" + + fuzz: + name: "🎲 ${{ matrix.target }} (${{ matrix.package }})" + needs: plan + runs-on: ubuntu-latest + # Ten minutes of fuzzing, and the build of a test binary instrumented for + # it around them, which for cmd/server and internal/tools/search is a few + # minutes cold. The bound ends a hang; it is not how long a run should take. + timeout-minutes: 30 + strategy: + # Every target reports. Stopping at the first failure would hide a + # second, different one. + fail-fast: false + matrix: ${{ fromJSON(needs.plan.outputs.matrix) }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + with: + go-version-file: go.mod + - run: go mod download + # One target of one package, anchored so it matches no other, and no + # test beside it: the target's seeds run first as part of fuzzing. The + # inputs reach go test through the environment, so a value is data + # whatever it holds. On a failure go test prints the input, writes it + # under testdata/fuzz/ and exits non-zero, which fails the job. + - name: Fuzz + env: + PACKAGE: ${{ matrix.package }} + TARGET: ${{ matrix.target }} + FUZZTIME: ${{ github.event_name == 'pull_request' && '1m' || '10m' }} + run: go test -run '^$' -fuzz "^${TARGET}\$" -fuzztime "$FUZZTIME" "$PACKAGE" + # The directory holds the target's committed seeds as well when it has + # any, which a reader tells apart by name: the go command names a + # failing input after its hash, and the log of the step above names it. + - name: Upload the failing input + if: failure() + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7 + with: + name: ${{ matrix.artifact }} + path: ${{ matrix.package }}/testdata/fuzz/${{ matrix.target }} + retention-days: 30 + if-no-files-found: ignore diff --git a/.golangci.yml b/.golangci.yml index 4a51c9cf9..75607a2aa 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -13,13 +13,14 @@ run: go: "1.27" timeout: 15m tests: true - # golangci-lint defaults to one worker per logical CPU. Across 226 packages - # and 55 linters that working set does not fit comfortably in memory on a - # many-threaded machine: measured on a cold cache, the default (16 workers on - # a 16-thread laptop) peaked at 4.4GB and had not finished after 18 minutes, + # golangci-lint defaults to one worker per logical CPU, and that working set + # does not fit comfortably in memory on a many-threaded machine. Measured in + # July 2026 on a cold cache, when the module had 226 packages and this file + # enabled 52 linters and 3 formatters, the default (16 workers on a + # 16-thread laptop) peaked at 4.4GB and had not finished after 18 minutes, # while the same run capped at 4 workers peaked at 3.6GB and took 3m01s. - # CI runners have 4 vCPUs, so this matches what CI already does implicitly and - # only changes behaviour on bigger machines — where it is 6x faster. + # CI runners have 4 vCPUs, so this matches what CI already does implicitly + # and only changes behaviour on bigger machines, where it was 6x faster. concurrency: 4 # Every e2e build tag, so that the invocation the header documents sees the # same files the Makefile's GO_ANALYSIS_TAGS does. A tagged file is invisible diff --git a/CLAUDE.md b/CLAUDE.md index c07c74fce..8b80c8636 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ | MCP Icons | 51 icons (50 domain + brand mark), each a 3-entry `[]mcp.Icon`: one SVG (base64 data URI, `Sizes: ["any"]`, `currentColor`) plus light/dark 16×16 lossless WebP fallbacks (`Theme`-tagged, `cmd/gen_icon_webp`) for clients that reject SVG. The brand mark is the generated "fan-out" (`cmd/gen_brand` → `brandmark_gen.go`), original artwork replacing the former tanuki | | Source files (tools) | 788 non-test Go files under `internal/tools/` | | Test files (tools) | 488 test files under `internal/tools/` | -| Go packages | 306 in the module (`go list ./...`); 333 directories hold tracked `.go` files, the twenty-seven more being what `go list` leaves out: twenty-six `testdata` fixture packages (the two stand-in programs, the coverage audit's planted trees, the closed-server audit's planted module, the stand-in module the SDK route reader's tests read and the platform module the condition measurement's discovery lists) and `test/e2e/modeleval/internal/provider`, whose every file sits behind a build tag. 182 under `internal/tools/...` (the root package plus 181 sub-packages) | +| Go packages | 307 in the module (`go list ./...`); 334 directories hold tracked `.go` files, the twenty-seven more being what `go list` leaves out: twenty-six `testdata` fixture packages (the two stand-in programs, the coverage audit's planted trees, the closed-server audit's planted module, the stand-in module the SDK route reader's tests read and the platform module the condition measurement's discovery lists) and `test/e2e/modeleval/internal/provider`, whose every file sits behind a build tag. 182 under `internal/tools/...` (the root package plus 181 sub-packages) | ### Orbit live tests @@ -102,7 +102,8 @@ gitlab-mcp-server/ │ ├── gen_model_corpus/ # Model evaluation corpus breadth ledger │ ├── gen_model_results/ # Folds model evaluation shards into the committed record and results page │ ├── format_md_tables/ # Markdown pipe table formatter (make check-md-tables) -│ ├── measure_conditions/ # Both ends of the cross-OS condition coverage workflow +│ ├── measure_conditions/ # The cross-OS condition and branch coverage workflow and the whole-tree figures (make coverage-conditions-tree) +│ ├── fuzz_targets/ # The fuzz targets the weekly fuzz.yml runs with go test -fuzz │ ├── gen_action_catalog_manifest/ # Audited action catalog manifest │ ├── gen_api_live/ # REST oracle evaluated from a booted GitLab: gitlab-api-live.json (make gen-api-live) │ ├── gen_action_grants/ # What each action sends and what a token needs for it (make gen-action-grants) diff --git a/Makefile b/Makefile index 8497b2c31..d66ec9691 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ .PHONY: build build-all build-linux-amd64 build-linux-arm64 build-windows-amd64 build-windows-arm64 build-darwin-amd64 build-darwin-arm64 \ - run brand brand-check brand-rasters ensure-mcp-publisher mcp-publisher-version test test-short test-race coverage-conditions coverage-mutants test-pkg test-integration test-e2e test-e2e-harness test-e2e-http test-e2e-stdio test-e2e-collector ensure-gotestsum test-e2e-docker test-e2e-docker-enterprise test-e2e-gitlab-com \ + run brand brand-check brand-rasters ensure-mcp-publisher mcp-publisher-version test test-short test-race coverage-conditions coverage-conditions-tree coverage-mutants test-pkg test-integration test-e2e test-e2e-harness test-e2e-http test-e2e-stdio test-e2e-collector ensure-gotestsum test-e2e-docker test-e2e-docker-enterprise test-e2e-gitlab-com \ e2e-server-binary test-e2e-ce test-e2e-ee test-e2e-gitlab e2e-clean-orphans \ orbit-setup-fixtures orbit-wait-indexer orbit-run-live-tests orbit-ensure-token gen-orbit-record check-orbit-record \ coverage \ @@ -787,8 +787,16 @@ modeleval-probe: # to instrument; it runs gobco there, names the files it left out, whose # conditions this run does not measure, and removes the copy whatever # happens. The copy is of the module so that import paths, relative paths and -# directory names stay the original's. A package gobco could already read, a -# constraint on the platform alone included, is run where it is, as before. +# directory names stay the original's. A package whose external test package +# gobco cannot read is staged too. gobco type-checks that package against the +# package's non-test files alone, so the copy makes each test file of the +# package that declares no test (an export_test.go) a file of the package +# behind //go:build gc, which go builds and gobco does not instrument; and +# gobco bridges to it through an import of the package it may not find, so an +# external test package that imports nothing of the package is given one, +# until a gobco release fixes both (upstream-bugs entries 97 and 140). A +# package gobco could already read, a constraint on the platform alone +# included, is run where it is, as before. # TAGS names build tags for a package behind one (TAGS=e2e), passed to go list # and to gobco's go test alike, and a report that measured no condition (0/0) # is refused. GOBCO_GATE=all turns the report into the gate a touched package @@ -803,6 +811,28 @@ coverage-conditions: @test -n "$(PKG)" || { echo "usage: make coverage-conditions PKG=./cmd/gen_llms [TAGS=e2e]"; exit 2; } @scripts/coverage-conditions.sh $(PKG) $(TAGS) +# Where coverage-conditions-tree leaves each package's record and raw output, +# how many packages it measures at once, and what gobco counts. +CONDITIONS_RECORDS ?= dist/conditions-tree +CONDITIONS_PARALLEL ?= 1 +CONDITIONS_MODE ?= condition + +## coverage-conditions-tree: condition or branch coverage (gobco) of every package under cmd/ and internal/, one row each and the total, written as the summary conditions.yml's tree dispatch writes. CONDITIONS_RECORDS keeps the records (dist/conditions-tree); CONDITIONS_PARALLEL packages run at once (1); CONDITIONS_MODE is condition or branch (condition). +# Each package is measured by scripts/coverage-conditions.sh, the recipe +# behind coverage-conditions, with GOBCO_GATE=all as the workflow's default, +# which decides only which conditions the summary marks as held and moves no +# figure, and GOBCO_MODE=$(CONDITIONS_MODE): condition is gobco's default, +# every operand of && and || and every comparison, and branch is gobco's +# -branch, the whole condition of each if and for and each switch case. The +# summary goes to stdout and to summary.md beside the records. It is the +# command docs/development/testing/README.md takes the recorded figures with, +# so they can be taken again on any machine without a dispatch; that page +# says how long each run behind them took, on how many cores and with how +# many packages at a time. +coverage-conditions-tree: + @mkdir -p "$(CONDITIONS_RECORDS)" + @bash -o pipefail -c 'go run ./cmd/measure_conditions tree -records "$(CONDITIONS_RECORDS)" -parallel "$(CONDITIONS_PARALLEL)" -mode "$(CONDITIONS_MODE)" -commit "$$(git rev-parse HEAD)" | tee "$(CONDITIONS_RECORDS)/summary.md"' + ## coverage-mutants: mutation-test PKG with gremlins, INVERT_LOGICAL on so `&&`/`||` independence is checked. The gate on a changed package is Lived 0 and Not covered 0. # The recipe itself is scripts/coverage-mutants.sh. It moved out of this file # when it gained the staging step that makes a `package main` measurable at all diff --git a/cmd/fuzz_targets/doc.go b/cmd/fuzz_targets/doc.go new file mode 100644 index 000000000..da73830d6 --- /dev/null +++ b/cmd/fuzz_targets/doc.go @@ -0,0 +1,31 @@ +// Command fuzz_targets lists this module's fuzz targets as the matrix that +// .github/workflows/fuzz.yml fans out over. +// +// go test runs a fuzz target's seed corpus as one more test and varies no +// input unless it is given -fuzz, and until the workflow existed nothing here +// gave it: every target was run on its seeds alone. The workflow fuzzes each +// target for a fixed time once a week, and this command is its plan: +// +// fuzz_targets [-dir ] +// +// It lists every package of the module with the toolchain's own go, reads the +// test files the go command builds for each on this platform, and prints one +// `matrix=` line for $GITHUB_OUTPUT with a cell per target: the package +// as ./ and its directory, the target's name, and the name of the artifact a +// failing input is uploaded under. A target is a top-level function, in a test +// file the go command builds here, whose name is Fuzz or starts with Fuzz +// followed by a character that is not a lower-case letter, which is the rule +// the go command reads a fuzz target by (FuzzMeterFor is one, Fuzzy is not). +// A file behind a build tag the listing is not given, such as the e2e suite's, +// is not built here and is not read, and the workflow could not fuzz a target +// in it either. +// +// The targets are found anew on every run rather than written into the +// workflow, so a target added to any package is fuzzed from the next run on. +// +// It exits 0 when it printed the matrix, 1 when the module could not be +// listed, a test file could not be parsed, two targets would upload under the +// same artifact name, or no target was found at all, since a plan with nothing +// in it would pass the workflow while it fuzzed nothing, and 2 on a usage +// error. +package main diff --git a/cmd/fuzz_targets/main.go b/cmd/fuzz_targets/main.go new file mode 100644 index 000000000..cb6cd548d --- /dev/null +++ b/cmd/fuzz_targets/main.go @@ -0,0 +1,62 @@ +package main + +import ( + "context" + "encoding/json" + "flag" + "fmt" + "io" + "os" + + "github.com/jmrplens/gitlab-mcp-server/v3/internal/cmdutil" +) + +// toolName is how every message this command prints names its source. +const toolName = "fuzz_targets" + +// osExit is a seam over os.Exit, so a test can observe the exit code main +// passes on without ending the test process. +var osExit = os.Exit + +func main() { + osExit(runMain(os.Args[1:], os.Stdout, os.Stderr, goList)) +} + +// runMain finds the fuzz targets of the module -dir names and prints the +// matrix as one `matrix=` line for $GITHUB_OUTPUT, and each target and +// their count on stderr. It returns 2 for a usage error, 1 when the targets +// could not be found or planned or there are none, and 0 otherwise. +func runMain(args []string, stdout, stderr io.Writer, list lister) int { + flags := flag.NewFlagSet(toolName, flag.ContinueOnError) + flags.SetOutput(stderr) + dir := flags.String("dir", ".", "module root the fuzz targets are looked for in") + if err := flags.Parse(args); err != nil { + return 2 + } + if flags.NArg() > 0 { + fmt.Fprintf(stderr, "%s: takes no arguments, and was given %q\n", toolName, flags.Args()) + return 2 + } + targets, err := findTargets(context.Background(), *dir, list) + if err != nil { + fmt.Fprintf(stderr, "%s: %v\n", toolName, err) + return 1 + } + if len(targets) == 0 { + fmt.Fprintf(stderr, "%s: no fuzz target found in %s, so the workflow would fuzz nothing and pass\n", toolName, *dir) + return 1 + } + planned, err := buildMatrix(targets) + if err != nil { + fmt.Fprintf(stderr, "%s: %v\n", toolName, err) + return 1 + } + fmt.Fprintf(stdout, "matrix=%s\n", cmdutil.Must(json.Marshal(planned))) + packages := map[string]bool{} + for _, each := range targets { + packages[each.pkg] = true + fmt.Fprintf(stderr, "%s: %s %s\n", toolName, each.pkg, each.name) + } + fmt.Fprintf(stderr, "%s: %d fuzz target(s) in %d package(s)\n", toolName, len(targets), len(packages)) + return 0 +} diff --git a/cmd/fuzz_targets/main_test.go b/cmd/fuzz_targets/main_test.go new file mode 100644 index 000000000..58ea06440 --- /dev/null +++ b/cmd/fuzz_targets/main_test.go @@ -0,0 +1,252 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "maps" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" + + yaml "go.yaml.in/yaml/v3" +) + +// TestRunMain_Arguments_PrintTheMatrixOrRefuse verifies every outcome of a +// run: a flag it does not define and an argument are usage errors, a listing +// that failed, a module with no target and two targets that would share an +// artifact are each refused with the reason, and a module with targets gets +// the matrix on stdout, in the order found, and one line per target with the +// count on stderr. The listing is asked in the directory -dir names. +func TestRunMain_Arguments_PrintTheMatrixOrRefuse(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFiles(t, root, map[string]string{ + "a/a_test.go": "package a\n\nimport \"testing\"\n\nfunc FuzzOne(f *testing.F) {}\n\nfunc FuzzTwo(f *testing.F) {}\n", + "b/b_test.go": "package b\n\nimport \"testing\"\n\nfunc FuzzThree(f *testing.F) {}\n", + "a-b/ab_test.go": "package ab\n\nimport \"testing\"\n\nfunc FuzzOne(f *testing.F) {}\n", + "a/b/b_test.go": "package b\n\nimport \"testing\"\n\nfunc FuzzOne(f *testing.F) {}\n", + "none/n_test.go": "package none\n\nimport \"testing\"\n\nfunc TestN(t *testing.T) {}\n", + }) + const module = "example.com/m" + found := row(module+"/a", module, filepath.Join(root, "a"), "a_test.go", "") + + row(module+"/b", module, filepath.Join(root, "b"), "", "b_test.go") + cases := []struct { + name string + args []string + listing string + err error + wantCode int + wantStdout string + wantStderr string + wantDir string + }{ + {name: "an unknown flag", args: []string{"-packages", "x"}, wantCode: 2, wantStderr: "flag provided but not defined: -packages\n"}, + {name: "an argument", args: []string{"extra"}, wantCode: 2, wantStderr: "fuzz_targets: takes no arguments, and was given [\"extra\"]\n"}, + { + name: "a listing that failed", err: errors.New("planted"), wantCode: 1, wantDir: ".", + wantStderr: "fuzz_targets: go list: planted\n", + }, + { + name: "no target", args: []string{"-dir", "elsewhere"}, wantDir: "elsewhere", wantCode: 1, + listing: row(module+"/none", module, filepath.Join(root, "none"), "n_test.go", ""), + wantStderr: "fuzz_targets: no fuzz target found in elsewhere, so the workflow would fuzz nothing and pass\n", + }, + { + name: "two targets on one artifact", wantDir: ".", wantCode: 1, + listing: row(module+"/a-b", module, filepath.Join(root, "a-b"), "ab_test.go", "") + row(module+"/a/b", module, filepath.Join(root, "a", "b"), "b_test.go", ""), + wantStderr: "fuzz_targets: FuzzOne in ./a-b and FuzzOne in ./a/b would both upload as fuzz-a-b-FuzzOne\n", + }, + { + name: "targets", wantDir: ".", listing: found, + wantStdout: `matrix={"include":[` + + `{"package":"./a","target":"FuzzOne","artifact":"fuzz-a-FuzzOne"},` + + `{"package":"./a","target":"FuzzTwo","artifact":"fuzz-a-FuzzTwo"},` + + `{"package":"./b","target":"FuzzThree","artifact":"fuzz-b-FuzzThree"}]}` + "\n", + wantStderr: "fuzz_targets: ./a FuzzOne\nfuzz_targets: ./a FuzzTwo\nfuzz_targets: ./b FuzzThree\n" + + "fuzz_targets: 3 fuzz target(s) in 2 package(s)\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var calls []listCall + var stdout, stderr bytes.Buffer + if got := runMain(tc.args, &stdout, &stderr, fakeLister(tc.listing, tc.err, &calls)); got != tc.wantCode { + t.Fatalf("runMain(%q) = %d, want %d; stderr:\n%s", tc.args, got, tc.wantCode, stderr.String()) + } + if stdout.String() != tc.wantStdout { + t.Errorf("stdout = %q, want %q", stdout.String(), tc.wantStdout) + } + if !strings.HasPrefix(stderr.String(), tc.wantStderr) { + t.Errorf("stderr = %q, want it to start %q", stderr.String(), tc.wantStderr) + } + if tc.wantDir == "" && len(calls) != 0 || tc.wantDir != "" && (len(calls) != 1 || calls[0].dir != tc.wantDir) { + t.Errorf("lister calls = %+v, want one in %q", calls, tc.wantDir) + } + }) + } +} + +// TestMain_UnknownFlag_ExitsWithTheUsageStatus verifies that main hands the +// process arguments after its own name to runMain and passes the status on to +// the exit seam, without listing anything for a usage error. +func TestMain_UnknownFlag_ExitsWithTheUsageStatus(t *testing.T) { + previousArgs, previousExit := os.Args, osExit + t.Cleanup(func() { os.Args, osExit = previousArgs, previousExit }) + got := -1 + os.Args = []string{"fuzz_targets", "-no-such-flag"} + osExit = func(code int) { got = code } + main() + if got != 2 { + t.Errorf("main() exit = %d, want 2", got) + } +} + +// workflowStep is one step of a workflow job, as the contract below reads it. +type workflowStep struct { + Name string `yaml:"name"` + ID string `yaml:"id"` + If string `yaml:"if"` + Run string `yaml:"run"` + Env map[string]string `yaml:"env"` + With map[string]any `yaml:"with"` +} + +// fuzzWorkflow is .github/workflows/fuzz.yml as the contract below reads it, +// with every named step keyed by its job and its name. +type fuzzWorkflow struct { + On map[string]any `yaml:"on"` + Permissions map[string]string `yaml:"permissions"` + Jobs map[string]struct { + Permissions any `yaml:"permissions"` + Needs any `yaml:"needs"` + Outputs map[string]string `yaml:"outputs"` + Strategy struct { + FailFast *bool `yaml:"fail-fast"` + Matrix any `yaml:"matrix"` + } `yaml:"strategy"` + Steps []workflowStep `yaml:"steps"` + } `yaml:"jobs"` + steps map[string]workflowStep +} + +// loadFuzzWorkflow reads and parses the workflow this command plans. +func loadFuzzWorkflow(t *testing.T) fuzzWorkflow { + t.Helper() + data, err := os.ReadFile(filepath.Join("..", "..", ".github", "workflows", "fuzz.yml")) + if err != nil { + t.Fatalf("read the workflow: %v", err) + } + var workflow fuzzWorkflow + if err = yaml.Unmarshal(data, &workflow); err != nil { + t.Fatalf("parse the workflow: %v", err) + } + workflow.steps = map[string]workflowStep{} + for name, job := range workflow.Jobs { + for _, step := range job.Steps { + workflow.steps[name+"/"+step.Name] = step + } + } + return workflow +} + +// TestFuzzWorkflow_Triggers_AreTheScheduleADispatchAndItsOwnChanges holds the +// workflow to when it runs: on a weekly schedule, when dispatched, and on a +// pull request that changes the workflow or this command and nothing else, +// since GitHub offers a workflow for dispatch only once it is on the default +// branch and the change would otherwise merge unrun. The token reads and +// nothing more, and no job raises it for itself. +func TestFuzzWorkflow_Triggers_AreTheScheduleADispatchAndItsOwnChanges(t *testing.T) { + t.Parallel() + workflow := loadFuzzWorkflow(t) + triggers := slices.Sorted(maps.Keys(workflow.On)) + if !reflect.DeepEqual(triggers, []string{"pull_request", "schedule", "workflow_dispatch"}) { + t.Errorf("on = %v, want pull_request, schedule and workflow_dispatch", triggers) + } + pullRequest, _ := workflow.On["pull_request"].(map[string]any) + if len(pullRequest) != 1 || !reflect.DeepEqual(pullRequest["paths"], []any{".github/workflows/fuzz.yml", "cmd/fuzz_targets/**"}) { + t.Errorf("on.pull_request = %v, want paths naming the workflow and this command alone", workflow.On["pull_request"]) + } + if !reflect.DeepEqual(workflow.Permissions, map[string]string{"contents": "read"}) { + t.Errorf("permissions = %v, want contents: read alone", workflow.Permissions) + } + if len(workflow.Jobs) != 2 { + t.Errorf("jobs = %d, want plan and fuzz", len(workflow.Jobs)) + } + for name, job := range workflow.Jobs { + t.Run(name, func(t *testing.T) { + t.Parallel() + if job.Permissions != nil { + t.Errorf("job %s sets permissions of its own: %v", name, job.Permissions) + } + }) + } +} + +// TestFuzzWorkflow_Jobs_RunThisCommandsPlan holds the workflow to the matrix +// this command prints: the plan step writes it into $GITHUB_OUTPUT, the fuzz +// job fans out over it without stopping at the first failure, and every +// matrix field the jobs read is one a cell carries. +func TestFuzzWorkflow_Jobs_RunThisCommandsPlan(t *testing.T) { + t.Parallel() + workflow := loadFuzzWorkflow(t) + plan := workflow.steps["plan/List the fuzz targets"] + if plan.ID != "plan" || !strings.HasPrefix(plan.Run, "go run ./cmd/fuzz_targets ") || !strings.HasSuffix(strings.TrimSpace(plan.Run), `>> "$GITHUB_OUTPUT"`) { + t.Errorf("the plan step has id %q and runs %q", plan.ID, plan.Run) + } + if got := workflow.Jobs["plan"].Outputs["matrix"]; got != "${{ steps.plan.outputs.matrix }}" { + t.Errorf("the plan job's matrix output is %q", got) + } + fuzz := workflow.Jobs["fuzz"] + if fuzz.Needs != "plan" || fuzz.Strategy.Matrix != "${{ fromJSON(needs.plan.outputs.matrix) }}" || fuzz.Strategy.FailFast == nil || *fuzz.Strategy.FailFast { + t.Errorf("the fuzz job needs %v and fans out over %v with fail-fast %v", fuzz.Needs, fuzz.Strategy.Matrix, fuzz.Strategy.FailFast) + } + encoded, err := json.Marshal(cell{}) + if err != nil { + t.Fatal(err) + } + fields := map[string]any{} + if err = json.Unmarshal(encoded, &fields); err != nil { + t.Fatal(err) + } + for _, field := range []string{"package", "target", "artifact"} { + t.Run(field, func(t *testing.T) { + t.Parallel() + if _, carried := fields[field]; !carried { + t.Errorf("a cell carries %v, not matrix.%s", fields, field) + } + }) + } +} + +// TestFuzzWorkflow_FuzzStep_FuzzesTheCellsTargetAndKeepsWhatFailed holds the +// fuzzing to the cell: the one target named, anchored so it matches no other, +// in the one package named, with no other test run beside it, for ten minutes +// on the schedule and a dispatch and one on a pull request; and on a failure +// the directory the go command writes a failing input into is uploaded under +// the cell's artifact name. +func TestFuzzWorkflow_FuzzStep_FuzzesTheCellsTargetAndKeepsWhatFailed(t *testing.T) { + t.Parallel() + workflow := loadFuzzWorkflow(t) + fuzz := workflow.steps["fuzz/Fuzz"] + wantEnv := map[string]string{ + "PACKAGE": "${{ matrix.package }}", + "TARGET": "${{ matrix.target }}", + "FUZZTIME": "${{ github.event_name == 'pull_request' && '1m' || '10m' }}", + } + if !reflect.DeepEqual(fuzz.Env, wantEnv) { + t.Errorf("the fuzz step's env = %v, want %v", fuzz.Env, wantEnv) + } + if want := `go test -run '^$' -fuzz "^${TARGET}\$" -fuzztime "$FUZZTIME" "$PACKAGE"`; strings.TrimSpace(fuzz.Run) != want { + t.Errorf("the fuzz step runs %q, want %q", fuzz.Run, want) + } + upload := workflow.steps["fuzz/Upload the failing input"] + if upload.If != "failure()" || upload.With["name"] != "${{ matrix.artifact }}" || + upload.With["path"] != "${{ matrix.package }}/testdata/fuzz/${{ matrix.target }}" { + t.Errorf("the upload step runs if %q and takes %v", upload.If, upload.With) + } +} diff --git a/cmd/fuzz_targets/targets.go b/cmd/fuzz_targets/targets.go new file mode 100644 index 000000000..ca60b0dc8 --- /dev/null +++ b/cmd/fuzz_targets/targets.go @@ -0,0 +1,156 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "go/ast" + "go/parser" + "go/token" + "os/exec" + "path/filepath" + "slices" + "strings" + "unicode" + "unicode/utf8" + + "github.com/jmrplens/gitlab-mcp-server/v3/cmd/internal/golist" +) + +// lister runs the go command in dir and returns its standard output. It is a +// parameter so a test can answer for the toolchain. +type lister func(ctx context.Context, dir string, args ...string) ([]byte, error) + +// goList is the lister production uses: the toolchain's own go binary, never +// one looked up on PATH. +func goList(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, golist.Executable(), args...) //#nosec G204 -- golist.Executable is the toolchain's own go binary, joined out of GOROOT + cmd.Dir = dir + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String())) + } + return out, nil +} + +// listFormat is the row asked of `go list` per package: its import path, its +// module's path, its directory, and the internal and external test files the +// go command builds for it on this platform, which are the only files a fuzz +// target it would run can be in. +const listFormat = `{{.ImportPath}} {{with .Module}}{{.Path}}{{end}} {{.Dir}} {{join .TestGoFiles " "}} {{join .XTestGoFiles " "}}` + +// target is one fuzz target: its package, written ./ and its directory below +// the module root as go test takes it, and its function's name. +type target struct { + pkg string + name string +} + +// cell is one run the matrix asks for: one target of one package, and the +// name of the artifact a failing input of it is uploaded under. +type cell struct { + Package string `json:"package"` + Target string `json:"target"` + Artifact string `json:"artifact"` +} + +// matrix is the strategy matrix the workflow fans out over, in the shape +// `strategy.matrix` reads: every cell is an include entry. +type matrix struct { + Include []cell `json:"include"` +} + +// isFuzzName reports whether the go command reads a top-level function of a +// test file by this name as a fuzz target: Fuzz alone, or Fuzz followed by a +// character that is not a lower-case letter (the rule of isTest in +// cmd/go/internal/load), so FuzzMeterFor is one and Fuzzy is not. +func isFuzzName(name string) bool { + rest, prefixed := strings.CutPrefix(name, "Fuzz") + if !prefixed { + return false + } + if rest == "" { + return true + } + next, _ := utf8.DecodeRuneInString(rest) + return !unicode.IsLower(next) +} + +// findTargets returns every fuzz target of the module in dir, package by +// package in the order the listing gives them and by name within a package. +// +// The listing is not asked for errors: a module that does not load cannot be +// fuzzed, and the go command fails the listing for it with its own words. +func findTargets(ctx context.Context, dir string, list lister) ([]target, error) { + out, err := list(ctx, dir, "list", "-f", listFormat, "./...") + if err != nil { + return nil, fmt.Errorf("go list: %w", err) + } + var found []target + for line := range strings.SplitSeq(string(out), "\n") { + line = strings.TrimSuffix(line, "\r") + if line == "" { + continue + } + fields := strings.Split(line, "\t") + if len(fields) != 5 { + return nil, fmt.Errorf("unexpected go list row %q", line) + } + names, unparsable := fuzzNames(fields[2], strings.Fields(fields[3]+" "+fields[4])) + if unparsable != nil { + return nil, unparsable + } + if len(names) == 0 { + continue + } + rel, below := strings.CutPrefix(fields[0], fields[1]+"/") + if !below { + return nil, fmt.Errorf("go list named %q, which is not below its module path %q", fields[0], fields[1]) + } + for _, name := range names { + found = append(found, target{pkg: "./" + rel, name: name}) + } + } + return found, nil +} + +// fuzzNames parses each named test file of the package in dir and returns the +// names of its fuzz targets, sorted. +func fuzzNames(dir string, files []string) ([]string, error) { + fset := token.NewFileSet() + var names []string + for _, file := range files { + parsed, err := parser.ParseFile(fset, filepath.Join(dir, file), nil, parser.SkipObjectResolution) + if err != nil { + return nil, fmt.Errorf("a test file the go command builds cannot be parsed: %w", err) + } + for _, decl := range parsed.Decls { + if fn, isFunc := decl.(*ast.FuncDecl); isFunc && fn.Recv == nil && isFuzzName(fn.Name.Name) { + names = append(names, fn.Name.Name) + } + } + } + slices.Sort(names) + return names, nil +} + +// buildMatrix makes a cell of each target, in the order given, and names its +// artifact after its package and its name. Two targets whose artifact names +// would be the same, which only directory names joined differently by a dash +// could make, are refused rather than left to overwrite each other's upload. +func buildMatrix(targets []target) (matrix, error) { + planned := matrix{Include: make([]cell, 0, len(targets))} + byArtifact := map[string]cell{} + for _, each := range targets { + artifact := "fuzz-" + strings.ReplaceAll(strings.TrimPrefix(each.pkg, "./"), "/", "-") + "-" + each.name + if other, taken := byArtifact[artifact]; taken { + return matrix{}, fmt.Errorf("%s in %s and %s in %s would both upload as %s", other.Target, other.Package, each.name, each.pkg, artifact) + } + entry := cell{Package: each.pkg, Target: each.name, Artifact: artifact} + byArtifact[artifact] = entry + planned.Include = append(planned.Include, entry) + } + return planned, nil +} diff --git a/cmd/fuzz_targets/targets_test.go b/cmd/fuzz_targets/targets_test.go new file mode 100644 index 000000000..33a080a80 --- /dev/null +++ b/cmd/fuzz_targets/targets_test.go @@ -0,0 +1,280 @@ +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// listCall is one call a fake lister received. +type listCall struct { + dir string + args []string +} + +// fakeLister answers every call with listing, or with err when it is set, +// and records each call it receives. +func fakeLister(listing string, err error, calls *[]listCall) lister { + return func(_ context.Context, dir string, args ...string) ([]byte, error) { + *calls = append(*calls, listCall{dir: dir, args: args}) + if err != nil { + return nil, err + } + return []byte(listing), nil + } +} + +// row writes one listing row the way listFormat does. +func row(path, module, dir, testFiles, xtestFiles string) string { + return strings.Join([]string{path, module, dir, testFiles, xtestFiles}, "\t") + "\n" +} + +// writeFiles writes each named file below dir with its content. +func writeFiles(t *testing.T, dir string, files map[string]string) { + t.Helper() + for name, content := range files { + path := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } +} + +// TestIsFuzzName_Names_FollowTheGoCommandsRule verifies the rule the go +// command reads a fuzz target's name by: Fuzz alone, or Fuzz followed by any +// character that is not a lower-case letter, in or out of ASCII. +func TestIsFuzzName_Names_FollowTheGoCommandsRule(t *testing.T) { + t.Parallel() + cases := []struct { + name string + want bool + }{ + {name: "Fuzz", want: true}, + {name: "FuzzMeterFor", want: true}, + {name: "Fuzz_lower", want: true}, + {name: "Fuzz1", want: true}, + {name: "FuzzÉcole", want: true}, + {name: "Fuzzy", want: false}, + {name: "Fuzzé", want: false}, + {name: "fuzzMeter", want: false}, + {name: "TestFuzzMeter", want: false}, + {name: "Fuz", want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := isFuzzName(tc.name); got != tc.want { + t.Errorf("isFuzzName(%q) = %t, want %t", tc.name, got, tc.want) + } + }) + } +} + +// TestFindTargets_Listing_ReadsEveryTestFileTheGoCommandBuilds verifies the +// discovery over a listing of three packages: the fuzz targets of a package's +// internal and external test files are found, sorted by name within it, and +// only top-level functions named as targets count, so a method, a Fuzzy, a +// test and a variable named like a target do not. A file in the directory the +// listing does not name is not read, which is how a file behind a build tag +// stays out, and a package without a target is passed over, the module root +// included. The listing is asked for every package of the module, in the +// directory given, with the row format the reader expects. +func TestFindTargets_Listing_ReadsEveryTestFileTheGoCommandBuilds(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFiles(t, root, map[string]string{ + "a/a_test.go": `package a + +import "testing" + +type suite struct{} + +func (suite) FuzzMethod(f *testing.F) {} + +func FuzzZeta(f *testing.F) {} + +func Fuzzy(f *testing.F) {} + +func TestFuzzLike(t *testing.T) {} + +var FuzzVariable = 1 + +func FuzzAlpha(f *testing.F) {} +`, + "a/a_ext_test.go": `package a_test + +import "testing" + +func FuzzMiddle(f *testing.F) {} +`, + "a/tagged_test.go": `//go:build never + +package a + +import "testing" + +func FuzzNever(f *testing.F) {} +`, + "b/b_test.go": `package b + +import "testing" + +func TestB(t *testing.T) {} +`, + "root_test.go": `package m + +import "testing" + +func TestRoot(t *testing.T) {} +`, + }) + const module = "example.com/m" + listing := row(module, module, root, "root_test.go", "") + + "\r\n" + + row(module+"/a", module, filepath.Join(root, "a"), "a_test.go", "a_ext_test.go") + + row(module+"/b", module, filepath.Join(root, "b"), "b_test.go", "") + + row(module+"/c", module, filepath.Join(root, "c"), "", "") + var calls []listCall + got, err := findTargets(context.Background(), "the-module", fakeLister(listing, nil, &calls)) + if err != nil { + t.Fatalf("findTargets() error = %v", err) + } + want := []target{ + {pkg: "./a", name: "FuzzAlpha"}, + {pkg: "./a", name: "FuzzMiddle"}, + {pkg: "./a", name: "FuzzZeta"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("findTargets() = %+v, want %+v", got, want) + } + wantCalls := []listCall{{dir: "the-module", args: []string{"list", "-f", listFormat, "./..."}}} + if !reflect.DeepEqual(calls, wantCalls) { + t.Errorf("lister calls = %+v, want %+v", calls, wantCalls) + } +} + +// TestFindTargets_Failures_SayWhatIsWrong verifies every way the discovery +// refuses rather than plans: a listing that failed, a row of another shape, a +// test file that does not parse or cannot be read, and a package holding a +// target that is not below its module's path, which no ./ name could reach. +func TestFindTargets_Failures_SayWhatIsWrong(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFiles(t, root, map[string]string{ + "broken_test.go": "package broken\n\nfunc FuzzBroken(f *testing.F) {\n", + "fuzz_test.go": "package m\n\nimport \"testing\"\n\nfunc FuzzRoot(f *testing.F) {}\n", + }) + const module = "example.com/m" + cases := []struct { + name string + listing string + err error + want string + }{ + {name: "the listing failed", err: errors.New("planted"), want: "go list: planted"}, + {name: "a row of four fields", listing: "a\tb\tc\td\n", want: `unexpected go list row "a\tb\tc\td"`}, + {name: "a row of six fields", listing: "a\tb\tc\td\te\tf\n", want: `unexpected go list row "a\tb\tc\td\te\tf"`}, + { + name: "a file that does not parse", listing: row(module+"/broken", module, root, "broken_test.go", ""), + want: "a test file the go command builds cannot be parsed: " + filepath.Join(root, "broken_test.go"), + }, + { + name: "a file that is not there", listing: row(module+"/gone", module, root, "", "gone_test.go"), + want: "a test file the go command builds cannot be parsed: open " + filepath.Join(root, "gone_test.go"), + }, + { + name: "a target at the module root", listing: row(module, module, root, "fuzz_test.go", ""), + want: `go list named "example.com/m", which is not below its module path "example.com/m"`, + }, + { + name: "a target in another module", listing: row("example.com/m2/x", module, root, "fuzz_test.go", ""), + want: `go list named "example.com/m2/x", which is not below its module path "example.com/m"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var calls []listCall + got, err := findTargets(context.Background(), ".", fakeLister(tc.listing, tc.err, &calls)) + if err == nil || !strings.HasPrefix(err.Error(), tc.want) || got != nil { + t.Errorf("findTargets() = %+v, %v; want nothing and an error starting %q", got, err, tc.want) + } + }) + } +} + +// TestFindTargets_Repository_FindsItsTargetsWithTheRealToolchain runs the +// discovery over this repository with the go command itself, and checks it +// finds a target in each of the packages that hold one today, among them +// cmd/server, whose files differ between platforms, and internal/tenancy, +// whose targets span three files and keep a committed corpus. +func TestFindTargets_Repository_FindsItsTargetsWithTheRealToolchain(t *testing.T) { + t.Parallel() + got, err := findTargets(context.Background(), filepath.Join("..", ".."), goList) + if err != nil { + t.Fatalf("findTargets() error = %v", err) + } + for _, known := range []target{ + {pkg: "./cmd/server", name: "FuzzRefuseUnreadable"}, + {pkg: "./internal/finegrained", name: "FuzzDecodeGrant"}, + {pkg: "./internal/tenancy", name: "FuzzBudgetOn"}, + {pkg: "./internal/tenancy", name: "FuzzMeterFor"}, + {pkg: "./internal/tools/search", name: "FuzzFormatCodeMarkdown"}, + {pkg: "./internal/toolutil", name: "FuzzScanLogMarkup_RawOffset"}, + } { + t.Run(known.pkg+" "+known.name, func(t *testing.T) { + t.Parallel() + found := false + for _, each := range got { + found = found || each == known + } + if !found { + t.Errorf("findTargets() = %+v, which does not hold %+v", got, known) + } + }) + } +} + +// TestGoList_MissingPackage_ReportsTheToolchainsWords verifies that a go +// command that failed is returned as an error carrying what it wrote to +// stderr, so the plan says why it could not list rather than only that. +func TestGoList_MissingPackage_ReportsTheToolchainsWords(t *testing.T) { + t.Parallel() + _, err := goList(context.Background(), ".", "list", "./nosuchpackagehere") + if err == nil || !strings.Contains(err.Error(), "nosuchpackagehere") { + t.Errorf("goList() error = %v, want one naming the missing package", err) + } +} + +// TestBuildMatrix_Targets_AreNamedOrRefusedOnACollision verifies that each +// target becomes one cell whose artifact is named after its package and its +// name, in the order given, and that two targets whose artifact names would +// be the same are refused rather than left to overwrite each other's upload. +func TestBuildMatrix_Targets_AreNamedOrRefusedOnACollision(t *testing.T) { + t.Parallel() + got, err := buildMatrix([]target{ + {pkg: "./internal/tenancy", name: "FuzzBusy"}, + {pkg: "./cmd/server", name: "FuzzRefuseUnreadable"}, + }) + if err != nil { + t.Fatalf("buildMatrix() error = %v", err) + } + want := matrix{Include: []cell{ + {Package: "./internal/tenancy", Target: "FuzzBusy", Artifact: "fuzz-internal-tenancy-FuzzBusy"}, + {Package: "./cmd/server", Target: "FuzzRefuseUnreadable", Artifact: "fuzz-cmd-server-FuzzRefuseUnreadable"}, + }} + if !reflect.DeepEqual(got, want) { + t.Errorf("buildMatrix() = %+v, want %+v", got, want) + } + _, err = buildMatrix([]target{{pkg: "./a-b", name: "FuzzX"}, {pkg: "./a/b", name: "FuzzX"}}) + if wantErr := "FuzzX in ./a-b and FuzzX in ./a/b would both upload as fuzz-a-b-FuzzX"; err == nil || err.Error() != wantErr { + t.Errorf("buildMatrix() error = %v, want %q", err, wantErr) + } +} diff --git a/cmd/measure_conditions/discover.go b/cmd/measure_conditions/discover.go index 638684ce0..940e1a2b8 100644 --- a/cmd/measure_conditions/discover.go +++ b/cmd/measure_conditions/discover.go @@ -101,6 +101,31 @@ func constrainedPackages(ctx context.Context, dir string, list lister) ([]string return dirs, nil } +// treePackages returns, as ./-relative package paths in the order the go +// command lists them, every package treePatterns names, listed under the +// platform the command runs on. The listing is not asked for errors: a tree +// that does not load cannot be measured, and the go command fails the +// listing for it with its own words. +func treePackages(ctx context.Context, dir string, list lister) ([]string, error) { + out, err := list(ctx, dir, nil, append([]string{"list", "-f", listFormat}, treePatterns...)...) + if err != nil { + return nil, fmt.Errorf("go list: %w", err) + } + rows, err := parseListing(out) + if err != nil { + return nil, fmt.Errorf("go list: %w", err) + } + packages := make([]string, 0, len(rows)) + for _, row := range rows { + packageDir, outside := relativePackage(row) + if outside != nil { + return nil, outside + } + packages = append(packages, packageDir) + } + return packages, nil +} + // parseListing reads the rows a `go list -f listFormat` run printed. A blank // line is skipped, and anything that is not exactly six tab-separated fields // is refused rather than guessed at. diff --git a/cmd/measure_conditions/discover_test.go b/cmd/measure_conditions/discover_test.go index 789f4e456..fd7cbef40 100644 --- a/cmd/measure_conditions/discover_test.go +++ b/cmd/measure_conditions/discover_test.go @@ -193,6 +193,40 @@ func TestGoList_MissingDirectory_ReportsTheToolchainsWords(t *testing.T) { } } +// TestTreePackages_Listings_AreTheModulesPackagesOrRefused verifies the +// tree's listing: every package it names, written ./ and its directory in +// the order the go command listed them, and a refusal, naming the listing, +// for a listing that failed, a row of another shape and a package that is not +// below its module. +func TestTreePackages_Listings_AreTheModulesPackagesOrRefused(t *testing.T) { + t.Parallel() + const module = "example.com/m" + got, err := treePackages(context.Background(), ".", treeLister(row(module+"/internal/z", module, "z.go", "", "", "")+row(module+"/cmd/a", module, "a.go", "", "", ""), nil)) + if want := []string{"./internal/z", "./cmd/a"}; err != nil || !reflect.DeepEqual(got, want) { + t.Errorf("treePackages() = %q, %v; want %q", got, err, want) + } + cases := []struct { + name string + list lister + wantErr string + }{ + {name: "a listing that failed", list: treeLister("", errors.New("planted failure")), wantErr: "go list: planted failure"}, + {name: "a row of another shape", list: treeLister("a\tb\n", nil), wantErr: `go list: unexpected go list row "a\tb"`}, + { + name: "a package outside its module", list: treeLister(row("example.com/other/cmd", module, "c.go", "", "", ""), nil), + wantErr: `go list named "example.com/other/cmd", which is not below its module path "example.com/m"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if refused, refusal := treePackages(context.Background(), ".", tc.list); refusal == nil || refusal.Error() != tc.wantErr || refused != nil { + t.Errorf("treePackages() = %q, %v; want nothing and %q", refused, refusal, tc.wantErr) + } + }) + } +} + // TestParseListing_Rows_AreReadOrRefused verifies the listing reader: blank // lines and carriage returns are dropped, the four file lists are kept as one // signature, and a row with another number of fields is refused. diff --git a/cmd/measure_conditions/doc.go b/cmd/measure_conditions/doc.go index de510e893..0f6a06dcb 100644 --- a/cmd/measure_conditions/doc.go +++ b/cmd/measure_conditions/doc.go @@ -1,6 +1,7 @@ -// Command measure_conditions plans and reports the condition coverage +// Command measure_conditions plans, runs and reports the condition coverage // measurement that .github/workflows/conditions.yml runs on demand (issue -// 1210). +// 1210), and takes the same measurement of the whole tree on the machine at +// hand. // // The condition gate is gobco, run through scripts/coverage-conditions.sh, // which is the recipe behind `make coverage-conditions`. It runs on Linux @@ -8,38 +9,69 @@ // those systems alone build. A figure for a whole package on Windows or macOS // had no way to be taken except by hand on a machine of that system, so the // figures quoted for internal/toolutil in pull request 1206 came from a -// Windows virtual machine and nobody could repeat them on macOS at all. The -// workflow runs the same script on each runner a dispatch names, and this -// command is the two ends of it that are not that script: +// Windows virtual machine and nobody could repeat them on macOS at all. Nor +// was there a figure for the tree as a whole, which is what the OpenSSF Best +// Practices criterion test_branch_coverage80 asks for. The workflow runs the +// same script on each runner a dispatch names, and this command is everything +// around that script: // -// measure_conditions plan -packages -systems -gate -// measure_conditions summarize -plan -records -gate -commit +// measure_conditions plan -packages -systems -gate [-mode ] [-tree] +// measure_conditions measure -packages -system -records +// measure_conditions summarize -plan -records -gate [-mode ] -commit +// measure_conditions tree -records -commit [-gate ] [-mode ] [-parallel ] +// +// The mode is the GOBCO_MODE the script runs gobco in, condition when it is +// not given: condition is gobco's default, which counts every atomic +// condition, each operand of && and || and each comparison wherever it is +// written, and branch is gobco's -branch, which counts the whole controlling +// condition of each if and for and each case of a switch, the branches +// test_branch_coverage80 asks about. measure takes none, since the script +// reads the mode from the environment the job gives it. // // plan validates what the dispatch asked for and prints the matrix the -// workflow fans out over, as one `matrix=` line for $GITHUB_OUTPUT. An -// empty package list means every package carrying a GOOS- or GOARCH-constrained -// file, found the way cmd/audit_dead_consts finds them: the files the go -// command builds for each package are listed under every platform the release -// builds (.goreleaser.yml's goos crossed with its goarch), and a package whose -// files differ between two of them is one. A file behind a tag alone, such as -// the race detector's or the e2e suite's, builds on no release target without +// workflow fans out over, as one `matrix=` line for $GITHUB_OUTPUT: a +// job per package and runner, each named after its package. An empty package +// list means every package carrying a GOOS- or GOARCH-constrained file, found +// the way cmd/audit_dead_consts finds them: the files the go command builds for +// each package are listed under every platform the release builds +// (.goreleaser.yml's goos crossed with its goarch), and a package whose files +// differ between two of them is one. A file behind a tag alone, such as the +// race detector's or the e2e suite's, builds on no release target without // that tag and so differs on none of them, which keeps it out: the script is -// run without tags here, and could not measure such a package anyway. +// run without tags here, and could not measure such a package anyway. With +// -tree the plan is every package below cmd/ and internal/ instead, dealt +// round twenty shards so the slow packages of one directory land in +// different jobs, each shard a job on each runner. +// +// measure is what each job runs: the script over each of its packages, one +// after another, with the output streamed to the log and kept, for each +// package, in a directory of the records named after it, beside the record of +// what ran where and how it ended. A package the script could not measure +// fails the job once its other packages are measured. +// +// summarize reads the records the jobs left, joins them to the plan, and +// writes the job summary: one row per package and runner with the figure +// gobco printed, the total of each runner over the packages it measured, and +// one row per condition gobco names as not evaluated both ways, with its file +// and line. Every figure is the one the script printed on that runner. Nothing +// is inferred from another platform, and nothing is computed that the script +// did not count but the totals, which add up its counts and round their +// share down. // -// summarize reads the record each run left beside gobco's raw output, joins it -// to the plan, and writes the job summary: one row per package and runner with -// the figure gobco printed, and one row per condition gobco names as not -// evaluated both ways, with its file and line. Every figure is the one the -// script printed on that runner. Nothing is inferred from another platform, -// and nothing is computed that the script did not count. +// tree is the whole of a -tree dispatch on the machine at hand, which is how +// the figure docs/development/testing/README.md records is taken again +// without a dispatch (`make coverage-conditions-tree`): it lists the tree, +// measures every package, -parallel at a time, into -records, and writes the +// summary to stdout, its system named local. // // It is a measurement, not a gate, and no figure fails it. The script's exit // status is read for one thing only, whether a measurement was taken: 0 when // it was and the gate held nothing, 3 when it was and the gate (GOBCO_GATE) // held at least one condition, and anything else when the script could not -// measure at all. A run in the third case has already failed its own job with -// the reason, and is reported here as not measured with the lines the script -// said it in. summarize itself exits 1 only when a record cannot be read or -// disagrees with itself, since then the table would be a guess, and 2 on a -// usage error. +// measure at all. A run in the third case is reported as not measured with the +// lines of its output that say why, the script's and those gobco or its go +// test printed before them, and fails the job that ran it. summarize and +// tree exit 1 only when a record cannot be read or disagrees with itself, or +// the packages cannot be listed or measured into the records, since then the +// table would be a guess, and every subcommand exits 2 on a usage error. package main diff --git a/cmd/measure_conditions/main.go b/cmd/measure_conditions/main.go index a631e9509..993962ac6 100644 --- a/cmd/measure_conditions/main.go +++ b/cmd/measure_conditions/main.go @@ -10,9 +10,11 @@ import ( const toolName = "measure_conditions" // usage is printed when the subcommand is missing or unknown. -const usage = `usage: measure_conditions [flags] +const usage = `usage: measure_conditions [flags] plan validate a dispatch and print the matrix the workflow runs + measure measure the packages of one job and leave their records summarize write the job summary from the records the runs left + tree measure every package below cmd/ and internal/ here, and summarize ` // osExit is a seam over os.Exit, so a test can observe the exit code main @@ -33,8 +35,12 @@ func runMain(args []string, stdout, stderr io.Writer) int { switch args[0] { case "plan": return runPlan(args[1:], stdout, stderr, goList) + case "measure": + return runMeasure(args[1:], stdout, stderr, runScript) case "summarize": return runSummarize(args[1:], stdout, stderr) + case "tree": + return runTree(args[1:], stdout, stderr, goList, runScript) default: fmt.Fprintf(stderr, "%s: unknown subcommand %q\n%s", toolName, args[0], usage) return 2 diff --git a/cmd/measure_conditions/main_test.go b/cmd/measure_conditions/main_test.go index 4f765c4b4..f08a11912 100644 --- a/cmd/measure_conditions/main_test.go +++ b/cmd/measure_conditions/main_test.go @@ -10,7 +10,7 @@ import ( // TestRunMain_Subcommands_DispatchOrRefuse verifies the dispatch: no // subcommand and an unknown one are usage errors that print the usage, and // each known subcommand is reached with the arguments after its name, which -// the plan's matrix and summarize's own usage error show. +// the plan's matrix and the other three's own usage errors show. func TestRunMain_Subcommands_DispatchOrRefuse(t *testing.T) { t.Parallel() cases := []struct { @@ -22,18 +22,26 @@ func TestRunMain_Subcommands_DispatchOrRefuse(t *testing.T) { }{ {name: "no subcommand", args: nil, wantCode: 2, wantStderr: usage}, { - name: "unknown subcommand", args: []string{"measure"}, wantCode: 2, - wantStderr: "measure_conditions: unknown subcommand \"measure\"\n" + usage, + name: "unknown subcommand", args: []string{"report"}, wantCode: 2, + wantStderr: "measure_conditions: unknown subcommand \"report\"\n" + usage, }, { name: "plan", args: []string{"plan", "-packages", "./cmd/server", "-systems", "macos-latest", "-gate", "all"}, - wantStdout: `matrix={"include":[{"system":"macos-latest","package":"./cmd/server","artifact":"gobco-macos-latest-cmd-server"}]}` + "\n", - wantStderr: "measure_conditions: 1 run(s) planned, ./cmd/server on macos-latest, with GOBCO_GATE=all\n", + wantStdout: `matrix={"include":[{"system":"macos-latest","name":"./cmd/server","packages":"./cmd/server","artifact":"gobco-macos-latest-cmd-server"}]}` + "\n", + wantStderr: "measure_conditions: 1 run(s) planned, ./cmd/server on macos-latest, with GOBCO_GATE=all and GOBCO_MODE=condition\n", + }, + { + name: "measure", args: []string{"measure"}, wantCode: 2, + wantStderr: "measure_conditions: measure takes -packages, -system and -records, all three non-empty, and no arguments\n", }, { name: "summarize", args: []string{"summarize"}, wantCode: 2, wantStderr: "measure_conditions: summarize takes -plan, -records, -gate and -commit, all four non-empty, and no arguments\n", }, + { + name: "tree", args: []string{"tree"}, wantCode: 2, + wantStderr: "measure_conditions: tree takes -records and -commit, both non-empty, a -parallel of at least 1, and no arguments\n", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -67,11 +75,11 @@ func TestMain_NoSubcommand_ExitsWithTheUsageStatus(t *testing.T) { } } -// TestUsage_NamesEverySubcommand verifies the usage text names both -// subcommands the dispatch accepts. +// TestUsage_NamesEverySubcommand verifies the usage text names every +// subcommand the dispatch accepts. func TestUsage_NamesEverySubcommand(t *testing.T) { t.Parallel() - for _, subcommand := range []string{"plan", "summarize"} { + for _, subcommand := range []string{"plan", "measure", "summarize", "tree"} { t.Run(subcommand, func(t *testing.T) { t.Parallel() if !strings.Contains(usage, " "+subcommand+" ") { diff --git a/cmd/measure_conditions/measure.go b/cmd/measure_conditions/measure.go new file mode 100644 index 000000000..0d2d3fefb --- /dev/null +++ b/cmd/measure_conditions/measure.go @@ -0,0 +1,154 @@ +package main + +import ( + "bytes" + "context" + "errors" + "flag" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" +) + +// coverageScript is the recipe every measurement runs, from the module root: +// the one behind `make coverage-conditions`, which ci.yml's legs run too. +const coverageScript = "scripts/coverage-conditions.sh" + +// scriptRunner runs coverageScript over one package in the module root dir, +// with env added to the process environment and both of its streams written +// to output, and returns its exit status. An error means it could not be run +// at all. It is a parameter so a test can answer for the script. +type scriptRunner func(ctx context.Context, dir, pkg string, env []string, output io.Writer) (int, error) + +// runScript is the scriptRunner production uses: the script through bash, as +// ci.yml's legs and `make coverage-conditions` run it. Both streams go to one +// writer, so the output reads in the order the script printed it, as `2>&1` +// would have it. +// +// bash is resolved to an absolute path first, which is how every other +// command here runs a tool it did not build (cmd/gen_orbit_record): the +// lookup is the one exec would do, decided here rather than by whatever PATH +// holds when the command starts. On a Windows runner it finds the Git Bash a +// bash step runs in. +func runScript(ctx context.Context, dir, pkg string, env []string, output io.Writer) (int, error) { + bin, err := exec.LookPath("bash") + if err != nil { + return 0, fmt.Errorf("find bash: %w", err) + } + // #nosec G204 -- the program is the absolute path just resolved, with the repository's own script and a package parsePackages or the go command named + cmd := exec.CommandContext(ctx, bin, coverageScript, pkg) + cmd.Dir = dir + cmd.Env = append(os.Environ(), env...) + cmd.Stdout = output + cmd.Stderr = output + err = cmd.Run() + if exit, exited := errors.AsType[*exec.ExitError](err); exited { + return exit.ExitCode(), nil + } + return 0, err +} + +// measured reports whether a status of the script is a measurement taken: 0 +// when the gate held nothing and 3 when it held a condition. +func measured(status int) bool { + return status == statusMeasured || status == statusGateHeld +} + +// verdict is how a progress line names what became of one package. +func verdict(status int) string { + if measured(status) { + return fmt.Sprintf("measured (exit %d)", status) + } + return fmt.Sprintf("not measured (exit %d)", status) +} + +// measurer is what every package of one run is measured with: the runner of +// the script, the module root it runs in, the system the records name, the +// directory of records they are left below, and what is added to the script's +// environment. +type measurer struct { + run scriptRunner + dir string + system string + records string + env []string +} + +// measurePackage runs the script over one package and leaves, in the +// directory of records named after the package, everything it printed and +// the record of what ran where and how it ended. echo receives a copy of the +// output as it is printed. It returns the script's status, or an error when +// the script could not be run or a file could not be written, in which case +// the package has no record. +func measurePackage(ctx context.Context, m measurer, pkg string, echo io.Writer) (int, error) { + var output bytes.Buffer + status, err := m.run(ctx, m.dir, pkg, m.env, io.MultiWriter(echo, &output)) + if err != nil { + return 0, fmt.Errorf("%s could not be run over %s: %w", coverageScript, pkg, err) + } + text := fmt.Sprintf("package=%s\nsystem=%s\nplatform=%s/%s\nstatus=%d\n", pkg, m.system, runtime.GOOS, runtime.GOARCH, status) + return status, writeRecord(filepath.Join(m.records, packageSlug(pkg)), output.Bytes(), text) +} + +// writeRecord leaves one run in recordDir: the raw output first and the +// record after it, so a directory holding a record always holds the output +// the record describes. +func writeRecord(recordDir string, output []byte, text string) error { + if err := os.MkdirAll(recordDir, 0o750); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(recordDir, outputFile), output, 0o600); err != nil { + return err + } + return os.WriteFile(filepath.Join(recordDir, recordFile), []byte(text), 0o600) +} + +// runMeasure measures the packages -packages names one after another, as a +// job of the workflow does, streaming the script's output to stdout and +// leaving each package's record below -records, where the upload step takes +// them from. It returns 2 for a usage error, 1 when a package could not be +// measured or a record could not be written, and 0 when every package was +// measured, whatever the figures say. +func runMeasure(args []string, stdout, stderr io.Writer, run scriptRunner) int { + flags := flag.NewFlagSet("measure", flag.ContinueOnError) + flags.SetOutput(stderr) + packagesFlag := flags.String("packages", "", "packages to measure one after another, ./cmd/server style, separated by spaces or commas") + system := flags.String("system", "", "the runner the records name") + records := flags.String("records", "", "directory each package's record and raw output are written below, one directory per package") + dir := flags.String("dir", ".", "module root the script runs in") + if err := flags.Parse(args); err != nil { + return 2 + } + packages, err := parsePackages(*packagesFlag) + if err != nil { + fmt.Fprintf(stderr, "%s: -packages: %v\n", toolName, err) + return 2 + } + if flags.NArg() > 0 || len(packages) == 0 || *system == "" || *records == "" { + fmt.Fprintf(stderr, "%s: measure takes -packages, -system and -records, all three non-empty, and no arguments\n", toolName) + return 2 + } + var unmeasured []string + m := measurer{run: run, dir: *dir, system: *system, records: *records} + for _, pkg := range packages { + status, measureErr := measurePackage(context.Background(), m, pkg, stdout) + if measureErr != nil { + fmt.Fprintf(stderr, "%s: %v\n", toolName, measureErr) + return 1 + } + fmt.Fprintf(stderr, "%s: %s on %s: %s\n", toolName, pkg, *system, verdict(status)) + if !measured(status) { + unmeasured = append(unmeasured, pkg) + } + } + if len(unmeasured) > 0 { + fmt.Fprintf(stderr, "%s: %d of %d package(s) could not be measured on %s: %s; the gobco: lines above say why\n", + toolName, len(unmeasured), len(packages), *system, strings.Join(unmeasured, " ")) + return 1 + } + return 0 +} diff --git a/cmd/measure_conditions/measure_test.go b/cmd/measure_conditions/measure_test.go new file mode 100644 index 000000000..5ec65f1ee --- /dev/null +++ b/cmd/measure_conditions/measure_test.go @@ -0,0 +1,310 @@ +package main + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "reflect" + "runtime" + "strconv" + "strings" + "sync" + "testing" +) + +// scriptCall is one call a fake script runner received. +type scriptCall struct { + dir string + pkg string + env []string +} + +// fakeScript answers for coverageScript: each package prints the output and +// exits with the status its entry gives, and a package without one fails to +// run with err. It records every call, and is safe to call from several +// goroutines at once, as the tree's workers do. +type fakeScript struct { + mu sync.Mutex + outputs map[string]string + status map[string]int + err error + calls []scriptCall +} + +// run is the fake's scriptRunner. +func (f *fakeScript) run(_ context.Context, dir, pkg string, env []string, output io.Writer) (int, error) { + f.mu.Lock() + f.calls = append(f.calls, scriptCall{dir: dir, pkg: pkg, env: env}) + f.mu.Unlock() + status, known := f.status[pkg] + if !known { + return 0, f.err + } + _, err := io.WriteString(output, f.outputs[pkg]) + return status, err +} + +// platform is the platform a record written by this test binary names. +var platform = runtime.GOOS + "/" + runtime.GOARCH + +// readFile returns the content of a file the test expects to exist. +func readFile(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(data) +} + +// TestRunMeasure_Usage_IsRefusedWithStatus2 verifies every way the command +// line can be wrong: an unknown flag, an argument, each of the three flags +// left empty, and a package name the plan would refuse too. +func TestRunMeasure_Usage_IsRefusedWithStatus2(t *testing.T) { + t.Parallel() + full := func(overrides ...string) []string { + values := map[string]string{"-packages": "./cmd/server", "-system": "s", "-records": "r"} + for i := 0; i+1 < len(overrides); i += 2 { + values[overrides[i]] = overrides[i+1] + } + return []string{"-packages", values["-packages"], "-system", values["-system"], "-records", values["-records"]} + } + const flagsWanted = "measure_conditions: measure takes -packages, -system and -records, all three non-empty, and no arguments\n" + cases := []struct { + name string + args []string + wantStderr string + }{ + {name: "an unknown flag", args: []string{"-package", "./a"}, wantStderr: "flag provided but not defined: -package\n"}, + {name: "an argument", args: append(full(), "extra"), wantStderr: flagsWanted}, + {name: "no package", args: full("-packages", " "), wantStderr: flagsWanted}, + {name: "no system", args: full("-system", ""), wantStderr: flagsWanted}, + {name: "no records", args: full("-records", ""), wantStderr: flagsWanted}, + { + name: "a package outside the module", args: full("-packages", "../x"), + wantStderr: "measure_conditions: -packages: \"../x\" is not a package directory below the module root, such as ./cmd/server\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + script := &fakeScript{} + var stdout, stderr bytes.Buffer + if got := runMeasure(tc.args, &stdout, &stderr, script.run); got != 2 { + t.Fatalf("runMeasure() = %d, want 2", got) + } + if stdout.Len() != 0 || !strings.HasPrefix(stderr.String(), tc.wantStderr) || len(script.calls) != 0 { + t.Errorf("stdout = %q, stderr = %q, %d script call(s); want nothing, %q and none", stdout.String(), stderr.String(), len(script.calls), tc.wantStderr) + } + }) + } +} + +// TestRunMeasure_Packages_AreMeasuredOneAfterAnother verifies a job's +// measuring end to end: each package is run in the module root -dir names, +// in the order named, with nothing added to the environment the step gives +// it; its output streams to stdout and is kept beside its record, below the +// package's own directory of -records; stderr says what became of each; and +// a package the script could not measure fails the job only after every +// other package has been measured, named with the reason's whereabouts. +func TestRunMeasure_Packages_AreMeasuredOneAfterAnother(t *testing.T) { + t.Parallel() + script := &fakeScript{ + outputs: map[string]string{"./cmd/a": "Condition coverage: 1/2\n", "./internal/b": "gobco: refused\n", "./internal/c": "Condition coverage: 3/4\n"}, + status: map[string]int{"./cmd/a": 0, "./internal/b": 1, "./internal/c": 3}, + } + records := t.TempDir() + var stdout, stderr bytes.Buffer + args := []string{"-packages", "./cmd/a ./internal/b,./internal/c", "-system", "ubuntu-latest", "-records", records, "-dir", "root"} + if got := runMeasure(args, &stdout, &stderr, script.run); got != 1 { + t.Fatalf("runMeasure() = %d, want 1; stderr:\n%s", got, stderr.String()) + } + wantCalls := []scriptCall{{dir: "root", pkg: "./cmd/a"}, {dir: "root", pkg: "./internal/b"}, {dir: "root", pkg: "./internal/c"}} + if !reflect.DeepEqual(script.calls, wantCalls) { + t.Errorf("script calls = %+v, want %+v", script.calls, wantCalls) + } + if want := "Condition coverage: 1/2\ngobco: refused\nCondition coverage: 3/4\n"; stdout.String() != want { + t.Errorf("stdout = %q, want %q", stdout.String(), want) + } + wantStderr := "measure_conditions: ./cmd/a on ubuntu-latest: measured (exit 0)\n" + + "measure_conditions: ./internal/b on ubuntu-latest: not measured (exit 1)\n" + + "measure_conditions: ./internal/c on ubuntu-latest: measured (exit 3)\n" + + "measure_conditions: 1 of 3 package(s) could not be measured on ubuntu-latest: ./internal/b; the gobco: lines above say why\n" + if stderr.String() != wantStderr { + t.Errorf("stderr =\n%s\nwant\n%s", stderr.String(), wantStderr) + } + for _, each := range []struct { + slug, pkg, output string + status int + }{ + {slug: "cmd-a", pkg: "./cmd/a", output: "Condition coverage: 1/2\n", status: 0}, + {slug: "internal-b", pkg: "./internal/b", output: "gobco: refused\n", status: 1}, + {slug: "internal-c", pkg: "./internal/c", output: "Condition coverage: 3/4\n", status: 3}, + } { + t.Run(each.pkg, func(t *testing.T) { + t.Parallel() + wantRecord := fmt.Sprintf("package=%s\nsystem=ubuntu-latest\nplatform=%s\nstatus=%d\n", each.pkg, platform, each.status) + if got := readFile(t, filepath.Join(records, each.slug, recordFile)); got != wantRecord { + t.Errorf("record = %q, want %q", got, wantRecord) + } + if got := readFile(t, filepath.Join(records, each.slug, outputFile)); got != each.output { + t.Errorf("raw output = %q, want %q", got, each.output) + } + }) + } +} + +// TestRunMeasure_EveryPackageMeasured_Passes verifies that a job whose every +// package was measured passes, whatever its gate held. +func TestRunMeasure_EveryPackageMeasured_Passes(t *testing.T) { + t.Parallel() + script := &fakeScript{outputs: map[string]string{"./a": "Condition coverage: 1/2\n"}, status: map[string]int{"./a": 3}} + var stdout, stderr bytes.Buffer + if got := runMeasure([]string{"-packages", "./a", "-system", "s", "-records", t.TempDir()}, &stdout, &stderr, script.run); got != 0 { + t.Fatalf("runMeasure() = %d, want 0; stderr:\n%s", got, stderr.String()) + } + if want := "measure_conditions: ./a on s: measured (exit 3)\n"; stderr.String() != want { + t.Errorf("stderr = %q, want %q", stderr.String(), want) + } +} + +// TestRunMeasure_Failures_StopTheJobAndSayWhy verifies that a package the +// script could not be run over, and a record that could not be written in +// any of its three steps, stop the job at that package with the reason, so +// the package after it is never run, and that the package is left without a +// record readRecords could read. +func TestRunMeasure_Failures_StopTheJobAndSayWhy(t *testing.T) { + t.Parallel() + cases := []struct { + name string + prepare func(t *testing.T, records string) string + want string + }{ + {name: "the script could not run", want: "measure_conditions: scripts/coverage-conditions.sh could not be run over ./gone: planted\n"}, + { + name: "the records directory is a file", + prepare: func(t *testing.T, records string) string { + t.Helper() + file := filepath.Join(records, "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + return file + }, + want: "measure_conditions: mkdir ", + }, + { + name: "the raw output cannot be written", + prepare: func(t *testing.T, records string) string { + t.Helper() + if err := os.MkdirAll(filepath.Join(records, "a", outputFile), 0o750); err != nil { + t.Fatal(err) + } + return records + }, + want: "measure_conditions: open " + filepath.Join("a", outputFile), + }, + { + name: "the record cannot be written", + prepare: func(t *testing.T, records string) string { + t.Helper() + if err := os.MkdirAll(filepath.Join(records, "a", recordFile), 0o750); err != nil { + t.Fatal(err) + } + return records + }, + want: "measure_conditions: open " + filepath.Join("a", recordFile), + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + records := t.TempDir() + pkg := "./gone" + if tc.prepare != nil { + records = tc.prepare(t, records) + pkg = "./a" + } + script := &fakeScript{outputs: map[string]string{"./a": "x\n"}, status: map[string]int{"./a": 0}, err: errors.New("planted")} + var stdout, stderr bytes.Buffer + if got := runMeasure([]string{"-packages", pkg + " ./later", "-system", "s", "-records", records}, &stdout, &stderr, script.run); got != 1 { + t.Fatalf("runMeasure() = %d, want 1", got) + } + got := strings.ReplaceAll(stderr.String(), records+string(filepath.Separator), "") + if !strings.HasPrefix(got, tc.want) || len(script.calls) != 1 { + t.Errorf("stderr = %q after %d script call(s), want one call and a line starting %q", got, len(script.calls), tc.want) + } + if found, _ := readRecords(records); len(found) != 0 { + t.Errorf("readRecords() = %+v, want no record of a package whose measuring failed", found) + } + }) + } +} + +// TestMeasurePackage_Record_ReadsBackAsTheSummaryReadsIt holds the writer of +// a record to its reader: what measurePackage leaves is what readRecords +// finds, field for field, with the platform the measuring ran on. +func TestMeasurePackage_Record_ReadsBackAsTheSummaryReadsIt(t *testing.T) { + t.Parallel() + script := &fakeScript{outputs: map[string]string{"./internal/tools/x": "Condition coverage: 5/6\n"}, status: map[string]int{"./internal/tools/x": 3}} + records := t.TempDir() + run := measurer{run: script.run, dir: ".", system: "macos-latest", records: records, env: []string{"GOBCO_GATE=all"}} + status, err := measurePackage(context.Background(), run, "./internal/tools/x", io.Discard) + if err != nil || status != 3 { + t.Fatalf("measurePackage() = %d, %v; want 3", status, err) + } + got, problems := readRecords(records) + want := []record{{pkg: "./internal/tools/x", system: "macos-latest", platform: platform, status: 3, output: "Condition coverage: 5/6\n"}} + if len(problems) != 0 || !reflect.DeepEqual(got, want) { + t.Errorf("readRecords() = %+v, %q; want %+v", got, problems, want) + } + if wantCalls := []scriptCall{{dir: ".", pkg: "./internal/tools/x", env: []string{"GOBCO_GATE=all"}}}; !reflect.DeepEqual(script.calls, wantCalls) { + t.Errorf("script calls = %+v, want %+v", script.calls, wantCalls) + } +} + +// TestRunScript_PlantedScript_RunsThroughBashInTheModuleRoot runs the +// production runner over a planted scripts/coverage-conditions.sh: the +// script is run through bash in the directory given, with the package as its +// argument and the environment added, both of its streams reach the writer +// in the order printed, and its exit status comes back as the status, 0 +// included. A directory that does not exist is an error, not a status. +func TestRunScript_PlantedScript_RunsThroughBashInTheModuleRoot(t *testing.T) { + t.Parallel() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "scripts"), 0o750); err != nil { + t.Fatal(err) + } + script := "echo \"out $1 $GOBCO_GATE\"\necho err >&2\nexit \"$EXIT\"\n" + if err := os.WriteFile(filepath.Join(root, filepath.FromSlash(coverageScript)), []byte(script), 0o600); err != nil { + t.Fatal(err) + } + for _, exit := range []int{0, 3} { + t.Run(strconv.Itoa(exit), func(t *testing.T) { + t.Parallel() + var output bytes.Buffer + status, err := runScript(context.Background(), root, "./x", []string{"GOBCO_GATE=all", fmt.Sprintf("EXIT=%d", exit)}, &output) + if err != nil || status != exit || output.String() != "out ./x all\nerr\n" { + t.Errorf("runScript() = %d, %v, output %q; want %d, no error and both streams", status, err, output.String(), exit) + } + }) + } + if _, err := runScript(context.Background(), filepath.Join(root, "absent"), "./x", nil, io.Discard); err == nil { + t.Errorf("runScript(absent directory) error = nil, want one") + } +} + +// TestRunScript_NoBashOnPath_IsTheLookupsFailure verifies that bash is found +// on PATH before the script is run through it, and that a PATH holding none +// is reported as the lookup's failure rather than as a run that failed. +func TestRunScript_NoBashOnPath_IsTheLookupsFailure(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + if _, err := runScript(context.Background(), t.TempDir(), "./x", nil, io.Discard); err == nil || !strings.HasPrefix(err.Error(), "find bash: ") { + t.Errorf("runScript() with no bash on PATH = %v, want the lookup's failure", err) + } +} diff --git a/cmd/measure_conditions/measurement.go b/cmd/measure_conditions/measurement.go index 38231f786..38e594497 100644 --- a/cmd/measure_conditions/measurement.go +++ b/cmd/measure_conditions/measurement.go @@ -1,7 +1,6 @@ package main import ( - "errors" "fmt" "regexp" "strconv" @@ -17,9 +16,34 @@ const ( ) // figurePattern is the line gobco ends its report on, read exactly as the -// script reads it: a total of zero is no measurement, and the script refuses -// it before it could be recorded as one. -var figurePattern = regexp.MustCompile(`^Condition coverage: (\d+)/([1-9]\d*)$`) +// script reads it: opened with the words of the mode it counted in, and a +// total of zero is no measurement, which the script refuses before it could +// be recorded as one. +var figurePattern = regexp.MustCompile(`^(Condition coverage|Branch coverage): (\d+)/([1-9]\d*)$`) + +// coverageMode is one GOBCO_MODE scripts/coverage-conditions.sh reads: the +// words gobco opens its figure with in that mode, and the sentence a summary +// says what the figures count in. +type coverageMode struct { + figure string + counts string +} + +// coverageModes are the GOBCO_MODE values the script reads. condition is +// gobco's default, which counts every atomic condition, each operand of && +// and || and each comparison wherever it is written. branch is gobco's +// -branch, which counts the whole controlling condition of each if and each +// for, and each case of a switch. +var coverageModes = map[string]coverageMode{ + "condition": { + figure: "Condition coverage", + counts: "gobco counts each condition twice, once per outcome, so a total is twice the number of conditions.", + }, + "branch": { + figure: "Branch coverage", + counts: "gobco counts each decision twice, once per outcome: the whole condition of an `if` or a `for`, and each case of a `switch`, so a total is twice the number of decisions.", + }, +} // conditionPattern is a condition gobco names as not evaluated both ways, in // the three shapes its report writes one in, with the file first: @@ -49,28 +73,34 @@ type condition struct { // measurement is what one run's output says: gobco's figure, the outcomes // evaluated of the outcomes there are, and the conditions it named. type measurement struct { - covered string - total string + covered int + total int conditions []condition held int } -// parseMeasurement reads the output of a run whose status says it measured. +// parseMeasurement reads the output of a run whose status says it measured, +// in the mode the runs were asked for, which is a key of coverageModes. // -// The figure must be there, since the script refuses to pass a run without -// one. A condition the script held is printed twice, once in gobco's report -// and once indented under the gate's verdict, and is marked held. A held -// line gobco's report does not name, or a status that disagrees with whether +// The figure must be there in the words of the mode, since the script +// refuses to pass a run without one; a figure in the other mode's words is +// not the measurement asked for, and is passed over like any other line. A +// condition the script held is printed twice, once in gobco's report and +// once indented under the gate's verdict, and is marked held. A held line +// gobco's report does not name, or a status that disagrees with whether // anything was held, means the output is not what the script writes, and is // refused rather than tabulated. -func parseMeasurement(output string, status int) (measurement, error) { +func parseMeasurement(output string, status int, mode string) (measurement, error) { var found measurement - figure := false + label := coverageModes[mode].figure + figure := "" heldLines := map[string]bool{} for line := range strings.SplitSeq(output, "\n") { line = strings.TrimSuffix(line, "\r") if match := figurePattern.FindStringSubmatch(line); match != nil { - found.covered, found.total, figure = match[1], match[2], true + if match[1] == label { + figure = line + } continue } if rest, indented := strings.CutPrefix(line, heldIndent); indented { @@ -89,8 +119,12 @@ func parseMeasurement(output string, status int) (measurement, error) { }) } } - if !figure { - return measurement{}, errors.New("the script's status says it measured, and its output carries no Condition coverage figure") + if figure == "" { + return measurement{}, fmt.Errorf("the script's status says it measured, and its output carries no %s figure", label) + } + var err error + if found.covered, found.total, err = readFigure(figure); err != nil { + return measurement{}, err } for i := range found.conditions { if heldLines[found.conditions[i].text] { @@ -107,6 +141,21 @@ func parseMeasurement(output string, status int) (measurement, error) { return found, nil } +// readFigure reads the two counts of a line figurePattern matched. A count +// too large for an int is refused rather than wrapped, since a total summed +// from it would be a number nobody measured; gobco prints counts of +// conditions, so no real report comes near that. +func readFigure(line string) (covered, total int, err error) { + match := figurePattern.FindStringSubmatch(line) + if covered, err = strconv.Atoi(match[2]); err != nil { + return 0, 0, fmt.Errorf("the figure %q does not fit a count: %w", line, err) + } + if total, err = strconv.Atoi(match[3]); err != nil { + return 0, 0, fmt.Errorf("the figure %q does not fit a count: %w", line, err) + } + return covered, total, nil +} + // lineBreak is a line break in a condition's code with the spaces and tabs // around it, which is what a condition written across lines carries where it // was broken: the end of one line, the break, and the next line's diff --git a/cmd/measure_conditions/measurement_test.go b/cmd/measure_conditions/measurement_test.go index b168da919..b37ac9908 100644 --- a/cmd/measure_conditions/measurement_test.go +++ b/cmd/measure_conditions/measurement_test.go @@ -6,18 +6,30 @@ import ( ) // TestParseMeasurement_Outputs_ReadTheFigureAndTheConditions verifies the -// reading of a run that measured: gobco's figure, every condition it names -// in each of its three shapes with the file written with slashes and the -// code unquoted, the held ones marked from the lines the gate printed under -// its verdict, and everything else in the output passed over, a condition -// evaluated both ways, an indented line that is not a condition and a -// carriage return included. +// reading of a run that measured, in each mode: gobco's figure in the words +// of the mode, every condition it names in each of its three shapes with the +// file written with slashes and the code unquoted, the held ones marked from +// the lines the gate printed under its verdict, and everything else in the +// output passed over, a condition evaluated both ways, an indented line that +// is not a condition and a carriage return included. func TestParseMeasurement_Outputs_ReadTheFigureAndTheConditions(t *testing.T) { t.Parallel() + for mode, figure := range map[string]string{"condition": "Condition coverage", "branch": "Branch coverage"} { + t.Run(mode, func(t *testing.T) { + t.Parallel() + parseEveryShape(t, mode, figure) + }) + } +} + +// parseEveryShape reads the output a run of the mode prints, its figure in +// the words given, and holds the reading to every condition it names. +func parseEveryShape(t *testing.T, mode, figure string) { + t.Helper() output := "go: downloading example.com/x v1.0.0\n" + "ok \texample.com/m/cmd/server\t1.2s\r\n" + "\n" + - "Condition coverage: 2236/2240\r\n" + + figure + ": 2236/2240\r\n" + `main.go:91:5: condition "x != \"\"" was 3 times true but never false` + "\n" + `cmd\server\conn_windows.go:12:9: condition "errors.Is(err, errRefused)" was once false but never true` + "\r\n" + `probe.go:7:2: condition "a | b == c" was never evaluated` + "\n" + @@ -26,13 +38,13 @@ func TestParseMeasurement_Outputs_ReadTheFigureAndTheConditions(t *testing.T) { ` main.go:91:5: condition "x != \"\"" was 3 times true but never false` + "\n" + ` probe.go:7:2: condition "a | b == c" was never evaluated` + "\r\n" + " --- FAIL: an indented line that is not a condition\n" - got, err := parseMeasurement(output, statusGateHeld) + got, err := parseMeasurement(output, statusGateHeld, mode) if err != nil { t.Fatalf("parseMeasurement() error = %v", err) } want := measurement{ - covered: "2236", - total: "2240", + covered: 2236, + total: 2240, held: 2, conditions: []condition{ { @@ -55,11 +67,13 @@ func TestParseMeasurement_Outputs_ReadTheFigureAndTheConditions(t *testing.T) { } // TestParseMeasurement_StatusAndOutput_MustAgree verifies that a measurement -// is refused when its output is not what the script writes for its status: -// no figure, a figure of zero (which the script refuses), a held line -// gobco's report does not name, a gate status with nothing held, and a -// passing status with something held. The last figure printed is the one -// read, and a passing run that holds nothing is accepted. +// is refused when its output is not what the script writes for its status +// and mode: no figure, a figure of zero (which the script refuses), a figure +// in the other mode's words, a figure whose count of either side does not +// fit an int, a held line gobco's report does not name, a gate status with +// nothing held, and a passing status with something held. The last figure of +// the mode printed is the one read, and a passing run that holds nothing is +// accepted in either mode. func TestParseMeasurement_StatusAndOutput_MustAgree(t *testing.T) { t.Parallel() const never = `main.go:1:2: condition "x" was never evaluated` @@ -67,16 +81,45 @@ func TestParseMeasurement_StatusAndOutput_MustAgree(t *testing.T) { name string output string status int + mode string wantErr string }{ { name: "no figure", output: never + "\n", status: statusMeasured, wantErr: "the script's status says it measured, and its output carries no Condition coverage figure", }, + { + name: "a branch figure read as conditions", output: "Branch coverage: 2/2\n", status: statusMeasured, + wantErr: "the script's status says it measured, and its output carries no Condition coverage figure", + }, + { + name: "a condition figure read as branches", output: "Condition coverage: 2/2\n", status: statusMeasured, mode: "branch", + wantErr: "the script's status says it measured, and its output carries no Branch coverage figure", + }, + { + name: "a branch figure of zero", output: "Branch coverage: 0/0\n", status: statusMeasured, mode: "branch", + wantErr: "the script's status says it measured, and its output carries no Branch coverage figure", + }, + { + name: "a branch total past an int", output: "Branch coverage: 2/99999999999999999999\n", status: statusMeasured, mode: "branch", + wantErr: `the figure "Branch coverage: 2/99999999999999999999" does not fit a count: strconv.Atoi: parsing "99999999999999999999": value out of range`, + }, + { + name: "a passing branch run, the last branch figure read", output: "Branch coverage: 1/9\nBranch coverage: 2/2\nCondition coverage: 1/9\n", + status: statusMeasured, mode: "branch", + }, { name: "a figure of zero", output: "Condition coverage: 0/0\n", status: statusMeasured, wantErr: "the script's status says it measured, and its output carries no Condition coverage figure", }, + { + name: "a covered count past an int", output: "Condition coverage: 99999999999999999999/2\n", status: statusMeasured, + wantErr: `the figure "Condition coverage: 99999999999999999999/2" does not fit a count: strconv.Atoi: parsing "99999999999999999999": value out of range`, + }, + { + name: "a total past an int", output: "Condition coverage: 2/99999999999999999999\n", status: statusMeasured, + wantErr: `the figure "Condition coverage: 2/99999999999999999999" does not fit a count: strconv.Atoi: parsing "99999999999999999999": value out of range`, + }, { name: "a held line gobco did not name", output: "Condition coverage: 1/2\n " + never + "\n", status: statusGateHeld, wantErr: "the script held 1 condition(s) and gobco's report names 0 of them", @@ -89,14 +132,18 @@ func TestParseMeasurement_StatusAndOutput_MustAgree(t *testing.T) { name: "a passing status with something held", output: "Condition coverage: 1/2\n" + never + "\n " + never + "\n", status: statusMeasured, wantErr: "the script exited 0 and its gate held 1 condition(s)", }, - {name: "a passing run, the last figure read", output: "Condition coverage: 1/9\nCondition coverage: 2/2\n", status: statusMeasured}, + {name: "a passing run, the last figure read", output: "Condition coverage: 1/9\nCondition coverage: 2/2\nBranch coverage: 1/9\n", status: statusMeasured}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got, err := parseMeasurement(tc.output, tc.status) + mode := tc.mode + if mode == "" { + mode = "condition" + } + got, err := parseMeasurement(tc.output, tc.status, mode) if tc.wantErr == "" { - if err != nil || got.covered != "2" || got.total != "2" || got.held != 0 { + if err != nil || got.covered != 2 || got.total != 2 || got.held != 0 { t.Errorf("parseMeasurement() = %+v, %v; want 2 of 2 and nothing held", got, err) } return diff --git a/cmd/measure_conditions/plan.go b/cmd/measure_conditions/plan.go index abb23a8d9..2bf30a8d1 100644 --- a/cmd/measure_conditions/plan.go +++ b/cmd/measure_conditions/plan.go @@ -25,17 +25,53 @@ var runners = []string{"windows-latest", "macos-latest", "ubuntu-latest"} // started is refused before any has. var gatePattern = regexp.MustCompile(`^(all|beyond:[a-z0-9]+/[a-z0-9]+)$`) +// defaultMode is the mode a subcommand measures or reads in when it is not +// given one: gobco's own default, which every run measured in before the +// mode could be chosen. +const defaultMode = "condition" + +// refuseMode says on stderr that a -mode is not a GOBCO_MODE the script +// reads, and reports whether it did, so a value the script would refuse +// after a runner has started is refused before any has, as the gate is. +func refuseMode(stderr io.Writer, mode string) bool { + if _, known := coverageModes[mode]; known { + return false + } + fmt.Fprintf(stderr, "%s: -mode %q is neither condition nor branch, which is what scripts/coverage-conditions.sh reads\n", toolName, mode) + return true +} + // segmentPattern is one directory of a package path. It admits what this // module's directories are named with and nothing a shell, a workflow // command or an artifact name would read as syntax, and since a segment // cannot start with a dot it refuses `.`, `..` and the `...` of a pattern. var segmentPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_.-]*$`) -// cell is one run the plan asks for: one package on one runner, and the name -// of the artifact its record and raw output are uploaded under. +// treeShards is how many jobs a -tree plan splits the packages below cmd/ and +// internal/ into on each runner. gobco caches nothing and takes from a few +// seconds to several minutes a package; docs/development/testing/README.md +// records how long the whole tree took on one machine and with how many +// packages at a time. Twenty shards make a job of fifteen or so packages one +// after another, which is expected to finish well inside the two hours a +// shard's job is given, and twenty shards on all three runners stay far below +// the 256 jobs a matrix may hold. +const treeShards = 20 + +// treePatterns are the packages a -tree plan measures: every package of the +// server and of the repository's tooling. test/ is left out, since every +// file of the e2e suite sits behind a build tag and gobco cannot measure it +// (docs/development/testing/README.md). +var treePatterns = []string{"./cmd/...", "./internal/..."} + +// cell is one job the plan asks for: the packages it measures on one runner, +// one after another, separated by spaces, what the job is called, and the +// name of the artifact the records and raw outputs of its packages are +// uploaded under. A plan of named packages gives each package a job of its +// own, named after it; a -tree plan gives each job a shard of the tree. type cell struct { System string `json:"system"` - Package string `json:"package"` + Name string `json:"name"` + Packages string `json:"packages"` Artifact string `json:"artifact"` } @@ -55,7 +91,9 @@ func runPlan(args []string, stdout, stderr io.Writer, list lister) int { packagesFlag := flags.String("packages", "", "packages to measure, ./cmd/server style, separated by spaces or commas; empty means every package carrying a GOOS- or GOARCH-constrained file") systemsFlag := flags.String("systems", "", "runners to measure on, separated by spaces or commas: "+strings.Join(runners, ", ")) gate := flags.String("gate", "", "the GOBCO_GATE the script runs with: all or beyond:GOOS/GOARCH") + mode := flags.String("mode", defaultMode, "the GOBCO_MODE the script runs with: condition or branch") dir := flags.String("dir", ".", "module root the packages are looked for in") + tree := flags.Bool("tree", false, "measure every package below cmd/ and internal/, in shards, instead of the packages -packages names") if err := flags.Parse(args); err != nil { return 2 } @@ -67,6 +105,10 @@ func runPlan(args []string, stdout, stderr io.Writer, list lister) int { fmt.Fprintf(stderr, "%s: -gate %q is neither all nor beyond:GOOS/GOARCH, which is what scripts/coverage-conditions.sh reads\n", toolName, *gate) return 2 } + if refuseMode(stderr, *mode) { + return 2 + } + measuredWith := fmt.Sprintf("GOBCO_GATE=%s and GOBCO_MODE=%s", *gate, *mode) systems, err := parseSystems(*systemsFlag) if err != nil { fmt.Fprintf(stderr, "%s: -systems: %v\n", toolName, err) @@ -77,6 +119,9 @@ func runPlan(args []string, stdout, stderr io.Writer, list lister) int { fmt.Fprintf(stderr, "%s: -packages: %v\n", toolName, err) return 2 } + if *tree { + return planTree(stdout, stderr, list, *dir, packages, systems, measuredWith) + } if len(packages) == 0 { packages, err = constrainedPackages(context.Background(), *dir, list) if err != nil { @@ -95,7 +140,7 @@ func runPlan(args []string, stdout, stderr io.Writer, list lister) int { return 2 } fmt.Fprintf(stdout, "matrix=%s\n", cmdutil.Must(json.Marshal(planned))) - fmt.Fprintf(stderr, "%s: %d run(s) planned, %s on %s, with GOBCO_GATE=%s\n", toolName, len(planned.Include), strings.Join(packages, " "), strings.Join(systems, " "), *gate) + fmt.Fprintf(stderr, "%s: %d run(s) planned, %s on %s, with %s\n", toolName, len(planned.Include), strings.Join(packages, " "), strings.Join(systems, " "), measuredWith) return 0 } @@ -149,22 +194,80 @@ func parsePackages(value string) ([]string, error) { } // buildMatrix crosses the packages with the systems, package by package, and -// names each cell's artifact after both. Two cells whose names would collide, -// which only directory names joined differently by a dash could do, are -// refused rather than left to overwrite each other's upload. +// gives each package a job of its own on each system, named after it, with an +// artifact named after both. Two cells whose names would collide, which only +// directory names joined differently by a dash could do, are refused rather +// than left to overwrite each other's upload. func buildMatrix(packages, systems []string) (matrix, error) { planned := matrix{Include: []cell{}} byArtifact := map[string]cell{} for _, pkg := range packages { for _, system := range systems { - artifact := "gobco-" + system + "-" + strings.ReplaceAll(strings.TrimPrefix(pkg, "./"), "/", "-") + artifact := "gobco-" + system + "-" + packageSlug(pkg) if other, taken := byArtifact[artifact]; taken { - return matrix{}, fmt.Errorf("%s and %s on %s would both upload as %s", other.Package, pkg, system, artifact) + return matrix{}, fmt.Errorf("%s and %s on %s would both upload as %s", other.Packages, pkg, system, artifact) } - entry := cell{System: system, Package: pkg, Artifact: artifact} + entry := cell{System: system, Name: pkg, Packages: pkg, Artifact: artifact} byArtifact[artifact] = entry planned.Include = append(planned.Include, entry) } } return planned, nil } + +// planTree prints the plan of a -tree dispatch: every package below cmd/ and +// internal/, in shards, and says on stderr what it planned and the gate and +// mode the runs measure with. A package list beside -tree is refused, since +// the dispatch would be asking for two things at once. +func planTree(stdout, stderr io.Writer, list lister, dir string, named, systems []string, measuredWith string) int { + if len(named) > 0 { + fmt.Fprintf(stderr, "%s: -tree measures every package below cmd/ and internal/, and -packages names %s besides; leave -packages empty\n", toolName, strings.Join(named, " ")) + return 2 + } + packages, err := treePackages(context.Background(), dir, list) + if err != nil { + fmt.Fprintf(stderr, "%s: -tree: the packages below cmd/ and internal/ could not be listed: %v\n", toolName, err) + return 1 + } + if len(packages) == 0 { + fmt.Fprintf(stderr, "%s: -tree: there is no package below cmd/ and internal/, so there is nothing to measure\n", toolName) + return 1 + } + planned := buildShards(packages, systems, treeShards) + fmt.Fprintf(stdout, "matrix=%s\n", cmdutil.Must(json.Marshal(planned))) + fmt.Fprintf(stderr, "%s: %d run(s) planned, the %d packages below cmd/ and internal/ in %d shard(s) on %s, with %s\n", + toolName, len(planned.Include), len(packages), min(treeShards, len(packages)), strings.Join(systems, " "), measuredWith) + return 0 +} + +// buildShards deals the packages, in the order given, into at most shards +// jobs on each system, the first package to the first shard, the second to +// the second and so on round again, so the slow packages of one directory +// do not all land in one job. Each shard is named by its number and its +// artifact by its system and number, which no two shards share. +func buildShards(packages, systems []string, shards int) matrix { + count := min(shards, len(packages)) + planned := matrix{Include: []cell{}} + for shard := range count { + var members []string + for i := shard; i < len(packages); i += count { + members = append(members, packages[i]) + } + for _, system := range systems { + planned.Include = append(planned.Include, cell{ + System: system, + Name: fmt.Sprintf("tree %02d of %02d", shard+1, count), + Packages: strings.Join(members, " "), + Artifact: fmt.Sprintf("gobco-%s-tree-%02d", system, shard+1), + }) + } + } + return planned +} + +// packageSlug is a package written as one directory name: its path below +// the module root with each slash a dash. It names a package's artifact in a +// plan of named packages and its record's directory everywhere. +func packageSlug(pkg string) string { + return strings.ReplaceAll(strings.TrimPrefix(pkg, "./"), "/", "-") +} diff --git a/cmd/measure_conditions/plan_test.go b/cmd/measure_conditions/plan_test.go index 1cac61e41..913aad09a 100644 --- a/cmd/measure_conditions/plan_test.go +++ b/cmd/measure_conditions/plan_test.go @@ -20,10 +20,24 @@ func answeringLister(listing func(target string) string, err error) lister { } } +// treeLister is a lister that answers the tree's listing with listing, or +// fails with err. +func treeLister(listing string, err error) lister { + return func(context.Context, string, []string, ...string) ([]byte, error) { + if err != nil { + return nil, err + } + return []byte(listing), nil + } +} + // TestRunPlan_Dispatches_PrintTheMatrixOrRefuse verifies the plan end to end: -// the matrix line it prints for $GITHUB_OUTPUT, what it says on stderr, and -// its status, for every input it accepts or refuses and for the three ways the -// discovery of an empty package list can end. +// the matrix line it prints for $GITHUB_OUTPUT, what it says on stderr (the +// gate and the mode the runs measure with among it), and its status, for +// every input it accepts or refuses, for the three ways the +// discovery of an empty package list can end, and for a -tree plan, which +// refuses a package list beside it and the two ways its listing can come back +// with nothing to plan. func TestRunPlan_Dispatches_PrintTheMatrixOrRefuse(t *testing.T) { t.Parallel() const module = "example.com/m" @@ -52,19 +66,34 @@ func TestRunPlan_Dispatches_PrintTheMatrixOrRefuse(t *testing.T) { args: []string{"-packages", "cmd/server, ./internal/toolutil ./cmd/server", "-systems", "ubuntu-latest,windows-latest ubuntu-latest", "-gate", "beyond:linux/amd64"}, list: broken, wantStdout: `matrix={"include":[` + - `{"system":"windows-latest","package":"./cmd/server","artifact":"gobco-windows-latest-cmd-server"},` + - `{"system":"ubuntu-latest","package":"./cmd/server","artifact":"gobco-ubuntu-latest-cmd-server"},` + - `{"system":"windows-latest","package":"./internal/toolutil","artifact":"gobco-windows-latest-internal-toolutil"},` + - `{"system":"ubuntu-latest","package":"./internal/toolutil","artifact":"gobco-ubuntu-latest-internal-toolutil"}]}` + "\n", - wantStderr: "measure_conditions: 4 run(s) planned, ./cmd/server ./internal/toolutil on windows-latest ubuntu-latest, with GOBCO_GATE=beyond:linux/amd64\n", + `{"system":"windows-latest","name":"./cmd/server","packages":"./cmd/server","artifact":"gobco-windows-latest-cmd-server"},` + + `{"system":"ubuntu-latest","name":"./cmd/server","packages":"./cmd/server","artifact":"gobco-ubuntu-latest-cmd-server"},` + + `{"system":"windows-latest","name":"./internal/toolutil","packages":"./internal/toolutil","artifact":"gobco-windows-latest-internal-toolutil"},` + + `{"system":"ubuntu-latest","name":"./internal/toolutil","packages":"./internal/toolutil","artifact":"gobco-ubuntu-latest-internal-toolutil"}]}` + "\n", + wantStderr: "measure_conditions: 4 run(s) planned, ./cmd/server ./internal/toolutil on windows-latest ubuntu-latest, with GOBCO_GATE=beyond:linux/amd64 and GOBCO_MODE=condition\n", }, { name: "no package named: the constrained ones are found", args: []string{"-systems", "macos-latest", "-gate", "all"}, list: discovered, - wantStdout: `matrix={"include":[{"system":"macos-latest","package":"./cmd/server","artifact":"gobco-macos-latest-cmd-server"}]}` + "\n", + wantStdout: `matrix={"include":[{"system":"macos-latest","name":"./cmd/server","packages":"./cmd/server","artifact":"gobco-macos-latest-cmd-server"}]}` + "\n", wantStderr: "measure_conditions: no package named; measuring the 1 whose built files differ between release targets: ./cmd/server\n" + - "measure_conditions: 1 run(s) planned, ./cmd/server on macos-latest, with GOBCO_GATE=all\n", + "measure_conditions: 1 run(s) planned, ./cmd/server on macos-latest, with GOBCO_GATE=all and GOBCO_MODE=condition\n", + }, + { + name: "branches counted", + args: []string{"-packages", "./cmd/server", "-systems", "macos-latest", "-gate", "all", "-mode", "branch"}, + list: broken, + wantStdout: `matrix={"include":[{"system":"macos-latest","name":"./cmd/server","packages":"./cmd/server","artifact":"gobco-macos-latest-cmd-server"}]}` + "\n", + wantStderr: "measure_conditions: 1 run(s) planned, ./cmd/server on macos-latest, with GOBCO_GATE=all and GOBCO_MODE=branch\n", + }, + { + name: "a mode the script would refuse", args: []string{"-systems", "macos-latest", "-gate", "all", "-mode", "branches"}, list: broken, wantCode: 2, + wantStderr: "measure_conditions: -mode \"branches\" is neither condition nor branch, which is what scripts/coverage-conditions.sh reads\n", + }, + { + name: "no mode", args: []string{"-systems", "macos-latest", "-gate", "all", "-mode", ""}, list: broken, wantCode: 2, + wantStderr: "measure_conditions: -mode \"\" is neither condition nor branch, which is what scripts/coverage-conditions.sh reads\n", }, { name: "no package named and none found", args: []string{"-systems", "macos-latest", "-gate", "all"}, list: nothing, wantCode: 1, @@ -110,6 +139,29 @@ func TestRunPlan_Dispatches_PrintTheMatrixOrRefuse(t *testing.T) { name: "two names for one artifact", args: []string{"-packages", "./a-b/c ./a/b-c", "-systems", "macos-latest", "-gate", "all"}, list: broken, wantCode: 2, wantStderr: "measure_conditions: ./a-b/c and ./a/b-c on macos-latest would both upload as gobco-macos-latest-a-b-c\n", }, + { + name: "the tree, a shard per package while there are fewer than the shards", + args: []string{"-tree", "-systems", "ubuntu-latest windows-latest", "-gate", "all"}, + list: treeLister(row(module+"/cmd/a", module, "a.go", "", "", "")+row(module+"/internal/b", module, "b.go", "", "", ""), nil), + wantStdout: `matrix={"include":[` + + `{"system":"windows-latest","name":"tree 01 of 02","packages":"./cmd/a","artifact":"gobco-windows-latest-tree-01"},` + + `{"system":"ubuntu-latest","name":"tree 01 of 02","packages":"./cmd/a","artifact":"gobco-ubuntu-latest-tree-01"},` + + `{"system":"windows-latest","name":"tree 02 of 02","packages":"./internal/b","artifact":"gobco-windows-latest-tree-02"},` + + `{"system":"ubuntu-latest","name":"tree 02 of 02","packages":"./internal/b","artifact":"gobco-ubuntu-latest-tree-02"}]}` + "\n", + wantStderr: "measure_conditions: 4 run(s) planned, the 2 packages below cmd/ and internal/ in 2 shard(s) on windows-latest ubuntu-latest, with GOBCO_GATE=all and GOBCO_MODE=condition\n", + }, + { + name: "the tree and packages besides", args: []string{"-tree", "-packages", "./cmd/server", "-systems", "ubuntu-latest", "-gate", "all"}, list: broken, wantCode: 2, + wantStderr: "measure_conditions: -tree measures every package below cmd/ and internal/, and -packages names ./cmd/server besides; leave -packages empty\n", + }, + { + name: "the tree, and the listing failed", args: []string{"-tree", "-systems", "ubuntu-latest", "-gate", "all"}, list: treeLister("", errors.New("planted failure")), wantCode: 1, + wantStderr: "measure_conditions: -tree: the packages below cmd/ and internal/ could not be listed: go list: planted failure\n", + }, + { + name: "the tree, and nothing below cmd/ and internal/", args: []string{"-tree", "-systems", "ubuntu-latest", "-gate", "all"}, list: treeLister("", nil), wantCode: 1, + wantStderr: "measure_conditions: -tree: there is no package below cmd/ and internal/, so there is nothing to measure\n", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -150,6 +202,48 @@ func TestRunPlan_DirFlag_IsWhereTheDiscoveryLooks(t *testing.T) { } } +// TestBuildShards_Packages_AreDealtRoundTheShards verifies how a -tree plan +// splits the packages: dealt one at a time round the shards in the order +// given, so neighbors land in different jobs, each shard on every system +// with the same name and members, and never more shards than packages. +func TestBuildShards_Packages_AreDealtRoundTheShards(t *testing.T) { + t.Parallel() + got := buildShards([]string{"./a", "./b", "./c", "./d", "./e"}, []string{"macos-latest", "ubuntu-latest"}, 2) + want := matrix{Include: []cell{ + {System: "macos-latest", Name: "tree 01 of 02", Packages: "./a ./c ./e", Artifact: "gobco-macos-latest-tree-01"}, + {System: "ubuntu-latest", Name: "tree 01 of 02", Packages: "./a ./c ./e", Artifact: "gobco-ubuntu-latest-tree-01"}, + {System: "macos-latest", Name: "tree 02 of 02", Packages: "./b ./d", Artifact: "gobco-macos-latest-tree-02"}, + {System: "ubuntu-latest", Name: "tree 02 of 02", Packages: "./b ./d", Artifact: "gobco-ubuntu-latest-tree-02"}, + }} + if !reflect.DeepEqual(got, want) { + t.Errorf("buildShards() =\n%+v\nwant\n%+v", got, want) + } + if fewer := buildShards([]string{"./a"}, []string{"macos-latest"}, 20); len(fewer.Include) != 1 || fewer.Include[0].Name != "tree 01 of 01" { + t.Errorf("buildShards(one package, twenty shards) = %+v, want one shard of one", fewer) + } +} + +// TestRunPlan_TreeDirFlag_IsWhereTheTreeIsListed verifies that -dir reaches +// the tree's listing too, and that the listing is asked for the packages +// below cmd/ and internal/ under the platform at hand, with no environment of +// its own. +func TestRunPlan_TreeDirFlag_IsWhereTheTreeIsListed(t *testing.T) { + t.Parallel() + var calls []listCall + list := func(_ context.Context, dir string, env []string, args ...string) ([]byte, error) { + calls = append(calls, listCall{dir: dir, env: env, args: args}) + return nil, errors.New("stop") + } + var stdout, stderr bytes.Buffer + if got := runPlan([]string{"-tree", "-dir", "elsewhere", "-systems", "macos-latest", "-gate", "all"}, &stdout, &stderr, list); got != 1 { + t.Fatalf("runPlan() = %d, want 1", got) + } + want := []listCall{{dir: "elsewhere", args: []string{"list", "-f", listFormat, "./cmd/...", "./internal/..."}}} + if !reflect.DeepEqual(calls, want) { + t.Errorf("the listing calls were %+v, want %+v", calls, want) + } +} + // TestParsePackages_Names_AreNormalizedOrRefused verifies the package reader: // a name with or without its ./ is the same package, each is kept once in the // order first named, and anything that is not a directory below the module diff --git a/cmd/measure_conditions/record_test.go b/cmd/measure_conditions/record_test.go index 59bb02e90..78462ead1 100644 --- a/cmd/measure_conditions/record_test.go +++ b/cmd/measure_conditions/record_test.go @@ -2,9 +2,11 @@ package main import ( "fmt" + "maps" "os" "path/filepath" "reflect" + "slices" "strings" "testing" @@ -159,8 +161,10 @@ type conditionsWorkflow struct { On map[string]any `yaml:"on"` Permissions map[string]string `yaml:"permissions"` Jobs map[string]struct { - Permissions any `yaml:"permissions"` - Steps []workflowStep `yaml:"steps"` + Name string `yaml:"name"` + Permissions any `yaml:"permissions"` + TimeoutMinutes any `yaml:"timeout-minutes"` + Steps []workflowStep `yaml:"steps"` } `yaml:"jobs"` steps map[string]workflowStep } @@ -212,32 +216,37 @@ func TestConditionsWorkflow_Triggers_AreADispatchThatOnlyReads(t *testing.T) { } // TestConditionsWorkflow_MeasureStep_WritesTheRecordThisCommandReads holds the -// measuring step to what summarize assumes of it: it runs -// scripts/coverage-conditions.sh through bash with the dispatch's gate, keeps -// both streams in the raw output file, takes the script's own status rather -// than tee's, reads as a measurement exactly the statuses this command does, -// and writes every key of the record into the record file, which the upload -// step sends under the artifact name the plan chose. It also removes -// setup-go's problem matcher before the script prints anything, since that -// matcher files every condition gobco names as an error annotation. +// measuring step to what summarize assumes of it: it runs this command's +// measure over the job's packages through bash, with the dispatch's gate and +// mode in the script's environment and the system and artifact the plan chose, writes +// the records below the directory the upload step sends under the artifact +// name, and fails the job with an annotation when the command says a package +// was not measured. It also removes setup-go's problem matcher before +// anything is printed, since that matcher files every condition gobco names +// as an error annotation. func TestConditionsWorkflow_MeasureStep_WritesTheRecordThisCommandReads(t *testing.T) { t.Parallel() workflow := loadConditionsWorkflow(t) measure := workflow.steps["measure/Measure"] - if measure.Shell != "bash" || measure.Env["GOBCO_GATE"] != "${{ inputs.gate }}" || measure.Env["ARTIFACT"] != "${{ matrix.artifact }}" { - t.Errorf("the measure step has shell %q and env %v", measure.Shell, measure.Env) + wantEnv := map[string]any{ + "PACKAGES": "${{ matrix.packages }}", + "SYSTEM": "${{ matrix.system }}", + "ARTIFACT": "${{ matrix.artifact }}", + "GOBCO_GATE": "${{ inputs.gate }}", + "GOBCO_MODE": "${{ inputs.mode }}", + "GOBCO_TEST_FLAGS": "-timeout=30m", + } + if measure.Shell != "bash" || !reflect.DeepEqual(measure.Env, wantEnv) { + t.Errorf("the measure step has shell %q and env %v, want bash and %v", measure.Shell, measure.Env, wantEnv) } if first, _, _ := strings.Cut(measure.Run, "\n"); first != `echo "::remove-matcher owner=go::"` { t.Errorf("the measure step opens with %q, want the removal of setup-go's problem matcher", first) } wanted := []string{ - `bash scripts/coverage-conditions.sh "$PACKAGE" 2>&1 | tee "$record/` + outputFile + `"`, - `status=${PIPESTATUS[0]}`, - `} >"$record/` + recordFile + `"`, - fmt.Sprintf("%d|%d)", statusMeasured, statusGateHeld), - } - for _, key := range recordKeys { - wanted = append(wanted, `echo "`+key+`=`) + `go run ./cmd/measure_conditions measure -packages "$PACKAGES" -system "$SYSTEM" -records "${RUNNER_TEMP//\\//}/$ARTIFACT" || status=$?`, + `if [ "$status" -ne 0 ]; then`, + `echo "::error title=Not measured::`, + `exit "$status"`, } for _, fragment := range wanted { t.Run(fragment, func(t *testing.T) { @@ -253,30 +262,92 @@ func TestConditionsWorkflow_MeasureStep_WritesTheRecordThisCommandReads(t *testi } } +// TestConditionsWorkflow_JobBounds_FollowWhatEachJobMeasures holds each job to +// its time bound: a measuring job of named packages measures one and keeps +// the hour it had before the tree existed, which on the Windows and macOS +// runners is billed at a multiple of the Linux rate, a shard of the tree +// measures fifteen or so packages one after another and gets two hours, and +// the plan and the summary, which run the command and nothing else, ten +// minutes each. +func TestConditionsWorkflow_JobBounds_FollowWhatEachJobMeasures(t *testing.T) { + t.Parallel() + workflow := loadConditionsWorkflow(t) + want := map[string]any{"plan": 10, "measure": "${{ inputs.tree && 120 || 60 }}", "summary": 10} + for name, bound := range want { + t.Run(name, func(t *testing.T) { + t.Parallel() + if got := workflow.Jobs[name].TimeoutMinutes; got != bound { + t.Errorf("job %s has timeout-minutes %v (%T), want %v", name, got, got, bound) + } + }) + } +} + // TestConditionsWorkflow_PlanAndSummary_RunThisCommand holds the two ends to -// this command: the plan prints its matrix into $GITHUB_OUTPUT, and the -// summary downloads every artifact by the prefix the plan names them with and -// writes the table into the job summary through bash, so a failure of the -// command is not hidden by the pipe. +// this command: the plan is handed every input of the dispatch, the tree +// one included, and prints its matrix into $GITHUB_OUTPUT; the jobs are +// named by the name a cell carries; and the summary downloads every artifact +// by the prefix the plan names them with, a shard's as much as a package's, +// and writes the table into the job summary through bash, so a failure of +// the command is not hidden by the pipe. func TestConditionsWorkflow_PlanAndSummary_RunThisCommand(t *testing.T) { t.Parallel() workflow := loadConditionsWorkflow(t) plan := workflow.steps["plan/Plan the runs"] - if !strings.Contains(plan.Run, "go run ./cmd/measure_conditions plan ") || !strings.Contains(plan.Run, `>> "$GITHUB_OUTPUT"`) { - t.Errorf("the plan step runs %q", plan.Run) + if want := `go run ./cmd/measure_conditions plan -packages "$PACKAGES" -systems "$SYSTEMS" -gate "$GATE" -mode "$MODE" -tree="$TREE" >> "$GITHUB_OUTPUT"`; plan.Run != want { + t.Errorf("the plan step runs %q, want %q", plan.Run, want) + } + if plan.Env["TREE"] != "${{ inputs.tree }}" || plan.Env["MODE"] != "${{ inputs.mode }}" { + t.Errorf("the plan step's env is %v, want TREE and MODE from the tree and mode inputs", plan.Env) + } + if name := workflow.Jobs["measure"].Name; name != "🔬 ${{ matrix.name }} on ${{ matrix.system }}" { + t.Errorf("the measure job is named %q", name) } planned, err := buildMatrix([]string{"./cmd/server"}, []string{"macos-latest"}) if err != nil { t.Fatal(err) } + shards := buildShards([]string{"./cmd/server"}, []string{"macos-latest"}, treeShards) download := workflow.steps["summary/Download the records"] - if pattern, isText := download.With["pattern"].(string); !isText || !strings.HasSuffix(pattern, "*") || - !strings.HasPrefix(planned.Include[0].Artifact, strings.TrimSuffix(pattern, "*")) { - t.Errorf("the download step takes %v, which does not match the artifact %s the plan names", download.With, planned.Include[0].Artifact) + for _, artifact := range []string{planned.Include[0].Artifact, shards.Include[0].Artifact} { + t.Run(artifact, func(t *testing.T) { + t.Parallel() + if pattern, isText := download.With["pattern"].(string); !isText || !strings.HasSuffix(pattern, "*") || + !strings.HasPrefix(artifact, strings.TrimSuffix(pattern, "*")) { + t.Errorf("the download step takes %v, which does not match the artifact %s the plan names", download.With, artifact) + } + }) } summary := workflow.steps["summary/Write the job summary"] - if !strings.Contains(summary.Run, "go run ./cmd/measure_conditions summarize ") || + if !strings.Contains(summary.Run, "go run ./cmd/measure_conditions summarize ") || !strings.Contains(summary.Run, ` -mode "$MODE" `) || !strings.Contains(summary.Run, `tee -a "$GITHUB_STEP_SUMMARY"`) || summary.Shell != "bash" { t.Errorf("the summary step runs %q with shell %q", summary.Run, summary.Shell) } + if summary.Env["MODE"] != "${{ inputs.mode }}" { + t.Errorf("the summary step's env is %v, want MODE from the mode input", summary.Env) + } +} + +// TestConditionsWorkflow_ModeInput_OffersTheModesTheScriptReads holds the +// dispatch form's mode to the modes this command and the script read, every +// one of them and no other, with gobco's default mode as its default, so a +// dispatch that leaves it alone measures what every dispatch measured before +// the mode existed. +func TestConditionsWorkflow_ModeInput_OffersTheModesTheScriptReads(t *testing.T) { + t.Parallel() + workflow := loadConditionsWorkflow(t) + dispatch, _ := workflow.On["workflow_dispatch"].(map[string]any) + inputs, _ := dispatch["inputs"].(map[string]any) + input, _ := inputs["mode"].(map[string]any) + var offered []string + options, _ := input["options"].([]any) + for _, option := range options { + offered = append(offered, fmt.Sprint(option)) + } + if want := slices.Sorted(maps.Keys(coverageModes)); !reflect.DeepEqual(slices.Sorted(slices.Values(offered)), want) { + t.Errorf("the mode input offers %q, want %q", offered, want) + } + if input["type"] != "choice" || input["default"] != "condition" { + t.Errorf("the mode input is %v, want a choice defaulting to condition", input) + } } diff --git a/cmd/measure_conditions/summarize.go b/cmd/measure_conditions/summarize.go index c97e470ea..84737cbba 100644 --- a/cmd/measure_conditions/summarize.go +++ b/cmd/measure_conditions/summarize.go @@ -6,18 +6,49 @@ import ( "fmt" "io" "maps" + "path" + "regexp" "slices" "strconv" "strings" ) // reasonTail is how many of the last lines of a run's output stand for its -// reason when the script printed none of its own. +// reason when no line of it says why (failureLine). const reasonTail = 20 +// packageRun is one package measured on one runner: what a plan asks of a +// job for each package it names, and where the job uploads that package's +// record. A tree measured on the machine at hand has no artifact, its records +// sitting directly below the directory it was given. +type packageRun struct { + Package string + System string + Artifact string +} + +// runsOf is every package run the cells of a plan ask for, cell by cell and, +// within a cell, in the order its packages are named. +func runsOf(cells []cell) []packageRun { + var runs []packageRun + for _, planned := range cells { + for pkg := range strings.FieldsSeq(planned.Packages) { + runs = append(runs, packageRun{Package: pkg, System: planned.System, Artifact: planned.Artifact}) + } + } + return runs +} + +// recordPath is where a run's record lies below the directory the records +// were downloaded or written into: the run's artifact, if it has one, and +// the package's own directory in it. +func (r packageRun) recordPath() string { + return path.Join(r.Artifact, packageSlug(r.Package)) +} + // outcome is what became of one planned run. type outcome struct { - cell + packageRun rec *record measured *measurement problem string @@ -32,6 +63,7 @@ func runSummarize(args []string, stdout, stderr io.Writer) int { planFlag := flags.String("plan", "", "the matrix plan printed, as JSON") records := flags.String("records", "", "directory the runs' artifacts were downloaded into") gate := flags.String("gate", "", "the GOBCO_GATE the runs used") + mode := flags.String("mode", defaultMode, "the GOBCO_MODE the runs used: condition or branch") commit := flags.String("commit", "", "the commit the runs measured") if err := flags.Parse(args); err != nil { return 2 @@ -40,15 +72,18 @@ func runSummarize(args []string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "%s: summarize takes -plan, -records, -gate and -commit, all four non-empty, and no arguments\n", toolName) return 2 } + if refuseMode(stderr, *mode) { + return 2 + } var planned matrix - if err := json.Unmarshal([]byte(*planFlag), &planned); err != nil || len(planned.Include) == 0 { + if err := json.Unmarshal([]byte(*planFlag), &planned); err != nil || len(runsOf(planned.Include)) == 0 { fmt.Fprintf(stderr, "%s: -plan is not a matrix plan with at least one run in it\n", toolName) return 2 } recs, problems := readRecords(*records) - outcomes, more := join(planned.Include, recs) + outcomes, more := join(runsOf(planned.Include), recs, *mode) problems = append(problems, more...) - render(stdout, *gate, *commit, outcomes, problems) + render(stdout, *gate, *mode, *commit, outcomes, problems) for _, problem := range problems { fmt.Fprintf(stderr, "%s: %s\n", toolName, problem) } @@ -59,11 +94,12 @@ func runSummarize(args []string, stdout, stderr io.Writer) int { } // join pairs every planned run with its record, in the plan's order, and -// reads the measurement of each record whose status says one was taken. A -// record the plan did not ask for, a second record for one run, and a record -// whose output disagrees with its status are problems, since a table built on -// any of them would be a guess. -func join(cells []cell, recs []record) (outcomes []outcome, problems []string) { +// reads the measurement of each record whose status says one was taken, in +// the mode the runs were asked for. A record the plan did not ask for, a +// second record for one run, and a record whose output disagrees with its +// status or carries no figure of the mode are problems, since a table built +// on any of them would be a guess. +func join(runs []packageRun, recs []record, mode string) (outcomes []outcome, problems []string) { byRun := map[string]*record{} for i := range recs { rec := &recs[i] @@ -74,13 +110,13 @@ func join(cells []cell, recs []record) (outcomes []outcome, problems []string) { } byRun[key] = rec } - outcomes = make([]outcome, 0, len(cells)) - for _, planned := range cells { + outcomes = make([]outcome, 0, len(runs)) + for _, planned := range runs { key := planned.Package + " on " + planned.System - result := outcome{cell: planned, rec: byRun[key]} + result := outcome{packageRun: planned, rec: byRun[key]} delete(byRun, key) - if result.rec != nil && (result.rec.status == statusMeasured || result.rec.status == statusGateHeld) { - found, err := parseMeasurement(result.rec.output, result.rec.status) + if result.rec != nil && measured(result.rec.status) { + found, err := parseMeasurement(result.rec.output, result.rec.status, mode) if err != nil { result.problem = err.Error() problems = append(problems, fmt.Sprintf("the record of %s cannot be read: %v", key, err)) @@ -96,17 +132,20 @@ func join(cells []cell, recs []record) (outcomes []outcome, problems []string) { return outcomes, problems } -// render writes the summary: the figures, then every condition not evaluated -// both ways, then what could not be measured and why. -func render(w io.Writer, gate, commit string, outcomes []outcome, problems []string) { - fmt.Fprintf(w, "## Condition coverage, measured\n\n") - fmt.Fprintf(w, "`GOBCO_GATE=%s` at `%s`. Every figure is the one gobco printed on the runner named, through `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`; none is inferred from another platform. gobco counts each condition twice, once per outcome, so a total is twice the number of conditions. No figure fails this workflow: it is a measurement, not a gate.\n\n", gate, commit) +// render writes the summary of the mode the runs measured in: the figures, +// the total of each system, then every condition not evaluated both ways, +// then what could not be measured and why. +func render(w io.Writer, gate, mode, commit string, outcomes []outcome, problems []string) { + measured := coverageModes[mode] + fmt.Fprintf(w, "## %s, measured\n\n", measured.figure) + fmt.Fprintf(w, "`GOBCO_GATE=%s` at `%s`. Every figure is the one gobco printed on the runner named, through `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`; none is inferred from another platform. %s No figure fails this workflow: it is a measurement, not a gate.\n\n", gate, commit, measured.counts) writeRow(w, "Package", "System", "Covered", "Not both ways", "Held by the gate", "Raw output") fmt.Fprintf(w, "| --- | --- | ---: | ---: | ---: | --- |\n") for _, result := range outcomes { writeRow(w, codeSpan(result.Package), systemCell(result), coveredCell(result), - countCell(result, len(conditionsOf(result))), countCell(result, heldOf(result)), codeSpan(result.Artifact)) + countCell(result, len(conditionsOf(result))), countCell(result, heldOf(result)), codeSpan(result.recordPath())) } + writeTotals(w, outcomes) writeConditions(w, outcomes) writeUnmeasured(w, outcomes) if len(problems) > 0 { @@ -117,6 +156,59 @@ func render(w io.Writer, gate, commit string, outcomes []outcome, problems []str } } +// total is what the measured runs of one system add up to. +type total struct { + system string + planned int + measured int + covered int + outcomes int +} + +// writeTotals writes, for each system in the order the runs name them, how +// many of its planned packages were measured and the outcomes gobco counted +// as evaluated of those there are, summed over the measured ones, with that +// share rounded down to a tenth of a percent so it never reads higher than +// it is. A run that took no measurement is in no total: the table above says +// which ones and why, and the share is of the packages measured. +func writeTotals(w io.Writer, outcomes []outcome) { + var totals []*total + bySystem := map[string]*total{} + for _, result := range outcomes { + sum := bySystem[result.System] + if sum == nil { + sum = &total{system: result.System} + bySystem[result.System] = sum + totals = append(totals, sum) + } + sum.planned++ + if result.measured != nil { + sum.measured++ + sum.covered += result.measured.covered + sum.outcomes += result.measured.total + } + } + fmt.Fprintf(w, "\n### Total\n\n") + fmt.Fprintf(w, "A package that was not measured is in no total: the table above says which and why, and each share is of the packages its system measured.\n\n") + writeRow(w, "System", "Packages measured", "Covered", "Share") + fmt.Fprintf(w, "| --- | ---: | ---: | ---: |\n") + for _, sum := range totals { + writeRow(w, codeSpan(sum.system), fmt.Sprintf("%d of %d", sum.measured, sum.planned), + fmt.Sprintf("%d of %d", sum.covered, sum.outcomes), share(sum.covered, sum.outcomes)) + } +} + +// share is covered of outcomes as a percentage rounded down to a tenth, so +// 799 of 1000 reads 79.9% and never 80.0%, and a dash when nothing was +// counted. +func share(covered, outcomes int) string { + if outcomes == 0 { + return "-" + } + tenths := covered * 1000 / outcomes + return fmt.Sprintf("%d.%d%%", tenths/10, tenths%10) +} + // writeConditions lists every condition gobco named as not evaluated both // ways, run by run. func writeConditions(w io.Writer, outcomes []outcome) { @@ -140,7 +232,8 @@ func writeConditions(w io.Writer, outcomes []outcome) { } // writeUnmeasured says, for every run that took no measurement, why: the -// lines the script said it in, or that no record came back at all. +// lines of its output that say it (reasonLines), or that no record came back +// at all. func writeUnmeasured(w io.Writer, outcomes []outcome) { header := false for _, result := range outcomes { @@ -153,7 +246,7 @@ func writeUnmeasured(w io.Writer, outcomes []outcome) { } fmt.Fprintf(w, "\n#### %s on %s\n\n", codeSpan(result.Package), codeSpan(result.System)) if result.rec == nil { - fmt.Fprintf(w, "No record came back in %s: the run ended before its measuring step wrote one (a setup step failed, or the run was cancelled, timed out or lost its runner), or it did not upload the one it wrote, and its job log says which.\n", codeSpan(result.Artifact)) + fmt.Fprintf(w, "No record came back in %s: the run ended before its measuring step wrote one (a setup step failed, or the run was cancelled, timed out or lost its runner), or it did not upload the one it wrote, and its job log says which.\n", codeSpan(result.recordPath())) continue } fmt.Fprintf(w, "`scripts/coverage-conditions.sh` exited %d, which means it could not measure, and said:\n\n", result.rec.status) @@ -163,15 +256,27 @@ func writeUnmeasured(w io.Writer, outcomes []outcome) { } } -// reasonLines returns the lines the script wrote about why it could not -// measure, which it prefixes with `gobco: `, or the end of the output when it -// wrote none. +// failureLine is a line of a run's output that says why it could not +// measure: one the script wrote, which it prefixes with `gobco: `, or one +// gobco or its go test printed when either failed, which is what the +// script's verdict that the run is not a measurement follows. Those are a +// panic, which is how gobco stops on a package it cannot type-check; a +// failing top-level test; a test package that did not build or set up; and +// the compiler's position for either, which is the only line naming the +// cause of a setup failure. A condition gobco names has a position too, and +// is told apart by conditionPattern, since a report printed beside a failure +// says nothing about the failure. +var failureLine = regexp.MustCompile(`^(gobco: |panic: |--- FAIL: |FAIL\t.*\[(build|setup) failed\]$|\S+\.go:\d+:\d+: )`) + +// reasonLines returns the lines of a run's output that say why it could not +// measure, failureLine's, in the order they were printed, or the end of the +// output when there is none. func reasonLines(output string) string { var said, all []string for line := range strings.SplitSeq(strings.TrimRight(output, "\r\n"), "\n") { line = strings.TrimSuffix(line, "\r") all = append(all, line) - if strings.HasPrefix(line, "gobco: ") { + if failureLine.MatchString(line) && !conditionPattern.MatchString(line) { said = append(said, line) } } @@ -196,7 +301,7 @@ func systemCell(result outcome) string { // reported as not covered however many tests reach it. func coveredCell(result outcome) string { if result.measured != nil { - return result.measured.covered + " of " + result.measured.total + return fmt.Sprintf("%d of %d", result.measured.covered, result.measured.total) } if result.problem != "" { return "unreadable record" diff --git a/cmd/measure_conditions/summarize_test.go b/cmd/measure_conditions/summarize_test.go index d2de3b912..9cc47a8b4 100644 --- a/cmd/measure_conditions/summarize_test.go +++ b/cmd/measure_conditions/summarize_test.go @@ -17,11 +17,7 @@ func planOf(t *testing.T, packages, systems []string) string { if err != nil { t.Fatal(err) } - data, err := json.Marshal(planned) - if err != nil { - t.Fatal(err) - } - return string(data) + return encodePlan(t, planned) } // recordFor is a record as the workflow writes it, for a platform of the @@ -32,7 +28,8 @@ func recordFor(pkg, system, platform string, status int) string { // TestRunSummarize_Usage_IsRefusedWithStatus2 verifies every way the command // line can be wrong: an unknown flag, an argument, each of the four flags -// left empty, a plan that is not JSON and a plan with no run in it. +// left empty, a plan that is not JSON, a plan with no run in it, and a mode +// the script would refuse. func TestRunSummarize_Usage_IsRefusedWithStatus2(t *testing.T) { t.Parallel() plan := planOf(t, []string{"./cmd/server"}, []string{"macos-latest"}) @@ -58,6 +55,10 @@ func TestRunSummarize_Usage_IsRefusedWithStatus2(t *testing.T) { {name: "no commit", args: full("-commit", ""), wantStderr: flagsWanted}, {name: "a plan that is not JSON", args: full("-plan", "matrix="+plan), wantStderr: planWanted}, {name: "a plan with no run", args: full("-plan", `{"include":[]}`), wantStderr: planWanted}, + { + name: "a mode the script would refuse", args: append(full(), "-mode", "both"), + wantStderr: "measure_conditions: -mode \"both\" is neither condition nor branch, which is what scripts/coverage-conditions.sh reads\n", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -86,14 +87,39 @@ func TestRunSummarize_EveryRunMeasured_WritesTheTableAndPasses(t *testing.T) { t.Fatalf("runSummarize() = %d, want 0; stderr:\n%s", got, stderr.String()) } want := summaryIntro("all", "c0ffee") + - "| `./cmd/server` | `macos-latest` (darwin/arm64) | 12 of 12 | 0 | 0 | `gobco-macos-latest-cmd-server` |\n" + "| `./cmd/server` | `macos-latest` (darwin/arm64) | 12 of 12 | 0 | 0 | `gobco-macos-latest-cmd-server/cmd-server` |\n" + + totalsTable("| `macos-latest` | 1 of 1 | 12 of 12 | 100.0% |") + if stdout.String() != want || stderr.Len() != 0 { + t.Errorf("stdout =\n%s\nwant\n%s\nstderr = %q", stdout.String(), want, stderr.String()) + } +} + +// TestRunSummarize_BranchMode_ReadsTheBranchFigures verifies a dispatch that +// asked gobco for branch coverage: the summary says so in its heading and in +// what it says the figures count, and reads each record's figure in the words +// gobco writes it in that mode. +func TestRunSummarize_BranchMode_ReadsTheBranchFigures(t *testing.T) { + t.Parallel() + records := t.TempDir() + writeRun(t, records, recordFor("./cmd/server", "ubuntu-latest", "linux/amd64", 0), "Condition coverage: 9/9\nBranch coverage: 7/8\n") + plan := planOf(t, []string{"./cmd/server"}, []string{"ubuntu-latest"}) + var stdout, stderr bytes.Buffer + if got := runSummarize([]string{"-plan", plan, "-records", records, "-gate", "all", "-mode", "branch", "-commit", "c0ffee"}, &stdout, &stderr); got != 0 { + t.Fatalf("runSummarize() = %d, want 0; stderr:\n%s", got, stderr.String()) + } + want := "## Branch coverage, measured\n\n" + + "`GOBCO_GATE=all` at `c0ffee`. Every figure is the one gobco printed on the runner named, through `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`; none is inferred from another platform. gobco counts each decision twice, once per outcome: the whole condition of an `if` or a `for`, and each case of a `switch`, so a total is twice the number of decisions. No figure fails this workflow: it is a measurement, not a gate.\n\n" + + "| Package | System | Covered | Not both ways | Held by the gate | Raw output |\n" + + "| --- | --- | ---: | ---: | ---: | --- |\n" + + "| `./cmd/server` | `ubuntu-latest` (linux/amd64) | 7 of 8 | 0 | 0 | `gobco-ubuntu-latest-cmd-server/cmd-server` |\n" + + totalsTable("| `ubuntu-latest` | 1 of 1 | 7 of 8 | 87.5% |") if stdout.String() != want || stderr.Len() != 0 { t.Errorf("stdout =\n%s\nwant\n%s\nstderr = %q", stdout.String(), want, stderr.String()) } } // summaryIntro is the heading, the paragraph and the table head every -// summary opens with. +// summary of condition coverage opens with. func summaryIntro(gate, commit string) string { return "## Condition coverage, measured\n\n" + "`GOBCO_GATE=" + gate + "` at `" + commit + "`. Every figure is the one gobco printed on the runner named, through `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`; none is inferred from another platform. gobco counts each condition twice, once per outcome, so a total is twice the number of conditions. No figure fails this workflow: it is a measurement, not a gate.\n\n" + @@ -101,6 +127,91 @@ func summaryIntro(gate, commit string) string { "| --- | --- | ---: | ---: | ---: | --- |\n" } +// totalsTable is the totals section every summary carries after its table, +// with the rows given. +func totalsTable(rows ...string) string { + return "\n### Total\n\n" + + "A package that was not measured is in no total: the table above says which and why, and each share is of the packages its system measured.\n\n" + + "| System | Packages measured | Covered | Share |\n" + + "| --- | ---: | ---: | ---: |\n" + + strings.Join(rows, "\n") + "\n" +} + +// TestRunSummarize_ShardedPlan_ReadsEveryPackageOfEveryJob verifies the +// summary of a -tree plan: every package of every shard is a row of its own, +// read from its directory inside its shard's artifact, the total of a system +// adds up the packages it measured and leaves out the one it did not, and a +// package the plan names with no record is reported missing under its own +// record path. +func TestRunSummarize_ShardedPlan_ReadsEveryPackageOfEveryJob(t *testing.T) { + t.Parallel() + records := t.TempDir() + writeRun(t, filepath.Join(records, "gobco-ubuntu-latest-tree-01", "cmd-a"), recordFor("./cmd/a", "ubuntu-latest", "linux/amd64", 0), "Condition coverage: 3/4\n") + writeRun(t, filepath.Join(records, "gobco-ubuntu-latest-tree-01", "internal-c"), recordFor("./internal/c", "ubuntu-latest", "linux/amd64", 1), "gobco: could not instrument\n") + writeRun(t, filepath.Join(records, "gobco-ubuntu-latest-tree-02", "internal-b"), recordFor("./internal/b", "ubuntu-latest", "linux/amd64", 3), + "Condition coverage: 5/6\n"+`b.go:1:2: condition "x" was never evaluated`+"\n"+"gobco: held:\n"+` b.go:1:2: condition "x" was never evaluated`+"\n") + plan := encodePlan(t, buildShards([]string{"./cmd/a", "./internal/b", "./internal/c", "./internal/d"}, []string{"ubuntu-latest"}, 2)) + var stdout, stderr bytes.Buffer + if got := runSummarize([]string{"-plan", plan, "-records", records, "-gate", "all", "-commit", "c0ffee"}, &stdout, &stderr); got != 0 { + t.Fatalf("runSummarize() = %d, want 0; stderr:\n%s", got, stderr.String()) + } + want := summaryIntro("all", "c0ffee") + + "| `./cmd/a` | `ubuntu-latest` (linux/amd64) | 3 of 4 | 0 | 0 | `gobco-ubuntu-latest-tree-01/cmd-a` |\n" + + "| `./internal/c` | `ubuntu-latest` (linux/amd64) | not measured (exit 1) | - | - | `gobco-ubuntu-latest-tree-01/internal-c` |\n" + + "| `./internal/b` | `ubuntu-latest` (linux/amd64) | 5 of 6 | 1 | 1 | `gobco-ubuntu-latest-tree-02/internal-b` |\n" + + "| `./internal/d` | `ubuntu-latest` | no record | - | - | `gobco-ubuntu-latest-tree-02/internal-d` |\n" + + totalsTable("| `ubuntu-latest` | 2 of 4 | 8 of 10 | 80.0% |") + + "\n### Not evaluated both ways\n\n" + + "| Package | System | File | Line | Condition | gobco says | Held |\n" + + "| --- | --- | --- | ---: | --- | --- | --- |\n" + + "| `./internal/b` | `ubuntu-latest` | `b.go` | 1 | `x` | never evaluated | yes |\n" + + "\n### Not measured\n" + + "\n#### `./internal/c` on `ubuntu-latest`\n\n" + + "`scripts/coverage-conditions.sh` exited 1, which means it could not measure, and said:\n\n" + + "```text\ngobco: could not instrument\n```\n" + + "\n#### `./internal/d` on `ubuntu-latest`\n\n" + + "No record came back in `gobco-ubuntu-latest-tree-02/internal-d`: the run ended before its measuring step wrote one (a setup step failed, or the run was cancelled, timed out or lost its runner), or it did not upload the one it wrote, and its job log says which.\n" + if stdout.String() != want || stderr.Len() != 0 { + t.Errorf("stdout =\n%s\nwant\n%s\nstderr = %q", stdout.String(), want, stderr.String()) + } +} + +// encodePlan writes a plan as the plan subcommand prints it after matrix=. +func encodePlan(t *testing.T, planned matrix) string { + t.Helper() + data, err := json.Marshal(planned) + if err != nil { + t.Fatal(err) + } + return string(data) +} + +// TestShare_Figures_AreRoundedDownToATenth verifies that a share never reads +// higher than it is: 799 of 1000 is 79.9% and not 80.0%, two thirds is 66.6%, +// a whole is 100.0%, nothing covered of something is 0.0%, and nothing +// counted is a dash rather than a division by zero. +func TestShare_Figures_AreRoundedDownToATenth(t *testing.T) { + t.Parallel() + cases := []struct { + covered, outcomes int + want string + }{ + {covered: 799, outcomes: 1000, want: "79.9%"}, + {covered: 2, outcomes: 3, want: "66.6%"}, + {covered: 5, outcomes: 5, want: "100.0%"}, + {covered: 0, outcomes: 4, want: "0.0%"}, + {covered: 0, outcomes: 0, want: "-"}, + } + for _, tc := range cases { + t.Run(tc.want, func(t *testing.T) { + t.Parallel() + if got := share(tc.covered, tc.outcomes); got != tc.want { + t.Errorf("share(%d, %d) = %q, want %q", tc.covered, tc.outcomes, got, tc.want) + } + }) + } +} + // TestRunSummarize_EveryOutcome_IsReportedAndProblemsFailIt drives the summary // through everything a run can leave: a measurement whose gate held one of // the three conditions it names, one of them written across lines, a clean @@ -148,12 +259,17 @@ func TestRunSummarize_EveryOutcome_IsReportedAndProblemsFailIt(t *testing.T) { "a record names ./x on macos-latest, which the plan did not ask for", } want := summaryIntro("beyond:linux/amd64", "abc123") + - "| `./cmd/server` | `windows-latest` (windows/amd64) | 8 of 12 | 3 | 1 | `gobco-windows-latest-cmd-server` |\n" + - "| `./cmd/server` | `macos-latest` (darwin/arm64) | 12 of 12 | 0 | 0 | `gobco-macos-latest-cmd-server` |\n" + - "| `./cmd/server` | `ubuntu-latest` (linux/amd64) | not measured (exit 1) | - | - | `gobco-ubuntu-latest-cmd-server` |\n" + - "| `./internal/toolutil` | `windows-latest` | no record | - | - | `gobco-windows-latest-internal-toolutil` |\n" + - "| `./internal/toolutil` | `macos-latest` (darwin/arm64) | unreadable record | - | - | `gobco-macos-latest-internal-toolutil` |\n" + - "| `./internal/toolutil` | `ubuntu-latest` (linux/amd64) | not measured (exit 2) | - | - | `gobco-ubuntu-latest-internal-toolutil` |\n" + + "| `./cmd/server` | `windows-latest` (windows/amd64) | 8 of 12 | 3 | 1 | `gobco-windows-latest-cmd-server/cmd-server` |\n" + + "| `./cmd/server` | `macos-latest` (darwin/arm64) | 12 of 12 | 0 | 0 | `gobco-macos-latest-cmd-server/cmd-server` |\n" + + "| `./cmd/server` | `ubuntu-latest` (linux/amd64) | not measured (exit 1) | - | - | `gobco-ubuntu-latest-cmd-server/cmd-server` |\n" + + "| `./internal/toolutil` | `windows-latest` | no record | - | - | `gobco-windows-latest-internal-toolutil/internal-toolutil` |\n" + + "| `./internal/toolutil` | `macos-latest` (darwin/arm64) | unreadable record | - | - | `gobco-macos-latest-internal-toolutil/internal-toolutil` |\n" + + "| `./internal/toolutil` | `ubuntu-latest` (linux/amd64) | not measured (exit 2) | - | - | `gobco-ubuntu-latest-internal-toolutil/internal-toolutil` |\n" + + totalsTable( + "| `windows-latest` | 1 of 2 | 8 of 12 | 66.6% |", + "| `macos-latest` | 1 of 2 | 12 of 12 | 100.0% |", + "| `ubuntu-latest` | 0 of 2 | 0 of 0 | - |", + ) + "\n### Not evaluated both ways\n\n" + "| Package | System | File | Line | Condition | gobco says | Held |\n" + "| --- | --- | --- | ---: | --- | --- | --- |\n" + @@ -163,9 +279,9 @@ func TestRunSummarize_EveryOutcome_IsReportedAndProblemsFailIt(t *testing.T) { "\n### Not measured\n" + "\n#### `./cmd/server` on `ubuntu-latest`\n\n" + "`scripts/coverage-conditions.sh` exited 1, which means it could not measure, and said:\n\n" + - "````text\ngobco: a line quoting ``` three backticks\ngobco: gobco exited 1 on ./cmd/server\n````\n" + + "````text\n--- FAIL: TestX\ngobco: a line quoting ``` three backticks\ngobco: gobco exited 1 on ./cmd/server\n````\n" + "\n#### `./internal/toolutil` on `windows-latest`\n\n" + - "No record came back in `gobco-windows-latest-internal-toolutil`: the run ended before its measuring step wrote one (a setup step failed, or the run was cancelled, timed out or lost its runner), or it did not upload the one it wrote, and its job log says which.\n" + + "No record came back in `gobco-windows-latest-internal-toolutil/internal-toolutil`: the run ended before its measuring step wrote one (a setup step failed, or the run was cancelled, timed out or lost its runner), or it did not upload the one it wrote, and its job log says which.\n" + "\n#### `./internal/toolutil` on `ubuntu-latest`\n\n" + "`scripts/coverage-conditions.sh` exited 2, which means it could not measure, and said:\n\n" + "```text\n" + strings.Join(tail, "\n") + "\n```\n" + @@ -180,8 +296,15 @@ func TestRunSummarize_EveryOutcome_IsReportedAndProblemsFailIt(t *testing.T) { } // TestReasonLines_Outputs_KeepWhatTheScriptSaid verifies the reason a run -// that could not measure is shown with: the script's own lines when it wrote -// any, and otherwise the whole of a short output or the end of a long one. +// that could not measure is shown with: the script's own lines, and the +// lines gobco or its go test said the failure in before the script's verdict +// (a panic, a top-level failing test, a package that did not build or set +// up, a compiler's position), each in the order printed and without the +// conditions a report beside the failure names; and when none of those is +// there, the whole of a short output or the end of a long one. The three +// failures are the three ways the tree's unmeasured packages failed: an +// export_test.go symbol gobco cannot resolve, the bridge it writes with an +// empty import path, and tests that read the source gobco rewrote. func TestReasonLines_Outputs_KeepWhatTheScriptSaid(t *testing.T) { t.Parallel() cases := []struct { @@ -193,6 +316,32 @@ func TestReasonLines_Outputs_KeepWhatTheScriptSaid(t *testing.T) { {name: "a short output", output: "a\r\nb\n\n", want: "a\nb"}, {name: "nothing", output: "", want: ""}, {name: "a prefix without its space", output: "gobco:x\n", want: "gobco:x"}, + { + name: "gobco panicked", + output: "panic: a_test.go:49:27: undefined: a.PublishedActionIDs\n\ngoroutine 1 [running]:\nmain.ok(...)\n" + + "\t/go/pkg/mod/github.com/rillig/gobco@v1.3.4/util.go:100\nmain.(*instrumenter).resolveTypes(0x1)\n" + + "\t/go/pkg/mod/github.com/rillig/gobco@v1.3.4/instrumenter.go:143 +0x445\nexit status 2\n" + + "gobco: gobco exited 1 on ./a, so any figure above is not a measurement\n", + want: "panic: a_test.go:49:27: undefined: a.PublishedActionIDs\ngobco: gobco exited 1 on ./a, so any figure above is not a measurement", + }, + { + name: "the test package did not set up", + output: "# example.com/m/a\n/tmp/gobco-1/module/a/gobco_bridge_test.go:3:9: invalid import path: \r\n" + + "FAIL\texample.com/m/a [setup failed]\nFAIL\ngo test .: exit status 1\nexit status 1\ngobco: gobco exited 1 on ./a\n", + want: "/tmp/gobco-1/module/a/gobco_bridge_test.go:3:9: invalid import path: \nFAIL\texample.com/m/a [setup failed]\ngobco: gobco exited 1 on ./a", + }, + { + name: "the test package did not build", + output: "FAIL\texample.com/m/b [build failed]\nFAIL\tsomething else\ngobco: gobco exited 1 on ./b\n", + want: "FAIL\texample.com/m/b [build failed]\ngobco: gobco exited 1 on ./b", + }, + { + name: "a test failed beside a report", + output: "--- FAIL: TestCases (0.00s)\n --- FAIL: TestCases/one (0.00s)\n c_test.go:20: no case labels were read\n" + + "FAIL\nFAIL\texample.com/m/c\t1.1s\n\nCondition coverage: 3/4\n" + `c.go:4:9: condition "x == 1" was never evaluated` + "\n" + + "exit status 1\ngobco: gobco exited 1 on ./c\n", + want: "--- FAIL: TestCases (0.00s)\ngobco: gobco exited 1 on ./c", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/measure_conditions/tree.go b/cmd/measure_conditions/tree.go new file mode 100644 index 000000000..a58d9e8f6 --- /dev/null +++ b/cmd/measure_conditions/tree.go @@ -0,0 +1,116 @@ +package main + +import ( + "context" + "errors" + "flag" + "fmt" + "io" + "sync" +) + +// localSystem is what a tree measured on the machine at hand names its +// system in its records and its summary, since no runner label describes it. +const localSystem = "local" + +// runTree measures every package below cmd/ and internal/ on the machine at +// hand, in the -mode it is given, and writes the summary the workflow's +// summary job would, the total included, to stdout: the one command that +// reproduces the whole-tree figure without a dispatch. The records are left +// below -records, one directory per package, and a record left there by an +// earlier run is replaced. -parallel packages are measured at once; each +// run's output goes to its record alone, and stderr says what became of each +// package as it finishes. +// +// It returns 2 for a usage error, 1 when the packages could not be listed, a +// record could not be written, or the summary found a record it could not +// use, and 0 otherwise, whatever the figures say and however many packages +// gobco could not measure, which the summary names with the reason. +func runTree(args []string, stdout, stderr io.Writer, list lister, run scriptRunner) int { + flags := flag.NewFlagSet("tree", flag.ContinueOnError) + flags.SetOutput(stderr) + records := flags.String("records", "", "directory each package's record and raw output are written below, one directory per package") + commit := flags.String("commit", "", "the commit the tree is at, which the summary names") + gate := flags.String("gate", "all", "the GOBCO_GATE the script runs with: all or beyond:GOOS/GOARCH") + mode := flags.String("mode", defaultMode, "the GOBCO_MODE the script runs with: condition or branch") + parallel := flags.Int("parallel", 1, "how many packages are measured at once") + dir := flags.String("dir", ".", "module root the packages are looked for and measured in") + if err := flags.Parse(args); err != nil { + return 2 + } + if flags.NArg() > 0 || *records == "" || *commit == "" || *parallel < 1 { + fmt.Fprintf(stderr, "%s: tree takes -records and -commit, both non-empty, a -parallel of at least 1, and no arguments\n", toolName) + return 2 + } + if !gatePattern.MatchString(*gate) { + fmt.Fprintf(stderr, "%s: -gate %q is neither all nor beyond:GOOS/GOARCH, which is what scripts/coverage-conditions.sh reads\n", toolName, *gate) + return 2 + } + if refuseMode(stderr, *mode) { + return 2 + } + ctx := context.Background() + packages, err := treePackages(ctx, *dir, list) + if err != nil { + fmt.Fprintf(stderr, "%s: the packages below cmd/ and internal/ could not be listed: %v\n", toolName, err) + return 1 + } + if len(packages) == 0 { + fmt.Fprintf(stderr, "%s: there is no package below cmd/ and internal/, so there is nothing to measure\n", toolName) + return 1 + } + fmt.Fprintf(stderr, "%s: measuring the %d packages below cmd/ and internal/, %d at a time, with GOBCO_GATE=%s and GOBCO_MODE=%s\n", toolName, len(packages), *parallel, *gate, *mode) + m := measurer{run: run, dir: *dir, system: localSystem, records: *records, env: []string{"GOBCO_GATE=" + *gate, "GOBCO_MODE=" + *mode}} + if err = measureAll(ctx, m, packages, *parallel, stderr); err != nil { + fmt.Fprintf(stderr, "%s: %v\n", toolName, err) + return 1 + } + runs := make([]packageRun, 0, len(packages)) + for _, pkg := range packages { + runs = append(runs, packageRun{Package: pkg, System: localSystem}) + } + recs, problems := readRecords(*records) + outcomes, more := join(runs, recs, *mode) + problems = append(problems, more...) + render(stdout, *gate, *mode, *commit, outcomes, problems) + for _, problem := range problems { + fmt.Fprintf(stderr, "%s: %s\n", toolName, problem) + } + if len(problems) > 0 { + return 1 + } + return 0 +} + +// measureAll measures the packages with m, parallel at a time, and says on +// progress what became of each as it finishes. Every package is measured, and +// every one that could not be run or recorded is returned as an error at the +// end, in the order they finished. +func measureAll(ctx context.Context, m measurer, packages []string, parallel int, progress io.Writer) error { + var ( + mu sync.Mutex + failures []error + wg sync.WaitGroup + ) + work := make(chan string) + for range parallel { + wg.Go(func() { + for pkg := range work { + status, err := measurePackage(ctx, m, pkg, io.Discard) + mu.Lock() + if err != nil { + failures = append(failures, err) + } else { + fmt.Fprintf(progress, "%s: %s: %s\n", toolName, pkg, verdict(status)) + } + mu.Unlock() + } + }) + } + for _, pkg := range packages { + work <- pkg + } + close(work) + wg.Wait() + return errors.Join(failures...) +} diff --git a/cmd/measure_conditions/tree_test.go b/cmd/measure_conditions/tree_test.go new file mode 100644 index 000000000..93bad072c --- /dev/null +++ b/cmd/measure_conditions/tree_test.go @@ -0,0 +1,258 @@ +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" +) + +// treeListing is a listing of the three packages the tree tests measure. +func treeListing() string { + const module = "example.com/m" + return row(module+"/cmd/a", module, "a.go", "", "", "") + + row(module+"/internal/b", module, "b.go", "", "", "") + + row(module+"/internal/c", module, "c.go", "", "", "") +} + +// treeScript answers for the three packages: one measured, one whose gate +// held its one condition, and one the script could not measure. +func treeScript() *fakeScript { + return &fakeScript{ + outputs: map[string]string{ + "./cmd/a": "Condition coverage: 9/10\n", + "./internal/b": "Condition coverage: 3/4\n" + `b.go:7:2: condition "y" was never evaluated` + "\n" + "gobco: held:\n" + ` b.go:7:2: condition "y" was never evaluated` + "\n", + "./internal/c": "gobco: gobco exited 1 on ./internal/c\n", + }, + status: map[string]int{"./cmd/a": 0, "./internal/b": 3, "./internal/c": 1}, + } +} + +// TestRunTree_Usage_IsRefusedWithStatus2 verifies every way the command line +// can be wrong, before anything is listed: an unknown flag, an argument, no +// records, no commit, a -parallel below one, and a gate or a mode the script +// would refuse. +func TestRunTree_Usage_IsRefusedWithStatus2(t *testing.T) { + t.Parallel() + const flagsWanted = "measure_conditions: tree takes -records and -commit, both non-empty, a -parallel of at least 1, and no arguments\n" + cases := []struct { + name string + args []string + wantStderr string + }{ + {name: "an unknown flag", args: []string{"-record", "r"}, wantStderr: "flag provided but not defined: -record\n"}, + {name: "an argument", args: []string{"-records", "r", "-commit", "c", "extra"}, wantStderr: flagsWanted}, + {name: "no records", args: []string{"-commit", "c"}, wantStderr: flagsWanted}, + {name: "no commit", args: []string{"-records", "r"}, wantStderr: flagsWanted}, + {name: "no parallelism", args: []string{"-records", "r", "-commit", "c", "-parallel", "0"}, wantStderr: flagsWanted}, + { + name: "a gate the script would refuse", args: []string{"-records", "r", "-commit", "c", "-gate", "some"}, + wantStderr: "measure_conditions: -gate \"some\" is neither all nor beyond:GOOS/GOARCH, which is what scripts/coverage-conditions.sh reads\n", + }, + { + name: "a mode the script would refuse", args: []string{"-records", "r", "-commit", "c", "-mode", "Branch"}, + wantStderr: "measure_conditions: -mode \"Branch\" is neither condition nor branch, which is what scripts/coverage-conditions.sh reads\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var calls []listCall + script := &fakeScript{} + var stdout, stderr bytes.Buffer + if got := runTree(tc.args, &stdout, &stderr, fakeLister(nil, "", &calls), script.run); got != 2 { + t.Fatalf("runTree() = %d, want 2", got) + } + if stdout.Len() != 0 || !strings.HasPrefix(stderr.String(), tc.wantStderr) || len(calls)+len(script.calls) != 0 { + t.Errorf("stdout = %q, stderr = %q, %d listing(s) and %d script call(s); want nothing, %q and none", stdout.String(), stderr.String(), len(calls), len(script.calls), tc.wantStderr) + } + }) + } +} + +// TestRunTree_Listing_FailuresStopBeforeMeasuring verifies that a tree that +// could not be listed, and one with no package below cmd/ and internal/, +// measure nothing and fail with the reason. +func TestRunTree_Listing_FailuresStopBeforeMeasuring(t *testing.T) { + t.Parallel() + cases := []struct { + name string + list lister + wantStderr string + }{ + { + name: "the listing failed", list: treeLister("", errors.New("planted failure")), + wantStderr: "measure_conditions: the packages below cmd/ and internal/ could not be listed: go list: planted failure\n", + }, + { + name: "nothing below cmd/ and internal/", list: treeLister("", nil), + wantStderr: "measure_conditions: there is no package below cmd/ and internal/, so there is nothing to measure\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + script := &fakeScript{} + var stdout, stderr bytes.Buffer + if got := runTree([]string{"-records", t.TempDir(), "-commit", "c"}, &stdout, &stderr, tc.list, script.run); got != 1 { + t.Fatalf("runTree() = %d, want 1", got) + } + if stdout.Len() != 0 || stderr.String() != tc.wantStderr || len(script.calls) != 0 { + t.Errorf("stdout = %q, stderr = %q after %d script call(s); want nothing, %q and none", stdout.String(), stderr.String(), len(script.calls), tc.wantStderr) + } + }) + } +} + +// TestRunTree_EveryPackage_IsMeasuredAndSummarized verifies the whole-tree +// run end to end, two packages at a time: every package listed is measured +// once, in the module root, with the gate in its environment; the summary is +// the workflow's, a row per package in the order listed, each record below +// its own directory of -records, and the total of the packages measured, +// with the one the script could not measure named and left out of it; and +// stderr says what became of each package. A package gobco could not +// measure is the summary's to report, not a failure of the run. +func TestRunTree_EveryPackage_IsMeasuredAndSummarized(t *testing.T) { + t.Parallel() + script := treeScript() + records := t.TempDir() + var stdout, stderr bytes.Buffer + args := []string{"-records", records, "-commit", "c0ffee", "-gate", "beyond:linux/amd64", "-parallel", "2", "-dir", "root"} + if got := runTree(args, &stdout, &stderr, treeLister(treeListing(), nil), script.run); got != 0 { + t.Fatalf("runTree() = %d, want 0; stderr:\n%s", got, stderr.String()) + } + slices.SortFunc(script.calls, func(a, b scriptCall) int { return strings.Compare(a.pkg, b.pkg) }) + env := []string{"GOBCO_GATE=beyond:linux/amd64", "GOBCO_MODE=condition"} + wantCalls := []scriptCall{{dir: "root", pkg: "./cmd/a", env: env}, {dir: "root", pkg: "./internal/b", env: env}, {dir: "root", pkg: "./internal/c", env: env}} + if !reflect.DeepEqual(script.calls, wantCalls) { + t.Errorf("script calls = %+v, want %+v", script.calls, wantCalls) + } + local := "`local` (" + platform + ")" + want := summaryIntro("beyond:linux/amd64", "c0ffee") + + "| `./cmd/a` | " + local + " | 9 of 10 | 0 | 0 | `cmd-a` |\n" + + "| `./internal/b` | " + local + " | 3 of 4 | 1 | 1 | `internal-b` |\n" + + "| `./internal/c` | " + local + " | not measured (exit 1) | - | - | `internal-c` |\n" + + totalsTable("| `local` | 2 of 3 | 12 of 14 | 85.7% |") + + "\n### Not evaluated both ways\n\n" + + "| Package | System | File | Line | Condition | gobco says | Held |\n" + + "| --- | --- | --- | ---: | --- | --- | --- |\n" + + "| `./internal/b` | `local` | `b.go` | 7 | `y` | never evaluated | yes |\n" + + "\n### Not measured\n" + + "\n#### `./internal/c` on `local`\n\n" + + "`scripts/coverage-conditions.sh` exited 1, which means it could not measure, and said:\n\n" + + "```text\ngobco: gobco exited 1 on ./internal/c\n```\n" + if stdout.String() != want { + t.Errorf("stdout =\n%s\nwant\n%s", stdout.String(), want) + } + lines := strings.Split(strings.TrimSuffix(stderr.String(), "\n"), "\n") + wantFirst := "measure_conditions: measuring the 3 packages below cmd/ and internal/, 2 at a time, with GOBCO_GATE=beyond:linux/amd64 and GOBCO_MODE=condition" + if len(lines) != 4 || lines[0] != wantFirst { + t.Fatalf("stderr lines = %q, want %q and one line per package", lines, wantFirst) + } + progress := slices.Sorted(slices.Values(lines[1:])) + wantProgress := []string{ + "measure_conditions: ./cmd/a: measured (exit 0)", + "measure_conditions: ./internal/b: measured (exit 3)", + "measure_conditions: ./internal/c: not measured (exit 1)", + } + if !reflect.DeepEqual(progress, wantProgress) { + t.Errorf("progress lines = %q, want %q in any order", progress, wantProgress) + } +} + +// TestRunTree_BranchMode_CountsBranchesThroughTheScript verifies the tree in +// gobco's branch mode: the mode reaches every run of the script beside the +// gate, and the summary is of branch coverage, read from the figure gobco +// writes in that mode, so a package whose report carries only a condition +// figure is a record the summary could not use and fails the run. +func TestRunTree_BranchMode_CountsBranchesThroughTheScript(t *testing.T) { + t.Parallel() + script := &fakeScript{ + outputs: map[string]string{ + "./cmd/a": "Branch coverage: 4/4\n", + "./internal/b": "Branch coverage: 5/6\n" + `b.go:3:4: condition "x && y" was once true but never false` + "\n" + + "gobco: held:\n" + ` b.go:3:4: condition "x && y" was once true but never false` + "\n", + "./internal/c": "Condition coverage: 9/10\n", + }, + status: map[string]int{"./cmd/a": 0, "./internal/b": 3, "./internal/c": 0}, + } + var stdout, stderr bytes.Buffer + args := []string{"-records", t.TempDir(), "-commit", "c0ffee", "-mode", "branch"} + if got := runTree(args, &stdout, &stderr, treeLister(treeListing(), nil), script.run); got != 1 { + t.Fatalf("runTree() = %d, want 1; stderr:\n%s", got, stderr.String()) + } + for _, call := range script.calls { + if want := []string{"GOBCO_GATE=all", "GOBCO_MODE=branch"}; !reflect.DeepEqual(call.env, want) { + t.Errorf("the script ran over %s with %q, want %q", call.pkg, call.env, want) + } + } + local := "`local` (" + platform + ")" + const problem = "the record of ./internal/c on local cannot be read: the script's status says it measured, and its output carries no Branch coverage figure" + want := "## Branch coverage, measured\n\n" + + "`GOBCO_GATE=all` at `c0ffee`. Every figure is the one gobco printed on the runner named, through `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`; none is inferred from another platform. gobco counts each decision twice, once per outcome: the whole condition of an `if` or a `for`, and each case of a `switch`, so a total is twice the number of decisions. No figure fails this workflow: it is a measurement, not a gate.\n\n" + + "| Package | System | Covered | Not both ways | Held by the gate | Raw output |\n" + + "| --- | --- | ---: | ---: | ---: | --- |\n" + + "| `./cmd/a` | " + local + " | 4 of 4 | 0 | 0 | `cmd-a` |\n" + + "| `./internal/b` | " + local + " | 5 of 6 | 1 | 1 | `internal-b` |\n" + + "| `./internal/c` | " + local + " | unreadable record | - | - | `internal-c` |\n" + + totalsTable("| `local` | 2 of 3 | 9 of 10 | 90.0% |") + + "\n### Not evaluated both ways\n\n" + + "| Package | System | File | Line | Condition | gobco says | Held |\n" + + "| --- | --- | --- | ---: | --- | --- | --- |\n" + + "| `./internal/b` | `local` | `b.go` | 3 | `x && y` | once true but never false | yes |\n" + + "\n### Records this summary could not use\n\n" + + "- " + problem + "\n" + if stdout.String() != want { + t.Errorf("stdout =\n%s\nwant\n%s", stdout.String(), want) + } + if !strings.HasPrefix(stderr.String(), "measure_conditions: measuring the 3 packages below cmd/ and internal/, 1 at a time, with GOBCO_GATE=all and GOBCO_MODE=branch\n") || + !strings.HasSuffix(stderr.String(), "measure_conditions: "+problem+"\n") { + t.Errorf("stderr =\n%s\nwant the mode named first and the unusable record last", stderr.String()) + } +} + +// TestRunTree_StaleRecord_IsAProblemOfTheSummary verifies that a record an +// earlier run left for a package the tree no longer holds is not counted: the +// summary names it as a record the plan did not ask for, and the run fails, +// while every package of the tree is still measured and its own earlier +// record replaced. +func TestRunTree_StaleRecord_IsAProblemOfTheSummary(t *testing.T) { + t.Parallel() + records := t.TempDir() + writeRun(t, filepath.Join(records, "cmd-gone"), recordFor("./cmd/gone", localSystem, "linux/amd64", 0), "Condition coverage: 1/2\n") + writeRun(t, filepath.Join(records, "cmd-a"), recordFor("./cmd/a", localSystem, "linux/amd64", 1), "an earlier run\n") + var stdout, stderr bytes.Buffer + if got := runTree([]string{"-records", records, "-commit", "c"}, &stdout, &stderr, treeLister(treeListing(), nil), treeScript().run); got != 1 { + t.Fatalf("runTree() = %d, want 1", got) + } + const problem = "a record names ./cmd/gone on local, which the plan did not ask for" + if !strings.Contains(stdout.String(), "- "+problem+"\n") || !strings.HasSuffix(stderr.String(), "measure_conditions: "+problem+"\n") { + t.Errorf("stdout =\n%s\nstderr =\n%s\nwant the stale record named in both", stdout.String(), stderr.String()) + } + if got := readFile(t, filepath.Join(records, "cmd-a", outputFile)); got != "Condition coverage: 9/10\n" { + t.Errorf("the record of ./cmd/a holds %q, want this run's output", got) + } +} + +// TestRunTree_UnwritableRecords_FailWithEveryPackagesReason verifies that a +// -records the records cannot be written below fails the run with the +// reason of each package, after all of them were run, and writes no summary. +func TestRunTree_UnwritableRecords_FailWithEveryPackagesReason(t *testing.T) { + t.Parallel() + file := filepath.Join(t.TempDir(), "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + script := treeScript() + var stdout, stderr bytes.Buffer + if got := runTree([]string{"-records", file, "-commit", "c"}, &stdout, &stderr, treeLister(treeListing(), nil), script.run); got != 1 { + t.Fatalf("runTree() = %d, want 1", got) + } + if stdout.Len() != 0 || len(script.calls) != 3 || strings.Count(stderr.String(), "mkdir "+file) != 3 { + t.Errorf("stdout = %q, %d script call(s), stderr =\n%s\nwant no summary, three calls and three mkdir failures", stdout.String(), len(script.calls), stderr.String()) + } +} diff --git a/docs/development/cmd-utilities.md b/docs/development/cmd-utilities.md index be4a098dc..79d1d4621 100644 --- a/docs/development/cmd-utilities.md +++ b/docs/development/cmd-utilities.md @@ -57,7 +57,8 @@ Repository tooling is a Go command under `cmd/`, sharing what it needs through ` | `gen_third_party_notices` | Generators | Writes `THIRD_PARTY_NOTICES`, the license, notice and patent texts of every module the release binaries link, read from their build information, the packages each links and the module cache | Release time: GoReleaser's `sboms`, the Dockerfile, `make mcpb` | | `format_md_tables` | Formatters | Normalizes Markdown pipe tables in `README.md`, `docs/` and `site/src/content/docs/` | part of `make audit-docs` | | `bench_resources` | Benchmarks | Measures what the server costs to run (memory, startup, a second credential), draws the published charts, measures whether a bound leaves a quiet tenant better off, and measures what a held call and a stateful session cost the process and what the process does with each past its descriptor limit | `make bench-resources`, `make bench-fairness`, `make bench-held`, `make bench-sessions` | -| `measure_conditions` | Measurements | Plans the on-demand condition coverage workflow (`conditions.yml`) and writes its job summary from the records the runs left | Dispatch only: `gh workflow run conditions.yml` | +| `measure_conditions` | Measurements | Plans and runs the on-demand condition coverage workflow (`conditions.yml`), one package or a shard of the tree a job, writes its job summary with each runner's total, and takes the whole tree's figure on the machine at hand | Dispatch: `gh workflow run conditions.yml`; here: `make coverage-conditions-tree` | +| `fuzz_targets` | Measurements | Lists every fuzz target of the module as the matrix the weekly fuzzing workflow (`fuzz.yml`) fans out over | The plan job of `fuzz.yml`; `go run ./cmd/fuzz_targets` | | `gen_model_corpus` | Evaluation | Renders the model evaluation corpus breadth ledger: what the corpus asks about, counted against the action catalog | `make gen-model-corpus`, `make check-model-corpus` | | `gen_model_results` | Evaluation | Folds a run's observation shards into the committed record, scores them and redraws the published results page | `make gen-model-results`, `make model-results-record`, `make model-results-refold`, `make check-model-results` | | `server` | Server | The main `gitlab-mcp-server` MCP binary (runtime entry point) | `make build`, `make run` | @@ -2557,7 +2558,7 @@ The measurement record, the SVG chart pairs under the site chart directory (one ### measure_conditions -The two ends of `.github/workflows/conditions.yml`, the condition coverage measurement dispatched by hand on Windows, macOS and Linux (issue 1210). The measuring itself is `scripts/coverage-conditions.sh`, the recipe behind `make coverage-conditions`, run through bash on each runner; this command plans the runs and writes the job summary from what they left. See [Testing Documentation](testing/README.md) for how to dispatch it. +Everything around `scripts/coverage-conditions.sh` in `.github/workflows/conditions.yml`, the condition coverage measurement dispatched by hand on Windows, macOS and Linux (issue 1210), and the same measurement of the whole tree on the machine at hand. The measuring itself is the script, the recipe behind `make coverage-conditions`, run through bash; this command plans the jobs, runs the script over each job's packages and writes the job summary from what they left. See [Testing Documentation](testing/README.md) for how to dispatch it and for the whole-tree figure. #### Usage @@ -2568,31 +2569,83 @@ go run ./cmd/measure_conditions/ plan -packages "./cmd/server" -systems "windows # An empty package list: every package carrying a GOOS- or GOARCH-constrained file go run ./cmd/measure_conditions/ plan -systems ubuntu-latest -gate beyond:linux/amd64 +# Every package below cmd/ and internal/, in shards, counting branches rather than conditions +go run ./cmd/measure_conditions/ plan -tree -systems ubuntu-latest -gate all -mode branch + +# What a job runs: its packages, one after another, into a directory of records +go run ./cmd/measure_conditions/ measure -packages "./cmd/server ./internal/toolutil" -system ubuntu-latest -records records/ + # Write the job summary from the downloaded artifacts go run ./cmd/measure_conditions/ summarize -plan "$MATRIX" -records records/ -gate all -commit "$(git rev-parse HEAD)" + +# The whole tree here, and its summary (make coverage-conditions-tree) +go run ./cmd/measure_conditions/ tree -records dist/conditions-tree -parallel 3 -commit "$(git rev-parse HEAD)" ``` +Every subcommand but `measure` takes `-mode`, the `GOBCO_MODE` the script runs with, `condition` when it is not given: `condition` is gobco's default, which counts every atomic condition (each operand of `&&` and `||`, and each comparison wherever it is written), and `branch` is gobco's `-branch`, which counts the whole controlling condition of each `if` and `for` and each case of a `switch`. A mode the script would refuse is refused with exit 2. `measure` reads none, since the script takes `GOBCO_MODE` from the environment the job gives it, and `summarize` reads each record's figure in the words of its mode, so a record of the other mode is one it cannot use. + #### plan -| Flag | Default | Description | -| ----------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | -| `-packages` | (empty) | Package directories, `./cmd/server` style, separated by spaces or commas; empty finds every package whose built files differ between release targets | -| `-systems` | (empty) | Runners: `windows-latest`, `macos-latest`, `ubuntu-latest`; at least one, and no other label | -| `-gate` | (empty) | The `GOBCO_GATE` the script runs with: `all` or `beyond:GOOS/GOARCH`, refused here as the script would refuse it | -| `-dir` | `.` | Module root the discovery lists | +| Flag | Default | Description | +| ----------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | +| `-packages` | (empty) | Package directories, `./cmd/server` style, separated by spaces or commas; empty finds every package whose built files differ between release targets | +| `-systems` | (empty) | Runners: `windows-latest`, `macos-latest`, `ubuntu-latest`; at least one, and no other label | +| `-gate` | (empty) | The `GOBCO_GATE` the script runs with: `all` or `beyond:GOOS/GOARCH`, refused here as the script would refuse it | +| `-mode` | `condition` | The `GOBCO_MODE` the script runs with: `condition` or `branch`, named on stderr with the gate | +| `-tree` | false | Plan every package below `cmd/` and `internal/` instead, in shards; refused beside a package list | +| `-dir` | `.` | Module root the discovery lists | + +A package is constrained when the files the go command builds for it differ between two of the platforms `.goreleaser.yml` builds (its `goos` crossed with its `goarch`), listed with `CGO_ENABLED=1` under each, which is the set `audit_dead_consts` reads a package again under. A file behind a tag alone differs on no release target and is left out. Without `-tree` each package is a job of its own on each runner, named after it. With `-tree` the packages the go command lists for `./cmd/... ./internal/...` are dealt round twenty shards, the first to the first shard, the second to the second and so on round again, so the slow packages of one directory land in different jobs; each shard is a job on each runner, named `tree NN of 20`, whose artifact is `gobco--tree-NN`. Exit 2 for an input it refuses, 1 when the discovery failed or found nothing. -A package is constrained when the files the go command builds for it differ between two of the platforms `.goreleaser.yml` builds (its `goos` crossed with its `goarch`), listed with `CGO_ENABLED=1` under each, which is the set `audit_dead_consts` reads a package again under. A file behind a tag alone differs on no release target and is left out. Exit 2 for an input it refuses, 1 when the discovery failed or found nothing. +#### measure + +| Flag | Default | Description | +| ----------- | ------- | ---------------------------------------------------------------------------------- | +| `-packages` | (empty) | The job's packages, measured one after another in the order named | +| `-system` | (empty) | The runner the records name | +| `-records` | (empty) | Directory the records are written below, one directory per package, named after it | +| `-dir` | `.` | Module root the script runs in | + +It runs the script through bash over each package, with `GOBCO_GATE`, `GOBCO_MODE` and `GOBCO_TEST_FLAGS` as the environment gives them, streams the output to stdout and leaves, in the package's directory of the records, `gobco.txt` with everything the script printed and then `run.env` (`package`, `system`, `platform`, `status`, one `key=value` each). A status of 0 or 3 is a measurement (3 is the gate holding a condition, which the script uses for nothing else) and any other is none. It exits 1 when a package could not be measured, once the job's other packages have been, or when the script could not be run or a record written, which stops it there, and 2 for a usage error. #### summarize -| Flag | Default | Description | -| ---------- | ------- | --------------------------------------------------------------- | -| `-plan` | (empty) | The matrix `plan` printed, as JSON | -| `-records` | (empty) | Directory the artifacts were downloaded into, read at any depth | -| `-gate` | (empty) | The gate the runs used, named in the summary | -| `-commit` | (empty) | The commit measured, named in the summary | +| Flag | Default | Description | +| ---------- | ----------- | ----------------------------------------------------------------------------------------------- | +| `-plan` | (empty) | The matrix `plan` printed, as JSON | +| `-records` | (empty) | Directory the artifacts were downloaded into, read at any depth | +| `-gate` | (empty) | The gate the runs used, named in the summary | +| `-mode` | `condition` | The mode the runs measured in, which heads the summary and whose figure each record is read for | +| `-commit` | (empty) | The commit measured, named in the summary | + +The summary is one row per package and runner with gobco's figure and where its record lies, each runner's total over the packages it measured (how many of its planned packages were measured, the outcomes evaluated of those there are, and that share rounded down to a tenth of a percent, so it never reads higher than it is), every condition not evaluated both ways with its file and line, and why each unmeasured package was not: the lines of its output that say so, which are the script's own `gobco:` lines and what gobco or its `go test` printed before them (a panic, a failing top-level test, a test package that did not build or set up, a compiler position), or its last twenty lines when none does. A package that was not measured is in no total. It exits 0 whatever the figures say, 1 when a record cannot be read, names a run the plan did not ask for, repeats one, or disagrees with its own status, and 2 for a usage error. + +#### tree + +| Flag | Default | Description | +| ----------- | ----------- | --------------------------------------------------------------------------------------------- | +| `-records` | (empty) | Directory the records are written below, one directory per package; an earlier one's replaced | +| `-commit` | (empty) | The commit the tree is at, named in the summary | +| `-gate` | `all` | The `GOBCO_GATE` the script runs with | +| `-mode` | `condition` | The `GOBCO_MODE` the script runs with, and the summary's mode | +| `-parallel` | 1 | How many packages are measured at once | +| `-dir` | `.` | Module root the packages are listed and measured in | + +The whole of a `-tree` dispatch on the machine at hand: it lists the packages below `cmd/` and `internal/`, measures each into the records, writing its output there alone and a line per package on stderr as it finishes, and writes the summary `summarize` would to stdout, with its system named `local`. `make coverage-conditions-tree` runs it with the records in `dist/conditions-tree` (`CONDITIONS_RECORDS`), `CONDITIONS_PARALLEL` packages at once and the mode `CONDITIONS_MODE` names, and keeps the summary beside them as `summary.md`. A package gobco could not measure is the summary's to name, not a failure: it exits 1 only when the packages could not be listed, a record could not be written, or the summary found a record it could not use (one an earlier run left for a package the tree no longer holds among them), and 2 for a usage error. + +### fuzz_targets + +The plan of `.github/workflows/fuzz.yml`, which fuzzes every target of the module with `go test -fuzz` once a week (see [Fuzzing](static-analysis.md#fuzzing)). It lists every package with the toolchain's own go, parses the internal and external test files the go command builds for each on this platform, and prints one `matrix=` line for `$GITHUB_OUTPUT` with a cell per target: the package as `./` and its directory, the target's name, and the artifact a failing input is uploaded under, `fuzz--`. Each target is also named on stderr, with the count. + +```bash +go run ./cmd/fuzz_targets/ +``` + +| Flag | Default | Description | +| ------ | ------- | ---------------------------------------------- | +| `-dir` | `.` | Module root the fuzz targets are looked for in | -Each run leaves `run.env` (`package`, `system`, `platform`, `status`, one `key=value` each) beside `gobco.txt`, everything the script printed. A status of 0 or 3 is a measurement (3 is the gate holding a condition, which the script uses for nothing else) and any other is none. The summary is one row per run with gobco's figure, every condition not evaluated both ways with its file and line, and the reason each unmeasured run gave. It exits 0 whatever the figures say, 1 when a record cannot be read, names a run the plan did not ask for, repeats one, or disagrees with its own status, and 2 for a usage error. +A target is a top-level function whose name is `Fuzz`, or `Fuzz` followed by a character that is not a lower-case letter, which is the rule the go command reads one by (`FuzzMeterFor` is one, `Fuzzy` is not); a method is not. A file behind a build tag the listing is not given is not read, and could not be fuzzed by the workflow either. The targets are found anew on every run, so a new one is fuzzed from the next run without an edit to the workflow. It exits 1 when the module could not be listed, a test file could not be parsed, two targets would upload under one artifact name, or there is no target at all, since a plan with nothing in it would pass the workflow while it fuzzed nothing, and 2 on a usage error. ## Evaluation diff --git a/docs/development/development.md b/docs/development/development.md index ce5a86667..dfc90b05d 100644 --- a/docs/development/development.md +++ b/docs/development/development.md @@ -235,6 +235,7 @@ Unit tests live alongside the code in each sub-package. They use `net/http/httpt make test # Standard tests with coverage make test-race # Tests with race detector (RACE_TIMEOUT=60m per package; the suite takes the better part of an hour) make coverage-conditions PKG=./internal/foo # gobco: boolean conditions never evaluated both ways (each is a missing case) +make coverage-conditions-tree CONDITIONS_PARALLEL=3 # gobco over every package of cmd/ and internal/, and the total (hours) make coverage-mutants PKG=./internal/foo # gremlins: mutation testing; Lived 0 and Not covered 0 is the gate on a changed package go test ./internal/... -count=1 # Run all unit tests (199 packages) go test ./internal/tools/branches/ -count=1 -v # Run one domain verbose diff --git a/docs/development/repository-map.md b/docs/development/repository-map.md index 3eac1cf35..0d54bbe2f 100644 --- a/docs/development/repository-map.md +++ b/docs/development/repository-map.md @@ -46,7 +46,8 @@ gitlab-mcp-server/ │ ├── gen_model_corpus/ # Renders the model evaluation corpus breadth ledger: which catalog actions, domains and tiers the corpus asks a model to reach, counted against the catalog this tree builds. Breadth and never coverage │ ├── gen_model_results/ # The fold: a model evaluation run's observation shards into the committed record, scored here and not there, and the record into the published results page. A run writes observation and no verdict, so a scoring rule corrected today re-scores a past run from its shards without spending a token (`-refold`, each dropped row named); a second fold of a run already published is refused rather than replacing it, and a run whose shards were not kept cannot be re-scored at all. `-check` is the offline gate │ ├── format_md_tables/ # Formats Markdown pipe tables in README.md, docs/ and the site's pages under site/src/content/docs, .mdx included (make check-md-tables) -│ ├── measure_conditions/ # The two ends of `.github/workflows/conditions.yml`, the condition coverage measurement dispatched by hand on Windows, macOS and Linux (issue 1210), whose runs are `scripts/coverage-conditions.sh` itself through bash. `plan` validates the dispatch (packages, the three hosted runners, a `GOBCO_GATE` the script reads) and prints the matrix for `$GITHUB_OUTPUT`; an empty package list means every package whose built files differ between two of the release's targets, listed with the toolchain's own `go` under each (today `cmd/server`, `internal/toolutil` and `internal/tools/packages`). `summarize` reads each run's `run.env` and `gobco.txt` wherever the artifact download put them and writes the job summary: gobco's figure per package and runner, every condition not evaluated both ways with its file and line, and why a run that could not measure could not. The script exits 3 for a gate that held a condition and for nothing else, so a status of 0 or 3 is a measurement and any other is none; no figure fails the workflow, and the command exits 1 only on a record it cannot read or that disagrees with the plan or with itself. Its tests hold the workflow to the record it reads +│ ├── measure_conditions/ # Everything around `scripts/coverage-conditions.sh` in `.github/workflows/conditions.yml`, the condition coverage measurement dispatched by hand on Windows, macOS and Linux (issue 1210). `plan` validates the dispatch (packages, the three hosted runners, a `GOBCO_GATE` and a `GOBCO_MODE` the script reads, the mode being gobco's condition counting or its `-branch`) and prints the matrix for `$GITHUB_OUTPUT`; an empty package list means every package whose built files differ between two of the release's targets, listed with the toolchain's own `go` under each (today `cmd/server`, `internal/toolutil` and `internal/tools/packages`), and `-tree` every package below `cmd/` and `internal/`, dealt round twenty shards. `measure` is what each job runs: the script through bash over each of its packages, one after another, leaving a `run.env` and a `gobco.txt` per package. `summarize` reads them wherever the artifact download put them and writes the job summary: gobco's figure per package and runner, each runner's total over the packages it measured, rounded down, every condition not evaluated both ways with its file and line, and why a package that could not be measured could not. `tree` is a whole `-tree` dispatch on the machine at hand (`make coverage-conditions-tree`), the command the figures in `docs/development/testing/README.md` are taken with, in either mode. The script exits 3 for a gate that held a condition and for nothing else, so a status of 0 or 3 is a measurement and any other is none; no figure fails the workflow, and `summarize` exits 1 only on a record it cannot read or that disagrees with the plan or with itself. Its tests hold the workflow to the record it reads +│ ├── fuzz_targets/ # The plan of `.github/workflows/fuzz.yml`, which fuzzes every target with `go test -fuzz` for ten minutes once a week, on dispatch, and for a minute on a pull request that changes the workflow or this command: lists every package with the toolchain's own `go`, parses the test files the go command builds for each, and prints a matrix cell per fuzz target (package, name, the artifact its failing input is uploaded under), refusing a plan with none. Its tests hold the workflow to the matrix │ ├── gen_action_catalog_manifest/ # Generates audited action catalog manifest │ ├── gen_api_live/ # The one oracle here that is **evaluated rather than parsed**: boots a released `gitlab-ee` image, asks the loaded Rails application what its REST API is through one `gitlab-rails runner` script, and writes `docs/development/gitlab-api-live.json` (make gen-api-live). It carries every Grape entity with the fields it exposes, the entity each field renders with and the condition gating it **with that condition's text read back from inside the image**; every mounted route with the entity its `desc` annotates and its params with type, requiredness and default; and the licensed-feature table evaluated. An exposure declared `merge: true` is marked, and `apilive` resolves it into the keys it contributes instead of a key of its own: it sends its child's keys on the parent and none of its own, so reading it literally inverted both directions of the comparison at once (a member "carrying" a `user` object it never sends, while the nine `UserBasic` keys it does send read as invented by us), which cost 34 phantom findings and hid 16 real ones. `-check` gates the committed record with no Docker and no network (make check-api-live), refusing a record too small to have come from a GitLab, one holding an entity that refused to describe itself, one holding a condition that carries neither text, hash nor symbol (schema version 2 recorded all 41 hash and symbol conditions of 19.3.1-ee as their kind alone, so the fields they gate read as sent on every response; the generator refuses to write such a record too), or one past the shared staleness window. It needs **no licence and no fixtures**, because a licence gates `feature_available?` when a request is served and not when a class is defined. Why it exists: a scan of the same Ruby cannot see a name that is not written down, and `GeoSiteStatus` exposes ~600 fields by iterating a constant assembled from two method calls, so the source says "expose the loop variable"; measured against the scanned record with inheritance resolved, 551 of 582 entities agree and the 31 that do not hold 1935 fields the scan never saw. Since schema version 4 the same boot records what a fine-grained token needs (issue 952): each route's `authorization` key for key, the `granular` vocabulary (every assignable permission with its raw expansion and GitLab's words for it, the assignable GitLab names for each raw permission and the first one a token can be granted, and the anonymous policy evaluated on an unsaved public project and group with every feature and licence available), and `graphql_authz` (each object type's enforcement, directives and object-typed field signatures, every union and interface with its members, every mutation, and the undeclared set computed with GitLab's own todo rule, held to the `authorization_todo.txt` the image ships); a route with no `authorization` now declares nothing rather than going unrecorded, which is why the version moved, and `-check` refuses either block missing or holding a shape a reader would misread, and a `source` block whose figures are not what the content gives │ ├── gen_action_grants/ # Derives what every catalog action sends from its handlers (`cmd/internal/actionrequests` for the handlers, `cmd/internal/sdkroutes` for what each client-go method sends, the statements read as sequences, alternatives and optional requests, a `//gitlab:request mandatory|optional|alternatives: ` directive where the syntax is wrong) and joins each request to what `gitlab-api-live.json` declares a fine-grained personal access token needs for it (REST routes with their primary and additional scopes; GraphQL positions along the answer spine, a non-null chain carried onto it, an abstract position judged as its worst member, an undeclared mutation refused and an undeclared payload committed and answered null), writing `docs/development/action-requests.json`, `internal/tools/actiongrants/table_gen.go` (keyed literals of constants, so the server runs no code to build it) and the site's `reference/fine-grained-permissions.mdx` with its Spanish twin (make gen-action-grants). Requests keep the order the handler makes them, which decides the refusal a path reports: a lookup no token passes stops the write after it, so the write never commits. What the walk or the record cannot answer is declared with a category and a reason (`request_declarations.go`, `grant_declarations.go`, the raw file `HEAD` and the integration slugs among them), and a declaration that answers nothing is a finding. It loads the program and builds itself with `table_gen.go` replaced by `table_stub.go.txt`, so a table a type change broke cannot stop its own regeneration. The same requests decide the classic scope each action needs (ADR-0026): GitLab's rule, `read_api` for a REST GET or HEAD and a GraphQL query and `api` for anything else, departed from only where `classic_declarations.go` declares a route with a category and a reason (a route granting `read_api` on every method, one granting every scope, one GitLab authenticates by a runner or trigger token the caller passes, the last held to the record's skip reason), folded as the strongest mandatory request of a way and the weakest way of an action, written as `Classic` on every operation and row of the table and into `action-requests.json` with the group's scopes. `-check-derivation` fails on any finding and on five gates (a second request on a path no directive qualifies, a denial naming something the record does not hold, a way of running that sends nothing, an action whose ways or optional requests need different classic scopes, and an action whose read or write classification departs from what `read_api` reaches with no declaration in `annotationDisagreements`) and is never deferred (make check-action-grants-derivation, step 26 of make analyze); `-check` compares the three artifacts (make check-action-grants, step 27) diff --git a/docs/development/static-analysis.md b/docs/development/static-analysis.md index 531f2bf20..396f91269 100644 --- a/docs/development/static-analysis.md +++ b/docs/development/static-analysis.md @@ -430,6 +430,28 @@ Every one of those runs carries an explicit `-timeout`, and so does `make test-r Nothing here reaches the `CI` workflow's graph, so the single required check is unchanged. +### Fuzzing + +`go test` runs a fuzz target's seed corpus as one more test and varies no input unless it is given `-fuzz`, so until `.github/workflows/fuzz.yml` existed every target here (`FuzzMeterFor`, `FuzzEvaluate_IsSoundAgainstGitLab`, `FuzzRefuseUnreadable` and the rest, eleven in five packages today) ran on its seeds alone. The workflow fuzzes each one with `go test -run '^$' -fuzz '^$' -fuzztime 10m`, one job per target, in three places: + +| When | How | Fuzzing time per target | +| ------------------------------------------------------------ | ---------------------------------------------------------------------- | ----------------------- | +| Wednesdays, weekly | the workflow's own `schedule`, on a day no other weekly workflow takes | 10 minutes | +| When dispatched | the Actions tab, or `gh workflow run fuzz.yml` | 10 minutes | +| A pull request that changes `fuzz.yml` or `cmd/fuzz_targets` | the `pull_request` trigger, limited to those two paths | 1 minute | + +The last row is there because GitHub offers a workflow for dispatch only once its file is on the default branch, so the pull request that adds `fuzz.yml` can run it no other way; a later change to either path could be dispatched from its branch, and the trigger runs it without anybody having to. No other pull request runs it, and it is a workflow of its own for the reason the race detector is: a job in `ci.yml` would gate every pull request through the one required check. + +The targets are not written into the workflow. Its plan job runs [`fuzz_targets`](cmd-utilities.md#fuzz_targets), which lists every package with the go command, reads the test files it builds for each and prints one matrix cell per target, so a target added anywhere is fuzzed from the next run on, and a plan that found none fails rather than passing with nothing fuzzed. + +A failure is a defect the fuzzer found. `go test` prints the failing input, writes it under the package's `testdata/fuzz//` and fails the job, and the job uploads that directory as the artifact `fuzz--` (with the target's committed seeds beside it when it has any; the log names the new file). To reproduce it, put the file in the same directory of a checkout and run the target on it alone: + +```bash +go test -run '^FuzzMeterFor/$' ./internal/tenancy +``` + +Committed there with the fix, the input stays a seed that every `go test` replays from then on, which is the regression test the fix needs. The same fuzzing runs locally with `go test -run '^$' -fuzz '^FuzzMeterFor$' -fuzztime 1m ./internal/tenancy`. + ## GitLab CI Example ```yaml @@ -472,10 +494,12 @@ golangci-lint run --timeout 20m ./... ``` If a cold run hits the timeout, suspect concurrency before suspecting the -timeout. `golangci-lint` defaults to one worker per logical CPU, and across 226 -packages and 55 linters that working set does not fit comfortably in memory on a -many-threaded machine — it thrashes rather than going faster. Measured on a cold -cache on the same 16-thread machine: +timeout. `golangci-lint` defaults to one worker per logical CPU, and that +working set does not fit comfortably in memory on a many-threaded machine: it +thrashes rather than going faster. Measured in July 2026 on a cold cache on the +same 16-thread machine, when the module had 226 packages and `.golangci.yml` +enabled 52 linters and 3 formatters (`golangci-lint linters` lists what it +enables today): | Concurrency | Peak RSS | Cold run | | --------------------- | -------- | ---------------- | diff --git a/docs/development/testing/README.md b/docs/development/testing/README.md index a1b72b182..8dca8a7ef 100644 --- a/docs/development/testing/README.md +++ b/docs/development/testing/README.md @@ -31,8 +31,8 @@ correctly. Statement coverage is at or near 100% across `cmd/` and `internal/`, so it no longer answers the question it is usually asked. Two tools answer what it -cannot, and both are run by hand on the package being worked on rather than -over the tree: +cannot, and both are run by hand on the package being worked on; gobco is also +run over the whole tree, for the figures [recorded below](#the-whole-tree): | Tool | Command | What it finds | | --------------------------------------- | ----------------------------------------------- | ------------------------------------------------------------------------------------------------------- | @@ -127,14 +127,29 @@ this build compiles and the named platform does not, and prints, counts and passes the rest. A value it cannot read is refused before gobco runs. A run the gate fails exits 3, a status no other failure of the script uses, so a caller can tell a figure the gate held from no figure at all: every other -failure exits 1 or 2, never 3 (1 for a package go cannot load, a failing test -or a `0/0` report, 2 for a gate value it cannot read, and 1 or 2 when a -command the script runs fails on its own, as GNU tar, awk and a crashing go -command exit 2). `make coverage-conditions` reports any failure of the script -as its own, with status 2. +failure exits 1 or 2, never 3 (1 for a package go cannot load, a failing test, +a `0/0` report or one without the figure of its mode, 2 for a gate or a mode +it cannot read, and 1 or 2 when a command the script runs fails on its own, as +GNU tar, awk and a crashing go command exit 2). `make coverage-conditions` +reports any failure of the script as its own, with status 2. `GOBCO_TEST_FLAGS` hands gobco's `go test` flags of its own (`-timeout=30m`), one word each. +**What gobco counts is chosen with `GOBCO_MODE`.** Unset, empty or +`condition`, it is gobco's own default: every atomic condition, which is each +operand of `&&`, `||` and `!` taken apart and each comparison wherever it is +written, a `return` statement or an assignment as much as an `if`, and the +report ends on `Condition coverage: N/M`. `GOBCO_MODE=branch` runs gobco with +`-branch`, which counts the whole controlling condition of each `if` and each +`for` and each case of a `switch`, and nothing else, and the report ends on +`Branch coverage: N/M`. A case of a switch with a tag is counted alike in +both, as the tag compared with the case. A report without the figure of its +mode is refused like a `0/0`, any other value is refused before gobco runs, +and the gate reads either mode's report alike, since gobco names a branch +taken one way in the words it uses for a condition. The two modes measure +different things: `return a == b || c` is two conditions and no branch, and +`if a && b` is two conditions and one branch. + **The platform halves the copy leaves out are measured where they build.** The Windows and non-Linux halves of `cmd/server` (four source files: `conn_refused_windows.go`, `socket_mode_windows.go`, `probe_other.go` and @@ -177,15 +192,25 @@ builds, which today are `cmd/server`, `internal/toolutil` and `macos-latest` and `ubuntu-latest`, the last for comparison. `gate` is the `GOBCO_GATE` the script runs with, `all` or `beyond:linux/amd64`; the figures are the same under either, and it decides only which of the conditions not -evaluated both ways the summary marks as held. The job summary has one row per -package and system with gobco's figure (outcomes evaluated of outcomes there -are, two per condition), then every condition not evaluated both ways with its -file and line, then, for a run that could not measure, the lines the script -said why in. Each run's raw output and record are uploaded as an artifact named -`gobco--`. No figure fails the workflow; a run that cannot -measure at all, such as a failing test, a package go cannot load or a Go -runtime crash of issue 467, fails its own job and says why. The plan and the -summary are `cmd/measure_conditions`. +evaluated both ways the summary marks as held. `mode` is the `GOBCO_MODE` the +script runs with, `condition` or `branch` (above), and `tree` measures every +package below `cmd/` and `internal/` instead of the packages named, in twenty +shards per system ([below](#the-whole-tree)). The job summary, headed by the +mode, has one row per package and system with gobco's figure (outcomes +evaluated of outcomes there are, two per condition or decision), then each +system's total over the packages it measured, then every condition not +evaluated both ways with its file and line, then, for a package that could +not be measured, the lines of its output that say why: the script's own and +what gobco or its `go test` printed before them (a panic, a failing test, a +test package that did not build or set up, a compiler position). Each job's +raw outputs and records are uploaded as an artifact, named +`gobco--` for a job of a named package and +`gobco--tree-NN` for a shard of the tree, with one directory per +package inside it. No figure fails the workflow; a package that cannot be +measured at all, such as one with a failing test, one go cannot load or one +hit by a Go runtime crash of issue 467, fails the job that ran it once the +job's other packages are measured, and says why. The plan, the loop each job +runs and the summary are `cmd/measure_conditions`. The figures it reports are measured, not inferred: each comes from gobco on the runner the row names, and a row without a measurement says so instead of @@ -205,14 +230,34 @@ cause. Without the blanking gobco declines every file and reports prints when no test wrote its counts, and it reads like a package with nothing left to test. -**Nor a package whose `export_test.go` hands a symbol to its external test -package.** It resolves the `_test` package against the non-test files alone, -so `internal/tools/todos` dies with `action_specs_test.go:49:27: undefined: -todos.PublishedActionIDs` before instrumenting anything. Twenty-five of the -169 packages that carry an `action_specs.go` are in that state today. The -limitation is the tool's rather than the package's, which is why -`make check-spec-conditions` reports such a package as not measured instead -of failing on it. +**An external test package gobco cannot read is staged as well.** gobco +resolves the `_test` package against the non-test files alone, so +`internal/tools/todos` died with `action_specs_test.go:49:27: undefined: +todos.PublishedActionIDs`, a name its `export_test.go` declares, before +instrumenting anything. And it bridges that package to the instrumented one +through an import of the package it finds in the directory, so +`internal/tools/awardemoji`, whose external tests reach it only through the +catalog, was given a bridge with an empty import path, which `go test` +refused at setup. The first whole-tree measurement [below](#the-whole-tree) +found twenty-seven packages in the first state, twenty-two of them among the +149 sub-packages of `internal/tools` that carry an `action_specs.go`, and +three in the second. The script measures both from the staged copy: each of +the package's own test files that declares no test, benchmark, fuzz test or +example (the `export_test.go`) becomes a file of the package behind +`//go:build gc`, which the go command builds and gobco's narrower context does +not instrument, so its names reach the external tests and its conditions stay +out of the report as a test file's do; and an external test package that +imports nothing of its package is given a test file that imports it. Both are +gobco's to fix, in +[upstream-bugs entry 97](../upstream-bugs.md#gobco-cannot-instrument-a-package-whose-export_testgo-feeds-its-external-test-package) +and +[entry 140](../upstream-bugs.md#gobco-bridges-a-black-box-test-package-through-an-import-it-may-not-find), +and the staging is temporary: the maintainer decided on 27 October 2026 to +keep it, superseding the decision of 6 October to leave both to gobco, until +gobco publishes a release with both fixes, and it goes then. +`make check-spec-conditions`, which reads those 149, still reports a package +gobco cannot instrument as not measured instead of failing on it, since that +is the tool's limitation rather than the package's. **`PKG` names one package and gremlins reads it as a directory to walk.** It mutates everything below the path it is given, so a package with anything under @@ -642,8 +687,8 @@ anything. The condition is then a decoration, and the mapping from action to metadata is stated nowhere a reader can check. It is narrow on purpose. gobco costs 60 to 80 seconds per package and caches -nothing, so the 169 packages carrying an `action_specs.go` would be about -three hours; a condition anywhere else in a package stays the sweep's subject +nothing, so the 149 packages it reads would be about three hours one after +another; a condition anywhere else in a package stays the sweep's subject and is read with `make coverage-conditions`. It runs gobco through the same `scripts/coverage-conditions.sh`, so a package with a file per platform is staged and measured rather than reported as not measured. A condition that @@ -654,6 +699,134 @@ declaration that answers nothing fails: if meta.usage != "" { // gobco: always true because every entry sets it ``` +### The whole tree + +The OpenSSF Best Practices gold criterion test_branch_coverage80 asks what +share of the code's branches the automated tests cover, measured with a FLOSS +tool. gobco is that tool here, and the figures are taken over every package +below `cmd/` and `internal/`; `test/` is left out, since every file of the e2e +suite sits behind a build tag and gobco cannot measure it (above). The tree is +measured in both of gobco's modes (`GOBCO_MODE`, above), which count different +things: + +- **Branch coverage** (`branch`, gobco's `-branch`) counts each decision + twice, once per outcome: the whole controlling condition of each `if` and + each `for`, and each case of a `switch`. That is what the criterion names. +- **Condition coverage** (`condition`, gobco's default, and the mode the gate + a touched package passes is held in) counts each atomic condition twice + instead: each operand of `&&`, `||` and `!` on its own, and each comparison + wherever it is written, a `return` statement or an assignment as much as an + `if`. It counts expressions no decision holds, so its total is the larger. + +Go evaluates `&&` and `||` from the left and stops once the outcome is known, +so the last condition a decision evaluates is the one that decides it, and a +decision whose every condition went both ways has gone both ways itself. Each +outcome branch coverage finds untaken therefore leaves a condition outcome +untaken in the same decision, and over the same packages the branch figure +can leave out no more outcomes than the condition figure does. The converse +does not hold: `if a && b` can take both branches while `b` is never false. + +The figures are taken one of two ways, through the same script and the same +summary: + +```bash +# On any machine, without a dispatch: the summary goes to stdout and to +# dist/conditions-tree/summary.md, beside one record per package +make coverage-conditions-tree CONDITIONS_PARALLEL=4 CONDITIONS_MODE=branch +make coverage-conditions-tree CONDITIONS_PARALLEL=4 CONDITIONS_MODE=condition + +# On GitHub's runners, twenty shards per runner, with the summary in the run +gh workflow run conditions.yml -f tree=true -f systems=ubuntu-latest -f gate=all -f mode=branch +gh workflow run conditions.yml -f tree=true -f systems=ubuntu-latest -f gate=all -f mode=condition +``` + +Taken on 26 October 2026 on linux/amd64, on the branch of the pull request +that added this section once its Go code was final, when it sat on `main` at +`ed18d7d12`. The branch was rebased onto a later `main` afterwards, so the +packages `main` gained since are in neither total, and the figures are those +of the tree as it was measured. They were taken with +`go run ./cmd/measure_conditions tree -parallel 4` and each `-mode` (the +target above), four packages at a time on ten cores of a machine other work +was loading too: the branch run took one hour and twelve minutes, and the +condition run one hour and twenty-four. + +| Mode | Packages below `cmd/` and `internal/` | Measured | Outcomes evaluated | Share | +| --------- | ------------------------------------: | -------: | -----------------: | ----: | +| Branch | 296 | 265 | 44936 of 44954 | 99.9% | +| Condition | 296 | 265 | 53412 of 53432 | 99.9% | + +Each share is rounded down to a tenth of a percent, so it never reads higher +than it is. + +The eighteen branch outcomes not taken are eighteen decisions that went one +way only, in nine packages: `cmd/audit_install_buttons` (1), +`cmd/format_md_tables` (2), `cmd/gen_lhm_manifest` (1), +`cmd/gen_model_corpus` (1), `internal/elicitation` (5), +`internal/tools/enterpriseusers` (1), `internal/tools/features` (4), +`internal/tools/groupepicboards` (2) and `internal/tools/groupsaml` (1). +Thirteen are the error branch of a call no test makes fail: three on a random +read, two on a request build, a stat, a write, a schema build, a catalog +build, a marshal, an encode, an unmarshal and a marshal back. Two are the +per-action metadata decorations `make check-spec-conditions` exists to find. +The other three are cases no test reaches: a decoding with no candidate to +try, a URL elicitation in a session negotiated at 2026-07-28 or later, and an +input schema without its `value` property. + +The twenty condition outcomes not evaluated are twenty conditions taken one +way only, in ten packages: the same eighteen places, the elicitation one +counted at its second operand, and two in `cmd/audit_edition_tier` that no +decision holds whole, a path segment with a placeholder on one side only (an +operand in a `return`) and a documented parameter the action does not offer +(the first operand of an `&&` whose decision goes both ways). The summary +names each with its file and line, and each is a test case to write or a +condition to declare. + +That run could not measure 31 packages, which are in neither total above. +None of them failed under `go test`; each met a limit of gobco's, and two a +limit of their own tests in gobco's copy: + +- Twenty-seven hand symbols from an `export_test.go` to their external test + package, which gobco type-checks against the non-test files alone (above): + `cmd/internal/graphqlintrospect`, `internal/clientcompat`, + `internal/gatewaycompat`, `internal/testutil/serialtypecheck` and + twenty-three packages under `internal/tools`. +- Three, `internal/tools/awardemoji`, `internal/tools/freezeperiods` and + `internal/tools/groupmarkdownuploads`, have an external test package that + does not import the package it tests, and gobco wrote the bridge it adds for + such a package with an empty import path, which `go test` refused at setup. +- `internal/completions` has tests that read its own source for the case + labels of two switches, which gobco's copy rewrites into calls of its own, + and `internal/testutil/serialtypecheck` a test that reads the imports of + every file of its package, gobco's own files among them in the copy. + +The script stages the first two shapes now +([above](#what-they-cost-and-why-the-figures-are-not-generated)), +and the two tests read the same thing in gobco's copy as in the source. The +31 were measured that way on 27 October 2026 on linux/amd64, over this +branch on `main` at `8f339f4e8`, with `scripts/coverage-conditions.sh` and +`GOBCO_GATE=all`, four packages at a time: + +| Mode | Packages | Measured | Outcomes evaluated | Share | +| --------- | -------: | -------: | -----------------: | ----: | +| Branch | 31 | 31 | 3773 of 3778 | 99.8% | +| Condition | 31 | 31 | 4113 of 4120 | 99.8% | + +The five branch outcomes not taken are five decisions that went one way only. +Four are per-action metadata decorations in `internal/tools/resourceevents` +(`action_specs.go:158` to `:167`), the shape `make check-spec-conditions` +exists to find, and the fifth is a comparison with the one individual tool +name the dispatch of `internal/tools/workitems` (`action_specs.go:90`) is +ever handed. The seven condition outcomes are the same five and two operands +of `internal/tools/workitems/clear_guard.go` whose decisions go both ways +without them: the YOLO mode never on (`:52`) and an elicitation flow that +never fails (`:66`). + +Over all 296 packages the two runs add up to 48709 of 48732 branch outcomes +and 57525 of 57552 condition outcomes, 99.9% each. That sum is of two trees +measured a day apart, the 265 packages at `ed18d7d12` and the 31 on the +rebased branch; one run over the merged tree, either way above, takes the +figure of one tree. + ## When To Use Each Layer Use unit tests for implementation changes and regression coverage. Use E2E diff --git a/docs/development/upstream-bugs.md b/docs/development/upstream-bugs.md index 5188dde98..cd26c7bc3 100644 --- a/docs/development/upstream-bugs.md +++ b/docs/development/upstream-bugs.md @@ -172,6 +172,7 @@ for the fork, branch, fix, test and MR workflow. - [go-selfupdate depends on the deprecated x/crypto/openpgp](#go-selfupdate-depends-on-the-deprecated-xcryptoopenpgp) - [gobco type-checks every file of a package directory, whatever its build constraints say](#gobco-type-checks-every-file-of-a-package-directory-whatever-its-build-constraints-say) - [gobco cannot instrument a package whose export_test.go feeds its external test package](#gobco-cannot-instrument-a-package-whose-export_testgo-feeds-its-external-test-package) + - [gobco bridges a black box test package through an import it may not find](#gobco-bridges-a-black-box-test-package-through-an-import-it-may-not-find) - [go/types reads an imported generic instance another checker is expanding](#gotypes-reads-an-imported-generic-instance-another-checker-is-expanding) ## Writing an entry @@ -345,7 +346,7 @@ readable without opening the tracker: | 94 | gitlab-org/gitlab | [A board name GitLab cannot save is answered as a success](#a-board-name-gitlab-cannot-save-is-answered-as-a-success) | Yes, by the merge request | Yes, [gitlab-org/gitlab!260458](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/260458), merged | **Yes, 19.5.0** | No | None taken | | 95 | client-go | [`ListGroupMilestonesOptions` misspells `include_descendants` and `GroupMilestone` drops the web URL and project](#listgroupmilestonesoptions-misspells-include_descendants-and-groupmilestone-drops-the-web-url-and-project) | `web_url`: yes, by another contributor; the rest: no | `web_url`: [gitlab-org/api/client-go!3126](https://gitlab.com/gitlab-org/api/client-go/-/merge_requests/3126), merged | **`web_url`: v3.26.0**, not ours | No | Retired for `web_url` at the v3.29.0 pin; yes for `include_descendants` and `project_id` | | 96 | client-go | [`CreateGroupIssueBoardListOptions` models only `label_id` where GitLab takes four list types](#creategroupissueboardlistoptions-models-only-label_id-where-gitlab-takes-four-list-types) | Yes | Yes, [gitlab-org/api/client-go!3085](https://gitlab.com/gitlab-org/api/client-go/-/merge_requests/3085), open, commit 8 | No | No; it narrows `group.group_board_create_list` to label lists | None, by decision | -| 97 | gobco | [gobco cannot instrument a package whose `export_test.go` feeds its external test package](#gobco-cannot-instrument-a-package-whose-export_testgo-feeds-its-external-test-package) | Yes, [rillig/gobco#42](https://github.com/rillig/gobco/issues/42) | Yes, [rillig/gobco#43](https://github.com/rillig/gobco/pull/43), open | No | No; the condition gate cannot measure a package it reaches | None, by decision | +| 97 | gobco | [gobco cannot instrument a package whose `export_test.go` feeds its external test package](#gobco-cannot-instrument-a-package-whose-export_testgo-feeds-its-external-test-package) | Yes, [rillig/gobco#42](https://github.com/rillig/gobco/issues/42) | Yes, [rillig/gobco#43](https://github.com/rillig/gobco/pull/43), open | No | No; the condition gate could not measure a package it reaches | Yes, temporarily: the staged copy of `scripts/coverage-conditions.sh` since 2026-10-27, kept by the maintainer's decision of that day, which supersedes that of 2026-10-06, until a gobco release carries the fix | | 98 | client-go | [`UpdateGroupIssueBoardOptions` models neither list switch the group board update takes](#updategroupissueboardoptions-models-neither-list-switch-the-group-board-update-takes) | Yes | Yes, [gitlab-org/api/client-go!3085](https://gitlab.com/gitlab-org/api/client-go/-/merge_requests/3085), open, commit 7, beside row 96 | No | No; the switches go out in a body of this server's own | Yes, since [issue 1241](https://github.com/jmrplens/gitlab-mcp-server/issues/1241); `group.group_board_update` sends both in a body that embeds `UpdateGroupIssueBoardOptions` | | 99 | gitlab-org/gitlab | [The roles and permissions page does not say when a Guest or a Planner can view pipelines and merge requests](#the-roles-and-permissions-page-does-not-say-when-a-guest-or-a-planner-can-view-pipelines-and-merge-requests) | Yes, by another user, [gitlab-org/gitlab#632031](https://gitlab.com/gitlab-org/gitlab/-/issues/632031), which the merge request closes, closed by its author on 2026-10-11 | Yes, [gitlab-org/gitlab!260353](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/260353), merged | **Yes, 19.5.0** | No | Not needed; `pipeline.latest`'s refusal names the role the policy names | | 100 | gitlab-org/gitlab | [The roles and permissions page gives Guest, Planner and Reporter the pipeline security report](#the-roles-and-permissions-page-gives-guest-planner-and-reporter-the-pipeline-security-report) | Yes, by the merge request | Yes, [gitlab-org/gitlab!260471](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/260471), merged | **Yes, 19.5.0** | No | Not needed; no hint of this server names who can view a pipeline's findings | @@ -388,6 +389,7 @@ readable without opening the tracker: | 137 | client-go and gitlab-org/gitlab | [`ListPipelineTriggerJobsOptions` models no `include_retried`, which the trigger job routes read and do not declare](#listpipelinetriggerjobsoptions-models-no-include_retried-which-the-trigger-job-routes-read-and-do-not-declare) | No | No | No | No | Yes, `job.list_bridges` adds `include_retried` to the `trigger_jobs` query through a request option, only when the caller asked | | 138 | client-go | [`BranchAccessDescription` and `GroupBranchAccessDescription` model no `member_role_name`](#branchaccessdescription-and-groupbranchaccessdescription-model-no-member_role_name) | No, not yet; it goes out as a small merge request at the pace of the ones that replace row 34's joint one | No, not yet | No | No | Yes, every project and group protected branch read and write, and the `audit_project_full` prompt, read it off the captured response | | 139 | client-go | [The protected environment approval rule options model an `access_level_description` no route declares](#the-protected-environment-approval-rule-options-model-an-access_level_description-no-route-declares) | No | No | No | No | Yes, the four project and group protect and update actions no longer offer it | +| 140 | gobco | [gobco bridges a black box test package through an import it may not find](#gobco-bridges-a-black-box-test-package-through-an-import-it-may-not-find) | Yes, by another user, [rillig/gobco#33](https://github.com/rillig/gobco/issues/33) | Yes, by another contributor, [rillig/gobco#39](https://github.com/rillig/gobco/pull/39), open | No | No; the condition gate could not measure a package it reaches | Yes, temporarily: the staged copy of `scripts/coverage-conditions.sh` since 2026-10-27, by the decision of row 97, until a gobco release carries a fix | States verified against the upstream trackers on 2026-09-12, and rows 8 to 23 again on 2026-09-13 when the go-sdk batch was filed. Rows 39 to 44 were added @@ -16052,27 +16054,47 @@ which had no entry until then and is 2026-10-13, its Go workflow run (37855282392) waits on a maintainer's approval to run, as entry 73's (36452317634) has since 2026-09-28, it has no review, and both pull requests still merge onto `master` without a - conflict. + conflict. The decision of 2026-10-06 against a workaround in the staging + script is superseded by the maintainer's decision of 2026-10-27 to carry + one until gobco releases the fix (Workaround, below); the fix and its pull + request stand. - **Merged**: no. -- **Blocking**: no for the server. It keeps the condition gate from measuring - a package it reaches. `scripts/check-spec-conditions.sh` reports such a - package as not measured and does not fail on it, and - `docs/development/testing/README.md` says the same; both count 25 of the +- **Blocking**: no for the server. It kept the condition gate from measuring + a package it reaches until the workaround below. `scripts/check-spec-conditions.sh` reported such a + package as not measured and did not fail on it, and + `docs/development/testing/README.md` said the same; both counted 25 of the 169 packages carrying an `action_specs.go` as in that state, a figure from - when they were written and not measured again since (read from the tree on + when they were written and not measured again then (read from the tree on 2026-10-07 and again on 2026-10-13, 22 of the 150 `action_specs.go` files sit beside an `export_test.go` whose package also has an external test package). `internal/clientcompat` is held by mutation testing instead (pull request 1047), and `internal/tools/issues` was measured once, 368 of 368 conditions, only through a workaround applied by hand to a copy of the module outside this repository (pull request 1067), which the - repository's tooling does not carry. -- **Workaround**: none, by the decision above. When the fix ships in a gobco - release and the pin of entry 73 moves to it, the "reported and does not - fail" bound of `scripts/check-spec-conditions.sh` retires with its - paragraph in `docs/development/testing/README.md`, and `internal/clientcompat`, - `internal/tools/issues` and the `action_specs.go` packages are measured and - closed like any other. + repository's tooling did not carry until the workaround below. +- **Workaround**: yes, since 2026-10-27, and temporary. On 2026-10-27 the + maintainer decided to keep it until gobco publishes a release with the fix, + which supersedes the decision of 2026-10-06 (In review, above) that left + this to gobco. The whole-tree measurement of 26 October 2026 found 27 packages in + this state (`docs/development/testing/README.md`, The whole tree), and the + OpenSSF Best Practices gold criterion `test_branch_coverage80` asks for a + figure over the whole tree, which waited on a release that has not come. + `scripts/coverage-conditions.sh` stages such a package: in its copy of the + module, each test file of the package that declares no test, benchmark, + fuzz test or example becomes a file of the package, renamed without its + `_test` suffix and opened with `//go:build gc`, which the go command + satisfies and gobco's own narrower build context does not, so it is + compiled into the test binary as before, its names reach the black box + test, and gobco leaves it uninstrumented, as it leaves a test file. It + moves no position gobco reports. The 27 are measured that way, the 22 + `action_specs.go` packages among them. The staging goes when gobco + publishes a release carrying the fixes, + [rillig/gobco#43](https://github.com/rillig/gobco/pull/43) for + [rillig/gobco#42](https://github.com/rillig/gobco/issues/42) and + [rillig/gobco#39](https://github.com/rillig/gobco/pull/39) for entry 140's + [rillig/gobco#33](https://github.com/rillig/gobco/issues/33), and the pin of + entry 73 moves to it, with the paragraph of + `docs/development/testing/README.md` that describes it. **Where**: `rillig/gobco` at `7a09995` (`master`, read on 2026-10-07), `resolveTypes` (`instrumenter.go:113-150`). It type-checks each package of the @@ -16096,6 +16118,56 @@ writes a single counter. and its cause by reading `resolveTypes`. It was recorded in issue 1099 beside entry 73's adoption until that issue closed on 2026-10-07. +### gobco bridges a black box test package through an import it may not find + +- **Reported**: yes, by another user, as + [rillig/gobco#33](https://github.com/rillig/gobco/issues/33) on + 2024-02-20, with a reproduction: a black box test that reaches its package + only through a helper package. Read on 2026-10-27 it is open, with nine + comments. +- **In review**: yes, by another contributor, as + [rillig/gobco#39](https://github.com/rillig/gobco/pull/39), opened on + 2026-05-21, which reads the import path from `go.mod` and the directory + instead of from the imports. Read on 2026-10-27 it is open, last updated on + 2026-05-22. +- **Merged**: no. No gobco release has been tagged since v1.3.4 (2024-03-08). +- **Blocking**: no for the server. It kept the condition gate from measuring + three packages until the workaround below: `internal/tools/awardemoji`, + `internal/tools/freezeperiods` and `internal/tools/groupmarkdownuploads`, + whose black box tests check the catalog's view of the package and import + nothing of the package itself. +- **Workaround**: yes, since 2026-10-27, and temporary, by the maintainer's + decision of that day that + [entry 97](#gobco-cannot-instrument-a-package-whose-export_testgo-feeds-its-external-test-package) + records. `scripts/coverage-conditions.sh` + stages such a package and, in its copy of the module, adds a test file to + the black box test package that imports the package and reads one name + declared, for that file alone, in a file of the package behind + `//go:build gc`, which the go command builds and gobco's narrower build + context leaves uninstrumented. gobco then finds the import and writes a + bridge that builds. The staging goes when gobco publishes a release carrying + a fix, [rillig/gobco#39](https://github.com/rillig/gobco/pull/39) or + another, and the pin of [entry 73](#gobco-type-checks-every-file-of-a-package-directory-whatever-its-build-constraints-say) + moves to it. + +**Where**: `rillig/gobco` at `v1.3.4`, `writeGobcoBlackBox` +(`instrumenter.go`). When a directory holds a package and its black box test +package, gobco writes `gobco_bridge_test.go`, which imports the package to +hand `GobcoCover` and `GobcoFinish` to the black box tests. It takes the +import path from an import whose name is the package's in any file of the +directory, and with none it writes the import with an empty path. + +**What**: `go test` refuses the bridge at setup, so nothing is measured: + +```text +gobco_bridge_test.go:3:9: invalid import path: +FAIL github.com/jmrplens/gitlab-mcp-server/v3/internal/tools/freezeperiods [setup failed] +``` + +**How we found it**: the first whole-tree measurement, on 26 October 2026, +reported the three packages as not measured, and reading +`writeGobcoBlackBox` gave the cause. + ### go/types reads an imported generic instance another checker is expanding - **Reported**: yes, by another user, as diff --git a/internal/completions/completions_test.go b/internal/completions/completions_test.go index 1891c4f6a..f060fb36d 100644 --- a/internal/completions/completions_test.go +++ b/internal/completions/completions_test.go @@ -2132,17 +2132,26 @@ func argumentSwitchCases(t *testing.T, method string) []string { } // A clause with no expressions is `default`, which answers empty // and is the one outcome that needs no credential. + // + // A label is looked for inside its expression rather than as the + // expression itself. gobco measures this package from a copy in + // which the switch compares the argument with each label inside a + // call of its own, GobcoCover(n, gobco0 == "label"), and these + // tests run there too; the labels read are the same in both. for _, expr := range clause.List { - literal, isLiteral := expr.(*ast.BasicLit) - if !isLiteral || literal.Kind != token.STRING { - continue - } - value, unquoteErr := strconv.Unquote(literal.Value) - if unquoteErr != nil { - t.Errorf("case label %s in %s is not a string literal this can read", literal.Value, method) - continue - } - names = append(names, value) + ast.Inspect(expr, func(part ast.Node) bool { + literal, isLiteral := part.(*ast.BasicLit) + if !isLiteral || literal.Kind != token.STRING { + return true + } + value, unquoteErr := strconv.Unquote(literal.Value) + if unquoteErr != nil { + t.Errorf("case label %s in %s is not a string literal this can read", literal.Value, method) + return false + } + names = append(names, value) + return false + }) } return true }) diff --git a/internal/testutil/serialtypecheck/serialtypecheck_test.go b/internal/testutil/serialtypecheck/serialtypecheck_test.go index c41dc0d1c..50eefca27 100644 --- a/internal/testutil/serialtypecheck/serialtypecheck_test.go +++ b/internal/testutil/serialtypecheck/serialtypecheck_test.go @@ -261,6 +261,14 @@ func importsOfThePackage(t *testing.T, dir string) []string { } var imports []string for _, name := range names { + // gobco measures this package from a copy into which it writes files + // of its own, gobco_fixed.go and a test file that imports testing + // among them, and these tests run there too. Those files are its + // instrumentation rather than this package, and no other binary + // links them. + if strings.HasPrefix(filepath.Base(name), "gobco_") { + continue + } file, parseErr := parser.ParseFile(token.NewFileSet(), name, nil, parser.ImportsOnly) if parseErr != nil { t.Fatalf("parse %s: %v", name, parseErr) diff --git a/scripts/check-spec-conditions.sh b/scripts/check-spec-conditions.sh index 8c773289f..c0f838ae7 100755 --- a/scripts/check-spec-conditions.sh +++ b/scripts/check-spec-conditions.sh @@ -14,19 +14,21 @@ # Three deliberate bounds, each because the alternative is worse: # # Changed packages only. gobco instruments into a temporary tree and -# caches nothing, so it costs 60 to 80 seconds per package: the 169 -# packages that carry an action_specs.go would be about three hours. +# caches nothing, so it costs 60 to 80 seconds per package: the 149 +# packages whose action_specs.go this reads would be about three hours. # # action_specs.go only. A condition elsewhere in a package is the mutation # sweep's subject and is reported by `make coverage-conditions`; the claim # here is narrow on purpose, so that it can gate. # -# A package gobco cannot instrument is reported and does not fail. It -# resolves the external test package against the non-test files alone, so -# a package whose export_test.go hands a symbol to its _test package dies -# with "undefined". Twenty-five of the packages carrying an action_specs.go -# are in that state today. Failing on them would fail for the tool's -# limits rather than for anything about this repository. +# A package gobco cannot instrument is reported and does not fail, since +# failing on it would fail for the tool's limits rather than for anything +# about this repository. gobco resolves the external test package against +# the non-test files alone, so a package whose export_test.go hands a +# symbol to its _test package died with "undefined": twenty-two of those +# 149 were in that state when the whole tree was measured on 26 October +# 2026 (docs/development/testing/README.md, The whole tree). The recipe +# below stages such a package now, and measures it. # # gobco is run through scripts/coverage-conditions.sh, the recipe behind # `make coverage-conditions`, rather than directly: gobco also parses every diff --git a/scripts/check_spec_conditions_sh_test.py b/scripts/check_spec_conditions_sh_test.py index 5008d04bb..b896c1f19 100644 --- a/scripts/check_spec_conditions_sh_test.py +++ b/scripts/check_spec_conditions_sh_test.py @@ -125,7 +125,7 @@ def test_a_package_not_measured_is_reported_with_the_recipes_reason(self): "gobco: gobco exited 1 on ./internal/tools/{pkg}, so any figure above is not a measurement"), ("no condition measured", NOTICE + "\nCondition coverage: 0/0\n" "gobco: the report above measured no condition of ./internal/tools/{pkg} (Condition coverage: 0/0, " - "or no figure at all), which is what gobco prints when no test wrote its counts or when it declined " + "or no such figure at all), which is what gobco prints when no test wrote its counts or when it declined " "to instrument every file; refusing to pass it on as a measurement\n", 1, "refusing to pass it on as a measurement"), ("refused before gobco ran", "gobco: ./internal/tools/{pkg} keeps no test file on linux/amd64 under " diff --git a/scripts/coverage-conditions.sh b/scripts/coverage-conditions.sh index 8eaddb50e..b7cf21ef0 100755 --- a/scripts/coverage-conditions.sh +++ b/scripts/coverage-conditions.sh @@ -61,10 +61,42 @@ # reports is the original's and the file is instrumented like the rest. A # package holding such a file is staged for that reason alone. # -# A package with neither kind of file, a constraint on the platform alone -# included (a _unix_test.go behind //go:build !windows with no Windows -# counterpart), is run where it is, exactly as the recipe did before, so the -# figures of every package it could already measure do not move. +# An external test package (package x_test) is the third shape gobco cannot +# read, in two ways. gobco type-checks it through the source importer of +# go/importer, which builds the package under test from its non-test files +# alone, so a name the package's own test files declare for it, which is what +# an export_test.go is for, is undefined there, and gobco panics before +# anything is measured: +# +# panic: action_specs_test.go:49:27: undefined: todos.PublishedActionIDs +# +# That was 27 packages here when the whole tree was first measured +# (rillig/gobco#42, with the fix in review as rillig/gobco#43). The copy makes +# each of the package's own test files that declares no test, benchmark, fuzz +# test or example a file of the package, renamed without its _test suffix and +# opened with //go:build gc: the go command satisfies that constraint, so the +# file is compiled into the test binary as before and its names reach the +# external test package, and gobco's narrower build context does not, so it +# leaves the file uninstrumented, as it leaves a test file. And gobco bridges +# the external test package to the instrumented one through an import of the +# package it finds in the directory, so an external test package that imports +# nothing of its package, as a test that reaches it through the catalog does, +# gets a bridge with an empty import path, which does not build (three +# packages, rillig/gobco#33). The copy adds a test file to that package which +# imports the package and reads one name a file of the package declares behind +# the same constraint. Neither change moves a position gobco reports. +# +# This staging of an external test package is TEMPORARY. The maintainer +# decided on 2026-10-27 to keep it, superseding the decision of 2026-10-06 +# (issue 1099) to leave both limits to gobco, until gobco publishes a release +# with the fixes of rillig/gobco#43 (for rillig/gobco#42) and rillig/gobco#39 +# (for rillig/gobco#33); it goes then (docs/development/upstream-bugs.md, +# entries 97 and 140). +# +# A package with none of these, a constraint on the platform alone included +# (a _unix_test.go behind //go:build !windows with no Windows counterpart), is +# run where it is, exactly as the recipe did before, so the figures of every +# package it could already measure do not move. # # Blanking has a cost the run cannot see: a test that reads its own files' # headers finds them blank. The e2e harness checks that every one of its @@ -99,8 +131,9 @@ # Any other value is refused before gobco runs. # # A run the gate fails exits 3, and no other path does. Every other failure -# means nothing was measured and exits 1 or 2, never 3: 2 for a gate value it -# cannot read; 1 for a package go cannot load, a 0/0 report, and a gobco or +# means nothing was measured and exits 1 or 2, never 3: 2 for a gate or a mode +# (GOBCO_MODE, below) it cannot read; 1 for a package go cannot load, a 0/0 +# report, a report without the figure of its mode, and a gobco or # test failure, since `go run` reports any non-zero exit of the program it # ran as 1; and 1 or 2 for a command of its own that fails under set -e # (GNU tar and awk exit 2 on a fatal error, as the go command does when it @@ -114,6 +147,26 @@ # spaces and each written in one word (-timeout=30m), since gobco runs it with # go test's own ten-minute timeout and cmd/server takes about three times as # long on Windows as on Linux. +# +# GOBCO_MODE chooses what gobco counts (markConds in instrumenter.go): +# +# condition (the default) gobco's own default: every atomic condition, +# which is each operand of &&, || and ! taken +# apart, and each comparison wherever it is +# written, a return statement or an assignment as +# much as an if; the report opens its figure with +# "Condition coverage" +# branch gobco's -branch: the whole controlling condition +# of each if and each for, and each case of a +# switch, and nothing else; the report opens its +# figure with "Branch coverage" +# +# A case of a switch with a tag is counted alike in both, as the tag compared +# with the case. Every condition a report names, and the gate above, read the +# same in either mode, since gobco names a branch taken one way in the words +# it uses for a condition. A report whose figure is in the other mode's words +# is refused like a 0/0, and any other value of the mode is refused before +# gobco runs. set -euo pipefail # The files a run leaves out are listed in the order a glob returns them, @@ -127,11 +180,27 @@ PKG=${1:?usage: coverage-conditions.sh [tags]} TAGS=${2:-} tag_args=() gobco_args=() + +# The mode is read before anything runs, as the gate is below, and decides +# the words the report's figure is read in. +MODE=${GOBCO_MODE:-condition} +case "$MODE" in + condition) figure="Condition coverage" ;; + branch) + figure="Branch coverage" + gobco_args+=(-branch) + ;; + *) + echo "gobco: GOBCO_MODE=$MODE is neither condition nor branch; refusing to measure" >&2 + exit 2 + ;; +esac + if [ -n "$TAGS" ]; then tag_args=(-tags "$TAGS") # gobco hands every -test option to its `go test` after the package, where # the go command still reads a build flag. - gobco_args=("-test=-tags=$TAGS") + gobco_args+=("-test=-tags=$TAGS") fi read -r -a test_flags <<<"${GOBCO_TEST_FLAGS:-}" for flag in ${test_flags[@]+"${test_flags[@]}"}; do @@ -154,9 +223,17 @@ fi # `go test` gobco runs. -e keeps a package go cannot load (a path that is not # there, a directory every file of which a constraint leaves out) from ending # the script with go's own message, which does not say what the run needed. +# +# Every line but the last is read with read, which fails at the end of its +# input, and $(...) drops the newlines a listing ends with, so the lines that +# can be empty in a package go loads are kept above one that is not: the files +# it builds. listing=$(go list -e ${tag_args[@]+"${tag_args[@]}"} -f '{{.Dir}} {{with .Module}}{{.Dir}}{{end}} {{len .TestGoFiles}} {{len .XTestGoFiles}} +{{.Name}} {{.ImportPath}} +{{join .TestGoFiles " "}} +{{join .XTestImports " "}} {{join .GoFiles " "}} {{join .CgoFiles " "}} {{join .TestGoFiles " "}} {{join .XTestGoFiles " "}} {{join .GoFiles " "}} {{join .CgoFiles " "}} {{with .Error}}{{.}}{{end}}' "$PKG") @@ -164,6 +241,11 @@ listing=$(go list -e ${tag_args[@]+"${tag_args[@]}"} -f '{{.Dir}} IFS= read -r pkgdir IFS= read -r moddir read -r tests xtests + read -r pkgname importpath + # The package's own test files, and what its external test package + # imports, which decide whether gobco can read that package (below). + read -r -a internal_tests + read -r -a xtest_imports read -r -a built # The files gobco instruments, which are the only ones whose conditions it # reports: it leaves the test files as they are. @@ -281,6 +363,29 @@ for name in ${built[@]+"${built[@]}"}; do fi done +# What gobco cannot read in an external test package (package x_test), only +# where the package has one. gobco type-checks it against the package's +# non-test files alone, so a name the package's own test files declare for it +# (export_test.go, Go's name for the file that hands such a test what the +# package keeps unexported) is undefined there, and gobco panics. Those are +# the support files below: the package's own test files that declare no test, +# benchmark, fuzz test or example. And gobco bridges that package to the +# instrumented one through an import of the package it finds there, so one +# that imports nothing of the package gets a bridge with an empty import path, +# which does not build. +support=() +unimported="" +if [ "$xtests" -gt 0 ]; then + for name in ${internal_tests[@]+"${internal_tests[@]}"}; do + if ! grep -qE '^func (Test|Benchmark|Fuzz|Example)' "$pkgdir/$name"; then + support+=("$name") + fi + done + if [ "$pkgname" != main ] && ! listed "$importpath" ${xtest_imports[@]+"${xtest_imports[@]}"}; then + unimported=yes + fi +fi + if [ "$tests $xtests" = "0 0" ] && [ -n "$left_out_tests" ]; then echo "gobco: $PKG keeps no test file on $context: its tests are among the files the go command leaves out here (${left_out[*]}), so no test would run and gobco would measure nothing; refusing to measure" >&2 echo "gobco: a package behind a build tag is measured with TAGS=, which reaches go list and gobco's go test alike" >&2 @@ -307,16 +412,22 @@ report=$(mktemp) target=$pkgdir staged="" -if [ "${#left_out[@]}" -gt 0 ] || [ "${#constrained[@]}" -gt 0 ]; then +if [ "${#left_out[@]}" -gt 0 ] || [ "${#constrained[@]}" -gt 0 ] || [ "${#support[@]}" -gt 0 ] || [ -n "$unimported" ]; then staged=yes if [ "${#left_out[@]}" -gt 0 ]; then echo "gobco: $PKG holds ${#left_out[@]} .go file(s) the go command does not build on $context (${left_out[*]}), which gobco would parse with the rest (issue 1017); measuring it in a staged copy of the module without them. The conditions of the files left out are not measured by this run: only a run on a platform that builds them could measure them" if [ "${#constrained[@]}" -gt 0 ]; then echo "gobco: the copy blanks the build constraint lines of ${constrained[*]}, line for line, since each names more than the platform and gobco's own narrower build context would decline to instrument it" fi - else + elif [ "${#constrained[@]}" -gt 0 ]; then echo "gobco: $PKG builds ${constrained[*]} under a build constraint naming more than the platform, which gobco's own narrower build context would decline to instrument; measuring it in a staged copy of the module with those lines blanked, line for line, so gobco instruments every file the go command builds here" fi + if [ "${#support[@]}" -gt 0 ]; then + echo "gobco: $PKG has an external test package, which gobco type-checks against the package's non-test files alone, and test files of its own that declare no test (${support[*]}), whose names that package may use; measuring it in a staged copy of the module in which those files are part of the package, behind //go:build gc, which the go command builds and gobco's own build context does not instrument, so they are compiled into the test binary as before and stay out of the report as a test file does" + fi + if [ -n "$unimported" ]; then + echo "gobco: $PKG has an external test package that does not import it, and gobco bridges that package to the instrumented one through an import it finds there; measuring it in a staged copy of the module with a test file in that package that imports it, beside a file of the package behind //go:build gc that declares the one name the import reads" + fi workdir=$(mktemp -d) module="$workdir/$(basename "$moddir")" mkdir "$module" @@ -334,6 +445,30 @@ if [ "${#left_out[@]}" -gt 0 ] || [ "${#constrained[@]}" -gt 0 ]; then "$target/$name" >"$target/$name.blanked" mv "$target/$name.blanked" "$target/$name" done + # A support file becomes a file of the package, under a name without the + # _test suffix, so that the go command builds it into the package and gobco + # reads its names when it type-checks the external test package. It opens + # with //go:build gc, which the go command satisfies and gobco's narrower + # context does not, so gobco leaves it uninstrumented, as it leaves a test + # file. The file builds here, so any constraint of its own already holds, + # and its lines are blanked rather than combined with that one. + for name in ${support[@]+"${support[@]}"}; do + { + printf '//go:build gc\n\n' + awk '!body && /^package[ \t]/ { body = 1 } !body && (/^\/\/go:build[ \t]/ || /^\/\/ [+]build[ \t]/) { print ""; next } { print }' "$target/$name" + } >"$target/${name%_test.go}_gobco.go" + rm -f "$target/$name" + done + # The import gobco's bridge copies, written where the external test package + # imports nothing of the package. It reads a name of the package declared in + # a file of the package behind the same constraint, so it compiles, and + # neither file is instrumented. + if [ -n "$unimported" ]; then + printf '//go:build gc\n\npackage %s\n\n// GobcoStagedImport is what gobco_staged_import_test.go reads.\nvar GobcoStagedImport struct{}\n' \ + "$pkgname" >"$target/gobco_staged_import_gobco.go" + printf 'package %s_test\n\nimport %s "%s"\n\nvar _ = %s.GobcoStagedImport\n' \ + "$pkgname" "$pkgname" "$importpath" "$pkgname" >"$target/gobco_staged_import_test.go" + fi fi # gobco's report is passed through as it is written and kept, so the figure @@ -356,8 +491,8 @@ fi exit "$status" } -if ! grep -qE '^Condition coverage: [0-9]+/[1-9][0-9]*$' "$report"; then - echo "gobco: the report above measured no condition of $PKG (Condition coverage: 0/0, or no figure at all), which is what gobco prints when no test wrote its counts or when it declined to instrument every file; refusing to pass it on as a measurement" >&2 +if ! grep -qE "^$figure: [0-9]+/[1-9][0-9]*\$" "$report"; then + echo "gobco: the report above measured no condition of $PKG ($figure: 0/0, or no such figure at all), which is what gobco prints when no test wrote its counts or when it declined to instrument every file; refusing to pass it on as a measurement" >&2 exit 1 fi diff --git a/scripts/coverage_conditions_sh_test.py b/scripts/coverage_conditions_sh_test.py index 6818a54f9..916161078 100644 --- a/scripts/coverage_conditions_sh_test.py +++ b/scripts/coverage_conditions_sh_test.py @@ -53,6 +53,7 @@ # initialisation slows every call down. STUB_GO = r'''import json import os +import re import sys import time @@ -136,6 +137,38 @@ def header_constraint(path): return False +def imports_of(path): + """The import paths a file names, written the way the fixtures write + them: one import on its line, or a parenthesized block of them.""" + found = [] + block = False + with open(path, encoding="utf-8") as fh: + for line in fh: + stripped = line.strip() + if block: + if stripped == ")": + block = False + elif '"' in stripped: + found.append(stripped.split('"')[1]) + elif stripped.startswith("import ("): + block = True + elif stripped.startswith("import ") and '"' in stripped: + found.append(stripped.split('"')[1]) + return found + + +def declared(path): + """The top-level names a file declares, one declaration per line.""" + with open(path, encoding="utf-8") as fh: + return set(re.findall(r"^(?:func|var|const|type) ([A-Za-z_]\w*)", fh.read(), re.MULTILINE)) + + +def import_path(pkgdir): + """The package's import path in the fixture module, example.com/m.""" + below = os.path.relpath(pkgdir, env["STUB_ROOT"]) + return "example.com/m" if below == "." else "example.com/m/" + below.replace(os.sep, "/") + + def render(template, fields): for placeholder, value in fields: template = template.replace(placeholder, value) @@ -176,6 +209,8 @@ def render(template, fields): if separator and pkgdir != env["STUB_ROOT"]: below = os.path.relpath(pkgdir, env["STUB_ROOT"]) shown = env["STUB_ROOT"] + separator + below.replace("/", separator) + name = clause(os.path.join(pkgdir, gofiles[0])) if gofiles else "" + xtest_imports = sorted({path for n in xtests for path in imports_of(os.path.join(pkgdir, n))}) print(render(args[args.index("-f") + 1], [ ("{{with .Module}}{{.Dir}}{{end}}", env["STUB_ROOT"]), ("{{.Dir}}", shown if exists else ""), @@ -185,6 +220,8 @@ def render(template, fields): ('{{join .CgoFiles " "}}', ""), ('{{join .TestGoFiles " "}}', " ".join(tests)), ('{{join .XTestGoFiles " "}}', " ".join(xtests)), + ("{{.Name}} {{.ImportPath}}", name + " " + import_path(pkgdir)), + ('{{join .XTestImports " "}}', " ".join(xtest_imports)), ("{{with .Error}}{{.}}{{end}}", error), ])) elif args[:2] == ["run", GOBCO]: @@ -213,12 +250,43 @@ def render(template, fields): os.path.relpath(os.path.join(d, n), root).replace(os.sep, "/") for d, _, ns in os.walk(root) for n in ns) unbuilt = [n for n in files if not builds(os.path.join(here, n), tags)] + # The external test package and what gobco reads it against: the names + # the package's non-test files declare, since go/importer's source + # importer loads the package under test from those alone. + xtest_files = [n for n in files if clause(os.path.join(here, n)).endswith("_test")] + package = next((clause(os.path.join(here, n)) for n in files if not n.endswith("_test.go")), "") + visible = set() + for n in files: + if not n.endswith("_test.go"): + visible |= declared(os.path.join(here, n)) + undefined = [] + for n in xtest_files: + with open(os.path.join(here, n), encoding="utf-8") as fh: + undefined += [(n, used) for used in re.findall(r"\b" + package + r"\.([A-Z]\w*)", fh.read()) + if used not in visible] + # gobco's bridge imports the package under the import it finds whose + # name is the package's, in any file of the directory. + bridged = any(path.rsplit("/", 1)[-1] == package + for n in files for path in imports_of(os.path.join(here, n))) if unbuilt: # What gobco does with a directory holding a file the go command # leaves out: it type-checks it with the rest, and a platform pair # declares the same function twice. print("panic: %s:5:6: f redeclared in this block" % unbuilt[0]) status = 2 + elif undefined: + # What gobco does with an external test package that uses a name the + # package's own test files declare: the type check finds it + # undefined, and gobco panics before anything is measured. + print("panic: %s:1:1: undefined: %s.%s" % (undefined[0][0], package, undefined[0][1])) + status = 2 + elif xtest_files and not bridged: + # What gobco's go test does with the bridge it writes when no file + # imports the package: an import with an empty path does not build. + print("# " + import_path(here)) + print("gobco_bridge_test.go:3:9: invalid import path: ") + print("FAIL\t%s [setup failed]" % import_path(here)) + status = 1 else: time.sleep(float(env.get("STUB_GOBCO_SLEEP", "0"))) figure = env.get("STUB_GOBCO_REPORT", "3/4") @@ -226,9 +294,13 @@ def render(template, fields): # No test binary ran, so no counts were written, and gobco # reports nothing measured rather than conditions unevaluated. figure = "0/0" + # gobco opens its figure with the mode it counted in: -branch counts + # whole controlling conditions, and its default every atomic one. A + # case sets STUB_GOBCO_KIND to have it answer in the other words. + kind = env.get("STUB_GOBCO_KIND", "Branch coverage" if "-branch" in args else "Condition coverage") print("") if figure != "none": - print("Condition coverage: " + figure) + print(kind + ": " + figure) # The conditions it names, one per line, which a case sets with # STUB_GOBCO_CONDITIONS (an empty value names none), or one in # the first file by default. @@ -294,6 +366,22 @@ def render(template, fields): # A package only Windows builds a file of. "internal/winonly/w_windows.go": "package winonly\n\nfunc w(c int) bool {\n\treturn c == 1\n}\n", "internal/winonly/w_windows_test.go": "package winonly\n", + # An external test package using a name the package's own test files + # declare for it, beside a test of the package's own and a support file + # under a constraint gobco's context misjudges. + "internal/exported/exported.go": "package exported\n\nfunc f(c int) bool {\n\treturn c == 1\n}\n", + "internal/exported/export_test.go": "// export_test.go hands f to the external tests.\npackage exported\n\n// F is f, for the tests.\nvar F = f\n", + "internal/exported/norace_test.go": "//go:build !race\n\npackage exported\n\nconst raced = false\n", + "internal/exported/internal_test.go": "package exported\n\nimport \"testing\"\n\nfunc TestInternal(t *testing.T) { f(2) }\n", + "internal/exported/exported_test.go": "package exported_test\n\nimport (\n\t\"testing\"\n\n\t\"example.com/m/internal/exported\"\n)\n\nfunc TestF(t *testing.T) { exported.F(1) }\n", + # An external test package that imports nothing of its package, which it + # exercises through another one, as a catalog test does. + "internal/unimported/unimported.go": "package unimported\n\nfunc g(c int) bool {\n\treturn c == 1\n}\n", + "internal/unimported/unimported_test.go": "package unimported_test\n\nimport \"testing\"\n\nfunc TestCatalog(t *testing.T) {}\n", + # An external test package gobco reads as it is: it imports the package + # and uses only what the package's non-test files declare. + "internal/blackbox/blackbox.go": "package blackbox\n\n// B is b.\nfunc B(c int) bool {\n\treturn c == 1\n}\n", + "internal/blackbox/blackbox_test.go": "package blackbox_test\n\nimport (\n\t\"testing\"\n\n\t\"example.com/m/internal/blackbox\"\n)\n\nfunc TestB(t *testing.T) { blackbox.B(1) }\n", # What the rest of the module holds: files a test reads by a relative # path, which the staged copy keeps, and the directories it leaves out. "docs/guide.md": "guide\n", @@ -549,6 +637,92 @@ def test_built_file_under_a_constraint_gobco_misjudges_is_staged_with_its_line_b self.assertNotIn("does not build", proc.stdout) self.assertEqual(self.staged_copies(), []) + def test_external_test_packages_run_where_they_are_fail_the_way_gobco_does(self): + # What the recipe did before the external test package was staged, + # reproduced against the stand-in, so the two cases below are held + # against the failures they prevent: a panic on the name an + # export_test.go hands over, and a bridge that does not build. + cases = [ + ("a name from the package's own test files", "exported", 2, "undefined: exported.F"), + ("no import of the package", "unimported", 1, "[setup failed]"), + ] + for name, rel, code, said in cases: + with self.subTest(name): + open(self.log, "w", encoding="utf-8").close() + proc = subprocess.run([os.path.join(self.bin, "go"), "run", GOBCO], + cwd=os.path.join(self.root, "internal", rel), env=self.environment(), + capture_output=True, text=True, timeout=60, check=False) + self.assertEqual(proc.returncode, code, proc.stdout + proc.stderr) + self.assertIn(said, proc.stdout) + + def test_support_files_of_an_external_test_package_become_files_of_the_package(self): + # gobco type-checks the external test package against the package's + # non-test files alone, so the copy makes each test file of the + # package that declares no test a file of the package, behind a + # constraint gobco does not instrument, and leaves the test of the + # package's own where it was. + before = self.snapshot("") + proc, calls = self.run_script("./internal/exported") + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + run = self.gobco(calls)[0] + self.assert_staged(run, os.path.join("internal", "exported")) + self.assertEqual(run["files"], ["export_gobco.go", "exported.go", "exported_test.go", "internal_test.go", + "norace_gobco.go"]) + self.assertNotIn("panic", proc.stdout) + self.assertIn("Condition coverage: 3/4", proc.stdout) + # Each moved file opens with the constraint and is otherwise the + # original, its own constraint lines blank, since it builds here. + for moved, original in (("export_gobco.go", "export_test.go"), ("norace_gobco.go", "norace_test.go")): + with self.subTest(moved=moved): + with open(os.path.join(self.root, "internal", "exported", original), encoding="utf-8") as fh: + lines = ["" if line.startswith("//go:build ") else line for line in fh.read().split("\n")] + self.assertEqual(run["contents"][moved], "//go:build gc\n\n" + "\n".join(lines)) + self.assertEqual(run["contents"]["internal_test.go"], FILES["internal/exported/internal_test.go"]) + self.assertIn("test files of its own that declare no test (export_test.go norace_test.go)", proc.stdout) + self.assertIn("behind //go:build gc", proc.stdout) + self.assertNotIn("does not import it", proc.stdout) + self.assertEqual(self.snapshot(""), before) + self.assertEqual(self.staged_copies(), []) + + def test_external_test_package_that_imports_nothing_of_the_package_is_given_an_import(self): + # gobco's bridge copies an import of the package from the files it + # parsed, so the copy adds an external test file importing it, which + # reads a name a file of the package declares behind the constraint + # gobco does not instrument. + before = self.snapshot("") + proc, calls = self.run_script("./internal/unimported") + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + run = self.gobco(calls)[0] + self.assert_staged(run, os.path.join("internal", "unimported")) + self.assertEqual(run["files"], ["gobco_staged_import_gobco.go", "gobco_staged_import_test.go", + "unimported.go", "unimported_test.go"]) + self.assertEqual(run["contents"]["gobco_staged_import_gobco.go"], + "//go:build gc\n\npackage unimported\n\n" + "// GobcoStagedImport is what gobco_staged_import_test.go reads.\n" + "var GobcoStagedImport struct{}\n") + self.assertEqual(run["contents"]["gobco_staged_import_test.go"], + "package unimported_test\n\nimport unimported \"example.com/m/internal/unimported\"\n\n" + "var _ = unimported.GobcoStagedImport\n") + self.assertNotIn("setup failed", proc.stdout) + self.assertIn("Condition coverage: 3/4", proc.stdout) + self.assertIn("has an external test package that does not import it", proc.stdout) + self.assertNotIn("declare no test", proc.stdout) + self.assertEqual(self.snapshot(""), before) + self.assertEqual(self.staged_copies(), []) + + def test_external_test_package_gobco_can_read_is_measured_where_it_is(self): + # An external test package that imports the package and uses only + # what its non-test files declare needs no copy of the module. + before = self.snapshot("internal/blackbox") + proc, calls = self.run_script("./internal/blackbox") + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + run = self.gobco(calls)[0] + self.assertEqual(run["cwd"], os.path.join(self.root, "internal", "blackbox")) + self.assertEqual(run["files"], ["blackbox.go", "blackbox_test.go"]) + self.assertNotIn("gobco:", proc.stdout + proc.stderr) + self.assertEqual(self.snapshot("internal/blackbox"), before) + self.assertEqual(self.staged_copies(), []) + def test_constraint_on_the_platform_alone_is_measured_where_it_is(self): # gobco's context evaluates an operating system, an architecture and # unix exactly as the go command does, so a file behind //go:build @@ -786,6 +960,74 @@ def test_gate_a_value_it_cannot_read_is_refused_before_anything_runs(self): self.assertEqual(calls, []) self.assertIn("GOBCO_GATE=%s is neither all nor beyond:GOOS/GOARCH" % value, proc.stderr) + def test_mode_chooses_what_gobco_counts_and_the_figure_read(self): + # GOBCO_MODE=branch runs gobco with -branch, which instruments the + # whole controlling condition of an if or a for and each switch case + # rather than every operand and comparison, and opens its figure with + # "Branch coverage". condition, unset and empty are gobco's default, + # as every run was before the mode existed. The flag comes before the + # tags and the test flags, and a staged package is run the same way. + cases = [ + ("unset", ("./internal/plain",), {}, ["run", GOBCO], "Condition coverage: 3/4"), + ("empty", ("./internal/plain",), {"GOBCO_MODE": ""}, ["run", GOBCO], "Condition coverage: 3/4"), + ("condition", ("./internal/plain",), {"GOBCO_MODE": "condition"}, ["run", GOBCO], "Condition coverage: 3/4"), + ("branch", ("./internal/plain",), {"GOBCO_MODE": "branch"}, ["run", GOBCO, "-branch"], "Branch coverage: 3/4"), + ("branch with tags and test flags", ("./internal/plain", "e2e"), + {"GOBCO_MODE": "branch", "GOBCO_TEST_FLAGS": "-timeout=30m"}, + ["run", GOBCO, "-branch", "-test=-tags=e2e", "-test=-timeout=30m"], "Branch coverage: 3/4"), + ("branch on a staged package", ("./cmd/tool",), {"GOBCO_MODE": "branch"}, ["run", GOBCO, "-branch"], + "Branch coverage: 3/4"), + ] + for name, args, env, argv, figure in cases: + with self.subTest(name): + proc, calls = self.run_script(*args, env=env) + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + runs = self.gobco(calls) + self.assertEqual(len(runs), 1, calls) + self.assertEqual(runs[0]["argv"], argv) + self.assertIn(figure, proc.stdout) + self.assertNotIn("GOBCO_MODE", proc.stdout + proc.stderr) + self.assertEqual(self.staged_copies(), []) + + def test_mode_a_value_it_cannot_read_is_refused_before_anything_runs(self): + for value in ("Branch", "branches", "conditions", "both", " branch", "branch,condition"): + with self.subTest(value): + proc, calls = self.run_script("./internal/plain", env={"GOBCO_MODE": value}) + self.assertEqual(proc.returncode, 2, proc.stdout + proc.stderr) + self.assertEqual(calls, []) + self.assertIn("GOBCO_MODE=%s is neither condition nor branch; refusing to measure" % value, proc.stderr) + + def test_mode_a_report_without_the_figure_of_its_mode_is_refused(self): + # A 0/0 is no measurement in either mode, and a report whose figure + # is in the other mode's words is not the measurement asked for: it + # is what gobco would print if it had not been given the flag, or had + # been given it unasked. + cases = [ + ("branch, 0/0", {"GOBCO_MODE": "branch", "STUB_GOBCO_REPORT": "0/0"}, "Branch coverage"), + ("branch, a condition figure", {"GOBCO_MODE": "branch", "STUB_GOBCO_KIND": "Condition coverage"}, + "Branch coverage"), + ("condition, a branch figure", {"GOBCO_MODE": "condition", "STUB_GOBCO_KIND": "Branch coverage"}, + "Condition coverage"), + ] + for name, env, figure in cases: + with self.subTest(name): + proc, calls = self.run_script("./internal/plain", env=env) + self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) + self.assertEqual(len(self.gobco(calls)), 1) + self.assertIn("measured no condition of ./internal/plain (%s: 0/0, or no such figure at all)" % figure, + proc.stderr) + self.assertEqual(self.staged_copies(), []) + + def test_mode_branch_is_gated_like_condition(self): + # gobco names a branch it took one way only in the words it uses for + # a condition, so the gate reads either mode's report alike. + proc, _ = self.run_script("./internal/plain", env={ + "GOBCO_MODE": "branch", "GOBCO_GATE": "all", + "STUB_GOBCO_CONDITIONS": 'plain.go:4:9: condition "c == 1" was once true but never false'}) + self.assertEqual(proc.returncode, 3, proc.stdout + proc.stderr) + self.assertIn("Branch coverage: 3/4", proc.stdout) + self.assertIn("GOBCO_GATE=all: 1 condition(s) in the files of ./internal/plain", proc.stderr) + def test_test_flags_reach_gobcos_go_test_one_per_word(self): cases = [ ("no tags", (), ["run", GOBCO, "-test=-timeout=30m", "-test=-v"]),