TRT-2836: Add integration tests for payload query functions - #3859
TRT-2836: Add integration tests for payload query functions#3859mstaeble wants to merge 1 commit into
Conversation
|
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
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2836 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble 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 |
openshift-ci
Bot
added
the
approved
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe change adds integration coverage for payload queries and configurable fixture helpers for release metadata. Tests cover payload diffs, acceptance metrics, failure aggregation, release-tag lookup, filtering, ordering, boundaries, isolation, and empty results. ChangesPayload query coverage
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/integration/payload_queries_test.go (2)
1271-1277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider fixture options instead of the raw UPDATE.
CreateReleasePullRequestdoes not exposePullRequestIDorBugURL, so this test writes them with raw SQL. AddingWithPullRequestIDandWithBugURLoptions to the fixture would keep all release-pull-request setup in one place and match the option pattern you already use forCreateReleaseTag.The current statement is safe. It uses
?placeholders with bound arguments and does not format user input into the query.🤖 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/payload_queries_test.go` around lines 1271 - 1277, Extend CreateReleasePullRequest with WithPullRequestID and WithBugURL fixture options, following the existing option pattern used by CreateReleaseTag. Update the test setup to pass these options when creating the pull request and remove the raw UPDATE statement, while preserving the existing values and subsequent LinkReleaseTagPullRequests call.
204-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a fixture option for the prow job run URL.
The pattern "create the run, then update
url" repeats in about twelve places in this file. AWithURLoption onCreateProwJobRun, or aCreateProwJobRunWithURLhelper intest/integration/util, would remove the extraUpdatecall and the duplicated URL literal at each call site. The URL literal currently appears twice per run: once in theUpdateand once inCreateReleaseJobRun.This is optional. The current code is correct.
♻️ Example option in test/integration/util/fixtures.go
type ProwJobRunOption func(*models.ProwJobRun) func WithProwJobRunURL(url string) ProwJobRunOption { return func(r *models.ProwJobRun) { r.URL = url } }Then the fixture setup becomes:
- f.run = intutil.CreateProwJobRun(t, dbc, f.job.ID, f.release, f.runTime, false, "F") - require.NoError(t, dbc.DB.Model(&f.run).Update("url", "https://prow/run/1").Error) - - intutil.CreateReleaseJobRun(t, dbc, f.tag.ID, f.run.ID, f.job.Name, "Blocking", "Failed", "https://prow/run/1") + const runURL = "https://prow/run/1" + f.run = intutil.CreateProwJobRunWithOptions(t, dbc, f.job.ID, f.release, f.runTime, false, "F", + intutil.WithProwJobRunURL(runURL)) + intutil.CreateReleaseJobRun(t, dbc, f.tag.ID, f.run.ID, f.job.Name, "Blocking", "Failed", runURL)🤖 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/payload_queries_test.go` around lines 204 - 207, Optionally refactor the repeated Prow run fixture setup by extending CreateProwJobRun with a URL option, such as WithProwJobRunURL, or adding a CreateProwJobRunWithURL helper. Apply it to the affected test fixtures so the URL is initialized during creation, removing the follow-up Update call and avoiding duplicated URL literals when calling CreateReleaseJobRun.
🤖 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 `@test/integration/payload_queries_test.go`:
- Around line 965-967: Guard rows[0].ReleaseTags with a length assertion before
accessing index 0 in the aggregation test, matching the existing pattern in
TestGetTestFailuresForPayloadStream_BasicAggregation; keep the current rows and
FailureCount assertions unchanged.
---
Nitpick comments:
In `@test/integration/payload_queries_test.go`:
- Around line 1271-1277: Extend CreateReleasePullRequest with WithPullRequestID
and WithBugURL fixture options, following the existing option pattern used by
CreateReleaseTag. Update the test setup to pass these options when creating the
pull request and remove the raw UPDATE statement, while preserving the existing
values and subsequent LinkReleaseTagPullRequests call.
- Around line 204-207: Optionally refactor the repeated Prow run fixture setup
by extending CreateProwJobRun with a URL option, such as WithProwJobRunURL, or
adding a CreateProwJobRunWithURL helper. Apply it to the affected test fixtures
so the URL is initialized during creation, removing the follow-up Update call
and avoiding duplicated URL literals when calling CreateReleaseJobRun.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 32d7351a-e720-41f2-9bed-ba7f9ae2e23b
📒 Files selected for processing (2)
test/integration/payload_queries_test.gotest/integration/util/fixtures.go
Add 53 integration tests covering all 11 functions in pkg/db/query/payload_queries.go, including the previously untested GetTestFailuresForPayloadStream. Tests validate business scenarios such as payload diffing, acceptance tracking, failure aggregation, phase streaks, and acceptance statistics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8619668 to
a017b4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/integration/util/fixtures.go`:
- Around line 137-152: Add a locally bounded context in each affected fixture
helper, then use dbc.DB.WithContext(ctx) for the GORM Create and
Association("PullRequests").Append calls. Apply this consistently to the helpers
corresponding to CreateReleaseTag and the fixture methods in the other specified
ranges, preserving existing assertions and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6459f4a4-66e6-4ebe-b6bc-cfc25361072e
📒 Files selected for processing (2)
test/integration/payload_queries_test.gotest/integration/util/fixtures.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/payload_queries_test.go
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@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. |
2 participants
Summary
pkg/db/query/payload_queries.goGetTestFailuresForPayloadStreamfunction (6-table join, array aggregation, 14-day window, stream/arch isolation)<vs<=), release/stream/architecture isolation, empty-result edge cases, and full column coverage forGetPayloadDifftest/integration/util/fixtures.goTest plan
gofmtpassesgo vet ./test/integration/...passesmake integrationpasses (190 tests, 0 failures)make lintpasses🤖 Generated with Claude Code
Summary by CodeRabbit