🧪 Add unit tests for Progress type in transfer-pvc - #290
Open
RanWurmbrand wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughExpanded ChangesProgress test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 38.4% Per-package coverage
Full function-level detailsPosted by CI |
aufi
reviewed
Apr 21, 2026
aufi
left a comment
aufi
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR Ran! Overall looks good.
I'd suggest add parseRsyncLogs https://github.com/migtools/crane/blob/main/cmd/transfer-pvc/progress.go#L399 method test. The methods seems quite important to be tested regarding to its logic.
(I'm not sure about pod-related methods in the end of original file, but those might not be required)
| } | ||
| } | ||
|
|
||
| func TestProgressMerge_PercentageCapAt100(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This function looks little weird to me, but seems to be in sync with the tested source code. We might keep in mind those tests are testing currently existing code as-is, not saying the code implementation is correct or no.
- Add resetGlobals() helper for test isolation - Add tests for NewProgress, Status(), Merge(), and AsString() - Cover percentage aggregation, retry handling, and pastAttempts logic - Document variable shadowing bug on line 262 via skipped test Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
- Test writing progress to file and reading it back - Test error handling for invalid paths Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
RanWurmbrand
force-pushed
the
test/progress-core
branch
from
July 21, 2026 13:59
1ac76b7 to
8b3396b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/transfer-pvc/progress_test.go (2)
640-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.Fatalfwhen the read fails to avoid a cascading unmarshal error.If
os.ReadFilefails,datais nil and the subsequentjson.Unmarshalwill also fail, producing a second, misleading error. Stop the test at the first failure.♻️ Proposed change
data, err := os.ReadFile(tmpPath) if err != nil { - t.Errorf("Failed to read written file: %v", err) + t.Fatalf("Failed to read written file: %v", err) } var readProgress Progress if err := json.Unmarshal(data, &readProgress); err != nil { - t.Errorf("Failed to unmarshal progress: %v", err) + t.Fatalf("Failed to unmarshal progress: %v", err) }🤖 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/progress_test.go` around lines 640 - 648, Use t.Fatalf instead of t.Errorf in the os.ReadFile error branch within the progress test, so the test stops immediately when reading tmpPath fails and does not attempt json.Unmarshal with invalid data.
397-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the percentage-merge scenarios into a table-driven test.
These six functions (
PercentageAggregation,PercentageBasicUpdate,PercentageAccumulationWithPastAttempts,PercentageCapAt100,PercentageOnlyUpdateIfHigher,PercentageDontUpdateIfLower) exercise the sameMergepercentage path with only differing inputs/expectations — a natural fit for a single table-driven test.Separately,
TestProgressMerge_PercentageAggregation(Lines 411-414) only asserts non-nil. WithresetGlobals(),pastAttemptsis empty, so this reduces to mergingin=40intop=50with no accumulation — despite the name, no aggregation is exercised, and the result (50) is never asserted. Fold it in with an explicit expected value.As per coding guidelines: "Use table-driven tests for multiple scenarios in Go test code".
🤖 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/progress_test.go` around lines 397 - 545, Consolidate the six percentage tests into one table-driven test covering each input, pastAttempts setup, and expected TransferPercentage value. Preserve the existing Merge scenarios, but make the former PercentageAggregation case explicitly expect 50 and include a case that actually exercises past-attempt accumulation. Use subtests and retain nil checks before dereferencing the result.Source: Coding guidelines
🤖 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/progress_test.go`:
- Around line 640-648: Use t.Fatalf instead of t.Errorf in the os.ReadFile error
branch within the progress test, so the test stops immediately when reading
tmpPath fails and does not attempt json.Unmarshal with invalid data.
- Around line 397-545: Consolidate the six percentage tests into one
table-driven test covering each input, pastAttempts setup, and expected
TransferPercentage value. Preserve the existing Merge scenarios, but make the
former PercentageAggregation case explicitly expect 50 and include a case that
actually exercises past-attempt accumulation. Use subtests and retain nil checks
before dereferencing the result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b42afce-da39-4440-82cd-80ed36809969
📒 Files selected for processing (1)
cmd/transfer-pvc/progress_test.go
RanWurmbrand
changed the title
Contributor
Author
|
/rfr |
midays
requested changes
Jul 23, 2026
| } | ||
| p.Merge(in) | ||
| if p.TransferPercentage == nil { | ||
| t.Errorf("Merge() TransferPercentage = nil, want non-nil") |
Contributor
There was a problem hiding this comment.
This only asserts TransferPercentage != nil, not the actual value. Given Merge()'s logic (pastAttempts + incoming, capped at 100, only updates if higher than current), this test would still pass even if Merge() computed a wrong percentage or did nothing at all to it, as long as the field stayed non-nil. The name suggests it's testing aggregation specifically, but TestProgressMerge_PercentageBasicUpdate and _PercentageAccumulationWithPastAttempts right below it already do this more rigorously (asserting exact values). Worth tightening this one to assert the expected value too, or dropping it if it's redundant with those.
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
4 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.
Add unit tests for Progress type in transfer-pvc
Summary
cmd/transfer-pvc/progress.goresetGlobals()helper to isolate tests from shared global state (pastAttempts,failedFiles)What's covered
parseRsyncLogs: parsing stdout/stderr from rsync, extracting transfer stats and failed filesNewProgress: initialization with PVC nameStatus(): all status transitions (exit codes, percentages, completion states)Merge(): field merging, percentage aggregation across retries, capping at 100%, pastAttempts accumulationAsString(): output formatting with various field combinationswriteProgressToFile: JSON output to file and error handling for invalid pathsAlso documents
TestProgressAsString_WithErrors— errors never appear in output because:=shadows the outer variable(see [BUG] [transfer-pvc]Variable shadowing bug in AsString() prevents errors from appearing in output #286)
Test plan
Summary by CodeRabbit