-
Notifications
You must be signed in to change notification settings - Fork 4.1k
serverutils: refactor the server test APIs #107058
Copy link
Copy link
Closed
Labels
A-multitenancyRelated to multi-tenancyRelated to multi-tenancyA-testingTesting tools and infrastructureTesting tools and infrastructureC-enhancementSolution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)T-multitenantIssues owned by the multi-tenant virtual teamIssues owned by the multi-tenant virtual teamdb-cy-23
Description
Context
As part of our work on #76378 we are observing a pattern:
s, sqlDB, kvDB := serverutils.StartServer(...)
// At this point, sqlDB and kvDB point to the "application layer" already. This is desirable!
// If a test needs something else (e.g. "AdminURL") it needs to do a bit extra work:
ts := s.TenantOrServer() // give access to the application layer.
someuse := ts.ClusterSettings()
someuse := ts.AdminURL()
// etc.Meanwhile, if the test needs explicit access to the storage layer, it is cumbersome:
_ = s.AdminURL() // this kinda works, but we feel it is ambiguous.
// Very cumbersome to retrieve a SQL connection:
systemSqlDB := serverutils.OpenDBConn(t, s.SQLAddr() /* don't mix with ServingSQLAddr()! */, ...)Description of problem
There are two shorcomings with this structure:
- if a test cares about the "application layer", it is too easy to mistakenly reach out for the TestServerInterface instead of thinking to call
TenantOrServer()first. - if a test cares about the storage layer / system tenant, retrieving a SQL connection is cumbersome.
And most importantly:
- a single run of the test only exercices EITHER the system tenant OR a secondary tenant, and never both. This halves test coverage.
Solution outline
More explicit testserver APIs
At the outset, we should make the "application" vs "storage" layer separation more clear. That is:
- don't make
TestServerInterfaceinherit fromTestTenantInterfaceany more - instead, make a method
StorageLayer() TestTenantInterfaceinsideTestServerInterfaceso that a test can spell out explicitly that it wants access to the storage layer. - rename
TenantOrServer()toApplicationLayer(). - add a
SQLConn() *goSQL.DBmethod toTestTenantInterface, it's been sorely missing! - spell out in docs that the 2nd and 3rd return value of
StartServerare equivalent to.ApplicationLayer().SQLConn()and.ApplicationLayer().DB(), for clarity. - rename
TestTenantInterfacetoTestClientInterface.
This would already clarify a great deal of tests. But it would still limit each test run to either test one or the other.
Helper to automate working with test servers
To increase test coverage, we should invert control of StartServer, that is deprecate
func StartServer(*testing.T, ...) (TestServerInterface, *sql.DB, *kv.DB)and instead introduce:
func RunWithServer(*testing.T, testName string, ..., runFn func(*testing.T, TestServerInterface, *sqlDB, *kv.DB))
So a test would change from:
s, sqlDB, kvDB := serverutils.StartServer(t, ...)
defer s.Stopper().Close(...)
ts := s.TenantOrServer() // give access to the application layer.
someuse := ts.ClusterSettings()
someuse := ts.AdminURL()to:
serverutils.RunWithServer(t, ... func(*testing.T, s ApplicationLayerInterface, sqlDB ..., kvDB ...) {
// No need for calling stopper explicitly.
someuse := ts.ClusterSettings()
someuse := ts.AdminURL()
})Once we have this API in place, we can make the sub-test run for both interfaces:
// in serverutils, RunWithServer:
s := startServerInternal(...)
defer s.Stopper().Close()
t.Run("system tenant", func(t *testing.T) {
ts := s.SystemLayer()
runFn(t, ts, ts.SQLConn(), ts.DB())
})
t.Run("secondary tenant, serverless-style", func(t *testing.T) {
tenant := s.StartTenant(...)
runFn(t, tenant, tenant.SQLConn(), tenant.DB())
})
t.Run("secondary tenant, dedicated-style", func(t *testing.T) {
tenant := s.StartTenant(...)
runFn(t, tenant, tenant.SQLConn(), tenant.DB())
})Jira issue: CRDB-29864
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
A-multitenancyRelated to multi-tenancyRelated to multi-tenancyA-testingTesting tools and infrastructureTesting tools and infrastructureC-enhancementSolution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)T-multitenantIssues owned by the multi-tenant virtual teamIssues owned by the multi-tenant virtual teamdb-cy-23