resolver/delegatingresolver: wait for proxy resolver build before update in tests #8304
Conversation
eshitachandwani added Type: Bug Area: Resolvers/Balancers 
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## master #8304 +/- ## ========================================== + Coverage 82.22% 82.24% +0.01% ========================================== Files 419 419 Lines 41954 41988 +34 ========================================== + Hits 34497 34531 +34 + Misses 5995 5993 -2 - Partials 1462 1464 +2 🚀 New features to boost your workflow:
|
| proxyResolverBuilt := make(chan struct{}) | ||
| proxyResolver.BuildCallback = func(resolver.Target, resolver.ClientConn, resolver.BuildOptions) { | ||
| close(proxyResolverBuilt) | ||
| } |
There was a problem hiding this comment.
Would it make more sense to set up this BuildCallback in the setupDNS function and have it return two values: (*manual.Resolver, chan struct{}). And then we can have the test wait on the second return value the same way you are currently doing in lines 208-215 in this test.
| // Wait for the proxy resolver to be built before calling UpdateState. | ||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
| select { | ||
| case <-proxyResolverBuilt: | ||
| case <-ctx.Done(): | ||
| t.Fatalf("Context timed out waiting for proxy resolver to be built.") | ||
| } |
There was a problem hiding this comment.
Another possible refactoring would be make this a function for this code block which is repeated in a lot of tests. Something like func mustBuildResolverBuild(ctx context.Context, t *testing.T, buildCh chan struct{}) { ... }.
See: go/go-style/decisions#must-functions
3 participants
Since the proxy resolver is now initialized in a goroutine triggered by the target resolver's update, we need to ensure that it is fully constructed before allowing it to send updates in tests. This mirrors the real-world behavior, where the proxy resolver cannot send updates unless it has been built. In tests, if we don't wait for this initialization, it may lead to a panic.
RELEASE NOTES: N/A