-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
testing: Avoid using context.Background #3949
Conversation
I have intentionally split the changes into commits based on the parent directory. This will make it easier for me to do any changes suggested during the review. @dfawley Could you please add |
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 want to change context.Background()
with context.TODO()
. What we want instead is what @dfawley mentioned in #1878 (comment).
Each test file (one per package) should define a defaultTestTimeout
. And each test must create a local context with the appropriate timeout.
const defaultTestTimeout = 10 * time.Second
func TestXxx(t *testing.T) {
ctx, cancel := context.WithTimeout(defaultTestTimeout)
defer cancel()
// Do stuff and pass `ctx` to functions which accept one.
}
And for places where we block on some event not happening, we should define a much shorter timeout (something of the order of 10ms), create a new context with that short deadline, and use that.
Hope this helps.
Thanks.
0c349d7
to
9219c52
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 checked the last commit. The changes look mostly OK. I left some minor comments. Thank you so very much for doing this.
9219c52
to
80784df
Compare
ac2ed64
to
9dd903c
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.
Thank you so very much.
test/end2end_test.go
Outdated
if err != nil { | ||
t.Fatalf("%v.FullDuplexCall(_) = _, %v, want <nil>", tc, err) | ||
} | ||
cancel() | ||
|
||
ctx, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) |
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't we use the ctx
created in line 1272 here? And do a defer cancel()
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.
That's right. I will do a fix and rebase. Thanks. :)
9dd903c
to
7ab6bc8
Compare
I'm super glad I could contribute. Please let me what I can pick next. 🚀 |
7ab6bc8
to
ea00658
Compare
@gauravgahlot : Could you please take care of the test failure here: https://github.com/grpc/grpc-go/pull/3949/checks?check_run_id=1340435962. Thanks. |
251441f
to
4299949
Compare
4299949
to
435f577
Compare
health/ metadata/ reflection/ stats/
435f577
to
ff2b0a1
Compare
@easwars I have fixed the issue and it should be good to go now. 🚀 |
Fixes: #1878