From 4cab370652b5dc556c35689cd457224c28067a3e Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 9 May 2026 06:52:20 +0200 Subject: [PATCH] ci: address review feedback on shard scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - partition-by-shard.sh: fix off-by-one. `NR % t == r` placed item 1 in shard `total`, item 2 in shard 1 — shifted by one. Switch to `(NR-1) % t == r` so shard 1 owns items 1, 1+t, 1+2t, ... as documented. - test-integration-shard.sh: the `[A-Z]` after `Test` rejected `Test_Foo`-style names (28 such tests in tests/integration). Drop the [A-Z] and broaden the comment to match the actual regex. - test-integration-shard.sh: drop `*testing.TB` alternative — there are zero matching tests in this codebase, so the simpler regex is exact. - find-gogit-test-pkgs.sh: merge the `grep -vE` exclusion into the awk filter. Avoids a pipefail edge case where empty grep output would abort the pipeline before the friendly empty-list message fires; also one fewer process per invocation. - Makefile test-integration: fail fast at Make level if TEST_SHARD is set without TEST_TOTAL_SHARDS, instead of letting the script's ${X:?...} surface a less obvious error. All locals in the new bash scripts use UPPER_CASE to match the existing tools/test-e2e.sh convention. Co-Authored-By: Claude (Opus 4.7) --- Makefile | 11 ++++------- tools/find-gogit-test-pkgs.sh | 18 +++++++++++++----- tools/partition-by-shard.sh | 19 ++++++++++++------- tools/test-integration-shard.sh | 19 +++++++------------ 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/Makefile b/Makefile index 3c6b6905f5..bf5b29ca53 100644 --- a/Makefile +++ b/Makefile @@ -384,22 +384,16 @@ test-backend: ## test backend files .PHONY: test-backend-gogit test-backend-gogit: ## test packages whose code or tests import the gogit-affected modules @pkgs=$$(./tools/find-gogit-test-pkgs.sh '$(TAGS)') && \ - if [ -z "$$pkgs" ]; then echo "no gogit-affected packages found" >&2; exit 1; fi && \ - echo "Running go test with $(GOTEST_FLAGS) -tags '$(TAGS)' over $$(echo $$pkgs | wc -w) gogit-affected packages..." && \ $(GO) test $(GOTEST_FLAGS) -tags='$(TAGS)' $$pkgs .PHONY: test-backend-shard test-backend-shard: ## run the TEST_SHARD/TEST_TOTAL_SHARDS slice of test-backend - @pkgs=$$(echo "$(GO_TEST_PACKAGES)" | tr ' ' '\n' | ./tools/partition-by-shard.sh | tr '\n' ' ') && \ - if [ -z "$$pkgs" ]; then echo "shard $$TEST_SHARD/$$TEST_TOTAL_SHARDS has no test-backend packages" >&2; exit 1; fi && \ - echo "Running shard $$TEST_SHARD/$$TEST_TOTAL_SHARDS of test-backend ($$(echo $$pkgs | wc -w) packages)..." && \ + @pkgs=$$(echo "$(GO_TEST_PACKAGES)" | tr ' ' '\n' | ./tools/partition-by-shard.sh) && \ $(GO) test $(GOTEST_FLAGS) -tags='$(TAGS)' $$pkgs .PHONY: test-backend-gogit-shard test-backend-gogit-shard: ## run the TEST_SHARD/TEST_TOTAL_SHARDS slice of test-backend-gogit @pkgs=$$(./tools/find-gogit-test-pkgs.sh '$(TAGS)' | ./tools/partition-by-shard.sh) && \ - if [ -z "$$pkgs" ]; then echo "shard $$TEST_SHARD/$$TEST_TOTAL_SHARDS has no gogit-affected packages" >&2; exit 1; fi && \ - echo "Running shard $$TEST_SHARD/$$TEST_TOTAL_SHARDS of test-backend-gogit ($$(echo $$pkgs | wc -w) packages)..." && \ $(GO) test $(GOTEST_FLAGS) -tags='$(TAGS)' $$pkgs .PHONY: test-frontend @@ -466,6 +460,9 @@ test-integration: @# they mutate the work directory, so cache inputs change between runs. $(GO) test $(GOTEST_FLAGS) -tags '$(TAGS)' -c code.gitea.io/gitea/tests/integration -o ./test-integration-$(GITEA_TEST_DATABASE).test ifdef TEST_SHARD +ifndef TEST_TOTAL_SHARDS + $(error TEST_TOTAL_SHARDS must be set when TEST_SHARD is set) +endif ./tools/test-integration-shard.sh ./test-integration-$(GITEA_TEST_DATABASE).test else ./test-integration-$(GITEA_TEST_DATABASE).test diff --git a/tools/find-gogit-test-pkgs.sh b/tools/find-gogit-test-pkgs.sh index 83809a6a05..1d60a55a06 100755 --- a/tools/find-gogit-test-pkgs.sh +++ b/tools/find-gogit-test-pkgs.sh @@ -10,11 +10,19 @@ set -euo pipefail # transitively by their consumers, so any tag-related compile error would # already surface in those consumers' test builds. -tags=${1:?usage: $0 TAGS} +TAGS=${1:?usage: $0 TAGS} # Exclusions mirror the Makefile's GO_TEST_PACKAGES filter — these packages # need a real database / dedicated harness and are tested separately. -go list -tags "$tags" -f '{{if or .TestGoFiles .XTestGoFiles}}{{.ImportPath}}|{{range .Imports}}{{.}};{{end}}{{range .TestImports}}{{.}};{{end}}{{range .XTestImports}}{{.}};{{end}}{{end}}' ./... \ - | awk -F'|' '$2 ~ /code\.gitea\.io\/gitea\/modules\/(git|gitrepo|lfs)[\/;]/ { print $1 }' \ - | grep -vE '^code\.gitea\.io/gitea/(models/migrations(/|$)|tests(/integration(/migration-test)?)?$)' \ - | sort -u +OUT=$(go list -tags "$TAGS" -f '{{if or .TestGoFiles .XTestGoFiles}}{{.ImportPath}}|{{range .Imports}}{{.}};{{end}}{{range .TestImports}}{{.}};{{end}}{{range .XTestImports}}{{.}};{{end}}{{end}}' ./... \ + | awk -F'|' ' + $2 ~ /code\.gitea\.io\/gitea\/modules\/(git|gitrepo|lfs)[\/;]/ && + $1 !~ /^code\.gitea\.io\/gitea\/(models\/migrations(\/|$)|tests(\/integration(\/migration-test)?)?$)/ { + print $1 + }' \ + | sort -u) +if [ -z "$OUT" ]; then + echo "no gogit-affected packages found" >&2 + exit 1 +fi +echo "$OUT" diff --git a/tools/partition-by-shard.sh b/tools/partition-by-shard.sh index 7e55368f27..acb5139251 100755 --- a/tools/partition-by-shard.sh +++ b/tools/partition-by-shard.sh @@ -5,16 +5,21 @@ set -euo pipefail # items), partitioned round-robin by line number after a deterministic sort. # Required env: TEST_SHARD (1..TEST_TOTAL_SHARDS), TEST_TOTAL_SHARDS (>= 1). -shard=${TEST_SHARD:?missing TEST_SHARD} -total=${TEST_TOTAL_SHARDS:?missing TEST_TOTAL_SHARDS} +SHARD=${TEST_SHARD:?missing TEST_SHARD} +TOTAL=${TEST_TOTAL_SHARDS:?missing TEST_TOTAL_SHARDS} -if ! [[ "$total" =~ ^[1-9][0-9]*$ ]]; then - echo "TEST_TOTAL_SHARDS must be a positive integer, got: $total" >&2 +if ! [[ "$TOTAL" =~ ^[1-9][0-9]*$ ]]; then + echo "TEST_TOTAL_SHARDS must be a positive integer, got: $TOTAL" >&2 exit 2 fi -if ! [[ "$shard" =~ ^[1-9][0-9]*$ ]] || [ "$shard" -gt "$total" ]; then - echo "TEST_SHARD must be in [1, $total], got: $shard" >&2 +if ! [[ "$SHARD" =~ ^[1-9][0-9]*$ ]] || [ "$SHARD" -gt "$TOTAL" ]; then + echo "TEST_SHARD must be in [1, $TOTAL], got: $SHARD" >&2 exit 2 fi -LC_ALL=C sort -u | awk -v r=$((shard - 1)) -v t="$total" 'NR % t == r' +OUT=$(LC_ALL=C sort -u | awk -v r=$((SHARD - 1)) -v t="$TOTAL" '(NR - 1) % t == r') +if [ -z "$OUT" ]; then + echo "shard $SHARD/$TOTAL has no items assigned" >&2 + exit 1 +fi +echo "$OUT" diff --git a/tools/test-integration-shard.sh b/tools/test-integration-shard.sh index f51543bb5f..6f007cebd8 100755 --- a/tools/test-integration-shard.sh +++ b/tools/test-integration-shard.sh @@ -6,18 +6,13 @@ set -euo pipefail # because TestMain boots the full Gitea environment and would panic without # a configured database. -binary=$1 +BINARY=$1 -# match `func Test*(t *testing.T|TB)` only — excludes TestMain (takes *testing.M) -names=$(grep -hE '^func Test[A-Z][A-Za-z0-9_]*\([a-zA-Z_][a-zA-Z0-9_]* \*testing\.(T|TB)\)' tests/integration/*.go \ - | sed -E 's/^func (Test[A-Z][A-Za-z0-9_]*).*/\1/' \ +# match `func Test...(t *testing.T)` only — `*testing.M` excludes TestMain +NAMES=$(grep -hE '^func Test[A-Za-z0-9_]*\([a-zA-Z_][a-zA-Z0-9_]* \*testing\.T\)' tests/integration/*.go \ + | sed -E 's/^func (Test[A-Za-z0-9_]*).*/\1/' \ | ./tools/partition-by-shard.sh) -if [ -z "$names" ]; then - echo "no tests assigned to shard $TEST_SHARD/$TEST_TOTAL_SHARDS — likely a misconfiguration" >&2 - exit 1 -fi - -pattern=$(echo "$names" | paste -sd '|' -) -echo "Running shard $TEST_SHARD/$TEST_TOTAL_SHARDS ($(echo "$names" | wc -l | tr -d ' ') tests)" -exec "$binary" -test.run "^($pattern)\$" +PATTERN=$(echo "$NAMES" | paste -sd '|' -) +echo "Running shard $TEST_SHARD/$TEST_TOTAL_SHARDS ($(echo "$NAMES" | wc -l | tr -d ' ') tests)" +exec "$BINARY" -test.run "^($PATTERN)\$"