Skip to content

serverutils: refactor the server test APIs #107058

@knz

Description

@knz

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 TestServerInterface inherit from TestTenantInterface any more
  • instead, make a method StorageLayer() TestTenantInterface inside TestServerInterface so that a test can spell out explicitly that it wants access to the storage layer.
  • rename TenantOrServer() to ApplicationLayer().
  • add a SQLConn() *goSQL.DB method to TestTenantInterface, it's been sorely missing!
  • spell out in docs that the 2nd and 3rd return value of StartServer are equivalent to .ApplicationLayer().SQLConn() and .ApplicationLayer().DB(), for clarity.
  • rename TestTenantInterface to TestClientInterface.

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

Metadata

Metadata

Assignees

Labels

A-multitenancyRelated to multi-tenancyA-testingTesting tools and infrastructureC-enhancementSolution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)T-multitenantIssues owned by the multi-tenant virtual teamdb-cy-23

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions