HYPERFLEET-1183 - test: add integration tests for Channel and Version CRUD#225
Conversation
|
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:
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1216 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
|
|
||
| // Version Delete | ||
| func TestVersionDelete(t *testing.T) { | ||
| t.Run("CascadeFromChannel", func(t *testing.T) { |
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
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
|
Tip nit — non-blocking suggestion Category: Standards The PR title appears truncated — it ends with Suggested title: |
| .PHONY: coverage-clean | ||
| coverage-clean: ## Remove all coverage files | ||
| @echo "Cleaning coverage files..." | ||
| @rm -f coverage.out coverage-integration.out coverage-unfiltered.out |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Bug
coverage-unfiltered.out is never created by any target — did you mean coverage-filtered.out? The intermediate files from test-coverage and test-coverage-integration are coverage-filtered.out and coverage-integration-filtered.out. A glob is simpler and catches all variants:
| @rm -f coverage.out coverage-integration.out coverage-unfiltered.out | |
| @rm -f coverage*.out |
| _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/channels" | ||
| _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/versions" |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
These plugin imports register resource descriptors that channels_test.go also depends on. Since they're only here, channels_test.go has a hidden dependency on this file's imports. Moving them to resource_helpers.go (the shared setup file) makes the dependency explicit and resilient to file reorganization.
// resource_helpers.go
import (
_ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/channels"
_ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/versions"
)
|
|
||
| // Test helpers | ||
|
|
||
| func createChannel(t *testing.T, svc services.ResourceService, name string) *api.Resource { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
There are two channel-creation helpers in this PR with different names and APIs:
createChannel(t, svc, name)here inchannels_test.gocreateTestChannel(t, svc)inversions_test.go(auto-generates name)
Since both files are in the same integration package, consider consolidating into a single helper. createTestChannel is the more convenient API for most tests. For tests that need a specific name (like UniqueConstraint or WithOrdering), you could add an optional name parameter or keep a separate named variant.
This also affects naming consistency — the createTest* prefix is used in versions_test.go but not here.
ma-hill
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene 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-merge-bot
Bot
merged commit b429415
into
openshift-hyperfleet:main
3 participants
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