serverutils: implement the testserver interfaces indirectly #109612
Conversation
knz
requested review from
DrewKimball,
ericharmeling,
jayshrivastava and
smg260
and removed request for
a team
|
This change is |
knz
force-pushed
the
20230828-test-interfaces
branch
from
b55dece to
fedab74
Compare
knz
changed the title
There was a problem hiding this comment.
Overall, I like that you've gone the extra mile to implement things like only printing the warning once per test server, an env var to enable a more verbose mode, and some facilities to test this test code.
I'm assuming we went with the code generation here to avoid having to maintain implementations of every interface function on the wrap struct manually.
If we get complaints about the noise of this, I think we could reconsider the warning on StorageLayerInterface since I don't think there is ambiguity there.
I left some very minor comments that I came across while reading through this. I think my main concern while reading is that the code seemed a little complex looking for what it does. But, each bit of complexity does come with a reason.
| @@ -242,6 +236,10 @@ func NewServer(params base.TestServerArgs) (TestServerInterface, error) { | |||
| return nil, errors.AssertionFailedf("TestServerFactory not initialized. One needs to be injected " + | |||
| "from the package's TestMain()") | |||
| } | |||
| tcfg := params.DefaultTestTenant | |||
| if !(tcfg.TestTenantAlwaysEnabled() || tcfg.TestTenantAlwaysDisabled()) { | |||
| return nil, errors.AssertionFailedf("programming error: DefaultTestTenant does not contain a decision\n(maybe call ShouldStartDefaultTestTennat?)") | |||
There was a problem hiding this comment.
s/ShouldStartDefaultTestTennat/ShouldStartDefaultTestTenant/
| func (w *wrap) fwTestServerController(m string) TestServerController { return w.ts } | ||
|
|
||
| func (w *wrap) fwApplicationLayerInterface(m string) ApplicationLayerInterface { | ||
| w.implicitAppLayerNotify(m) | ||
| return w.implicitAppLayer() | ||
| } | ||
|
|
||
| func (w *wrap) fwStorageLayerInterface(m string) StorageLayerInterface { | ||
| w.implicitStorageLayerNotify(m) | ||
| return w.implicitStorageLayer | ||
| } | ||
|
|
||
| func (w *wrap) fwTenantControlInterface(m string) TenantControlInterface { | ||
| w.implicitTenantControlNotify(m) | ||
| return w.implicitTenantControl | ||
| } |
There was a problem hiding this comment.
I think it would be worth a comment clarifying that these are called by the generated forwarding code.
| @@ -0,0 +1,193 @@ | |||
| package main | |||
There was a problem hiding this comment.
For generators like this, I think it is often useful to put a documentation comment that shows how to use it and a small example of the code it attempts to generate.
| if outy[0] >= 'a' && outy[0] <= 'z' { | ||
| cons = "new" + string(outy[0]-'a'+'A') + outy[1:] | ||
| } |
There was a problem hiding this comment.
:D This "trick" always makes me smile.
| } | ||
| } | ||
|
|
||
| // makeBenignNotifyFn constructs a function that strongly recommends |
There was a problem hiding this comment.
s/makeBenignNotifyFn/makeSeriousNotifyFn/
| loggerFn: func(format string, args ...interface{}) { fmt.Printf(format, args...) }, | ||
|
|
||
| // Used to implement the implicit TestServerController interface, | ||
| // and the explicit .StorageLayer() and .TenantControl() accessors. |
There was a problem hiding this comment.
s/TenantControl/TenantController/
| } | ||
| id := name.Name | ||
| if id == "_" { | ||
| id = fmt.Sprintf("arg%d", argNum) |
There was a problem hiding this comment.
Maybe prepend and _ to continue to signal that this is unused.
| } | ||
|
|
||
| p("// %s is part of the interface %s.\n", m.Names[0].Name, itname) | ||
| p("func (f *%s) %s(", outy, m.Names[0].Name) |
There was a problem hiding this comment.
I think we are safe for now, but I imagine this breaks from generic functions.
knz
force-pushed
the
20230828-test-interfaces
branch
from
fedab74 to
4936fb7
Compare
There was a problem hiding this comment.
I'm assuming we went with the code generation here to avoid having to maintain implementations of every interface function on the wrap struct manually.
Correct.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @samiskin, and @stevendanna)
pkg/testutils/serverutils/conditional_wrap.go line 55 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
s/TenantControl/TenantController/
Done.
pkg/testutils/serverutils/conditional_wrap.go line 168 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
I think it would be worth a comment clarifying that these are called by the generated forwarding code.
Done.
pkg/testutils/serverutils/conditional_wrap.go line 198 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
s/makeBenignNotifyFn/makeSeriousNotifyFn/
Done.
pkg/testutils/serverutils/conditional_wrap.go line 202 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
We could mention that we require a function pointer here because of the late binding of the log function.
Done.
pkg/testutils/serverutils/test_server_shim.go line 241 at r4 (raw file):
Previously, stevendanna (Steven Danna) wrote…
s/ShouldStartDefaultTestTennat/ShouldStartDefaultTestTenant/
Done.
pkg/testutils/serverutils/fwgen/gen.go line 1 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
For generators like this, I think it is often useful to put a documentation comment that shows how to use it and a small example of the code it attempts to generate.
Done.
pkg/testutils/serverutils/fwgen/gen.go line 123 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
I think we are safe for now, but I imagine this breaks from generic functions.
IMHO we're a long way from this from a go language definition perspective. Added a disclaimer in docstring.
pkg/testutils/serverutils/fwgen/gen.go line 143 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Maybe prepend and
_to continue to signal that this is unused.
discussed - all args are used.
knz
force-pushed
the
20230828-test-interfaces
branch
from
4936fb7 to
b3ae828
Compare
knz
force-pushed
the
20230828-test-interfaces
branch
from
b3ae828 to
9c046f2
Compare
|
TFYRs! bors r=stevendanna |
|
Build failed: |
knz
force-pushed
the
20230828-test-interfaces
branch
from
9c046f2 to
365db1f
Compare
|
bors r=stevendanna |
|
Build failed: |
This commit simplifies as follows: - it removes use of .SystemLayer() and .ApplicationLayer() from within the testServer code. This is ahead of a smarter implementation of these accessors in a later commit. - it renames "Stopper" to "AppStopper" in "ApplicationLayerInterface", to disambiguate it from the stopper at the top level testServer. Release note: None
This repairs the printing of the informational message printed when a unit test (or test package) opts out of cluster virtualization via `TestingSetDefaultTenantSelectionOverride`. Release note: None
This will prevent warnings in a subsequent commit. Release note: None
Prior to this patch, the assertion on the value of DefaultTestTenant was inside `ShouldStartDefaultTestTenant()`. However, this function is only ever called from `StartServer`; it is not called from `NewServer()`. This means that tests that call `NewServer` directly do not get the benefit of the validity check. This patch moves the assertion to the better place. Release note: None
knz
force-pushed
the
20230828-test-interfaces
branch
from
365db1f to
30b176b
Compare
TLDR: unit tests using TestServer/TestCluster now produce warning and
notice messages upon certain suspicious API usage.
For example:
```
$ dev test //pkg/server/storage_api -f TestStatusGetFiles
=== RUN TestStatusGetFiles
...
pkg/server/storage_api/storage_api_test_test/pkg/server/storage_api/files_test.go:46: (TestStatusGetFiles)
NOTICE: .GetStatusClient() called via implicit interface ApplicationLayerInterface;
HINT: consider using .ApplicationLayer().GetStatusClient() instead.
conditional_wrap.go:193: TIP: consider replacing the test server initialization from:
ts, db, kvDB := serverutils.StartServer(t, ...)
// or:
tc := serverutils.StartCluster(t, ...)
ts := tc.Server(0)
To:
srv, db, kvDB := serverutils.StartServer(t, ...)
defer srv.Stop(...)
ts := srv.ApplicationLayer()
// or:
tc := serverutils.StartCluster(t, ...)
ts := tc.Server(0).ApplicationLayer()
```
The following warnings/notices are implemented.
**ApplicationLayerInterface**
The problem here is that the implicit implementation of
`ApplicationLayerInterface` inside `TestServerInterface` is likely
misused. If/when the test server automatically starts a secondary
tenant, the *implicit* `ApplicationLayerInterface` refers to the
system tenant, whereas only `.ApplicationLayer()` refers to the
secondary tenant. It's likely that the test code should use the
latter.
For this, a warning is printed when a method from the *implicit*
`ApplicationLayerInterface` is called.
The warning is not printed if the test has indicated (via
`DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant`)
that it is ever only interested in the storage layer.
**StorageLayerInterface**
We want to promote the use of the explicit interface accessor
`.StorageLayer()` to access the methods of `StorageLayerInterface`.
For this, a notice is printed when a method from the *implicit*
`StorageLayerInterface` is called.
The notice is not printed if the test has indicated (via
`DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant`)
that it is ever only interested in the storage layer.
**TenantControlInterface**
We want to promote the use of the explicit interface accessor
`.TenantController()` to access the methods of
`TenantControlInterface`.
For this, a notice is printed when a method from the *implicit*
`TenantControlInterface` is called.
Release note: None
knz
force-pushed
the
20230828-test-interfaces
branch
from
30b176b to
f615230
Compare
|
bors r=stevendanna |
|
Build succeeded: |
First commit from #109591.
Epic: CRDB-18499
Fixes #107058.
TLDR: unit tests using TestServer/TestCluster now produce warning and
notice messages upon certain suspicious API usage.
For example:
The following warnings/notices are implemented.
ApplicationLayerInterface
The problem here is that the implicit implementation of
ApplicationLayerInterfaceinsideTestServerInterfaceis likelymisused. If/when the test server automatically starts a secondary
tenant, the implicit
ApplicationLayerInterfacerefers to thesystem tenant, whereas only
.ApplicationLayer()refers to thesecondary tenant. It's likely that the test code should use the
latter.
For this, a warning is printed when a method from the implicit
ApplicationLayerInterfaceis called.The warning is not printed if the test has indicated (via
DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant)that it is ever only interested in the storage layer.
StorageLayerInterface
We want to promote the use of the explicit interface accessor
.StorageLayer()to access the methods ofStorageLayerInterface.For this, a notice is printed when a method from the implicit
StorageLayerInterfaceis called.The notice is not printed if the test has indicated (via
DefaultTestTenant: base.TestIsForStorageLayerAndNeedsASystemTenant)that it is ever only interested in the storage layer.
TenantControlInterface
We want to promote the use of the explicit interface accessor
.TenantController()to access the methods ofTenantControlInterface.For this, a notice is printed when a method from the implicit
TenantControlInterfaceis called.