mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-03 04:38:48 +00:00
Shard internal/api race tests and drop probation E2E from PRs
Across the last 25 bot PRs the median open to merge time was about 33 minutes for one PR and about 45 when three opened together. The critical path was the required Backend tests (api) check, where one go test -race ./internal/api step took about 27 of its 31 minutes. Every other required check finishes within about 12 minutes. internal/api now runs as four Backend tests (api-N) shards. Each shard lists the package's tests with go test -race -list from the commit under test and runs one contiguous quarter of that list in go test's own order, so every test, including ones added later, runs in exactly one shard, and a shard that resolves no tests fails. The split is contiguous rather than interleaved by name because some internal/api tests depend on package state left by the tests just before them. A round-robin split by sorted name failed dozens of tests locally (admin bypass and session store globals), while the four contiguous slices and the full run all pass. A Backend tests (api) verdict job keeps the required check name and fails unless every shard succeeded. Backend tests (rest-0) and (rest-1) keep their names and package split. Local non-race timings put the heaviest slice at about 42 percent of the package, so the api check should drop from about 31 minutes to roughly 14. Core E2E no longer runs the non-gating probation tier on pull requests. It could not gate a PR and the promotion rule counts only main runs, so on a PR it only held each of the eight shard runners about eight minutes longer, which fed the runner queueing seen with concurrent PRs. Push and manual runs still execute it. Go test steps still run for frontend-only changes. Go tests read frontend sources directly (internal/api contract tests, internal/unifiedresources walking frontend-modern/src, and the internal/telemetry repository-wide wording scan), so skipping them by path would drop real coverage.
This commit is contained in:
parent
8619ab2ec8
commit
46fd0b713e
5 changed files with 219 additions and 10 deletions
121
.github/workflows/build-and-test.yml
vendored
121
.github/workflows/build-and-test.yml
vendored
|
|
@ -267,9 +267,9 @@ jobs:
|
|||
strategy:
|
||||
fail-fast: false
|
||||
matrix:
|
||||
# internal/api is by far the slowest package under -race (~10m), so it
|
||||
# gets a dedicated shard; every other package splits across the rest.
|
||||
shard: [api, rest-0, rest-1]
|
||||
# internal/api has its own sharded job below; every other package
|
||||
# splits across these two.
|
||||
shard: [rest-0, rest-1]
|
||||
|
||||
steps:
|
||||
- name: Report documentation-only change
|
||||
|
|
@ -304,15 +304,11 @@ jobs:
|
|||
if: needs.changes.outputs.code == 'true'
|
||||
env:
|
||||
PULSE_DATA_DIR: /tmp/pulse-test-data
|
||||
# internal/api alone takes ~22m under -race on a fast machine since
|
||||
# the demo estate scaled to 50 nodes, and CI runners have needed
|
||||
# roughly double the fast-machine time, hence the large per-binary
|
||||
# budget. Remaining packages are split deterministically by list
|
||||
# position.
|
||||
# Packages other than internal/api are split deterministically by
|
||||
# list position.
|
||||
run: |
|
||||
set -euo pipefail
|
||||
case "${{ matrix.shard }}" in
|
||||
api) pkgs="./internal/api" ;;
|
||||
rest-0) pkgs=$(go list ./... | grep -v '/internal/api$' | awk 'NR % 2 == 0') ;;
|
||||
rest-1) pkgs=$(go list ./... | grep -v '/internal/api$' | awk 'NR % 2 == 1') ;;
|
||||
*) echo "unknown shard ${{ matrix.shard }}" >&2; exit 1 ;;
|
||||
|
|
@ -324,6 +320,113 @@ jobs:
|
|||
printf 'Packages in this shard:\n%s\n' "$pkgs"
|
||||
go test -race -timeout 50m $pkgs
|
||||
|
||||
# internal/api alone took ~27 of the ~31 minutes of the old single
|
||||
# `Backend tests (api)` job under -race, which made it the critical path of
|
||||
# every pull request. Its top-level tests are split across these shards
|
||||
# as contiguous runs of the package's own test order, and the
|
||||
# `Backend tests (api)` job below keeps the required check name by passing
|
||||
# only when every shard passed.
|
||||
backend-api:
|
||||
name: Backend tests (api-${{ matrix.index }})
|
||||
needs: changes
|
||||
# Like the rest shards, expand even for documentation-only changes so the
|
||||
# verdict job sees real shard results rather than skipped jobs.
|
||||
runs-on: ubuntu-24.04
|
||||
timeout-minutes: 45
|
||||
strategy:
|
||||
fail-fast: false
|
||||
matrix:
|
||||
# Keep API_SHARD_COUNT equal to the number of indexes listed here.
|
||||
index: [0, 1, 2, 3]
|
||||
env:
|
||||
API_SHARD_COUNT: 4
|
||||
|
||||
steps:
|
||||
- name: Report documentation-only change
|
||||
if: needs.changes.outputs.code != 'true'
|
||||
run: echo "No backend code changed. Tests are not required for this shard."
|
||||
|
||||
- name: Checkout repository
|
||||
if: needs.changes.outputs.code == 'true'
|
||||
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
with:
|
||||
persist-credentials: false
|
||||
|
||||
- name: Set up Go
|
||||
if: needs.changes.outputs.code == 'true'
|
||||
uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
|
||||
with:
|
||||
go-version-file: go.mod
|
||||
cache: true
|
||||
|
||||
- name: Provide frontend embed stub
|
||||
if: needs.changes.outputs.code == 'true'
|
||||
run: |
|
||||
mkdir -p internal/api/frontend-modern/dist
|
||||
[ -f internal/api/frontend-modern/dist/index.html ] || \
|
||||
printf '<!doctype html><title>ci embed stub</title>\n' > internal/api/frontend-modern/dist/index.html
|
||||
|
||||
- name: Go unit tests (internal/api shard ${{ matrix.index }})
|
||||
if: needs.changes.outputs.code == 'true'
|
||||
env:
|
||||
PULSE_DATA_DIR: /tmp/pulse-test-data
|
||||
API_SHARD_INDEX: ${{ matrix.index }}
|
||||
# The test list is computed from the checked-out commit at run time,
|
||||
# so every shard sees the same list and a test added later lands in
|
||||
# exactly one shard with no workflow edit. The list keeps go test's
|
||||
# own order (file name, then declaration), which is also run order,
|
||||
# and each shard takes one contiguous slice of it. Some internal/api
|
||||
# tests still lean on package state left by the tests just before
|
||||
# them (for example admin bypass or session store globals), and an
|
||||
# interleaved split by name broke dozens of them, while contiguous
|
||||
# slices keep those neighbours together. Listing with -race builds
|
||||
# the same test binary the run then reuses from cache. Benchmarks
|
||||
# are left out because plain go test never runs them.
|
||||
run: |
|
||||
set -euo pipefail
|
||||
listing=$(go test -race -list . ./internal/api)
|
||||
tests=$(printf '%s\n' "$listing" | grep -E '^(Test|Fuzz|Example)[^[:space:]]*$' || true)
|
||||
if [ -z "$tests" ]; then
|
||||
echo "::error::go test -list found no tests in ./internal/api"
|
||||
exit 1
|
||||
fi
|
||||
total=$(printf '%s\n' "$tests" | wc -l | tr -d ' ')
|
||||
selected=$(printf '%s\n' "$tests" | awk -v n="$API_SHARD_COUNT" -v i="$API_SHARD_INDEX" -v total="$total" \
|
||||
'NR > int(total * i / n) && NR <= int(total * (i + 1) / n)')
|
||||
if [ -z "$selected" ]; then
|
||||
echo "::error::internal/api shard ${API_SHARD_INDEX} of ${API_SHARD_COUNT} resolved to an empty test list"
|
||||
exit 1
|
||||
fi
|
||||
pattern="^($(printf '%s\n' "$selected" | paste -sd '|' -))\$"
|
||||
# Linux rejects a single argument over 128 KiB. Fail with a clear
|
||||
# instruction instead of an opaque exec error.
|
||||
if [ "${#pattern}" -gt 120000 ]; then
|
||||
echo "::error::internal/api shard pattern is ${#pattern} bytes; add a shard index and raise API_SHARD_COUNT"
|
||||
exit 1
|
||||
fi
|
||||
echo "internal/api shard ${API_SHARD_INDEX} of ${API_SHARD_COUNT} runs $(printf '%s\n' "$selected" | wc -l | tr -d ' ') of ${total} tests"
|
||||
go test -race -timeout 50m -run "$pattern" ./internal/api
|
||||
|
||||
# Required check name. Branch protection matches `Backend tests (api)`
|
||||
# exactly, so this job must keep that name and must fail unless every
|
||||
# internal/api shard succeeded (a skipped or cancelled shard fails it too).
|
||||
backend-api-verdict:
|
||||
name: Backend tests (api)
|
||||
needs: backend-api
|
||||
if: always()
|
||||
runs-on: ubuntu-24.04
|
||||
timeout-minutes: 5
|
||||
steps:
|
||||
- name: Require every internal/api shard
|
||||
env:
|
||||
API_SHARDS_RESULT: ${{ needs.backend-api.result }}
|
||||
run: |
|
||||
if [ "$API_SHARDS_RESULT" != success ]; then
|
||||
echo "::error::internal/api shards did not all pass (result: ${API_SHARDS_RESULT})"
|
||||
exit 1
|
||||
fi
|
||||
echo "Every internal/api shard passed."
|
||||
|
||||
scripts-and-build:
|
||||
name: Script smoke tests & backend build
|
||||
needs: changes
|
||||
|
|
|
|||
5
.github/workflows/test-e2e.yml
vendored
5
.github/workflows/test-e2e.yml
vendored
|
|
@ -203,9 +203,12 @@ jobs:
|
|||
# Runs even when the stable tier failed (its data still counts toward
|
||||
# promotion), but not when the stable tier was skipped — that means the
|
||||
# environment never came up and every probation spec would fail on it.
|
||||
# Pull requests skip it. It cannot gate a PR, and the promotion rule in
|
||||
# e2e-tiering.mjs counts only main runs, so on a PR it only held each
|
||||
# of the eight shard runners for about eight more minutes.
|
||||
- name: Run probation-tier E2E suite (non-gating)
|
||||
id: probation
|
||||
if: ${{ !cancelled() && steps.stable.conclusion != 'skipped' }}
|
||||
if: ${{ !cancelled() && github.event_name != 'pull_request' && steps.stable.conclusion != 'skipped' }}
|
||||
continue-on-error: true
|
||||
# A broken probation surface can otherwise consume the entire 60m job
|
||||
# budget through retries and cancel the job before continue-on-error
|
||||
|
|
|
|||
|
|
@ -24,6 +24,29 @@ Concurrency groups continue to isolate workflows and refs. Cancellation of an
|
|||
obsolete PR run supplies no passing evidence for its replacement, which still
|
||||
needs its own checks. Release publication workflows are unaffected.
|
||||
|
||||
### Sharded internal/api backend tests
|
||||
|
||||
Build and Test runs the `internal/api` race tests as four `Backend tests
|
||||
(api-N)` shards instead of one job, because that one package took about 27 of
|
||||
the 31 minutes of the old `Backend tests (api)` job and set the critical path
|
||||
of every pull request. Each shard lists the package's tests with `go test
|
||||
-race -list` from the commit under test and runs one contiguous quarter of
|
||||
that list in go test's own order, so the shards cover every test exactly once,
|
||||
including tests added later, and a shard that resolves no tests fails. The
|
||||
order is kept on purpose. Some `internal/api` tests depend on package state
|
||||
left by the tests just before them, and an interleaved split by name broke
|
||||
dozens of them. The required check name
|
||||
`Backend tests (api)` belongs to a verdict job that passes only when every
|
||||
shard succeeded, and `Backend tests (rest-0)` and `Backend tests (rest-1)` keep
|
||||
their names and package split. All shards still expand for documentation-only
|
||||
changes so no required check is left pending or reads skipped shards as a
|
||||
pass. Go test steps are not skipped for frontend-only changes, because Go tests
|
||||
read frontend sources (the `internal/api` contract tests, the
|
||||
`internal/unifiedresources` code standards walk of `frontend-modern/src`, and
|
||||
the `internal/telemetry` repository-wide wording scan). Core E2E runs its
|
||||
non-gating probation tier only outside pull requests, since promotion counts
|
||||
only main runs.
|
||||
|
||||
### Docker SDK dependency compatibility
|
||||
|
||||
The Docker consumers use Moby API v1.56.0 and client v0.6.0 together, without
|
||||
|
|
|
|||
|
|
@ -3797,6 +3797,84 @@ func TestBenchmarkQualificationRetainsProvenance(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestBackendAPIShardsKeepRequiredCheckExhaustive(t *testing.T) {
|
||||
content, err := os.ReadFile(repoFile(".github", "workflows", "build-and-test.yml"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
workflow := string(content)
|
||||
|
||||
// Branch protection requires these exact check names. The rest shards stay
|
||||
// matrix entries and internal/api keeps its name on the verdict job.
|
||||
backend := workflowJobBlock(t, workflow, "backend")
|
||||
for _, required := range []string{
|
||||
"name: Backend tests (${{ matrix.shard }})",
|
||||
"shard: [rest-0, rest-1]",
|
||||
"grep -v '/internal/api$'",
|
||||
"go test -race -timeout 50m $pkgs",
|
||||
} {
|
||||
if !strings.Contains(backend, required) {
|
||||
t.Fatalf("backend rest shards missing %q", required)
|
||||
}
|
||||
}
|
||||
|
||||
shards := workflowJobBlock(t, workflow, "backend-api")
|
||||
indexes := regexp.MustCompile(`(?m)^ index: \[([0-9, ]+)\]$`).FindStringSubmatch(shards)
|
||||
count := regexp.MustCompile(`(?m)^ API_SHARD_COUNT: ([0-9]+)$`).FindStringSubmatch(shards)
|
||||
if len(indexes) != 2 || len(count) != 2 {
|
||||
t.Fatal("internal/api shard job must declare its index matrix and API_SHARD_COUNT")
|
||||
}
|
||||
want, _ := strconv.Atoi(count[1])
|
||||
listed := strings.Split(indexes[1], ", ")
|
||||
if want < 2 || len(listed) != want {
|
||||
t.Fatalf("internal/api shard matrix %v must list exactly API_SHARD_COUNT=%d indexes", listed, want)
|
||||
}
|
||||
for i, value := range listed {
|
||||
if value != strconv.Itoa(i) {
|
||||
t.Fatalf("internal/api shard indexes must be 0..%d in order, got %v", want-1, listed)
|
||||
}
|
||||
}
|
||||
for _, required := range []string{
|
||||
"needs: changes",
|
||||
"fail-fast: false",
|
||||
// The list comes from the commit under test, so a new test cannot be
|
||||
// missed, and contiguous slices of go test's own order put it in
|
||||
// exactly one shard while keeping order-coupled neighbours together.
|
||||
"go test -race -list . ./internal/api",
|
||||
`'NR > int(total * i / n) && NR <= int(total * (i + 1) / n)'`,
|
||||
"resolved to an empty test list",
|
||||
"go test -list found no tests in ./internal/api",
|
||||
`go test -race -timeout 50m -run "$pattern" ./internal/api`,
|
||||
"PULSE_DATA_DIR: /tmp/pulse-test-data",
|
||||
} {
|
||||
if !strings.Contains(shards, required) {
|
||||
t.Fatalf("internal/api shard job missing %q", required)
|
||||
}
|
||||
}
|
||||
if strings.Contains(shards, "sort") {
|
||||
t.Fatal("internal/api shards must keep go test's run order; sorting splits order-coupled tests")
|
||||
}
|
||||
if strings.Contains(shards, "\n if:") {
|
||||
t.Fatal("internal/api shards must expand for every change so the verdict never sees skipped shards")
|
||||
}
|
||||
|
||||
verdict := workflowJobBlock(t, workflow, "backend-api-verdict")
|
||||
for _, required := range []string{
|
||||
"name: Backend tests (api)\n",
|
||||
"needs: backend-api",
|
||||
"if: always()",
|
||||
"API_SHARDS_RESULT: ${{ needs.backend-api.result }}",
|
||||
`if [ "$API_SHARDS_RESULT" != success ]; then`,
|
||||
} {
|
||||
if !strings.Contains(verdict, required) {
|
||||
t.Fatalf("Backend tests (api) verdict missing %q", required)
|
||||
}
|
||||
}
|
||||
if got := strings.Count(workflow, "name: Backend tests (api)\n"); got != 1 {
|
||||
t.Fatalf("exactly one job may carry the required Backend tests (api) name, found %d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) {
|
||||
workflowPath := repoFile(".github", "workflows", "build-and-test.yml")
|
||||
assertFileContainsAll(t, workflowPath,
|
||||
|
|
|
|||
|
|
@ -21,6 +21,8 @@ class E2EWorkflowContractTest(unittest.TestCase):
|
|||
probation_step = workflow[probation_start:report_start]
|
||||
|
||||
self.assertIn("continue-on-error: true", probation_step)
|
||||
# Non-gating and promotion counts only main runs, so PRs skip it.
|
||||
self.assertIn("github.event_name != 'pull_request'", probation_step)
|
||||
self.assertIn("timeout-minutes: 12", probation_step)
|
||||
self.assertIn("--max-failures=5", probation_step)
|
||||
self.assertIn("--global-timeout=600000", probation_step)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue