From 2673cd4ab2174097cf81c60cc20d97b2ebbea410 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 27 Oct 2026 10:29:04 +0000 Subject: [PATCH] Keep each test database when the pool replaces its connection (closes #209) SQLite deletes a shared in-memory database when its last connection closes. A query cancelled while it runs makes the pool close its connection, so a test could lose all its tables and rows at random. NewTestDatabaseURL in internal/db returns the URL of a new in-memory database with a random name. It holds one connection to it open until the test ends. The db, handlers, ircserver, server and service tests now take their database URL from it. A new test cancels a query on such a database, checks with a TEMP table that the connection was replaced, and checks that a row is still there. Model: opus-5-5 --- internal/db/db_test.go | 46 +++++++++++++++++++++++++++++++ internal/db/export_test.go | 15 +++------- internal/db/queries_test.go | 2 +- internal/db/testing.go | 30 ++++++++++++++++++++ internal/handlers/api_test.go | 9 +----- internal/ircserver/server_test.go | 10 +------ internal/server/server_test.go | 6 +--- internal/service/service_test.go | 9 +----- 8 files changed, 85 insertions(+), 42 deletions(-) diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 9b9833f..a9c2037 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -155,6 +155,52 @@ func TestForeignKeysOnAfterCancelledQuery(t *testing.T) { } } +// TestNewTestDatabaseURLKeepsDataAfterCancelledQuery cancels a query +// on a test database while it runs, which makes the pool close its +// only connection and open a new one. A TEMP table lasts only as long +// as its connection, so its absence shows the connection was replaced. +// The database must still have its tables and rows. +func TestNewTestDatabaseURLKeepsDataAfterCancelledQuery(t *testing.T) { + t.Parallel() + + database, err := startDatabase(t, db.NewTestDatabaseURL(t)) + if err != nil { + t.Fatal(err) + } + + _, err = database.GetOrCreateChannel(t.Context(), "#test") + if err != nil { + t.Fatal(err) + } + + _, err = database.GetDB().ExecContext( + t.Context(), "CREATE TEMP TABLE first_connection (x)", + ) + if err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithTimeout(t.Context(), endlessQueryTimeout) + defer cancel() + + _, err = database.GetDB().ExecContext(ctx, endlessQuery) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("endless query: err = %v", err) + } + + tempTables := queryInt( + t, database, "SELECT COUNT(*) FROM sqlite_temp_schema", + ) + if tempTables != 0 { + t.Fatal("the pool kept its first connection") + } + + channels := queryInt(t, database, "SELECT COUNT(*) FROM channels") + if channels != 1 { + t.Errorf("channels = %d, want 1", channels) + } +} + // TestDBURLSettingForeignKeysFailsStartup checks that the database // refuses to start when DBURL sets foreign_keys itself, however it // is spelled. diff --git a/internal/db/export_test.go b/internal/db/export_test.go index f233380..3d21997 100644 --- a/internal/db/export_test.go +++ b/internal/db/export_test.go @@ -5,21 +5,14 @@ import ( "database/sql" "fmt" "log/slog" - "sync/atomic" + "testing" ) -//nolint:gochecknoglobals // test counter -var testDBCounter atomic.Int64 - // NewTestDatabase creates an in-memory database for testing. -func NewTestDatabase() (*Database, error) { - counter := testDBCounter.Add(1) +func NewTestDatabase(tb testing.TB) (*Database, error) { + tb.Helper() - dsn := fmt.Sprintf( - "file:testdb%d?mode=memory"+ - "&cache=shared&_pragma=foreign_keys(1)", - counter, - ) + dsn := NewTestDatabaseURL(tb) + "&_pragma=foreign_keys(1)" conn, err := sql.Open("sqlite", dsn) if err != nil { diff --git a/internal/db/queries_test.go b/internal/db/queries_test.go index 881fec6..a39f88a 100644 --- a/internal/db/queries_test.go +++ b/internal/db/queries_test.go @@ -13,7 +13,7 @@ import ( func setupTestDB(t *testing.T) *db.Database { t.Helper() - database, err := db.NewTestDatabase() + database, err := db.NewTestDatabase(t) if err != nil { t.Fatal(err) } diff --git a/internal/db/testing.go b/internal/db/testing.go index d036611..40439f0 100644 --- a/internal/db/testing.go +++ b/internal/db/testing.go @@ -2,10 +2,40 @@ package db import ( "context" + "crypto/rand" "database/sql" "log/slog" + "testing" ) +// NewTestDatabaseURL returns the URL of a new, empty in-memory +// database for one test. SQLite deletes an in-memory database when +// its last connection closes. A pool closes a connection when a query +// on it is cancelled while it runs. So this function holds a +// connection of its own open until the test ends. +func NewTestDatabaseURL(tb testing.TB) string { + tb.Helper() + + // A random name, so that no other test can share the database. + dbURL := "file:" + rand.Text() + "?mode=memory&cache=shared" + + pool, err := sql.Open("sqlite", dbURL) + if err != nil { + tb.Fatalf("open test database: %v", err) + } + + tb.Cleanup(func() { _ = pool.Close() }) + + conn, err := pool.Conn(tb.Context()) + if err != nil { + tb.Fatalf("connect to test database: %v", err) + } + + tb.Cleanup(func() { _ = conn.Close() }) + + return dbURL +} + // NewTestDatabaseFromConn creates a Database wrapping an // existing *sql.DB connection. Intended for integration // tests in other packages. diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 9201f1c..b0020b1 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -3,7 +3,6 @@ package handlers_test import ( "bytes" "context" - "crypto/rand" "database/sql" "encoding/json" "errors" @@ -164,13 +163,7 @@ func newTestServerWith( viperMu.Lock() defer viperMu.Unlock() - // A random name, so that no other test can share it, not - // even one that has finished but still holds its database - // open. - dbURL := fmt.Sprintf( - "file:%s?mode=memory&cache=shared", - rand.Text(), - ) + dbURL := db.NewTestDatabaseURL(t) copyTemplateDB(t, dbURL) var ( diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index a530b36..28a0336 100644 --- a/internal/ircserver/server_test.go +++ b/internal/ircserver/server_test.go @@ -3,7 +3,6 @@ package ircserver_test import ( "bufio" "context" - "crypto/rand" "database/sql" "errors" "fmt" @@ -107,14 +106,7 @@ func newTestEnvWithResolver( ) *testEnv { t.Helper() - // A random name, so that no other test environment, not - // even an earlier run of the same test, can share it. - dsn := fmt.Sprintf( - "file:%s?mode=memory&cache=shared", - rand.Text(), - ) - - conn, err := sql.Open("sqlite", dsn) + conn, err := sql.Open("sqlite", db.NewTestDatabaseURL(t)) if err != nil { t.Fatalf("open db: %v", err) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 148f28e..31e0707 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -5,7 +5,6 @@ package server_test import ( - "crypto/rand" "fmt" "net" "net/http" @@ -241,10 +240,7 @@ func newApp( stats.New, ), fx.Decorate(func(cfg *config.Config) *config.Config { - // A random name, so that no other test can share it. - cfg.DBURL = fmt.Sprintf( - "file:%s?mode=memory&cache=shared", rand.Text(), - ) + cfg.DBURL = db.NewTestDatabaseURL(t) cfg.Port = port return cfg diff --git a/internal/service/service_test.go b/internal/service/service_test.go index 3ad3279..64338b3 100644 --- a/internal/service/service_test.go +++ b/internal/service/service_test.go @@ -6,7 +6,6 @@ package service_test import ( "context" - "crypto/rand" "encoding/json" "errors" "fmt" @@ -46,13 +45,7 @@ type testEnv struct { func newTestEnv(t *testing.T) *testEnv { t.Helper() - // A random name, so that no other test can share it, not - // even one that has finished but still holds its database - // open. - dbURL := fmt.Sprintf( - "file:%s?mode=memory&cache=shared", - rand.Text(), - ) + dbURL := db.NewTestDatabaseURL(t) var ( database *db.Database