Skip to content

go: honor [go-test].force_race and the test_race/test_msan/test_asan fields - #23640

Open
aegis-cipherowl wants to merge 1 commit into
pantsbuild:mainfrom
aegis-cipherowl:henry-yang/INF-1373/go-test-honor-force-race
Open

go: honor [go-test].force_race and the test_race/test_msan/test_asan fields#23640
aegis-cipherowl wants to merge 1 commit into
pantsbuild:mainfrom
aegis-cipherowl:henry-yang/INF-1373/go-test-honor-force-race

Conversation

@aegis-cipherowl

Copy link
Copy Markdown

prepare_go_test_binary builds its GoBuildOptionsFromTargetRequest without for_tests=True:

go_extract_build_options_from_target(
    GoBuildOptionsFromTargetRequest(request.field_set.address), **implicitly()
),

go_extract_build_options_from_target only populates test_target_fields when that flag is set:

if request.for_tests and GoTestBuildOptionsFieldSet.is_applicable(target):
    test_target_fields = GoTestBuildOptionsFieldSet.create(target)

and test_target_fields gates both the [go-test].force_race branch and the go_package(test_race=...) field (likewise test_msan / test_asan).

go_package also does not carry the non-test race / cgo_enabled fields, so GoBuildOptionsFieldSet does not apply to it either and that fallback misses as well. The net effect is that the only field the resolution chain can still reach is race on the owning go_mod, so both documented ways to enable the race detector for a test are silently ignored — tests keep passing while no longer detecting races.

Introduced in #17825, which split test-binary production from running it.

The existing coverage in build_opts_test.py passes for_tests=True explicitly, which is why it never caught the buggy caller.

Fix

Pass for_tests=True. One line.

Tests

Three tests in test_test.py, exercising [go-test].force_race, go_package(test_race=True), and a default-off control so the probe cannot pass for the wrong reason.

They use a real unsynchronised write rather than a //go:build race file. That was my first attempt, and it does not work: package analysis passes CGO_ENABLED and EXTRA_BUILD_TAGS but never the race build tag, so //go:build race files are not selected even when -race does reach the compiler. That looks like a separate gap and I have deliberately left it alone here — happy to file it separately if it is not already known.

Verified red/green on macOS/arm64:

before after
test_race_detector_enabled_by_force_race_option fails (race unreported) passes
test_race_detector_enabled_by_test_race_field fails (race unreported) passes
test_race_detector_disabled_by_default passes passes

The tests skip on platforms where race_detector_supported() is false.

Note: build_opts_test.py::test_runtime_check_enable_fields_work_as_expected[msan|asan] and test_test.py::test_external_test_with_use_coverage already fail on my machine on an unmodified main, so they are unrelated to this change.

…t_asan` fields

`prepare_go_test_binary` built its `GoBuildOptionsFromTargetRequest` without
`for_tests=True`. `go_extract_build_options_from_target` only populates
`test_target_fields` when that flag is set, and it gates both the
`[go-test].force_race` branch and the `go_package` `test_race` field (likewise
for msan/asan).

`go_package` also does not carry the non-test `race`/`cgo_enabled` fields, so
`GoBuildOptionsFieldSet` does not apply to it either. The net effect was that
the only field the resolution chain could still reach was `race` on the owning
`go_mod` target -- both documented ways to enable the race detector for a test
were silently ignored, and tests kept passing while no longer detecting races.

Regression tests use a real unsynchronised write rather than the `race` build
constraint, because package analysis passes `CGO_ENABLED` and
`EXTRA_BUILD_TAGS` but never the `race` tag, so `//go:build race` files are not
selected even when `-race` reaches the compiler. Verified red/green: both
positive tests fail on the unfixed tree and pass with this change, and the
default-off control passes either way.

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

Labels

category:bugfix Bug fixes for released features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants