Skip to content

test(cubemaster): add race and docker-backed test gates, de-flake unit tests - #1491

Open
fslongjin wants to merge 1 commit into
TencentCloud:masterfrom
fslongjin:test-cubemaster-race-docker
Open

test(cubemaster): add race and docker-backed test gates, de-flake unit tests#1491
fslongjin wants to merge 1 commit into
TencentCloud:masterfrom
fslongjin:test-cubemaster-race-docker

Conversation

@fslongjin

@fslongjin fslongjin commented Aug 24, 2026

Copy link
Copy Markdown
Member

Summary

Add race and Docker-backed test gates for CubeMaster unit tests, and replace time.Sleep polling with channel-based synchronization to remove flakiness.

  • CI: extend the CubeMaster test matrix with Race and Docker jobs, introducing three run modes: root_make, cubemaster_builder, and cubemaster_host
  • Makefile: add test-race and test-docker targets; test-docker fails fast when the Docker daemon is unreachable instead of skipping
  • queueworker / recov tests: replace time.Sleep polling with channel-based synchronization to avoid intermittent failures
  • tests/unittest: gate the full race sweep and the Docker-backed database tests

Testing

  • The race gate covers all CubeMaster unit-test packages (-race -count=1)
  • The Docker gate covers MySQL/PostgreSQL-related DAO and template-center tests, run on the host against the Docker daemon
  • The regular -short suite is unaffected

…t tests

- CI: expand CubeMaster test matrix with Race and Docker jobs, introducing
  root_make / cubemaster_builder / cubemaster_host run modes
- Makefile: add test-race and test-docker targets; test-docker fails fast
  when no Docker daemon is reachable
- queueworker, recov: replace time.Sleep polling with channel-based
  synchronization in tests to remove flakes
- tests/unittest: gate the new race sweep and docker-backed db tests

Signed-off-by: jinlong <jinlong@tencent.com>
Comment thread CubeMaster/Makefile
@command -v docker >/dev/null 2>&1 || { echo "error: docker is not installed"; exit 1; }
@docker info >/dev/null 2>&1 || { echo "error: docker daemon is not reachable"; exit 1; }
$(Q)CI=true CUBEMASTER_REQUIRE_DOCKER_TESTS=1 go test -v -count=1 \
-gcflags=all=-l -timeout=20m -run '$(DOCKER_TEST_PATTERN)' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Docker gate can silently pass with zero tests executed. go test -run exits 0 with "no tests to run" when the regex matches nothing, so if any of the six hardcoded test names is renamed/split or this regex drifts, the gate goes green without exercising anything — defeating the purpose of a hard docker-backed gate. Consider asserting the expected tests actually ran (e.g. fail when a package reports zero matching tests, or parse go test -v/-json output for each expected test name).

Comment thread CubeMaster/Makefile
$(call msg,Run race-enabled unit tests)
$(Q)go clean -testcache
@( for pkg in ${COVERAGE_PACKAGES}; do \
CI=true CGO_ENABLED=1 go test -short -v -race -count=1 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This race sweep covers ./pkg/templatecenter, whose gomonkey-based tests the existing test target already documents as needing -gcflags=all=-l because "a stub intermittently fails to take effect." gomonkey patches a function's entry instructions; the race detector instruments function prologues with tsan calls, changing the instruction layout gomonkey rewrites. This combination is a known source of intermittent "stub did not take effect" failures — worth validating this gate is stable across several CI runs before relying on it. If it flakes, scoping -race to packages that don't use gomonkey would reduce noise.

@cubesandboxbot

Copy link
Copy Markdown

Review: test(cubemaster) — add race and docker-backed test gates, de-flake unit tests

AI-generated review of PR #1491 (base branch master).

What the PR does

  • Adds two new CubeMaster CI jobs (CubeMaster Race, CubeMaster Docker) to unit-test-check.yml, expanding the jq-generated matrix per-arch, and introduces run_mode dispatch (root_make / cubemaster_builder / cubemaster_host).
  • Adds test-race (full -race -count=1 -short sweep over all unit packages, run inside the builder) and test-docker (Docker-backed DB tests, run on the host, fail-fast when the daemon is unreachable) to CubeMaster/Makefile.
  • Registers both as gated components in tests/unittest/run.sh.
  • Replaces time.Sleep polling with channel synchronization in queueworker and recov tests.

What's good

  • The test rewrites are correct and actually fix real race-detector violations in the base code: the old TestQueueBlock wrote got from a goroutine and read it unsynchronized, and the old retry tests read panicTime non-atomically while the goroutine wrote it via atomic.AddInt32. The new channel-close / atomic.LoadInt32 versions establish proper happens-before edges, so the new -race gate is doing real work.
  • The -short skip checks in all Docker-backed helpers (newMySQLDockerEnv, newPGDockerEnv, newPostgresTestEnv, newArtifactGCMySQL) are honored, so the regular test and test-race targets in the builder (which has no reachable daemon) stay green.
  • test-docker failing fast on docker info rather than skipping matches the stated intent, and requireDockerTests() is consistent with it.
  • The workflow matrix/jq expansion and run-mode dispatch look correct; BUILDER_IMAGE is only consumed by modes that need it, and the host Go setup is scoped to cubemaster_host.

Findings

1. [Medium] The Docker gate can silently pass with zero tests executed.
CubeMaster/Makefile:158 selects tests purely by the anchored -run '^(...|...)$' regex. go test -run exits 0 with "no tests to run" when nothing matches, so if any of the six hardcoded test names is renamed/split or the regex drifts, the gate goes green without exercising anything — defeating the purpose of a hard gate. Consider asserting the expected tests actually ran (e.g. check go test -v/-json output for each expected test name, or fail when a package reports zero matching tests).

2. [Medium] -race + gomonkey is a plausible flakiness source for the new race gate.
COVERAGE_PACKAGES includes ./pkg/templatecenter, whose gomonkey-based tests the existing test target comment already describes as intermittently failing without -gcflags=all=-l ("a stub intermittently fails to take effect and the real function runs"). gomonkey patches a function's entry instructions; the race detector instruments function prologues with tsan calls, changing the instruction layout gomonkey computes against. This combination is a known source of intermittent "stub did not take effect" / rewrite failures. Worth validating the gate is stable across a few CI runs before relying on it; if it flakes, scoping -race to packages without gomonkey usage (or isolating the templatecenter package) would reduce noise.

3. [Low] CI cost roughly triples for CubeMaster.
test-race compiles the whole module with inlining disabled (-gcflags=all=-l) and -race, -count=1, on both amd64 and arm64; test-docker additionally spins up 5 DB containers per arch. CubeMaster goes from 2 jobs to 6, with the race jobs being the slowest. If a lighter gate is acceptable, consider running the race sweep on amd64 only (arch-independent findings) and/or trimming the package set.

4. [Low] Latent footgun: test and test-race stay green only because every Docker test honors testing.Short().
Both targets run with CI=true (set in the Makefile recipes), so requireDockerTests() returns true; the only thing preventing a t.Fatal inside the builder (no daemon) is the -short skip. A future Docker-backed test that forgets the Short() guard will break the regular test/test-race gates, not just test-docker. A one-line comment on test/test-race noting this contract would help.

Notes

  • test-docker runs on both arches and does no explicit go mod download; the cold-run module fetch happens inside the 20m -timeout. Fine, but the first run on a fresh arm64 runner will be slow.
  • Non-blocking: the workflow's Set up Go step relies on actions/setup-go@v6 and go-version-file: CubeMaster/go.mod (go 1.25.7), consistent with the builder's GO_VERSION. No issue found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant