sneak/neoirc
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions 15 Packages Projects Releases Wiki Activity

Move the test-only helpers out of the shipped db package #248

New Issue
Open
opened 2026-11-03 07:49:30 +01:00 by clawbot · 2 comments
clawbot commented 2026-11-03 07:49:30 +01:00
Collaborator
Copy Link
Copy Source

Four exported names in internal/db exist for tests, and only tests call them: NewTestDatabaseFromConn and RunMigrations in internal/db/testing.go, GetDB in internal/db/db.go (returns the raw *sql.DB), and SetBcryptCost in internal/db/auth.go (lowers the password hash cost). All four are compiled into neoircd.

.golangci.yml states the policy in its test-support depguard rule: test-support code must not be compiled into the shipped binary; it belongs in test files or in a package whose directory name ends in test, such as internal/dbtest. These four give a caller what the server must never have: raw SQL past the db.Database methods, and a weaker password hash. AGENTS.md says the server uses the database only through the methods of db.Database; GetDB lets server code step around that.

Found during the fresh read of AGENTS.md for #190. The document is right; the code is not. How the tests get a migrated *db.Database without these helpers is the implementer's call.

Definition of done:

  • internal/db/testing.go is gone, and no non-test file in internal/db exports a function, method or setting that only tests call.
  • The tests in internal/db, internal/handlers, internal/ircserver and internal/service that used them get what they need from test files (export_test.go), from internal/dbtest, or from a constructor the server itself uses, and still pass.
  • make check passes.

Model: fable-5-1

Four exported names in `internal/db` exist for tests, and only tests call them: `NewTestDatabaseFromConn` and `RunMigrations` in `internal/db/testing.go`, `GetDB` in `internal/db/db.go` (returns the raw `*sql.DB`), and `SetBcryptCost` in `internal/db/auth.go` (lowers the password hash cost). All four are compiled into `neoircd`. `.golangci.yml` states the policy in its `test-support` depguard rule: test-support code must not be compiled into the shipped binary; it belongs in test files or in a package whose directory name ends in `test`, such as `internal/dbtest`. These four give a caller what the server must never have: raw SQL past the `db.Database` methods, and a weaker password hash. `AGENTS.md` says the server uses the database only through the methods of `db.Database`; `GetDB` lets server code step around that. Found during the fresh read of `AGENTS.md` for https://git.eeqj.de/sneak/neoirc/issues/190. The document is right; the code is not. How the tests get a migrated `*db.Database` without these helpers is the implementer's call. Definition of done: - `internal/db/testing.go` is gone, and no non-test file in `internal/db` exports a function, method or setting that only tests call. - The tests in `internal/db`, `internal/handlers`, `internal/ircserver` and `internal/service` that used them get what they need from test files (`export_test.go`), from `internal/dbtest`, or from a constructor the server itself uses, and still pass. - `make check` passes. Model: fable-5-1
clawbot commented 2026-11-03 08:00:52 +01:00
Author
Collaborator
Copy Link
Copy Source

Judgement call: this is in scope, not a refactor for taste. .golangci.yml says test-support code stays out of the shipped binary, and these four names give a caller raw SQL and a weaker password hash.

Plan, in test files only apart from the deletions:

  • NewTestDatabaseFromConn and RunMigrations: tests in other packages build the database with db.New, the constructor the server uses, on a URL from dbtest.NewURL.
  • GetDB: a test that needs raw SQL opens its own connection to that same URL. Tests in internal/db may use export_test.go.
  • SetBcryptCost: delete it and the bcryptCost variable. Hash with bcrypt.DefaultCost as a constant. Other packages' tests then run at the real cost, which is few hashes. If make test passes 20 seconds because of this, say so on the PR with the time; do not add a setting for it.

No new setting, no new exported name in internal/db.

Model: opus-5-5

Judgement call: this is in scope, not a refactor for taste. `.golangci.yml` says test-support code stays out of the shipped binary, and these four names give a caller raw SQL and a weaker password hash. Plan, in test files only apart from the deletions: - `NewTestDatabaseFromConn` and `RunMigrations`: tests in other packages build the database with `db.New`, the constructor the server uses, on a URL from `dbtest.NewURL`. - `GetDB`: a test that needs raw SQL opens its own connection to that same URL. Tests in `internal/db` may use `export_test.go`. - `SetBcryptCost`: delete it and the `bcryptCost` variable. Hash with `bcrypt.DefaultCost` as a constant. Other packages' tests then run at the real cost, which is few hashes. If `make test` passes 20 seconds because of this, say so on the PR with the time; do not add a setting for it. No new setting, no new exported name in `internal/db`. Model: opus-5-5
clawbot commented 2026-11-03 09:42:39 +01:00
Author
Collaborator
Copy Link
Copy Source

Built in #255. The other names only tests called moved or went too, and the pull request lists them.

Model: opus-5-5

Built in https://git.eeqj.de/sneak/neoirc/pulls/255. The other names only tests called moved or went too, and the pull request lists them. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/neoirc#248