xdsclient: cleanup listener watchers test #5506

easwars · 2022-07-14T23:26:23Z

Current tests use way too much magic and rely on a lot of internal state. These were written prior to us having a go-control-plane management server.

This change tests exactly all the functionality that was being tested earlier, but uses a real management server and relies on actual events happening instead of internal state.

Once this change is merged, I will send PRs to switch the other watcher tests as well. Then, we will be able to get rid of some complicated test setup logic.

RELEASE NOTES: none

easwars · 2022-07-14T23:43:38Z

I'd appreciate if @dfawley can also take a quick look at this. But I really want @zasweq to take a good look. I felt that the existing tests were very hard to read, over-specified, relying too much on internal state etc. So, I want to make sure the new ones are easier to read for someone who is not super familiar with them.

easwars · 2022-07-21T22:51:07Z

Gentle ping ...

zasweq

Looks good, thanks for doing this, much cleaner. The old tests were very hard to understand. Minor nits throughout.

zasweq · 2022-07-21T23:40:30Z

xds/internal/xdsclient/watchers.go

@@ -28,6 +28,7 @@ import (
 // after the watcher is canceled. The caller needs to handle this case.
 func (c *clientImpl) WatchListener(serviceName string, cb func(xdsresource.ListenerUpdate, error)) (cancel func()) {
 	n := xdsresource.ParseName(serviceName)
+	logger.Infof("easwars: parsed name is %+V, %s", n, n)


Oops .. sorry

zasweq · 2022-07-22T16:48:06Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// TestLDSWatch_ThreeWatchesForDifferentResourceNames covers the case where
+// three watchers exist across two listener resources. The following sequence of
+// events happen:
+// 1. Three watches are registered for two listener resources


"resources: two watches for one, one watch for the other"

zasweq · 2022-07-22T16:53:49Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// happen:
+// 1. Two watches are registered for two listener resources
+// 2. Management server sends an update containing both resources.
+// 3. Management server sends an update removing one of the above resources


"removing one of the above resources, should trigger callback with resource removed error". To keep it consistent with Menghan's comment

Reworded the comment with this suggestion.

zasweq · 2022-07-22T16:54:39Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// 1. Two watches are registered for two listener resources
+// 2. Management server sends an update containing both resources.
+// 3. Management server sends an update removing one of the above resources
+// 4. Management server sends an update which modifies the exisiting resource.


"resource. The second watch shouldn't receive any update."

Reworded the comment with this suggestion.

zasweq · 2022-07-22T16:55:02Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// TestLDSWatch_ResourceRemoved covers the cases where a resource being watched
+// is removed from the management server. The following sequence of events
+// happen:
+// 1. Two watches are registered for two listener resources


Nit: Some of your events have periods after, some don't. I'd prefer periods. Here and other parts of the test.

Removed lists altogether. So, no more periods or the lack of them.

zasweq · 2022-07-22T22:03:01Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+
+	// Register the third watch for a different listener resource.
+	updateCh3 := testutils.NewChannel()
+	ldsCancel3 := client.WatchListener(fmt.Sprintf("xdstp:///envoy.config.listener.v3.Listener/%s", ldsName), func(u xdsresource.ListenerUpdate, err error) {


Optional: Make this fmt.Sprintf(expression) a var since used twice.

Actually, this is used 10 times. Package level const like ldsName/rdsName?

zasweq · 2022-07-22T22:04:43Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	defaultTestShortTimeout       = 10 * time.Millisecond // For events expected to *not* happen.
+)
+
+var cmpOpts = []cmp.Option{


Is there a style reason this variable only used once is declared here rather than in the function it's used in?

When I first added this, I though it would be used from multiple places. Didn't realize that it was being used only from one place. Moved it to that function.

zasweq · 2022-07-22T22:08:18Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	defer client.Close()
+
+	const (
+		ldsName = "xdsclient-test-lds-resource"


Nit: These two consts are declared 7 times, taking up 14 lines. Make this a package scoped const?

zasweq · 2022-07-22T22:15:18Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	}
+
+	// Update the second listener resource on the management server. The first
+	// watcher should not see an update, while the second wather should.


zasweq · 2022-07-22T22:18:49Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+		t.Fatalf("update received with error: %v, want %q", gotErr, wantNACKErr)
+	}
+
+	// Verify that the watcher requesting for the good resource receives a good update.


This comment wraps past 180. Perhaps reword to "Verify that the watcher watching the good resource". "Requesting for the good resource" feels off.

easwars · 2022-07-25T21:15:43Z

Thanks for review @zasweq. Addressed all comments.

zasweq

LGTM outside of very minor comment nits. Code looks good.

zasweq · 2022-07-26T17:11:20Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// TestLDSWatch covers the case where a single watcher exists for a single
+// listener resource. The test verifies the following scenarios:
+// 1. An update from the management server containing the resource being
+//    watched, results in the invocation of the watch callback


Super nit: s results/should result, should not result for 2 and 3. Also periods after?

Done. Here and in other comments. Thanks.

zasweq · 2022-07-26T17:13:50Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	tests := []struct {
+		desc                   string
+		resourceName           string
+		watchedResource        *v3listenerpb.Listener // The resource being watched.


Yeah, it's fine, I feel mildly about it. Maybe will help future readers.

zasweq · 2022-07-26T17:14:59Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// exist for a single listener resource.  The test verifies the following
+// scenarios:
+// 1. An update from the management server containing the resource being
+//    watched, results in the invocation of both watch callbacks


Similar to above, s/results to should result and s/does not result/should not result and add periods.

zasweq · 2022-07-26T17:18:20Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+
+// TestLDSWatch_ResourceCaching covers the case where a watch is registered for
+// a resource which is already in the cache.  The test verifies that the new
+// watch is handled from the cache, without a request being sent to the


Super nit: handled from cache meaning? handle could mean a lot of things, including invoking watch callback with error. Can you switch this statement to receives update from resources present in cache or something.

Done. Thanks.

zasweq · 2022-07-26T17:21:19Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// is removed from the management server. The test verifies that removing
+// resources from the management server triggers the watch callback with
+// resource removed error. It also verifies that the watch callback for an
+// unrelated resource is not invoked during the above flow.


Optional: also talk about line 656 here.

zasweq · 2022-07-26T17:22:23Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	})
+	defer ldsCancel2()
+
+	// Configure the management server two listener resources. One of these is a


server with two

zasweq · 2022-07-26T17:23:42Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// TestLDSWatch_PartialValid covers the case where a response from the
+// management server contains both valid and invalid resources and is expected
+// to be NACK'ed by the xdsclient. The test verifies that watchers corresponding
+// to the valid resource (in the response) receive the update, while watchers


in the response is implied?

zasweq · 2022-07-26T17:30:47Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+
+	// Register two watches for two listener resources and have the
+	// callbacks push the received updates on to a channel.
+	resourceName1 := ldsName


Why are you aliasing here? I actually think it improves readability but wanted to hear an explanation from you.

Same. I thought the distinction between the resources (just the fact that they correspond to different resources) was more important here than whether the resource name is of the old style or the new style.

easwars · 2022-07-27T00:10:51Z

Thank you very much for the detailed review.

dfawley · 2022-07-27T16:12:56Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+	rdsNameNewStyle = "xdstp:///envoy.config.listener.v3.Listener/xdsclient-test-rds-resource"
+)
+
+func newStringP(s string) *string {


Is this unused? Or used by other tests that weren't changed here?

Removed. Maybe it was used at some point in time.

dfawley · 2022-07-27T16:13:54Z

xds/internal/xdsclient/e2e_test/lds_watchers_test.go

+// A very short deadline is used while waiting for the update, as this function
+// is intended to be used when an update is not expected.
+func verifyNoListenerUpdate(ctx context.Context, updateCh *testutils.Channel) error {


In this case, why require a context parameter?

The incoming context will have the overall test deadline. And then we apply a shorter deadline here. This way, the callsite can have a single context and pass it to both verifyNoListenerUpdate and verifyListenerUpdate.

zasweq · 2022-07-27T16:36:02Z

LGTM.

easwars added the Type: Testing label Jul 14, 2022

easwars added this to the 1.49 Release milestone Jul 14, 2022

easwars requested review from dfawley and zasweq July 14, 2022 23:41

easwars assigned zasweq Jul 14, 2022

zasweq requested changes Jul 22, 2022

View reviewed changes

zasweq assigned easwars and dfawley and unassigned zasweq Jul 22, 2022

easwars assigned zasweq and unassigned easwars Jul 25, 2022

zasweq approved these changes Jul 26, 2022

View reviewed changes

zasweq assigned easwars and unassigned zasweq Jul 26, 2022

dfawley approved these changes Jul 27, 2022

View reviewed changes

easwars removed their assignment Jul 27, 2022

easwars added 5 commits July 27, 2022 11:06

xdsclient: cleanup listener watchers test

77580ff

review comments

a6dff43

review comments

4d0cb72

remove newStringP

4975011

pull in a grpc#5491 which got missed as part of the rebase

1d2733d

easwars force-pushed the xdsclient_simplify_watcher_tests branch from b2c81ec to 1d2733d Compare July 27, 2022 18:32

attempt to get rid of race

4b759f0

easwars merged commit fd4700c into grpc:master Jul 28, 2022

easwars deleted the xdsclient_simplify_watcher_tests branch July 28, 2022 17:06

github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2023

xdsclient: cleanup listener watchers test #5506

xdsclient: cleanup listener watchers test #5506

Conversation

easwars commented Jul 14, 2022 • edited Loading

easwars commented Jul 14, 2022

easwars commented Jul 21, 2022

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jul 25, 2022

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Jul 27, 2022

easwars commented Jul 14, 2022 •

edited

Loading

zasweq Jul 22, 2022 •

edited

Loading