xds/internal/resolver: final bit of test cleanup #6725
Conversation
easwars
force-pushed
the
xds_resolver_cleanup_4
branch
from
9d9646b to
4fdb846
Compare
| c, cancel, err := xdsclient.NewWithConfigForTesting(bootstrapCfg, defaultTestTimeout, defaultTestTimeout) | ||
| return c, func() { | ||
| return c, grpcsync.OnceFunc(func() { |
There was a problem hiding this comment.
Curious, wrapping it with a once func doesn't change this test technically right? The verification on line 327 just requires one read.
There was a problem hiding this comment.
The cancel func returned by xdsclient.NewWithConfigForTesting is wrapped inside of a grpcsync.OnceFunc. So calling it any number of times is safe. But that is not the case with the cancel func that we are returning here.
And this becomes a real issue because the helper buildResolverForTarget calls t.Cleanup(r.Close()) and this makes sense for most other tests which don't have to then manually call defer r.Close(). But here we have an explicit call to r.Close(). So, this means that we end up with two calls to r.Close(), which both end up calling xdsClientClose which is safe in the real code because of what I mentioned at the top of this comment.
|
|
||
| target := resolver.Target{URL: *testutils.MustParseURL("xds:///target")} | ||
| if _, err := builder.Build(target, nil, resolver.BuildOptions{}); err == nil { | ||
| t.Fatalf("xds Resolver Build(%v) succeeded when expected to fail, because there is not bootstrap configuration for the xDS client", target) |
There was a problem hiding this comment.
Whoops there's actualy a lot of Build(....) comments in this file. I'm fine with either one actually, I don't think we need to add it to all.
There was a problem hiding this comment.
Got to a couple more.
Summary of changes:
resolver_testpackageTestXDSResolverHTTPFiltersinto twopathsinstead of using a channelxds/internal/resolverand move things to be overridden in tests to that package#resource-agnostic-xdsclient-api
RELEASE NOTES: none