test(db): fail instead of skip when Postgres is unreachable#574
test(db): fail instead of skip when Postgres is unreachable#574nikolasmatt wants to merge 10 commits into
Conversation
Integration-tagged DB helpers (v1alpha1store.NewTestPool and the
freshDB/newDB helpers in database, legacymigrate, and orchestrator)
skipped tests when the admin connection failed, turning a deliberate
-tags=integration run silently green. They now t.Fatalf with a hint to
start Postgres ('make run-docker') or run 'make test-unit'.
The admin DSN can be overridden via AGENT_REGISTRY_TEST_DATABASE_URL
(default unchanged: postgres://agentregistry:agentregistry@localhost:5432/postgres?sslmode=disable).
Per-test database URIs are now derived by parsing the admin URI and
swapping the database path, so overrides propagate.
The cli_e2e_test.go self-skip is untouched: it is documented behavior
of the no-docker test-cli-e2e tier.
Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
url.Parse accepts kv-form strings as opaque paths, so Redacted() returned them verbatim, password included. Only URL-scheme DSNs are redacted field-wise now; anything else is masked entirely. Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
'make test' runs unit and integration tests together and needs Postgres; 'make test-integration' does not exist. Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
…n redaction The testAdminDSN/redactDSN/testDBURI trio existed as four copies; they now live once in dbtest. RedactDSN also masks password/sslpassword query parameters (pgx accepts them; url.Redacted only masks userinfo), and DBURI drops a dbname query parameter, which pgx would otherwise apply after the path and redirect every per-test URI back to the override's database. Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
dbtest.ConnectAdmin resolves, validates, and connects in one place — a keyword/value DSN (pgx accepts it, url.Parse mangles it) now fails fast with a clear URL-form-only message instead of producing broken derived URIs, and the four copy-pasted connect/Fatalf blocks collapse into it. Unit tests pin RedactDSN masking, DBURI derivation, and the validation. AGENTS.md no longer claims 'make test' runs all tests (unit- and e2e-tagged files are excluded). Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
…lution Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
| // AGENT_REGISTRY_TEST_DATABASE_URL when set, otherwise the local dev default | ||
| // (localhost:5432, user/password agentregistry). The value must be a | ||
| // URL-form DSN (postgres://...); keyword/value DSNs are not supported. | ||
| func AdminDSN() string { |
There was a problem hiding this comment.
looks like this isn't used directly anywhere outside of ConnectAdmin, does it need to be exported?
Seems a bit awkward to load the dsn and return it without doing validation, should validation be wrapped into this helper?
There was a problem hiding this comment.
looks like this isn't used directly anywhere outside of ConnectAdmin, does it need to be exported?
Downstream projects that add their own db migrations sources need to execute these tests as part of their suite. This lets the 2 suites coordinate.
Seems a bit awkward to load the dsn and return it without doing validation, should validation be wrapped into this helper?
🤔 probably should validate. Though validation already in ConnectAdmin.
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
There was a problem hiding this comment.
a bit overkill to test a test helper, but no complaints if you leave it
There was a problem hiding this comment.
fair. i started this the RedactDSN function more generically, so when i switched it to be a test helper i just kept the tests.
| // DBURI returns adminURI with its database replaced by dbName. A dbname | ||
| // query parameter is dropped — pgx would apply it after the path, silently | ||
| // redirecting every per-test URI back to the override's database. | ||
| func DBURI(adminURI, dbName string) (string, error) { |
There was a problem hiding this comment.
I think it would be simpler to accept an optional name param and build the admin URI with the db name at connect time, than reparse and inject after the fact. Not sure if there is a usecase for having both the hardcoded adminURI + the db named specific one?
| make test-coverage-report | ||
|
|
||
| # Run integration tests | ||
| make test-integration |
There was a problem hiding this comment.
I think it makes sense to allow running integration tests independently still, we should leave this as a unique make target independent of the unit tests
There was a problem hiding this comment.
make test is the integration make target. make test-integration doesn't exist. so this is just correcting the AGENTS.md instruction to stop tripping agents. Alternatively i can fix the make targets but i decided to not do it as part of this PR (not a strong opinion), happy to update them if you think it's worth it.
NewTestPoolWithDSN(t, dsn) (pool, testDSN) and ConnectAdminDSN(ctx, t, dsn) let downstream suites own their database configuration instead of syncing on the AGENT_REGISTRY_TEST_DATABASE_URL name; NewTestPool/ConnectAdmin remain as env-resolving wrappers. Failure messages no longer include the DSN at all — connect errors already name user/database/host — which removes RedactDSN and its masking tests wholesale. Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
…abase Group the three Postgres-backed integration test files (legacy OSS bridge, migrator search_path, orchestrator) into one external package at test/integration/database. Their near-identical per-test database plumbing (freshDB x2 + newDB, distinct db-name prefixes) collapses into a single freshDB in freshdb_test.go with one test_db_ prefix. With the scattered files gone, the dbtest bridge package has no remaining consumers outside v1alpha1store, so delete it and fold the DSN helpers into pkg/registry/v1alpha1store/testutil.go as unexported adminDSN/validateAdminDSN/testDBURI. The exported test surface shrinks to NewTestPool + NewTestPoolWithDSN. The dbtest behavior pins move to an in-package testutil_test.go. Organization-only: assertions and exercised code paths are unchanged; failure messages still never echo the DSN. Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
Signed-off-by: Nik Matthiopoulos <nikolaos.matthiopoulos@solo.io>
2 participants
Description
The integration-tagged DB test helpers (
v1alpha1store.NewTestPooland thefreshDB/newDBcopies underpkg/registry/database) skip when Postgres is unreachable, somake testwithout a database goes silently green. They now fail fast with a hint to start Postgres or runmake test-unit. Failure messages never echo the DSN (connect errors already name user/database/host, and credentials stay out of CI logs). CI already provisions Postgres beforemake testand is unaffected.Also:
legacymigrate,migrate_searchpath,orchestrator) consolidate into onetest/integration/database/package with a single sharedfreshDB; still covered bymake test's-tags=integration ./....AGENT_REGISTRY_TEST_DATABASE_URL(URL-form; default unchanged) or passed explicitly viaNewTestPoolWithDSN(t, dsn) (pool, testDSN)for suites that own their database configuration; the exported test surface is exactlyNewTestPool+NewTestPoolWithDSN, with the DSN plumbing unexported behind them (pinned by in-package tests).test-cli-e2etier's documented self-skip is untouched.Change Type
/kind cleanup
Changelog