Add a proxy to allow tests written to the generated RegistryServer interface to call remote servers #640
Conversation
Codecov Report
@@ Coverage Diff @@
## main #640 +/- ##
=======================================
Coverage 53.91% 53.91%
=======================================
Files 77 77
Lines 7222 7222
=======================================
Hits 3894 3894
Misses 2914 2914
Partials 414 414 Continue to review full report at Codecov.
|
| ) | ||
|
|
||
| // Proxy implements a local proxy for a remote Registry server. | ||
| // This allows tests written to the generated "RegistryServer" and "AdminServer" interfaces to be run against remote servers. |
There was a problem hiding this comment.
Is this just for testing? It almost seems like it could be general-purpose (aside from the Open() function behavior). If this is just intended for supporting tests, we should probably place it in a package under the "tests" folder or some other similar "test" oriented folder.
There was a problem hiding this comment.
I don't object to moving it - I think it would be useful for supporting external usage of the seeder package. Perhaps we could put it alongside that, in server/registry/test.
There was a problem hiding this comment.
I just moved the remote proxy into server/registry/test/remote
| } | ||
|
|
||
| // Delete everything in the remote server to be tested. | ||
| it := p.adminClient.ListProjects(ctx, &rpc.ListProjectsRequest{}) |
There was a problem hiding this comment.
Deleting everything in the remote server would be an astonishing side-effect of an Open() function (at least to me). We should make the delete an explicit call or perhaps change the name of the function to indicate this behavior as well as add a comment to that effect. (All public functions should have comments anyway.)
There was a problem hiding this comment.
For sure, yes, that's putting it politely. In addition to warning people about this, I wonder if we could limit its blast radius by adopting some testing conventions -- if all projects used in testing matched a certain pattern (e.g. ".*test.*"), then we could only clear those projects and not others, though this might still break a few tests that enumerate all the projects in a running server.
| ) | ||
|
|
||
| // DeleteApiTask deletes a specified API. | ||
| type DeleteApiTask struct { |
There was a problem hiding this comment.
Should these Tasks be just wipeout-related? Or would such Tasks (and processing of Tasks) be an interesting abstraction to make available in general?
There was a problem hiding this comment.
@seaneganx suggested general tasks in this comment and a preceeding issue. I'm open to that, which might mean moving these elsewhere.
| @@ -1,74 +0,0 @@ | |||
| // Copyright 2022 Google LLC. All Rights Reserved. | |||
There was a problem hiding this comment.
I'm proposing deleting this test because it's arguably redundant (as @seaneganx pointed out) and doesn't work well with the TestServer interface-based approach used above. If we revive it, I think it should be in the storage package.
pkg/wipeout/wipeout_test.go
Outdated
| Project: &rpc.Project{}, | ||
| }); err != nil { | ||
| t.Fatalf("Setup: Failed to create test project: %s", err) | ||
| seed := []*rpc.Artifact{ |
There was a problem hiding this comment.
I'm leaning towards writing some loops to make this more compact.
There was a problem hiding this comment.
Done. I could go either way with this.
|
Subsumed by #664 |
|
#664 was merged and includes everything here. |
Everything prior to this commit is from #639.
This addresses #611 by adding a proxy that makes it possible to run the server tests against any remote server that includes the admin service. This allows testing of arbitrary configurations and builds that include archived containers. The proxy uses the default client configuration (currently
APG_REGISTRY_ADDRESS, etc), and is used when the-remoteflag is passed togo test ./servers/registry, e.g.