xds: move e2e tests into grpc/test/xds directory #5363

easwars · 2022-05-17T23:00:42Z

This change helps to ensure that we don't inadvertently use some xDS features we are not supposed to use in a e2e test.

RELEASE NOTES: none

dfawley · 2022-05-18T16:48:04Z

xds/xds.go

@@ -74,22 +77,69 @@ func init() {
 		v3statusgrpc.RegisterClientStatusDiscoveryServiceServer(grpcServer, csdss)
 		return csdss.Close, nil
 	})
+
+	internal.NewXDSResolverWithConfigForTesting = func(bootstrapConfig []byte) (resolver.Builder, error) {


Move this to the xdsresolver package? And can it be just:

internal.NewXDSResolverWithConfigForTesting = xdsresolver.NewBuilderForTesting

dfawley · 2022-05-18T16:50:27Z

internal/testutils/xds/e2e/setup_certs.go

@@ -0,0 +1,98 @@
+/*
+ *
+ * Copyright 2020 gRPC authors.


😢 why didn't it detect these as moves? Is it possible to fix?

I initially did this as a git mv. But then realized that we had some commonly used helper functions like this which were defined in some _test.go files (probably the file where it was first defined). I thought it would be better to move these out into separate files which only have related helpers.

Ah, ok.. I think in that case, since it's a brand new file, the (C) date should be 2022.

dfawley · 2022-05-18T16:51:44Z

test/xds/xds_client_affinity_test.go

@@ -1,6 +1,3 @@
-//go:build !386


Why was this here before?

I can't remember exactly. But the one issue I remember is with go-control-plane, where they had an int64 in a struct which was accessed atomically, but wasn't aligned on 64-bit boundaries. I fixed it, but we had to wait a while for a release. So, I think we started adding these and over time, I'm guessing newer files just mimicked the existing ones.

dfawley · 2022-05-18T16:55:46Z

xds/xds.go

 //
 // Experimental
 //
-// Notice: This API is EXPERIMENTAL and may be changed or removed in a
-// later release.
-func NewXDSResolverWithConfigForTesting(bootstrapConfig []byte) (resolver.Builder, error) {


Why was this moved to internal but these other, temporary, RLS/RBAC testing functions were added here?

This function is used from a couple of places: internal/testutils/xds/e2e and test/xds while the other temporary register/unregister funcs are only used from test/xds. There are still some tests in xds/internal/ which use the internal/testutils/xds/e2e package. So, there were some import loops. For example: top-level xds package --> xds/csds --> internal/testutils/xds/e2e --> top-level-xds-package (for the NewXDSResolverWithConfigForTesting func).

I will try to move those tests out after this PR.

Couldn't these also live in internal, though, just like the new NewXDSResolverWithConfigForTesting? That should avoid import loops, too, right?

The import loop was only with NewXDSResolverWithConfigForTesting, which is why I moved it to the internal package. I felt like moving the other ones to the internal package would add too many symbols to it. Please let me know if you feel strongly about this. Thanks.

IMO it's better for them to be in internal than visible in our external godoc. I should have originally done NewXDSResolver... that way in hindsight. I imagine it shouldn't be that hard to move these, but if it is, then you can leave them because they're also expected to be temporary.

Also, would this work??

package internal var NewXDSResolverBlah interface{} // as always ..... package internal/xds func NewXDSResolverBlah(args) returns { // optionally handle `nil`?? Or can we actually import the thing that sets it from here?? Seems plausible. return internal.NewXDSResolverBlah(func(args)returns)(args) }

That way we don't have to type cast the functions whenever we use them. This could be a follow-up cleanup, and we could even do something similar with all the other stuff in internal that needs to be type cast every time we use it.

Or can we actually import the thing that sets it from here??

I guess in the xds cases, "no", because of the visibility problem. But in the other cases (e.g. ParseServiceConfig) then I think we could.

Moved everything to internal package. Thanks.

easwars

Thanks for the review.

easwars · 2022-05-18T18:32:05Z

xds/xds.go

@@ -74,22 +77,69 @@ func init() {
 		v3statusgrpc.RegisterClientStatusDiscoveryServiceServer(grpcServer, csdss)
 		return csdss.Close, nil
 	})
+
+	internal.NewXDSResolverWithConfigForTesting = func(bootstrapConfig []byte) (resolver.Builder, error) {


easwars · 2022-05-18T18:34:17Z

test/xds/xds_client_affinity_test.go

@@ -1,6 +1,3 @@
-//go:build !386


I can't remember exactly. But the one issue I remember is with go-control-plane, where they had an int64 in a struct which was accessed atomically, but wasn't aligned on 64-bit boundaries. I fixed it, but we had to wait a while for a release. So, I think we started adding these and over time, I'm guessing newer files just mimicked the existing ones.

easwars · 2022-05-18T18:37:55Z

internal/testutils/xds/e2e/setup_certs.go

@@ -0,0 +1,98 @@
+/*
+ *
+ * Copyright 2020 gRPC authors.


I initially did this as a git mv. But then realized that we had some commonly used helper functions like this which were defined in some _test.go files (probably the file where it was first defined). I thought it would be better to move these out into separate files which only have related helpers.

easwars · 2022-05-18T18:46:06Z

xds/xds.go

 //
 // Experimental
 //
-// Notice: This API is EXPERIMENTAL and may be changed or removed in a
-// later release.
-func NewXDSResolverWithConfigForTesting(bootstrapConfig []byte) (resolver.Builder, error) {


This function is used from a couple of places: internal/testutils/xds/e2e and test/xds while the other temporary register/unregister funcs are only used from test/xds. There are still some tests in xds/internal/ which use the internal/testutils/xds/e2e package. So, there were some import loops. For example: top-level xds package --> xds/csds --> internal/testutils/xds/e2e --> top-level-xds-package (for the NewXDSResolverWithConfigForTesting func).

I will try to move those tests out after this PR.

xds: move e2e tests into grpc/test/xds directory

9b35024

easwars marked this pull request as draft May 17, 2022 23:01

easwars added the Type: Testing label May 17, 2022

easwars added this to the 1.47 Release milestone May 17, 2022

easwars marked this pull request as ready for review May 17, 2022 23:13

dfawley self-assigned this May 18, 2022

dfawley self-requested a review May 18, 2022 16:44

dfawley reviewed May 18, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley May 18, 2022

set internal.NewXDSResolverWithConfigForTesting from xdsresolver package

56cf2cf

easwars commented May 18, 2022

View reviewed changes

easwars assigned dfawley and unassigned easwars May 18, 2022

dfawley assigned easwars and unassigned dfawley May 18, 2022

update year in copyright notice

9437eae

easwars assigned dfawley and unassigned easwars May 18, 2022

move all register/unregister testing funcs to internal package

8eed9c2

easwars assigned easwars and dfawley and unassigned dfawley and easwars May 19, 2022

zasweq modified the milestones: 1.47 Release, 1.48 Release May 23, 2022

dfawley approved these changes May 24, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley May 24, 2022

easwars merged commit c0e3573 into grpc:master May 24, 2022

easwars deleted the xds_e2e_tests_move branch May 24, 2022 18:58

github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: move e2e tests into grpc/test/xds directory #5363

xds: move e2e tests into grpc/test/xds directory #5363

easwars commented May 17, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 18, 2022

easwars May 18, 2022

dfawley May 19, 2022

dfawley May 19, 2022

easwars May 19, 2022

easwars left a comment

easwars May 18, 2022

easwars May 18, 2022

easwars May 18, 2022

easwars May 18, 2022

xds: move e2e tests into grpc/test/xds directory #5363

xds: move e2e tests into grpc/test/xds directory #5363

Conversation

easwars commented May 17, 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

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 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