🧪 Add unit tests for transfer-pvc Progress and dataSize types & fixed typo - #356
Open
RanWurmbrand wants to merge 3 commits into
Open
🧪 Add unit tests for transfer-pvc Progress and dataSize types & fixed typo#356RanWurmbrand wants to merge 3 commits into
RanWurmbrand wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds test helpers and a tolerant comparator, many new and expanded tests exercising Progress construction, merge logic, file I/O, and dataSize parsing/formatting, and fixes rsync retry regex in parseRsyncLogs. ChangesProgress Package Test Enhancement and Public API Addition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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: 42.5% 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/transfer-pvc/progress_test.go (1)
212-735: 🏗️ Heavy liftConsolidate repeated scenario tests into table-driven tests
The
TestProgressStatus_*,TestProgressMerge_*, andTestProgressAsString_*groups contain 22 individual test functions, each testing a single scenario. These should be consolidated into table-driven tests per the coding guideline: "Use table-driven tests for multiple scenarios in Go test code."Examples:
TestProgressStatus_*: 6 functions testing different status conditions (exit codes, percentages)TestProgressMerge_*: 10 functions testing different merge scenariosTestProgressAsString_*: 6 functions testing different output formatting casesTable-driven consolidation will improve maintainability, reduce duplication, and align with repo standards.
🤖 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 212 - 735, Multiple single-scenario tests for Progress.Status, Progress.Merge, and Progress.AsString should be consolidated into table-driven tests to reduce duplication; replace the 6 TestProgressStatus_* functions (e.g., TestProgressStatus_ExitCodeZero, TestProgressStatus_PercentageLessThan100), the ~10 TestProgressMerge_* functions (e.g., TestProgressMerge_BasicFields, TestProgressMerge_PercentageAccumulationWithPastAttempts, TestProgressMerge_DataSizeAggregation), and the TestProgressAsString_* functions (e.g., TestProgressAsString_BasicFormat, TestProgressAsString_WithAllFields, TestProgressAsString_WithErrors) with three table-driven tests: one table for Status cases, one for Merge cases, and one for AsString cases; for each table define a slice of test cases (name, input Progress, expected values/state), iterate with t.Run, call resetGlobals() per subtest, run the target method (Status, Merge using a copy/inputs, or AsString), and assert expected outcomes using descriptive t.Run names, preserving special behaviors like the skipped TestProgressAsString_WithErrors case (keep as a skipped table entry) and any mutable globals like pastAttempts and retries by initializing them inside the subtest setup.
🤖 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 `@cmd/transfer-pvc/progress_test.go`:
- Around line 726-734: The test TestWriteProgressToFile_InvalidPath should avoid
relying on a platform-specific non-existent path; change it to pass a directory
path to writeProgressToFile to deterministically trigger a file-write error. Use
t.TempDir() (or create a temp dir) and pass that directory string (not a file
inside it) as the output path when calling writeProgressToFile so the call fails
consistently; keep the same Progress struct setup and the existing error
assertion. Reference: TestWriteProgressToFile_InvalidPath and
writeProgressToFile.
- Around line 318-336: The test TestProgressMerge_PercentageAggregation
currently only checks that p.TransferPercentage is non-nil; update it to assert
the actual aggregated percentage after calling Progress.Merge(in). Specifically,
after p.Merge(in) dereference p.TransferPercentage and compare it to the
expected aggregated value (e.g., for pct1=50 and pct2=40 expect 45 if Merge
averages percentages) and fail the test if the value is not equal; reference the
Progress.Merge method and the TransferPercentage field when locating where to
add the assertion.
---
Nitpick comments:
In `@cmd/transfer-pvc/progress_test.go`:
- Around line 212-735: Multiple single-scenario tests for Progress.Status,
Progress.Merge, and Progress.AsString should be consolidated into table-driven
tests to reduce duplication; replace the 6 TestProgressStatus_* functions (e.g.,
TestProgressStatus_ExitCodeZero, TestProgressStatus_PercentageLessThan100), the
~10 TestProgressMerge_* functions (e.g., TestProgressMerge_BasicFields,
TestProgressMerge_PercentageAccumulationWithPastAttempts,
TestProgressMerge_DataSizeAggregation), and the TestProgressAsString_* functions
(e.g., TestProgressAsString_BasicFormat, TestProgressAsString_WithAllFields,
TestProgressAsString_WithErrors) with three table-driven tests: one table for
Status cases, one for Merge cases, and one for AsString cases; for each table
define a slice of test cases (name, input Progress, expected values/state),
iterate with t.Run, call resetGlobals() per subtest, run the target method
(Status, Merge using a copy/inputs, or AsString), and assert expected outcomes
using descriptive t.Run names, preserving special behaviors like the skipped
TestProgressAsString_WithErrors case (keep as a skipped table entry) and any
mutable globals like pastAttempts and retries by initializing them inside the
subtest setup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ea2a734-5788-4d6c-a945-5109fed63cef
📒 Files selected for processing (1)
cmd/transfer-pvc/progress_test.go
Comment on lines
+726
to
+734
| func TestWriteProgressToFile_InvalidPath(t *testing.T) { | ||
| resetGlobals() | ||
| p := &Progress{ | ||
| TransferredFiles: 100, | ||
| } | ||
| err := writeProgressToFile("/nonexistent/directory/file.json", p) | ||
| if err == nil { | ||
| t.Errorf("writeProgressToFile() with invalid path should return error") | ||
| } |
There was a problem hiding this comment.
Make the invalid-path test deterministic across environments
Line 731 relies on "/nonexistent/directory/file.json", which is environment-specific. A safer negative case is passing a directory path as the output file path.
Suggested fix
func TestWriteProgressToFile_InvalidPath(t *testing.T) {
resetGlobals()
p := &Progress{
TransferredFiles: 100,
}
- err := writeProgressToFile("/nonexistent/directory/file.json", p)
+ err := writeProgressToFile(t.TempDir(), p) // directory path, not file path
if err == nil {
t.Errorf("writeProgressToFile() with invalid path should return error")
}
}🤖 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 726 - 734, The test
TestWriteProgressToFile_InvalidPath should avoid relying on a platform-specific
non-existent path; change it to pass a directory path to writeProgressToFile to
deterministically trigger a file-write error. Use t.TempDir() (or create a temp
dir) and pass that directory string (not a file inside it) as the output path
when calling writeProgressToFile so the call fails consistently; keep the same
Progress struct setup and the existing error assertion. Reference:
TestWriteProgressToFile_InvalidPath and writeProgressToFile.
RanWurmbrand
force-pushed
the
progress-test-unit-tests
branch
from
May 11, 2026 11:43
6b7e1f7 to
d61af4e
Compare
RanWurmbrand
changed the title
Adds 35 unit tests covering the dataSize type, Progress type, status resolution, merge logic, string formatting, and file I/O. Updates dataEqual helper to support tolerance-based comparison. All tests use in-memory state with no external dependencies. - dataSize: parsing with/without units, decimal values, addition with same/different units, nil input, zero values, String(), MarshalJSON() - dataEqual: updated signature to accept tolerance parameter, switched from exact equality to math.Abs comparison - NewProgress: initializes PVC, empty slices, and timestamp - Status: exit code 0 (succeeded), non-zero with/without data (failed/partiallyFailed), nil exit code (preparing), percentage >= 100 (finishingUp), percentage < 100 (transferInProgress) - Merge: basic field accumulation, percentage aggregation with past attempts, capping at 100, only-update-if-higher guard, don't-update-if-lower guard, retry reset with pastAttempts, data size aggregation, error and failed file appending - AsString: basic format, all fields populated, nil fields, completed output, failed files output - AsString error output test skipped due to variable shadowing bug - writeProgressToFile: valid write and invalid path error Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
…ern didn't match the actual retry message from backube/pvc-transfer, so retries were never detected. Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
- Drop TestProgressMerge_PercentageAggregation (subsumed by other percentage merge tests that assert exact values) - Merge TestNewDataSize_ParseDecimalValues unique cases into TestNewDataSize_ParseWithUnit and remove the duplicate test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
RanWurmbrand
force-pushed
the
progress-test-unit-tests
branch
from
July 21, 2026 14:46
d61af4e to
5a2d6e5
Compare
RanWurmbrand
changed the title
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
/rfr |
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 comprehensive unit test coverage for the
ProgressanddataSizetypes intransfer-pvc. These types handle rsync transfer tracking and had minimal test coverage before.What's covered
Progress type:
NewProgressinitializationStatus()method across all states (succeeded, failed, partiallyFailed, preparing, finishingUp, transferInProgress)Merge()logic including percentage aggregation, retry handling, and data accumulationAsString()output formatting for various scenariosdataSize type:
Helpers:
dataEqualto support tolerance-based comparison for floating point valuesNotes
TestProgressAsString_WithErrors) is skipped due to a known variable shadowing bug on line 262 of progress.go (see [BUG] [transfer-pvc]Variable shadowing bug in AsString() prevents errors from appearing in output #286)Summary by CodeRabbit
Tests
New Features