-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
tests: add tests for go/atomic2
#14975
Conversation
Signed-off-by: Manik Rana <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14975 +/- ##
==========================================
- Coverage 47.26% 47.26% -0.01%
==========================================
Files 1138 1138
Lines 238842 238842
==========================================
- Hits 112880 112878 -2
+ Misses 117368 117362 -6
- Partials 8594 8602 +8 ☔ View full report in Codecov by Sentry. |
|
||
func TestCompareAndSwapWriteBarrier(t *testing.T) { | ||
writeBarrier.enabled = true | ||
require.True(t, writeBarrier.enabled, "writeBarrier should be enabled") |
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 seems like a very tautological tests and not really useful.
} | ||
|
||
func TestCompareAndSwapWriteBarrier(t *testing.T) { | ||
writeBarrier.enabled = true |
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 modifies global state and at least we need to revert it back at the end of the test, but it also could introduce risk for flaky tests if anything runs in parallel.
Also this links to runtime stuff, so I think we're tbh probably better off not adding this test here. Because this hooks into the Go runtime.
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 see, this test was made just to cover this conditional. Is there a way to test it without modifying global state?
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 don’t think there’s a good way to test so I think we should skip trying to further test this.
Code coverage is a tool to find areas that might need more tests. It’s not a goal in itself to get to 100% coverage. That can have detrimental effects and this is one of those cases.
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.
Okay, I'll close this PR
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 don't think we should add this test here. This hooks into the Go runtime and modifies things there which isn't something I think we want to do from these tests.
Description
achieve 100% coverage for
go/atomic2
Related Issue(s)
#14931
Checklist
Deployment Notes