[Review] fix: always convert singular to plural
#32
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughLegacy conversion logic modified to build plural slices by copying existing plural fields and conditionally appending singular legacy fields. Changes applied to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workflow/convert/legacy_types.go(2 hunks)workflow/convert/legacy_types_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workflow/convert/legacy_types_test.go (1)
workflow/convert/legacy_types.go (2)
LegacySynchronization(14-23)LegacyCronWorkflowSpec(102-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint
- GitHub Check: Build Binaries (cli)
- GitHub Check: Windows Unit Tests
- GitHub Check: Build Binaries (controller)
- GitHub Check: Unit Tests
- GitHub Check: argo-images (argocli)
- GitHub Check: argo-images (argoexec)
| func TestLegacyCronWorkflowSpec_ToCurrent(t *testing.T) { | ||
| t.Run("appends singular to plural", func(t *testing.T) { | ||
| lcs := &LegacyCronWorkflowSpec{ | ||
| Schedule: "0 0 * * *", | ||
| Schedules: []string{"0 * * * *", "30 * * * *"}, | ||
| } | ||
| result := lcs.ToCurrent() | ||
|
|
||
| assert.Len(t, result.Schedules, 3) | ||
| assert.Equal(t, "0 * * * *", result.Schedules[0]) | ||
| assert.Equal(t, "30 * * * *", result.Schedules[1]) | ||
| assert.Equal(t, "0 0 * * *", result.Schedules[2]) | ||
|
|
||
| // Verify no aliasing - original slice unchanged | ||
| assert.Len(t, lcs.Schedules, 2) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test cases for edge cases.
Similar to the synchronisation tests, this test lacks coverage for:
- Only plural schedules present (singular empty string)
- Only singular schedule present (plural nil/empty)
- Neither present (empty struct)
Add the following test cases:
t.Run("only plural schedules", func(t *testing.T) {
lcs := &LegacyCronWorkflowSpec{
Schedules: []string{"0 * * * *", "30 * * * *"},
}
result := lcs.ToCurrent()
assert.Len(t, result.Schedules, 2)
assert.Equal(t, "0 * * * *", result.Schedules[0])
assert.Equal(t, "30 * * * *", result.Schedules[1])
})
t.Run("only singular schedule", func(t *testing.T) {
lcs := &LegacyCronWorkflowSpec{
Schedule: "0 0 * * *",
}
result := lcs.ToCurrent()
assert.Len(t, result.Schedules, 1)
assert.Equal(t, "0 0 * * *", result.Schedules[0])
})
t.Run("empty schedules", func(t *testing.T) {
lcs := &LegacyCronWorkflowSpec{}
result := lcs.ToCurrent()
assert.Nil(t, result.Schedules)
})🤖 Prompt for AI Agents
In workflow/convert/legacy_types_test.go around lines 51 to 67, the
TestLegacyCronWorkflowSpec_ToCurrent test covers only the case where both
Schedule and Schedules are set; add three additional subtests to cover edge
cases: (1) "only plural schedules" where Schedule is empty and Schedules
contains two entries — assert result.Schedules has length 2 and entries
preserved, (2) "only singular schedule" where Schedules is nil/empty and
Schedule is set — assert result.Schedules has length 1 with the singular value,
and (3) "empty schedules" where both fields are empty — assert result.Schedules
is nil; implement these t.Run subtests in the same style as existing assertions
to ensure full coverage and no aliasing.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
Signed-off-by: Alan Clucas <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workflow/convert/legacy_types_test.go (1)
92-127: Good coverage, consider adding nil receiver test for consistency.The tests cover all the key scenarios for schedule conversion. For consistency with
TestLegacySynchronization_ToCurrent, consider adding a nil receiver test case.func TestLegacyCronWorkflowSpec_ToCurrent(t *testing.T) { + t.Run("nil receiver", func(t *testing.T) { + var lcs *LegacyCronWorkflowSpec + // Note: This would panic - if nil handling is needed, + // add a nil check to LegacyCronWorkflowSpec.ToCurrent() + }) + t.Run("neither", func(t *testing.T) {Note: If
LegacyCronWorkflowSpec.ToCurrent()should support nil receivers (likeLegacySynchronization.ToCurrent()does), the method would need a nil check added. Otherwise, this is a nitpick for test symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workflow/convert/legacy_types.go(2 hunks)workflow/convert/legacy_types_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workflow/convert/legacy_types_test.go (1)
workflow/convert/legacy_types.go (2)
LegacySynchronization(14-23)LegacyCronWorkflowSpec(100-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: argo-images (argocli)
- GitHub Check: argo-images (argoexec)
🔇 Additional comments (4)
workflow/convert/legacy_types.go (2)
31-45: LGTM! Correctly preserves plural fields and appends singular.The implementation now properly handles all cases by always copying the plural slice first, then conditionally appending the singular field. This addresses the data loss bug from the previous implementation where plural fields were discarded when singular fields were absent.
117-126: LGTM! Schedule conversion follows the same correct pattern.The schedules conversion mirrors the synchronisation logic correctly: copy the plural slice first, then append the singular schedule if non-empty. This ensures plural-only schedules are preserved.
workflow/convert/legacy_types_test.go (2)
1-10: Good test structure with appropriate imports.The test file correctly imports the required testing framework (testify/assert) and Argo Workflows types needed for comprehensive testing.
12-90: Excellent test coverage for LegacySynchronization.ToCurrent().The tests comprehensively cover all edge cases previously flagged in reviews:
- Nil receiver handling
- Empty struct (neither singular nor plural)
- Only singular fields
- Only plural fields
- Combined fields with aliasing verification
The aliasing test at lines 86-88 correctly verifies that the original slices are not mutated after conversion.
Signed-off-by: Alan Clucas <[email protected]>
Duplicate of argoproj#15092 for CodeRabbit review.
In argoproj#14996 the singular version of each of
semaphore,mutexandschedulewould not get moved over to the plural in the presence of both fields.This is not really correct as if you have both sets in 3.7 you will get the behaviour of the appended list+singular. So this PR makes convert append instead of only replace.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.