xds: Refactor/cleanup xds client tests. #3920
Conversation
|
@dfawley |
xds/internal/client/client_test.go
Outdated
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
It would be preferable to propagate context instead.
| clusterUpdateCh.Send(clusterUpdateErr{u: u, err: err}) | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| ctx, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) |
There was a problem hiding this comment.
Can we keep using the same context throughout the test? Maybe it needs to be higher to avoid flakes, and consequently takes a few seconds longer when there is a failure, but it removes clutter and we aren't trying to enforce specific time bounds on these operations.
There was a problem hiding this comment.
Bumped up the defaultTestTimeout to 5s and changed the tests to be using that all the time instead of creating new contexts for every call that needs a context. I couldn't avoid having to create a new one for the calls which are expected to fail though.
xds/internal/client/client_test.go
Outdated
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() |
| clusterUpdateCh.Send(clusterUpdateErr{u: u, err: err}) | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| ctx, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) |
There was a problem hiding this comment.
Bumped up the defaultTestTimeout to 5s and changed the tests to be using that all the time instead of creating new contexts for every call that needs a context. I couldn't avoid having to create a new one for the calls which are expected to fail though.
| ctx, cancel = context.WithTimeout(context.Background(), defaultTestShortTimeout) | ||
| defer cancel() | ||
| if u, err := clusterUpdateCh.Receive(ctx); err != context.DeadlineExceeded { | ||
| sCtx, sCancel := context.WithTimeout(context.Background(), defaultTestShortTimeout) |
There was a problem hiding this comment.
FWIW these could also derive from ctx so they persist the overall test timeout. But since this is so short it doesn't actually matter.
TestValidateClusterinto failure and success cases. I will adding more cases here as part of other PR which adds support to process security configuration at thexdsClient.testutils.Channelto push the fake API client. This allows tests to block on this with a deadline. This addresses a TODO in the code.verify*Updatefunctions to test successfully received updates.xds.Clientobject instead of going through thefakeAPIClient. This addresses a TODO in the code.c->client,v2Client->apiClient,v2ClientCh->apiClientChgo test google.golang.org/grpc/xds/internal/client/now takes~5s. Used to take~30s.#improve-xds-tests