HYPERFLEET-1183 - test: Adding additional integration tests for gaps …#225
Open
ma-hill wants to merge 1 commit into
Open
HYPERFLEET-1183 - test: Adding additional integration tests for gaps …#225ma-hill wants to merge 1 commit into
ma-hill wants to merge 1 commit into
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
4 tasks
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1211 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/factories/channels.go`:
- Around line 50-74: The NewChannelWithLabels factory function is directly
mutating channel labels via database writes instead of using the
resources.Service validation and patch logic, which can cause integration tests
to miss label-handling regressions. Replace the direct database update calls
(the dbSession.Model and Update operations) with a call to the appropriate
resources.Service method that properly validates and patches labels, ensuring
the factory follows the same code path as production usage.
In `@test/integration/channels_test.go`:
- Around line 64-67: The test assertion in the channel creation block expects a
500 Internal Server Error when creating a channel with an empty Spec field, but
empty Spec is malformed client input that should be validated at the system
boundary and return a 400 Bad Request error instead. Modify the assertion that
checks svcErr.HTTPCode to expect 400 (Equal(400)) rather than 500, and update
the corresponding assertion message to reflect that this is a client input
validation error rather than an internal server error.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca5b332c-c51a-45b9-97dd-1fc73eb0ab05
📒 Files selected for processing (9)
.gitignoreMakefilepkg/services/resource_test.gotest/factories/channels.gotest/factories/versions.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- test/integration/resource_delete_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/factories/channels.go`:
- Around line 25-28: The Create method call for the Channel resource is
returning raw errors without wrapping them with call-site context. Replace the
bare return err statements (both the one after resourceService.Create and the
similar one mentioned at lines 37-40) with wrapped errors that include
contextual information about the channel being created, such as the channel ID
or index. Use Go's error wrapping pattern with fmt.Errorf and the %w verb to
maintain the original error while adding descriptive context that will help with
debugging integration setup failures.
In `@test/integration/channels_test.go`:
- Around line 409-410: The test on line 409 discards the resource returned by
the Create method call and instead relies on implicit mutation of the input
defaultChannel object. This is problematic because if Create does not mutate the
input pointer to populate the ID field, the later comparison at lines 434-439
will fail when it tries to use defaultChannel.ID. Fix this by capturing the
returned resource from the svc.Create call into a variable and use that returned
resource's ID field for the comparison instead of relying on the input parameter
being mutated.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e5863093-4982-46ca-a26d-c14ec24e604c
📒 Files selected for processing (9)
.gitignoreMakefilepkg/services/resource_test.gotest/factories/channels.gotest/factories/versions.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- test/integration/resource_delete_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/services/resource_test.go
- test/factories/versions.go
- test/integration/versions_test.go
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/resource_helpers.go`:
- Line 21: The checkResourceCount function creates a DB session using
context.Background() at line 21, which prevents propagation of test context and
can cause unbounded hangs. Add a ctx context.Context parameter to the
checkResourceCount function signature, then replace the context.Background()
call in the dbSession initialization with the passed ctx parameter. Update all
test call sites that invoke checkResourceCount to pass the appropriate test
context (typically the test's *testing.T context or a timeout-bounded context)
instead of relying on the background context.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 942ba138-7a68-4df5-9c3a-9d42a25d9704
📒 Files selected for processing (7)
.gitignoreMakefilepkg/services/resource_test.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- test/integration/resource_delete_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/services/resource_test.go
- test/integration/channels_test.go
- test/integration/versions_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/channels_test.go`:
- Around line 70-80: The json.Marshal and json.Unmarshal operations in the test
are discarding their error return values, which can hide encoding/decoding
failures and produce false test results. Replace the underscore placeholder in
the json.Marshal call that sets channel.Labels with an actual error variable and
add an Expect assertion to verify the error is nil. Similarly, capture the error
returned by the json.Unmarshal call when unmarshaling into retrievedLabels
(instead of ignoring it) and add an Expect assertion to verify that error is
also nil before proceeding with the label comparison assertion.
In `@test/integration/versions_test.go`:
- Around line 371-374: The assertion on the version.ID in the test loop is
incorrectly using BeNil() on a string type, which cannot be nil. In the
createTestVersion function call loop where version.ID is being checked, replace
Expect(version.ID).ToNot(BeNil()) with Expect(version.ID).ToNot(BeEmpty()) to
properly assert that the string ID is not empty rather than attempting an
invalid nil-check on a non-pointer type.
- Around line 152-153: The json.Marshal call in the test setup is ignoring its
error return value by using the blank identifier, which violates error handling
guidelines and can corrupt test preconditions. Capture the error returned by
json.Marshal(labels) instead of discarding it with underscore, then add an
assertion to verify the error is nil before proceeding with the svc.Create call.
Apply the same fix to all other occurrences of this pattern in the file (also at
lines 328-336) where errors are being silently ignored.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f3b513d9-3d45-4c16-84ee-7cd97cbd506e
📒 Files selected for processing (7)
.gitignoreMakefilepkg/services/resource_test.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- test/integration/resource_delete_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/resource_test.go
rh-amarin
reviewed
Jun 19, 2026
rh-amarin
reviewed
Jun 19, 2026
|
|
||
| // Version Delete | ||
| func TestVersionDelete(t *testing.T) { | ||
| t.Run("CascadeFromChannel", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
I think this test is a bit misleading.
Reading the name CascadeFromChannel I would expect that the OnParentDeletePolicy for Versions should be cascade and that we don't need to manually delete the Versions like is happening here
Contributor
Author
There was a problem hiding this comment.
Kinda over thought out these tests I think
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/channels_test.go`:
- Around line 18-25: The test helper function createChannel is missing the
t.Helper() call at the beginning. Add t.Helper() as the very first statement in
the createChannel function body to ensure test failure messages correctly point
to the call site of the helper rather than inside the helper function itself.
In `@test/integration/versions_test.go`:
- Around line 19-30: Add t.Helper() as the first statement in each test helper
function to properly attribute test failures. In the createTestChannel function
starting at line 19, and any other test helper functions defined at lines 32-39
and 41-46, insert a t.Helper() call immediately after the function signature
before any other logic to ensure failures are correctly attributed to the
calling test function.
- Around line 534-550: The UpdatesTimestamps test is timing-flaky because the
assertion on patched.UpdatedTime uses a strict greater-than comparison with
BeTemporally(">", originalUpdatedTime), but due to coarse timestamp precision
the create and patch operations can occur within the same timestamp unit,
causing the test to intermittently fail. Change the temporal comparison operator
from ">" to ">=" in the BeTemporally assertion to allow for equal timestamps,
which accounts for systems where timestamp precision may be coarse enough that
both operations complete within the same time unit.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83e113d2-6dcd-4486-8110-a94154b63591
📒 Files selected for processing (7)
.gitignoreMakefilepkg/services/resource_test.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- test/integration/resource_delete_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/resource_test.go
Comment on lines
+18
to
+25
| func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource { | ||
| channel := newChannelResource(name) | ||
| created, err := svc.Create(t.Context(), "Channel", channel) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create channel: %v", err) | ||
| } | ||
| return created | ||
| } |
There was a problem hiding this comment.
Add t.Helper() in the test helper (CWE-398).
Line 18 defines a helper without t.Helper(), so failures point to the helper instead of the call site.
Suggested fix
func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource {
+ t.Helper()
channel := newChannelResource(name)
created, err := svc.Create(t.Context(), "Channel", channel)As per coding guidelines, "t.Helper() MUST be called in test helper functions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource { | |
| channel := newChannelResource(name) | |
| created, err := svc.Create(t.Context(), "Channel", channel) | |
| if err != nil { | |
| t.Fatalf("Failed to create channel: %v", err) | |
| } | |
| return created | |
| } | |
| func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource { | |
| t.Helper() | |
| channel := newChannelResource(name) | |
| created, err := svc.Create(t.Context(), "Channel", channel) | |
| if err != nil { | |
| t.Fatalf("Failed to create channel: %v", err) | |
| } | |
| return created | |
| } |
🤖 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/channels_test.go` around lines 18 - 25, The test helper
function createChannel is missing the t.Helper() call at the beginning. Add
t.Helper() as the very first statement in the createChannel function body to
ensure test failure messages correctly point to the call site of the helper
rather than inside the helper function itself.
Source: Coding guidelines
Comment on lines
+19
to
+30
| func createTestChannel(t *testing.T, svc services.ResourceService) *api.Resource { | ||
| // unique string for channel name | ||
| uniqueSuffix := uuid.NewString()[:8] | ||
| channelName := fmt.Sprintf("channel-%s", uniqueSuffix) | ||
|
|
||
| channel := newChannelResource(channelName) | ||
| created, err := svc.Create(t.Context(), "Channel", channel) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create channel: %v", err) | ||
| } | ||
| return created | ||
| } |
There was a problem hiding this comment.
Mark all local test helpers with t.Helper() (CWE-398).
Line 19, Line 32, and Line 41 define helper functions without t.Helper(), which degrades failure attribution.
Suggested fix
func createTestChannel(t *testing.T, svc services.ResourceService) *api.Resource {
+ t.Helper()
// unique string for channel name
uniqueSuffix := uuid.NewString()[:8]
@@
func createTestVersion(t *testing.T, svc services.ResourceService, name, channelID string) *api.Resource {
+ t.Helper()
version := newVersionResource(name, channelID)
@@
func expectCreateError(t *testing.T, svc services.ResourceService,
resource *api.Resource, expectedCode int, msg string) {
+ t.Helper()
_, svcErr := svc.Create(t.Context(), resource.Kind, resource)As per coding guidelines, "t.Helper() MUST be called in test helper functions."
Also applies to: 32-39, 41-46
🤖 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/versions_test.go` around lines 19 - 30, Add t.Helper() as
the first statement in each test helper function to properly attribute test
failures. In the createTestChannel function starting at line 19, and any other
test helper functions defined at lines 32-39 and 41-46, insert a t.Helper() call
immediately after the function signature before any other logic to ensure
failures are correctly attributed to the calling test function.
Source: Coding guidelines
Comment on lines
+534
to
+550
| t.Run("UpdatesTimestamps", func(t *testing.T) { | ||
| svc, _ := setupResourceTest(t) | ||
|
|
||
| channel := createTestChannel(t, svc) | ||
| version := createTestVersion(t, svc, "timestamp-update-test", channel.ID) | ||
| originalUpdatedTime := version.UpdatedTime | ||
|
|
||
| // Patch the version | ||
| newSpec := map[string]any{"enabled": false} | ||
| req := &api.ResourcePatchRequest{ | ||
| Spec: &newSpec, | ||
| } | ||
| patched, svcErr := svc.Patch(t.Context(), "Version", version.ID, req) | ||
|
|
||
| Expect(svcErr).To(BeNil()) | ||
| Expect(patched.UpdatedTime).To(BeTemporally(">", originalUpdatedTime), | ||
| "updated_time should be later than original after patch") |
There was a problem hiding this comment.
UpdatedTime assertion is timing-flaky as written (CWE-754).
Line 549 requires strict > immediately after create; with coarse timestamp precision, create and patch can share the same value and intermittently fail.
Suggested fix
originalUpdatedTime := version.UpdatedTime
+ time.Sleep(5 * time.Millisecond)
@@
Expect(svcErr).To(BeNil())
Expect(patched.UpdatedTime).To(BeTemporally(">", originalUpdatedTime),
"updated_time should be later than original after patch")🤖 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/versions_test.go` around lines 534 - 550, The
UpdatesTimestamps test is timing-flaky because the assertion on
patched.UpdatedTime uses a strict greater-than comparison with BeTemporally(">",
originalUpdatedTime), but due to coarse timestamp precision the create and patch
operations can occur within the same timestamp unit, causing the test to
intermittently fail. Change the temporal comparison operator from ">" to ">=" in
the BeTemporally assertion to allow for equal timestamps, which accounts for
systems where timestamp precision may be coarse enough that both operations
complete within the same time unit.
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
2 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
Refactors integration tests for Channels and Versions to use organized subtests with helper functions, reducing test boilerplate by 60-80%. Previously, tests were scattered across multiple files with significant duplication and inconsistent patterns. This consolidates all Channel and Version CRUD operations into two well-structured test files using
t.Run()subtests, making tests easier to read, maintain, and debug.HYPERFLEET-1183
Changes
Added
test/integration/channels_test.gowith comprehensive test coverage organized into 5 test groups:TestChannelCreate(4 subtests): UniqueConstraint, EmptyName, WithLabels, SetsTimestampsTestChannelDelete(4 subtests): NotFound, SetsDeletedTime, ReDeleteReturns404, HardDeleteRemovesRowTestChannelList(3 subtests): Basic, Pagination, WithOrderingTestChannelGet(2 subtests): NotFound, SuccessTestChannelPatch(4 subtests): NotFound, SpecChanged, LabelsOnly, UpdatesTimestampsAdded
test/integration/versions_test.gowith comprehensive test coverage organized into 5 test groups:TestVersionCreate(9 subtests): UniqueConstraintPerChannel, SameVersionNameInDifferentChannels, EmptyName, WrongParentKind, ParentDeleted, ParentNotFound, WithDeletedParent, WithLabels, SetsTimestampsTestVersionDelete(6 subtests): CascadeFromChannel, NotFound, SetsDeletedTime, ReDeleteReturns404, HardDeleteRemovesRow, RestrictBlocksWithActiveParentTestVersionList(4 subtests): ListByOwnerID, ListByLabel, Pagination, ByOwnerTestVersionGet(4 subtests): ByOwnerWrongParent, NotFound, ByOwnerNotFound, ByOwnerSuccessTestVersionPatch(4 subtests): NotFound, SpecChanged, LabelsOnly, UpdatesTimestampsRemoved
test/integration/resource_delete_test.goas delete tests are now organized within resource-specific test filesUpdated
.gitignoreto exclude test output and temporary filesUpdated
Makefilewith additional test coverage targetsAdded
pkg/services/resource_test.gowith unit tests for List validation logicNotes
All tests use UUID-based unique naming to prevent conflicts when running in parallel or against shared test databases. The helper functions follow the pattern established in the existing codebase where setup uses
t.Fatalf()for fatal errors and test assertions use GomegaExpect()matchers.Tests now properly validate through the service layer rather than directly mutating the database, ensuring all business logic and validation rules are exercised.
In conjunction with change: openshift-hyperfleet/hyperfleet-e2e#129
Test Plan
make test-allpassesmake lintpassesmake test-coverageshows improved coverage for Channel and Version operations