-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
kvserver: unskip TestNewTruncateDecision
#98792
kvserver: unskip TestNewTruncateDecision
#98792
Conversation
1fef267
to
675271a
Compare
Saw this fail under stressrace:
|
Epic: none Release note: None
675271a
to
563309c
Compare
Disabling quiescence, as indicated in #38584 (comment), fixed it under stressrace too. Not pretty, but it works. |
@@ -450,7 +449,11 @@ func TestNewTruncateDecision(t *testing.T) { | |||
defer leaktest.AfterTest(t)() | |||
defer log.Scope(t).Close(t) | |||
|
|||
skip.WithIssue(t, 38584) | |||
// Unquiescing can add spurious empty log entries. Just disable it. | |||
testingDisableQuiescence = 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.
Can it race with other tests that use this when tests are run in parallel?
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.
Race or (worse) interact in random ways.
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.
We don't run tests in parallel in the same package, only separate packages (which are run as separate binaries).
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.
Does dev test --stress
run separate binaries too?
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.
Yes.
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.
FWIW, to run Go tests in parallel, they must explicitly opt in to it by calling t.Parallel()
. We only do this in a handful of packages, and not in kvserver
.
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.
Still LGTM, assuming parallel test runs aren't possible.
bors r+ |
Build failed (retrying...): |
Build succeeded: |
Passed after 10k stress runs. Has been skipped since 2019, issue seems to have been fixed in the meanwhile.
Resolves #38584.
Epic: none
Release note: None