test: Replace WithDialer usages with WithContextDialer#8788
test: Replace WithDialer usages with WithContextDialer#8788twz123 wants to merge 2 commits intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8788 +/- ##
==========================================
- Coverage 83.48% 83.28% -0.21%
==========================================
Files 414 414
Lines 32720 32723 +3
==========================================
- Hits 27315 27252 -63
- Misses 4025 4069 +44
- Partials 1380 1402 +22
🚀 New features to boost your workflow:
|
twz123
force-pushed
the
test-withcontextdialer
branch
from
debddcb to
e5d0794
Compare
| dl := pl.Dialer() | ||
| conn, err := dl("", time.Duration(0)) | ||
| dl := pl.ContextDialer() | ||
| conn, err := dl(t.Context(), "") |
There was a problem hiding this comment.
We do not use t.Context() rather we do something like this :
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
where defaultTestTimeout = 10 * time.Second
internal/testutils/pipe_listener.go
Outdated
| case <-p.done: | ||
| return nil, errClosed | ||
| case <-ctx.Done(): | ||
| return nil, context.Cause(ctx) |
There was a problem hiding this comment.
Can you change this to return ctx.Err()
twz123
force-pushed
the
test-withcontextdialer
branch
from
e5d0794 to
7965ceb
Compare
| opts, scheme := te.configDial() | ||
| dw := &dialerWrapper{} | ||
| // overwrite the dialer before | ||
| opts = append(opts, grpc.WithDialer(dw.dialer)) |
There was a problem hiding this comment.
I think we should keep some tests that also check the behaviour of grpc.WithDialer since we say it will be supported throughout 1.x
There was a problem hiding this comment.
Hmm, we could add a bool to the test struct, so that tests can select which implementation to use, and then pick some tests to explicitly use the old behavior. However, I'm a bit concerned that this might clash with #8666, which actually prompted me to change the e2e tests to respect the context. Without it, there will probably be test failures, because some goroutines will hang around for 10 secs when closing the connection. But I'd need to verify that, to be sure.
Looking at the implementation of grpc.WithDialer, it simply delegates to grpc.WithContextDialer. Maybe adding a unit test that asserts that WithDialer properly forwards to WithContextDialer might already be sufficient?
There was a problem hiding this comment.
Regardless of the decision regarding the e2e tests, I figured a simple unit test would be useful nonetheless, so I added one in 6398f6e.
test/end2end_test.go
Outdated
|
|
||
| func (te *test) withServerTester(fn func(st *serverTester)) { | ||
| c, err := te.e.dialer(te.srvAddr, 10*time.Second) | ||
| ctx, cancel := context.WithTimeout(te.ctx, 10*time.Second) |
There was a problem hiding this comment.
please use defaultTestTimeout instead of 10*time.Second
This will properly pass on cancellation requests, and reduce usage of a deprecated method. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
twz123
force-pushed
the
test-withcontextdialer
branch
from
7965ceb to
0471bbf
Compare
After replacing all usages of grpc.WithDialer in the tests, there was no longer any test coverage for it. Since WithContextDialer is used behind the scenes and has proper coverage, we just need to ensure that the timeouts passed to the callback function are derived properly from the connect deadline. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
5 participants
This will properly pass on cancellation requests, and reduce usage of a deprecated method.
See:
RELEASE NOTES: none