xds/resolver: move service watching tests to resolver_test package #6682
Conversation
resolver/resolver.go
Outdated
| // String returns a string representation of Target. | ||
| func (t Target) String() string { | ||
| return t.URL.String() | ||
| } | ||
|
|
There was a problem hiding this comment.
These changes collide with the other pr you sent out, just a heads up. This and scheme.
There was a problem hiding this comment.
I've rebased now and cleanup things a bit. Will merge once GitHub Actions is happy.
| ) | ||
|
|
||
| func (s) TestFindBestMatchingVirtualHost(t *testing.T) { |
There was a problem hiding this comment.
This seems like a pretty clean and appropriately scoped unit test to delete. This helper function is still needed in the generic client api? Would prefer leaving this, and moving the tests that are e2e to separate package. I feel like some unit tests are appropriate.
There was a problem hiding this comment.
I moved these tests to the xdsresource package as part of another PR that you approved and I've already merged it. Actually, if and when I rebase this to master, this diff will disappear.
| // Verifies that no update is pushed on the fake resolver.ClientConn. Calls | ||
| // t.Fatal() if an update is received before defaultTestShortTimeout expires. | ||
| func verifyNoUpdateFromResolver(ctx context.Context, t *testing.T, tcc *testutils.ResolverClientConn) { | ||
| t.Helper() | ||
|
|
||
| sCtx, sCancel := context.WithTimeout(ctx, defaultTestShortTimeout) | ||
| defer sCancel() | ||
| select { | ||
| case <-sCtx.Done(): | ||
| case u := <-tcc.StateCh: | ||
| t.Fatalf("Received update from resolver %v when none expected", u) |
There was a problem hiding this comment.
Will need to rework this to callback style API.
There was a problem hiding this comment.
I copied over the update I made on the ResolverClientConn to switch it to use callbacks, and I've made changes to the call sites here.
| case <-ctx.Done(): | ||
| t.Fatalf("Timeout waiting for an update from the resolver: %v", ctx.Err()) | ||
| case state = <-tcc.StateCh: | ||
| if err := state.ServiceConfig.Err; err != nil { | ||
| t.Fatalf("Received error in service config: %v", state.ServiceConfig.Err) | ||
| } | ||
| if wantSC == "" { | ||
| break | ||
| } | ||
| wantSCParsed := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(wantSC) | ||
| if !internal.EqualServiceConfigForTesting(state.ServiceConfig.Config, wantSCParsed.Config) { | ||
| t.Fatalf("Got service config:\n%s \nWant service config:\n%s", cmp.Diff(nil, state.ServiceConfig.Config), cmp.Diff(nil, wantSCParsed.Config)) | ||
| } |
There was a problem hiding this comment.
Same here (also part of your other PR)
| // - A reference to the xDS management server | ||
| // - A reference to the test resolver.ClientConn to verify service config pushed | ||
| // on the channel | ||
| // - A channel to read requested route configuration resource names from |
There was a problem hiding this comment.
Nit: the above bullets have periods haha
There was a problem hiding this comment.
Do you really want me to fix that? This is not going to be part of a godoc.
| } | ||
| t.Cleanup(mgmtServer.Stop) |
There was a problem hiding this comment.
Why t.Cleanup over defer? Just curious.
There was a problem hiding this comment.
t.Cleanup is better in helpers for a number of reasons:
- the helper does not have to return a cancel func that the callsites have to
defer - if the helper does things multiple things that can fail, it can be tricky to correctly cleanup when one of those things fail and an appropriate cancel func has to be returned to the caller. With
t.Cleanupthough, things are very simple. The helper can simply add things to be run at the end of the test and callt.Fatalwhen something fails.
Also, t.Cleanup gets run at the end of the test and defer runs at the end of the function. So, you really can't defer cleanup things from a helper, because you really want that cleanup to happen at the end of the test and not at the end of the helper.
| serviceUpdateCh.Send(serviceUpdateErr{u: update, err: err}) | ||
| }, nil) | ||
| defer cancelWatch() | ||
| // Waits for the wantNames to be pushed on to namesCh. Fails the test by calling |
There was a problem hiding this comment.
Optional: waitForResourceNames waits
There was a problem hiding this comment.
In general, it is considered good practice to follow the function level comment style used for exported functions for unexported functions as well. But it makes things easy when you might decide to export it. But here, there is not a very good chance of exporting this. But anyways, changed it.
| // the new resource, and once successfully resolved, sends an update on the | ||
| // channel. |
There was a problem hiding this comment.
s "sends an update on the channel" to "verifies receives update from channel" although this is callback based now.
| // Tests the case where the listener resource starts pointing to a new route | ||
| // configuration resource after the xDS resolver has successfully resolved the |
There was a problem hiding this comment.
Mention old system (i.e. listener resource was already points to a route configuration, and then gets a new update pointing to a new one). And optionally put test name at beginning of comment.
There was a problem hiding this comment.
Mention old system
That information is conveyed by this:
after the xDS resolver has successfully resolved the service name and pushed an update on the channel
And optionally put test name at beginning of comment
I used to do that earlier, but after a comment from Doug, I've switched to this approach of starting the comment with "Tests .....".
| // Tests the case where the listener resource changes to contain an inline route | ||
| // configuration and changes back to having a route configuration resource name. | ||
| // Verifies that the expected xDS resource names are requested by the resolver | ||
| // and the update pushed to the channel contains the expected service config. |
There was a problem hiding this comment.
Change this comment to callback based API is how verification happens mechanically.
easwars
force-pushed
the
xds_resolver_cleanup_3
branch
from
5d56708 to
f5c1545
Compare
easwars
force-pushed
the
xds_resolver_cleanup_3
branch
from
f5c1545 to
4cdc587
Compare
The
testutils.ResolverClientConnfound ininternal/testutils/resolver.gois part of an in-progress PR: #6668. So, you can skip reviewing it here unless we get done here first before the other PR.Summary of changes:
Moves the tests in
xds/internal/resolver/watch_service_test.gotoresolver_testpackage, thereby making them not rely on any internal detailsAlso removes the following two tests:
TestServiceWatchLDSUpdateMaxStreamDuration: This will already tested in one of the config selector tests, and the same test will be enhanced to also verify the service config pushed to the channelTestServiceNotCancelRDSOnSameLDSUpdateAdds some helper functions (more to come in other PRs) that are used to make tests shorter and will be used in other tests as well
Export scheme from the
resolverpackageAdd a
String()method on theresolver.Targettype for better logging output#resource-agnostic-xdsclient-api
RELEASE NOTES: none