TRT-2833: Add integration test tier for PostgreSQL-backed API methods - #3829
Merged
openshift-merge-bot[bot] merged 1 commit intoJul 27, 2026
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
mstaeble
changed the title
|
@mstaeble: This pull request references TRT-2833 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 5 minutes. |
Contributor
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1079)
📒 Files selected for processing (11)
WalkthroughAdds containerized PostgreSQL integration tests, supporting database utilities, a Makefile command, and SQL query updates that use half-open reporting intervals. ChangesIntegration testing and reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestMain
participant PostgreSQL
participant IntegrationTests
participant ReportingQueries
TestMain->>PostgreSQL: start container and clone test database
IntegrationTests->>PostgreSQL: seed jobs and job runs
IntegrationTests->>ReportingQueries: execute reporting queries
ReportingQueries-->>IntegrationTests: return current and previous aggregates
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (1)
98-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
integrationas.PHONY.Static analysis flags the new target as not
.PHONY. Add it alongside other phony targets (e.g.,test) to avoid accidental conflicts with a file/directory of the same name.🔧 Proposed fix
+.PHONY: integration integration: ifeq ($(ARTIFACT_DIR),)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 98 - 104, Declare the Makefile integration target as .PHONY, alongside the existing phony target declarations such as test, so it always runs regardless of a matching file or directory.Source: Linters/SAST tools
test/integration/util/testdb.go (2)
213-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile manual DSN parsing in
replaceDBName.Byte-by-byte scanning for
/and?to swap the database name is brittle against DSN format variations (e.g., credentials/hosts containing/, multiple query params). Consider usingnet/url.Parseto setu.Pathinstead, which is more robust and readable.♻️ Proposed refactor
-func replaceDBName(dsn, newDB string) string { - // testcontainers DSN format: postgres://user:pass@host:port/dbname?params - // We find the last / before ? and replace the db name. - for i := len(dsn) - 1; i >= 0; i-- { - if dsn[i] == '?' { - // find the / before the ? - for j := i - 1; j >= 0; j-- { - if dsn[j] == '/' { - return dsn[:j+1] + newDB + dsn[i:] - } - } - } - if dsn[i] == '/' && (i == len(dsn)-1 || dsn[i+1] != '/') { - return dsn[:i+1] + newDB - } - } - return dsn + "/" + newDB -} +func replaceDBName(dsn, newDB string) (string, error) { + u, err := url.Parse(dsn) + if err != nil { + return "", fmt.Errorf("parsing dsn: %w", err) + } + u.Path = "/" + newDB + return u.String(), nil +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 213 - 231, Replace the manual byte-scanning logic in replaceDBName with net/url parsing: parse the DSN, update the parsed URL’s Path to use newDB while preserving the existing path structure and query parameters, then return the serialized URL. Keep the current fallback behavior for parse failures if required by the surrounding tests.
142-145: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider quoting dynamic identifiers in DDL statements.
templateDB/dbNameare interpolated viafmt.SprintfintoDROP/CREATE/ALTER DATABASEstatements. Since Postgres doesn't support placeholders for identifiers, this pattern is unavoidable to some degree, butdbNameis already constrained to[a-z0-9_]bysanitize(), which mitigates real injection risk here. For stricter guideline compliance and defense-in-depth against future changes tosanitize()ort.Name()sources, consider quoting the identifier explicitly (e.g.,pgx.Identifier{name}.Sanitize()) rather than relying solely on charset filtering.As per path instructions, "database/sql with placeholders; no fmt.Sprintf in queries" and "Do not build SQL queries by concatenating or formatting user input directly; use placeholders and prepared statements instead."
Also applies to: 165-165, 184-184, 192-192, 204-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 142 - 145, Quote dynamic database identifiers in the DDL statements within the test database setup flow, including DROP, CREATE, and ALTER operations around templateDB and dbName. Replace direct fmt.Sprintf interpolation with the project’s identifier-sanitization mechanism, such as pgx.Identifier(name).Sanitize(), while preserving the existing sanitized-name behavior and error handling.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 176: Update the golang.org/x/crypto dependency from v0.51.0 to the
patched v0.54.0 or later release in go.mod, refresh the corresponding go.sum
entries, and run go mod tidy to reconcile dependency metadata. Prefer obtaining
the upgrade through the dependency that requires it if applicable.
---
Nitpick comments:
In `@Makefile`:
- Around line 98-104: Declare the Makefile integration target as .PHONY,
alongside the existing phony target declarations such as test, so it always runs
regardless of a matching file or directory.
In `@test/integration/util/testdb.go`:
- Around line 213-231: Replace the manual byte-scanning logic in replaceDBName
with net/url parsing: parse the DSN, update the parsed URL’s Path to use newDB
while preserving the existing path structure and query parameters, then return
the serialized URL. Keep the current fallback behavior for parse failures if
required by the surrounding tests.
- Around line 142-145: Quote dynamic database identifiers in the DDL statements
within the test database setup flow, including DROP, CREATE, and ALTER
operations around templateDB and dbName. Replace direct fmt.Sprintf
interpolation with the project’s identifier-sanitization mechanism, such as
pgx.Identifier(name).Sanitize(), while preserving the existing sanitized-name
behavior and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
mstaeble
force-pushed
the
worktree-polished-dreaming-lagoon
branch
from
July 26, 2026 14:18
73738ba to
aa0567b
Compare
mstaeble
changed the title
mstaeble
marked this pull request as ready for review
July 26, 2026 14:59
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/util/testdb.go (1)
92-171: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThread
ctxintocreateTemplateDBfor cancellation/timeouts.
StartPostgresContainerreceives actxbut drops it before callingcreateTemplateDB, so none of the DROP/CREATE/ALTER DATABASE calls (Lines 143, 146, 166) ordb.Newcan be cancelled or bounded by a timeout — if the container becomes unresponsive, this setup can hang indefinitely with no way to interrupt it. The same gap exists inNewTestDB(Lines 175-212), which never receives or uses a context at all.As per path instructions, Go code should use
context.Contextfor cancellation and timeouts.♻️ Sketch of context propagation
-func createTemplateDB(baseDSN string) error { +func createTemplateDB(ctx context.Context, baseDSN string) error { adminDB, err := sql.Open("pgx", baseDSN) ... - if _, err := adminDB.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s", templateDB)); err != nil { + if _, err := adminDB.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", templateDB)); err != nil {and call it as
createTemplateDB(ctx, connStr).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/util/testdb.go` around lines 92 - 171, Thread context.Context through the database setup flow: change createTemplateDB to accept ctx and pass it from StartPostgresContainer, using context-aware SQL and GORM connection operations for DROP, CREATE, ALTER, and db.New. Update NewTestDB to accept and propagate its context into the same setup path, preserving existing cleanup and error behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/integration/util/testdb.go`:
- Around line 92-171: Thread context.Context through the database setup flow:
change createTemplateDB to accept ctx and pass it from StartPostgresContainer,
using context-aware SQL and GORM connection operations for DROP, CREATE, ALTER,
and db.New. Update NewTestDB to accept and propagate its context into the same
setup path, preserving existing cleanup and error behavior.
Contributor
|
Scheduling required tests: |
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
force-pushed
the
worktree-polished-dreaming-lagoon
branch
2 times, most recently
from
July 27, 2026 02:06
a37f9ba to
b6c468b
Compare
Contributor
|
Scheduling required tests: |
Introduce a new test tier between unit tests (no DB) and e2e tests (full server) that validates SQL query logic against a real PostgreSQL instance using testcontainers-go. Infrastructure: - test/integration/util/testdb.go: container lifecycle with Podman auto-detection, per-test database cloning via template pattern - test/integration/util/schema.go: schema setup (AutoMigrate all models as regular tables, install SQL functions) - Makefile: new "make integration" target Tests cover ProwJobSimilarName, VariantReports, and JobReports with edge cases including boundary timestamps, zero-denominator safety, multi-job aggregation, empty variants, cross-release isolation, filter/sort/limit, bug status filtering, and PR retest calculations. Three bugs found and fixed: - job_results SQL function returned column "previous_failures" but the Go struct maps to "previous_fails"; renamed to match (also fixed in BuildClusterHealth query) - All period-boundary queries used BETWEEN for both previous and current periods, causing runs at the boundary timestamp to be double-counted; changed previous period to half-open interval [start, boundary) - job_results open_bugs counted COUNT(DISTINCT bug_jobs.bug_id) which includes all associated bugs regardless of status; changed to COUNT(DISTINCT bugs.id) so closed/verified/modified/on_qa bugs are excluded via the existing LEFT JOIN filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
worktree-polished-dreaming-lagoon
branch
from
July 27, 2026 02:27
b6c468b to
7b09025
Compare
Contributor
|
Scheduling required tests: |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 34 minutes. |
Contributor
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make integrationtarget; 21 tests coverProwJobSimilarName,VariantReports, andJobReportswith edge cases (boundary timestamps, zero-denominator safety, multi-job aggregation, empty variants, cross-release isolation, filter/sort/limit, bug status filtering, PR retest calculations)job_resultsfunction returnedprevious_failuresbut Go struct expectsprevious_fails(column always scanned as zero)BETWEENfor both periods, double-counting runs at the boundary timestamp; previous period changed to half-open interval[start, boundary)job_resultsopen_bugs countedCOUNT(DISTINCT bug_jobs.bug_id)which includes all associated bugs regardless of status; changed toCOUNT(DISTINCT bugs.id)so closed/verified/modified/on_qa bugs are excluded via the existing LEFT JOIN filterjob_resultsLEFT JOIN tobug_jobs/bugsinside theresultsCTE caused row multiplication, inflating all aggregate counts (current_runs,current_passes, etc.) by the number of associated bugs; extracted bug counting into a separatejob_bugsCTE joined at the outer SELECT levelTest plan
make lintpassesmake testpassesmake e2epassesmake integrationpasses (21 tests)🤖 Generated with Claude Code