🧪 Add comprehensive unit tests for transfer-pvc command - #355
Open
RanWurmbrand wants to merge 3 commits into
Open
🧪 Add comprehensive unit tests for transfer-pvc command#355RanWurmbrand wants to merge 3 commits into
RanWurmbrand wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR expands ChangesTransfer-PVC test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Test Coverage ReportTotal: 45.3% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/transfer-pvc/transfer-pvc_test.go (1)
1696-1710: ⚡ Quick winDon’t ignore
Listerrors in verification assertions.These checks use
_ = client.List(...); if listing fails, the test can still pass on zero-length slices and mask regressions. Asserterr == nilbefore checking item counts.Suggested patch
- _ = srcClient.List(context.TODO(), podList, client.InNamespace("src-ns"), client.MatchingLabels(labels)) + if err := srcClient.List(context.TODO(), podList, client.InNamespace("src-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list source pods: %v", err) + } @@ - _ = srcClient.List(context.TODO(), cmList, client.InNamespace("src-ns"), client.MatchingLabels(labels)) + if err := srcClient.List(context.TODO(), cmList, client.InNamespace("src-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list source configmaps: %v", err) + } @@ - _ = srcClient.List(context.TODO(), secretList, client.InNamespace("src-ns"), client.MatchingLabels(labels)) + if err := srcClient.List(context.TODO(), secretList, client.InNamespace("src-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list source secrets: %v", err) + } @@ - _ = destClient.List(context.TODO(), ingressList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)) + if err := destClient.List(context.TODO(), ingressList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list destination ingresses: %v", err) + } @@ - _ = destClient.List(context.TODO(), podList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)) + if err := destClient.List(context.TODO(), podList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list destination pods: %v", err) + } @@ - _ = destClient.List(context.TODO(), routeList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)) + if err := destClient.List(context.TODO(), routeList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list destination routes: %v", err) + } @@ - _ = destClient.List(context.TODO(), podList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)) + if err := destClient.List(context.TODO(), podList, client.InNamespace("dest-ns"), client.MatchingLabels(labels)); err != nil { + t.Fatalf("failed to list destination pods: %v", err) + }As per coding guidelines, "Handle Kubernetes API errors gracefully (not found, forbidden, etc.)" and "Prefer explicit error messages with context in Go code".
Also applies to: 1760-1768, 1818-1826
🤖 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 `@cmd/transfer-pvc/transfer-pvc_test.go` around lines 1696 - 1710, The test currently ignores errors from srcClient.List when building podList, cmList, and secretList (and the similar checks at the other noted locations), which can mask failures; update each call to srcClient.List (used to populate podList, cmList, secretList) to capture the returned error and assert err == nil (use t.Fatalf or t.Fatalf-like assertion with a clear message including the resource type and namespace) before checking len(...Items), and do the same for the other occurrences referenced (around the 1760–1768 and 1818–1826 blocks) so failures in the Kubernetes API surface as test errors with context.
🤖 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 `@cmd/transfer-pvc/transfer-pvc_test.go`:
- Around line 1696-1710: The test currently ignores errors from srcClient.List
when building podList, cmList, and secretList (and the similar checks at the
other noted locations), which can mask failures; update each call to
srcClient.List (used to populate podList, cmList, secretList) to capture the
returned error and assert err == nil (use t.Fatalf or t.Fatalf-like assertion
with a clear message including the resource type and namespace) before checking
len(...Items), and do the same for the other occurrences referenced (around the
1760–1768 and 1818–1826 blocks) so failures in the Kubernetes API surface as
test errors with context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66d11575-a913-4d09-9a5f-d699d71a0e9a
📒 Files selected for processing (1)
cmd/transfer-pvc/transfer-pvc_test.go
Adds 41 unit tests covering the core functionality of the transfer-pvc command using fake Kubernetes clients for all cluster interactions. - Flag types and parsing: mappedNameVar, quantityVar, endpointType, and parseSourceDestinationMapping with valid/invalid inputs - Validation: TransferPVCCommand, EndpointFlags, and PvcFlags covering nil contexts, same-cluster, and empty field detection - PVC building: field copying, storage/class overrides, and VolumeMode/VolumeName clearing for destination binding - Rsync options: verify, restrictedContainers, and verbose flag application and removal - Route and node helpers: hostname truncation, ingress lookup, pod-to-node resolution filtering by phase - Namespace IDs: UID/GID extraction from security annotations - Cleanup: deleteResourcesIteratively label/namespace scoping and garbageCollect for both nginx and route endpoints - Resource naming: getValidatedResourceName length validation with MD5 fallback Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
Update expected hostname to use truncateWithHash instead of plain truncation, matching upstream's Route hostname collision fix (migtools#625). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
- Drop TestMappedNameVar_Set (delegates to parseSourceDestinationMapping, already thoroughly tested) - Drop TestDeleteResourcesIteratively_SuccessfulDeletion (strict subset of MultipleResourceTypes test) - Drop "succeeded pod" case from SkipsNonRunningPods (same branch as pending/failed, mixed-phase case already covers it) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
RanWurmbrand
force-pushed
the
transfer-pvc-unit-tests
branch
from
July 21, 2026 14:28
c752cb2 to
9a03be4
Compare
RanWurmbrand
changed the title
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
/rfr |
midays
requested changes
Jul 23, 2026
| Namespace: mappedNameVar{source: "src-ns", destination: "dest-ns"}, | ||
| }, | ||
| wantErr: true, | ||
| errMsg: "destnation pvc name cannot be empty", |
Contributor
There was a problem hiding this comment.
nit:fmt.Errorf("destnation pvc name cannot be empty") (missing the second "i").
| if !strings.HasPrefix(got, "crane-") { | ||
| t.Errorf("getValidatedResourceName() = %v, expected to start with 'crane-'", got) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The PR description says tests were added for getIDsForNamespace (the function that reads UID/GID info from namespace annotations), but I couldn't find a test for it anywhere in this diff
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
Adds 41 unit tests for the
transfer-pvccommand, covering validation, flag parsing, PVC construction, rsync options, cluster helpers, and resource cleanup. All tests use fake Kubernetes clients with no external dependencies.What's covered
Flag types and parsing
mappedNameVar—String(),Set(),Type()for source:destination mappingquantityVar—String(),Set(),Type()for storage quantity parsingendpointType—Set(),Type()for nginx-ingress and route validationparseSourceDestinationMapping— valid mappings, edge cases, empty inputValidation
TransferPVCCommand.Validate— nil contexts, same-cluster detection, cascading PVC and endpoint validation errors, success pathEndpointFlags.Validate— nginx requires subdomain, route does not, empty type defaults to nginxPvcFlags.Validate— empty source/destination name and namespace checksPVC building
buildDestinationPVC— copies labels, access modes, and storage requests from source; overrides storage requests and storage class from flags; clearsVolumeModeandVolumeNameso destination binds to a new PVRsync transfer options
verify— adds--checksumwhen enabled, strips--checksumand-cwhen disabled while preserving other flagsrestrictedContainers— disables privilege options and adds--omit-dir-timeswhen true; enables them when falseverbose— sets Info array and appends--progressflagRoute and node helpers
getRouteHostName— returns nil for short prefixes, truncates to 62 chars and appends ingress domain for long prefixes, errors when Ingress config is missinggetNodeNameForPVC— finds running pod with matching PVC volume, returns empty when no pods match, skips non-running podsNamespace ID parsing
getIDsForNamespace— extracts UID/GID from security annotations, returns nil fields when annotations are missing, errors on nonexistent namespaceResource cleanup
deleteResourcesIteratively— deletes by label, scoped to namespace, handles multiple resource types, no error when emptygarbageCollect— cleans up source cluster resources (Pods, ConfigMaps, Secrets) and destination endpoint resources (Ingress for nginx, Route for route)Resource name validation
getValidatedResourceName— returns original name under 63 chars, returnscrane-prefixed MD5 hash for long namesTest plan
go test -v ./cmd/transfer-pvc/...— all 41 tests passSummary by CodeRabbit