HYPERFLEET-1183 - test: Adding additional integration tests for gaps …#225
HYPERFLEET-1183 - test: Adding additional integration tests for gaps …#225ma-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 |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
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 |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1578 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
| } | ||
|
|
||
| func checkResourceCount(h *test.Helper, ids []string, expected int64) error { | ||
| dbSession := h.DBFactory.New(context.Background()) |
There was a problem hiding this comment.
Propagate test context into DB session creation to prevent unbounded hangs (CWE-400).
Line 21 creates the DB session with context.Background(), which drops caller cancellation/timeouts. Update checkResourceCount to accept ctx context.Context and pass it through from each test call site.
As per coding guidelines, "Propagate context.Context — flag context.Background() when parent context exists."
🤖 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/resource_helpers.go` at 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.
Source: Coding guidelines
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
| channel.Labels, _ = json.Marshal(labels) | ||
| createdChannel, svcErr := svc.Create(context.Background(), "Channel", channel) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(createdChannel.Labels).NotTo(BeNil()) | ||
|
|
||
| // Get the resource and verify labels persisted | ||
| retrieved, getErr := svc.Get(context.Background(), "Channel", createdChannel.ID) | ||
| Expect(getErr).To(BeNil(), "should retrieve channel") | ||
| var retrievedLabels map[string]string | ||
| json.Unmarshal(retrieved.Labels, &retrievedLabels) | ||
| Expect(retrievedLabels).To(Equal(labels), "retrieved labels should match") |
There was a problem hiding this comment.
Stop discarding JSON encode/decode errors in assertions.
json.Marshal/json.Unmarshal errors are ignored in multiple assertions, which can silently invalidate test intent and produce false positives/negatives (CWE-393). Check every returned error before comparing payloads.
As per coding guidelines, "Error handling (ERR-01 to ERR-04): every error return MUST be checked — flag silently discarded errors."
Also applies to: 309-321, 344-346
🤖 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 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.
Source: Coding guidelines
| version.Labels, _ = json.Marshal(labels) | ||
| createdVersion, svcErr := svc.Create(context.Background(), "Version", version) |
There was a problem hiding this comment.
Do not ignore json.Marshal errors in test setup.
Discarding marshal errors in setup can corrupt test preconditions and mask root causes (CWE-703). Assert the error is nil before create calls.
As per coding guidelines, "Error handling (ERR-01 to ERR-04): every error return MUST be checked — flag silently discarded errors."
Also applies to: 328-336
🤖 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 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.
Source: Coding guidelines
| for i := range 15 { | ||
| version := createTestVersion(t, svc, fmt.Sprintf("version-%d", i), channel.ID) | ||
| Expect(version.ID).ToNot(BeNil()) | ||
| } |
There was a problem hiding this comment.
Fix invalid nil-check on string ID assertion.
Line 373 asserts version.ID (string) with BeNil(), which is always wrong for non-pointer types and can fail deterministically. Use NotTo(BeEmpty()) instead.
🤖 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 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.
| expectCreateError(svc, version, 404, "wrong parent kind should fail") | ||
| }) | ||
|
|
||
| t.Run("ParentDeleted", func(t *testing.T) { |
There was a problem hiding this comment.
Is this tests duplicating WithDeletedParent?
But, does it even make sense to try to create a Version with a Deleted Channel?
Isn't it the same as the ParentNotFound one?
|
|
||
| // 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
2 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