diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02e72637..a4d30325 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -513,11 +513,28 @@ jobs: run: node .github/reviewer-routing.test.js test-focused: - name: Test (changed packages) + name: "Test (focused: ${{ matrix.shard }})" needs: lint if: ${{ needs.lint.outputs.changelog_only != 'true' && needs.lint.outputs.docs_only != 'true' && needs.lint.outputs.full_suite != 'true' }} runs-on: ubuntu-latest + # Each shard owns one bounded slice of the impacted set, so no single job + # carries internal/app together with every reverse dependency. The shard + # list and per-shard execution below mirror test-race, which runs the same + # shards at full-suite scope; release-scripts is included because its + # dedicated job only runs at full-suite or release-sensitive scope, and + # dropping it here would stop testing test/scripts changes entirely. timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + shard: + - app + - generators + - helpers + - cli + - smoke + - remaining + - release-scripts steps: - name: Check out repository uses: actions/checkout@v4 @@ -546,23 +563,77 @@ jobs: with: go-version-file: go.mod - - name: Test changed packages and reverse dependencies + - name: Select impacted packages for shard + id: select shell: bash env: - DWS_PACKAGE_VERSION: 0.0.0-test + TEST_SHARD: ${{ matrix.shard }} run: | set -euo pipefail package_output="$( ./scripts/ci/changed-test-packages.sh \ - list "$TEST_BASE_REF" "$TEST_HEAD_REF" + list-shard "$TEST_SHARD" "$TEST_BASE_REF" "$TEST_HEAD_REF" )" if [ -z "$package_output" ]; then - echo "No buildable Go package is affected by this revision." \ + echo "No buildable Go package in shard $TEST_SHARD is affected by this revision." \ >> "$GITHUB_STEP_SUMMARY" + echo "affected=false" >> "$GITHUB_OUTPUT" exit 0 fi - mapfile -t packages <<< "$package_output" - go test -v -race -count=1 -timeout=15m "${packages[@]}" + # The package list travels through a file rather than a step output: + # reading it with `mapfile < file` has unambiguous line semantics, + # whereas a here-string over a multi-line output would append an extra + # empty element if the value ever carried a trailing newline, and an + # empty element would reach go test as an empty package argument. + printf '%s\n' "$package_output" > "$RUNNER_TEMP/focused-shard-packages.txt" + echo "affected=true" >> "$GITHUB_OUTPUT" + + - name: Build + if: ${{ matrix.shard == 'remaining' && steps.select.outputs.affected == 'true' }} + run: make build + + - name: Install archive tooling + if: ${{ matrix.shard == 'release-scripts' && steps.select.outputs.affected == 'true' }} + run: sudo apt-get update && sudo apt-get install -y zip unzip + + - name: Test shard with Race Detection + if: ${{ steps.select.outputs.affected == 'true' }} + shell: bash + env: + DWS_PACKAGE_VERSION: 0.0.0-test + TEST_SHARD: ${{ matrix.shard }} + run: | + set -euo pipefail + mapfile -t packages < "$RUNNER_TEMP/focused-shard-packages.txt" + test "${#packages[@]}" -gt 0 + for package in "${packages[@]}"; do + test -n "$package" || { + echo "shard package list contains an empty entry" >&2 + exit 1 + } + done + if [ "$TEST_SHARD" = "app" ]; then + # A single long-lived app test process retains every constructed + # command tree in framework registries. Isolate Schema assembly and + # bounded name ranges so each process releases that state on exit. + test "${#packages[@]}" -eq 1 + ./scripts/ci/run-app-race-tests.sh run "${packages[0]}" + exit 0 + fi + if [ "$TEST_SHARD" = "release-scripts" ]; then + # Mirror the dedicated release-contract job: these suites shell out + # to archive tooling and are not race-instrumented there. + go test -v -count=1 -timeout=10m "${packages[@]}" + exit 0 + fi + # cli/smoke own heavy NewRootCommand / Schema assembly under -race; + # give them a dedicated package timeout on slower hosted runners. + timeout_budget=12m + if [ "$TEST_SHARD" = "cli" ] || + [ "$TEST_SHARD" = "smoke" ]; then + timeout_budget=15m + fi + go test -v -race -count=1 -timeout="$timeout_budget" "${packages[@]}" test-race: name: "Test (race: ${{ matrix.shard }})" @@ -715,7 +786,7 @@ jobs: failed=0 if [ "$CHANGELOG_ONLY" = true ] || [ "$DOCS_ONLY" = true ]; then for shard in \ - "changed packages:$FOCUSED_RESULT" \ + "focused shards:$FOCUSED_RESULT" \ "race shards:$RACE_RESULT" \ "release scripts:$RELEASE_SCRIPTS_RESULT" \ "cross-platform compile:$CROSS_PLATFORM_RESULT" \ @@ -744,7 +815,7 @@ jobs: release_expected=success fi for shard in \ - "changed packages:$FOCUSED_RESULT:$focused_expected" \ + "focused shards:$FOCUSED_RESULT:$focused_expected" \ "race shards:$RACE_RESULT:$race_expected" \ "release scripts:$RELEASE_SCRIPTS_RESULT:$release_expected" \ "cross-platform compile:$CROSS_PLATFORM_RESULT:success" diff --git a/scripts/ci/changed-test-packages.sh b/scripts/ci/changed-test-packages.sh index 3558b689..37d31e1c 100755 --- a/scripts/ci/changed-test-packages.sh +++ b/scripts/ci/changed-test-packages.sh @@ -7,16 +7,32 @@ set -eu ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd)" usage() { - printf '%s\n' "usage: $0 BASE_REF HEAD_REF" >&2 + printf '%s\n' \ + "usage: $0 BASE_REF HEAD_REF" \ + " $0 list-shard SHARD BASE_REF HEAD_REF" >&2 exit 2 } -[ "${1:-}" = changed ] || [ "${1:-}" = list ] || usage -[ "$#" -eq 3 ] || usage - -MODE="$1" -BASE_REF="$2" -HEAD_REF="$3" +# list-shard narrows the impacted set to one test shard so focused CI runs can +# fan the same package plan across the shard matrix instead of testing every +# impacted package in a single long-lived job. +SHARD="" +case "${1:-}" in + changed|list) + [ "$#" -eq 3 ] || usage + MODE="$1" + BASE_REF="$2" + HEAD_REF="$3" + ;; + list-shard) + [ "$#" -eq 4 ] || usage + MODE="$1" + SHARD="$2" + BASE_REF="$3" + HEAD_REF="$4" + ;; + *) usage ;; +esac cd "$ROOT" git rev-parse --verify --quiet "${BASE_REF}^{commit}" >/dev/null || { @@ -116,9 +132,29 @@ while IFS= read -r file; do [ "$embedded_owner_found" = true ] || continue done < "$files" +# emit_selection prints the planned packages, restricted to SHARD when the +# caller asked for one. Restriction reuses scripts/ci/test-packages.sh as the +# single source of shard membership, which also means an unknown shard name +# aborts there instead of being reported as an empty selection: a silently empty +# shard would let a mistyped CI shard name skip every test and still succeed. +emit_selection() { + selection="$1" + if [ -z "$SHARD" ]; then + cat "$selection" + return 0 + fi + shard_packages="$workdir/shard-packages" + "$ROOT/scripts/ci/test-packages.sh" list "$SHARD" > "$shard_packages" + LC_ALL=C sort -u "$shard_packages" -o "$shard_packages" + LC_ALL=C sort -u "$selection" -o "$selection" + # An empty intersection is a normal outcome for a shard no change touched, so + # comm's silence must not be treated as an error. + LC_ALL=C comm -12 "$selection" "$shard_packages" +} + LC_ALL=C sort -u "$changed_packages" -o "$changed_packages" if [ "$MODE" = changed ] || [ ! -s "$changed_packages" ]; then - cat "$changed_packages" + emit_selection "$changed_packages" exit 0 fi @@ -133,4 +169,5 @@ while IFS= read -r package; do done < "$all_packages" > "$impacted_packages" cat "$changed_packages" >> "$impacted_packages" -LC_ALL=C sort -u "$impacted_packages" +LC_ALL=C sort -u "$impacted_packages" -o "$impacted_packages" +emit_selection "$impacted_packages" diff --git a/test/scripts/changed_test_packages_test.go b/test/scripts/changed_test_packages_test.go index 3f228c0a..f696e569 100644 --- a/test/scripts/changed_test_packages_test.go +++ b/test/scripts/changed_test_packages_test.go @@ -129,6 +129,245 @@ func Value() string { } } +// shardChangedPackagesShards mirrors the shard names scripts/ci/test-packages.sh +// asserts complete coverage for in its verify mode. Keeping the list here in +// full is deliberate: the union assertion below fails if a shard is added +// upstream without being selected, and an unknown-shard error fails if one is +// removed, so shard-plan drift cannot silently shrink focused-test coverage. +var shardChangedPackagesShards = []string{ + "app", + "generators", + "helpers", + "cli", + "smoke", + "remaining", + "release-scripts", +} + +func TestChangedTestPackagesShardSelectionCoversEveryImpactedPackageExactlyOnce(t *testing.T) { + repository := newShardChangedPackagesRepository(t) + baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture") + + writeChangedTestPackagesFixture(t, repository, "core/core.go", `package core + +func Value() string { + return "changed" +} +`) + headRef := commitChangedTestPackagesFixture(t, repository, "change core") + + impacted := runChangedTestPackages(t, repository, "list", baseRef, headRef) + if len(impacted) == 0 { + t.Fatal("shard fixture produced no impacted packages") + } + + owner := map[string]string{} + union := make([]string, 0, len(impacted)) + for _, shard := range shardChangedPackagesShards { + for _, pkg := range runChangedTestPackagesShard(t, repository, shard, baseRef, headRef) { + if previous, ok := owner[pkg]; ok { + t.Fatalf("package %q selected by both shard %q and %q", pkg, previous, shard) + } + owner[pkg] = shard + union = append(union, pkg) + } + } + + slices.Sort(union) + if !slices.Equal(union, impacted) { + t.Fatalf("shard selection union = %q, want every impacted package %q", union, impacted) + } +} + +func TestChangedTestPackagesShardSelectionFailsClosedOnUnknownShard(t *testing.T) { + repository := newShardChangedPackagesRepository(t) + baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture") + + writeChangedTestPackagesFixture(t, repository, "core/core.go", `package core + +func Value() string { + return "changed" +} +`) + headRef := commitChangedTestPackagesFixture(t, repository, "change core") + + script := filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh") + command := exec.Command(script, "list-shard", "nonexistent", baseRef, headRef) + command.Dir = repository + output, err := command.CombinedOutput() + if err == nil { + t.Fatalf("unknown shard unexpectedly succeeded:\n%s", output) + } + // An unknown shard must abort rather than report an empty selection: a + // silently empty shard would let a mistyped CI shard name skip every test + // while still reporting success. + if !strings.Contains(string(output), "unknown test package shard") { + t.Fatalf("unknown shard failure = %q, want fail-closed diagnostic", output) + } + if strings.TrimSpace(string(output)) == "" { + t.Fatal("unknown shard produced empty output instead of a diagnostic") + } +} + +func TestChangedTestPackagesShardSelectionIsEmptyForUnaffectedShard(t *testing.T) { + repository := newShardChangedPackagesRepository(t) + baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture") + + writeChangedTestPackagesFixture(t, repository, "internal/helpers/helpers.go", `package helpers + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " helpers changed" +} +`) + headRef := commitChangedTestPackagesFixture(t, repository, "change helpers only") + + if selected := runChangedTestPackagesShard(t, repository, "helpers", baseRef, headRef); len(selected) == 0 { + t.Fatal("helpers shard selected nothing for a helpers-only change") + } + // An unaffected shard is a normal outcome and must exit zero with no + // output so the CI step can no-op instead of failing the job. + if selected := runChangedTestPackagesShard(t, repository, "app", baseRef, headRef); len(selected) != 0 { + t.Fatalf("app shard = %q, want empty for a helpers-only change", selected) + } +} + +func TestChangedTestPackagesShardSelectionRejectsMissingShardArgument(t *testing.T) { + repository := newShardChangedPackagesRepository(t) + baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture") + + script := filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh") + command := exec.Command(script, "list-shard", baseRef, baseRef) + command.Dir = repository + output, err := command.CombinedOutput() + if err == nil { + t.Fatalf("list-shard without a shard argument unexpectedly succeeded:\n%s", output) + } + if !strings.Contains(string(output), "usage:") { + t.Fatalf("missing shard argument failure = %q, want usage diagnostic", output) + } +} + +func runChangedTestPackagesShard(t *testing.T, repository, shard, baseRef, headRef string) []string { + t.Helper() + + output := runChangedTestPackagesCommand( + t, + repository, + filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh"), + "list-shard", + shard, + baseRef, + headRef, + ) + return strings.Fields(output) +} + +// newShardChangedPackagesRepository builds a fixture whose directory layout +// matches every shard scripts/ci/test-packages.sh knows about, so shard +// selection can be exercised without depending on the real repository's working +// tree state. Every package imports core, letting a single core edit mark the +// whole module impacted. +func newShardChangedPackagesRepository(t *testing.T) string { + t.Helper() + + repository := t.TempDir() + for path, contents := range map[string]string{ + "go.mod": `module example.com/shardfixture + +go 1.22 +`, + "core/core.go": `package core + +func Value() string { + return "core" +} +`, + "internal/app/app.go": `package app + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " app" +} +`, + "internal/generator/gen/gen.go": `package gen + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " gen" +} +`, + "internal/helpers/helpers.go": `package helpers + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " helpers" +} +`, + "internal/cli/cli.go": `package cli + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " cli" +} +`, + "test/smoke/smoke.go": `package smoke + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " smoke" +} +`, + "test/scripts/scripts.go": `package scripts + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " scripts" +} +`, + "pkg/extra/extra.go": `package extra + +import "example.com/shardfixture/core" + +func Value() string { + return core.Value() + " extra" +} +`, + } { + writeChangedTestPackagesFixture(t, repository, path, contents) + } + + projectRoot, err := filepath.Abs(filepath.Join("..", "..")) + if err != nil { + t.Fatalf("resolve project root: %v", err) + } + for _, name := range []string{"changed-test-packages.sh", "test-packages.sh"} { + script, err := os.ReadFile(filepath.Join(projectRoot, "scripts", "ci", name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + scriptPath := filepath.Join(repository, "scripts", "ci", name) + if err := os.MkdirAll(filepath.Dir(scriptPath), 0o755); err != nil { + t.Fatalf("create script directory: %v", err) + } + if err := os.WriteFile(scriptPath, script, 0o755); err != nil { + t.Fatalf("copy %s: %v", name, err) + } + } + + runChangedTestPackagesCommand(t, repository, "git", "init", "-q", "-b", "main") + runChangedTestPackagesCommand(t, repository, "git", "config", "user.name", "DWS CI") + runChangedTestPackagesCommand(t, repository, "git", "config", "user.email", "dws-ci@example.invalid") + return repository +} + func newChangedTestPackagesRepository(t *testing.T) string { t.Helper() diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 8f6be15a..849d9191 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -1151,13 +1151,13 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) { "filename.startsWith('pkg/cmdutil/')", "filename === 'skills_embed.go'", "filename.startsWith('test/mock_mcp/')", - "name: Test (changed packages)", + `name: "Test (focused: ${{ matrix.shard }})"`, "changed-test-packages.sh", "Verify authoritative synthetic merge", `test "$(git rev-parse HEAD^1)" = "$PR_BASE_SHA"`, `test "$(git rev-parse HEAD^2)" = "$PR_HEAD_SHA"`, `echo "TEST_HEAD_REF=$(git rev-parse HEAD)"`, - "list \"$TEST_BASE_REF\" \"$TEST_HEAD_REF\"", + `list-shard "$TEST_SHARD" "$TEST_BASE_REF" "$TEST_HEAD_REF"`, "needs.lint.outputs.full_suite != 'true'", `name: "Test (race: ${{ matrix.shard }})"`, "name: Test (workflow and release contracts)", @@ -1217,8 +1217,30 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) { if !strings.Contains(focusedJob, "timeout-minutes: 20") { t.Error("focused test job must allow the scoped race suite up to 20 minutes") } - if !strings.Contains(focusedJob, `go test -v -race -count=1 -timeout=15m "${packages[@]}"`) { - t.Error("focused race tests must retain enough package-level time for internal/app") + // The focused path fans the impacted set across the same shards as test-race + // and runs each shard the way test-race runs it, so no single job carries + // internal/app together with its reverse dependencies. internal/app keeps its + // package-level headroom through the process-isolating helper instead of one + // long -timeout, which is strictly stronger: every process releases the + // framework registries it populated. release-scripts is asserted because its + // dedicated job only runs at full-suite or release-sensitive scope, so losing + // it here would silently stop testing test/scripts changes. + for _, want := range []string{ + `if [ "$TEST_SHARD" = "app" ]; then`, + `test "${#packages[@]}" -eq 1`, + `./scripts/ci/run-app-race-tests.sh run "${packages[0]}"`, + `if [ "$TEST_SHARD" = "release-scripts" ]; then`, + `go test -v -count=1 -timeout=10m "${packages[@]}"`, + "timeout_budget=12m", + `if [ "$TEST_SHARD" = "cli" ] ||`, + `[ "$TEST_SHARD" = "smoke" ]; then`, + "timeout_budget=15m", + `go test -v -race -count=1 -timeout="$timeout_budget" "${packages[@]}"`, + "- release-scripts", + } { + if !strings.Contains(focusedJob, want) { + t.Errorf("focused test job missing shard contract %q", want) + } } raceStart := focusedEnd