feat(e2e): add Postgres DB layer e2e test suite - #1477
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
debdf91 to
f5fb456
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several assertions can produce false positives and do not fully validate the advertised database behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds PostgreSQL-specific end-to-end coverage against the deployed API server and live database.
Changes:
- Adds schema, error-mapping, filtering, sorting, and pagination tests.
- Centralizes e2e client configuration.
- Wires database tests and PostgreSQL port-forwarding into CI.
File summaries
| File | Description |
|---|---|
test/e2e/README.md |
Documents database tests. |
test/e2e/e2e.sh |
Runs database tests against port-forwarded PostgreSQL. |
test/e2e/e2e_test.go |
Uses shared client configuration. |
test/e2e/e2e_gcs_test.go |
Updates shared token usage. |
test/e2e/client/config.go |
Adds reusable client environment and TLS configuration. |
test/e2e/db/README.md |
Documents database suite setup and scope. |
test/e2e/db/db_test.go |
Initializes API clients and database connection. |
test/e2e/db/error_mapping_test.go |
Tests PostgreSQL error translation and cascades. |
test/e2e/db/lister_test.go |
Tests filtering, ordering, and pagination. |
test/e2e/db/schema_test.go |
Validates live PostgreSQL schema behavior. |
test/e2e/db/labels_test.go |
Adds future label-test placeholders. |
test/e2e/db/migrations_test.go |
Adds future migration-test placeholders. |
test/e2e/db/relationships_test.go |
Adds future relationship-test placeholders. |
test/e2e/db/retention_test.go |
Adds future retention-test placeholder. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f5fb456 to
6ffaafb
Compare
| t.Fatalf("ListResults with ORDER BY ASC failed: %v", err) | ||
| } | ||
|
|
||
| for i := 1; i < len(resp.Results); i++ { |
There was a problem hiding this comment.
We should also assert the length of the results returned.
|
|
||
| run_db_tests() { | ||
| REPO="$1" | ||
| local LOCAL_PG_PORT=15432 |
There was a problem hiding this comment.
Instead of hardcoding this, we should have something like "${PG_E2E_PORT:-15432}" Otherwise, only one test can be run, not parallel run with multiple PG instances..
| }) | ||
| if err != nil { | ||
| t.Fatalf("CreateResult failed: %v", err) | ||
| } |
There was a problem hiding this comment.
Why no cleanup here? If there's a failure, then there's leak. In case of pass, that cleanup can be a no ops.
There was a problem hiding this comment.
🟡 Changes recommended
The HA-tagged test suite references a removed token constant and therefore cannot compile.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
| defaultCertPath = "/tmp/tekton-results/ssl" | ||
| defaultTokenPath = "/tmp/tekton-results/tokens" | ||
| defaultNamespace = "default" | ||
| allNamespacesReadAccessToken = "all-namespaces-read-access" |
| |---|---|---| | ||
| | `DB_URL` | (none) | Full Postgres DSN. When set, takes precedence over individual `POSTGRES_*` vars. The `e2e.sh` script constructs this automatically. | | ||
| | `POSTGRES_HOST` | `localhost` | Fallback: Postgres host (via port-forward) | | ||
| | `POSTGRES_PORT` | `15432` | Fallback: local forwarded port | |
There was a problem hiding this comment.
I see that you multiple vars here. Can you check whether you are using these var with default value in the script?
There was a problem hiding this comment.
The POSTGRES_* variables are used as fallbacks in db_test.go's buildDSN() for manual runs when DB_URL is not set, the e2e.sh CI script always constructs DB_URL directly, so these individual vars are never used in the CI path. I am mentioning this in README.md file only.
- Add DB layer e2e tests (error mapping, schema validation, lister behavior) under test/e2e/db/ with build tag e2e - Fix seedResults in lister_test.go: lowercase status names to match API name regex, populate mandatory Summary.Record and Summary.Type - Remove InsecureSkipVerify TLS fallback in test/e2e/client/config.go to resolve CodeQL critical alert; fail explicitly on missing certs - Add run_db_tests function in e2e.sh with port-forward and DB_URL setup; exclude db tests from main e2e run - Fix stale variable reference in e2e_gcs_test.go: allNamespacesReadAccessTokenFile to allNamespacesReadAccessToken
6ffaafb to
3cac059
Compare
Changes
Add Postgres-specific e2e test suite (
test/e2e/db/) that exercises databasebehavior the SQLite-backed unit tests cannot reach:
pgconn.PgErrorSQLSTATEerror mapping,
jsonbcolumn types and operators, CEL -> SQL filter/sort/paginationon real Postgres, FK cascade, and schema correctness.
Tests run against the deployed API server in the kind cluster and its live
Postgres instance via gRPC - not an in-process server or throwaway database.
What's included:
jsonb @>)pageSize≥5, ListRecords)t.Skipfor future stories (labels, migrations, relationships, retention)test/e2e/client/config.go(eliminates duplication betweene2e_test.goanddb_test.go)e2e.sh: port-forward on15432,DB_URLenv var, password suppressed from traces, cleanup on failuretest/e2e/README.mdandtest/e2e/db/README.mdupdated/created/kind feature
Submitter Checklist
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes