From 773759739dd90543812f917a8838c8ce26265ca6 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. The new package internal/dbtest has NewURL. It returns the URL of a new in-memory database with a random name and holds one connection to it open until the test ends. The db, handlers, ircserver, server and service tests take their database URL from it. The test-support lint rule keeps the package out of the server binary. 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 --- .golangci.yml | 4 +++ README.md | 1 + internal/db/db_test.go | 47 +++++++++++++++++++++++++++++++ internal/db/export_test.go | 17 ++++------- internal/db/queries_test.go | 2 +- internal/dbtest/dbtest.go | 39 +++++++++++++++++++++++++ internal/handlers/api_test.go | 10 ++----- internal/ircserver/server_test.go | 11 ++------ internal/server/server_test.go | 7 ++--- internal/service/service_test.go | 10 ++----- 10 files changed, 106 insertions(+), 42 deletions(-) create mode 100644 internal/dbtest/dbtest.go diff --git a/.golangci.yml b/.golangci.yml index 1b73eb9..945ef89 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -61,6 +61,10 @@ linters: desc: >- Test-support code belongs in test files and in packages whose directory name ends in test, not in the shipped binary. + - pkg: sneak.berlin/go/neoirc/internal/dbtest + desc: >- + Test-support code belongs in test files and in packages whose + directory name ends in test, not in the shipped binary. # Only decisions already recorded in the Go package defaults are # listed here. Every entry matches the module path exactly. gomodguard_v2: diff --git a/README.md b/README.md index 411b41a..5489b9a 100644 --- a/README.md +++ b/README.md @@ -3342,6 +3342,7 @@ neoirc/ │ │ └── schema/ │ │ ├── 000.sql # schema_migrations bootstrap │ │ └── 001_initial.sql +│ ├── dbtest/ # Gives each test its own database; only tests import it │ ├── globals/ # Application-wide metadata │ │ └── globals.go │ ├── handlers/ # HTTP request handlers diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 9b9833f..6d10283 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -12,6 +12,7 @@ import ( "go.uber.org/fx/fxtest" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" + "sneak.berlin/go/neoirc/internal/dbtest" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/logger" ) @@ -155,6 +156,52 @@ func TestForeignKeysOnAfterCancelledQuery(t *testing.T) { } } +// TestNewURLKeepsDataAfterCancelledQuery 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 TestNewURLKeepsDataAfterCancelledQuery(t *testing.T) { + t.Parallel() + + database, err := startDatabase(t, dbtest.NewURL(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..5dd9384 100644 --- a/internal/db/export_test.go +++ b/internal/db/export_test.go @@ -5,21 +5,16 @@ import ( "database/sql" "fmt" "log/slog" - "sync/atomic" + "testing" + + "sneak.berlin/go/neoirc/internal/dbtest" ) -//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 := dbtest.NewURL(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/dbtest/dbtest.go b/internal/dbtest/dbtest.go new file mode 100644 index 0000000..add7f03 --- /dev/null +++ b/internal/dbtest/dbtest.go @@ -0,0 +1,39 @@ +// Package dbtest gives each test a database of its own. Only tests +// may import it. +package dbtest + +import ( + "crypto/rand" + "database/sql" + "testing" + + _ "modernc.org/sqlite" // registers the sqlite driver +) + +// NewURL 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 NewURL(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 +} diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 9201f1c..a883330 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" @@ -28,6 +27,7 @@ import ( api "sneak.berlin/go/neoirc/internal/cli/api" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" + "sneak.berlin/go/neoirc/internal/dbtest" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/handlers" "sneak.berlin/go/neoirc/internal/hashcash" @@ -164,13 +164,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 := dbtest.NewURL(t) copyTemplateDB(t, dbURL) var ( diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index a530b36..f30b826 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" @@ -20,6 +19,7 @@ import ( "sneak.berlin/go/neoirc/internal/broker" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" + "sneak.berlin/go/neoirc/internal/dbtest" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/ircserver" "sneak.berlin/go/neoirc/internal/logger" @@ -107,14 +107,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", dbtest.NewURL(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..874231e 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" @@ -19,6 +18,7 @@ import ( "sneak.berlin/go/neoirc/internal/broker" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" + "sneak.berlin/go/neoirc/internal/dbtest" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/handlers" "sneak.berlin/go/neoirc/internal/healthcheck" @@ -241,10 +241,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 = dbtest.NewURL(t) cfg.Port = port return cfg diff --git a/internal/service/service_test.go b/internal/service/service_test.go index 3ad3279..a51710a 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" @@ -23,6 +22,7 @@ import ( "sneak.berlin/go/neoirc/internal/broker" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/db" + "sneak.berlin/go/neoirc/internal/dbtest" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/hashcash" "sneak.berlin/go/neoirc/internal/logger" @@ -46,13 +46,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 := dbtest.NewURL(t) var ( database *db.Database -- 2.54.0