New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support Go data race detector #17510
Conversation
Bike shed for naming the fields: #17517 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
227a404
to
0301c43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the field renames and help messages. Thanks!
# Extract the `with_race_detector` value for this target. | ||
with_race_detector: bool | None = None | ||
if go_test_subsystem.force_race: | ||
with_race_detector = True | ||
if test_target_fields is not None and test_target_fields.test_race.value is not None: | ||
with_race_detector = test_target_fields.test_race.value | ||
if with_race_detector is None: | ||
if target_fields is not None and target_fields.race.value is not None: | ||
with_race_detector = target_fields.race.value | ||
if with_race_detector is None: | ||
if go_mod_target_fields.race is not None: | ||
with_race_detector = go_mod_target_fields.race.value | ||
if with_race_detector is None: | ||
with_race_detector = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be valuable to extract into a helper function so that you can unit test. Lots of conditions here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how come not using elif
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of those if with_race_detector is None
clauses have an inner if
clause that may not be triggered. Thus with_race_detector
could continue to be None
. elif
would have different semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the code by introducing a _first_non_none_value
helper. Added tests as well of the various combinations.
0301c43
to
1c0cbf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great refactor and tests. Thank you!
Support the Go data race detector.
Adds a
race
field to thego_mod
andgo_binary
target types and atest_race
field to forgo_package
for tests. The--go-test-force-race
option will enable the race detector for tests regardless of the fields. The "outermost" relevant field will take effect; that is, arace
field ongo_binary
will override anyrace
field on the relevantgo_mod
, and with tests atest_race
field on the relevantgo_package
will override anyrace
field on the relevantgo_mod
.See https://go.dev/doc/articles/race_detector for additional information about the Go data race detector.
Fixes #17437.