Add comprehensive test coverage for restore plugins #312
Conversation
openshift-ci
bot
added
the
approved
kaovilai
force-pushed
the
unit_tests
branch
2 times, most recently
from
10fbc8e to
c20f908
Compare
- Add test files for all restore.go files that were missing tests - Update existing test files to improve coverage: - horizontalpodautoscaler: Add Execute() tests for all scenarios - pod: Add tests for AppliesTo(), PodHasVolumesToBackUp(), and PodHasRestoreHookAnnotations() - route: Add Execute() tests for route host generation scenarios - Add documentation comments explaining where Execute() functionality is tested for plugins that only test AppliesTo() - Fix failing tests by adjusting to match implementation behavior
|
/cherry-pick oadp-1.5 oadp-1.4 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/cherry-pick oadp-1.4 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/test all |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
WalkthroughThis PR adds unit test coverage across 22 Velero restore plugins under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (5)
velero-plugins/service/restore_test.go (1)
22-98: Good coverage of service types.The table-driven test covers all four Kubernetes service types. The verification logic correctly handles the unstructured type conversion.
Consider adding a test case for a LoadBalancer service with no initial ExternalIPs to verify the plugin handles nil/empty gracefully.
{ name: "ExternalName service - keeps ExternalIPs", serviceType: corev1.ServiceTypeExternalName, externalIPs: []string{"1.2.3.4", "5.6.7.8"}, expectExternalIPsCleared: false, }, + { + name: "LoadBalancer service with no ExternalIPs", + serviceType: corev1.ServiceTypeLoadBalancer, + externalIPs: nil, + expectExternalIPsCleared: true, + }, }velero-plugins/serviceaccount/restore_test.go (1)
146-154: Consider adding assertions for secret names, not just counts.The test verifies the correct count of secrets but doesn't validate that the correct secrets remain. If the implementation has a bug that removes the wrong secret, this test would still pass.
// Check secrets outputSecrets, ok := outputObj["secrets"].([]interface{}) require.True(t, ok) assert.Len(t, outputSecrets, len(tt.expectedSecrets)) + for i, expected := range tt.expectedSecrets { + secretMap := outputSecrets[i].(map[string]interface{}) + assert.Equal(t, expected.Name, secretMap["name"]) + } // Check imagePullSecrets outputImagePullSecrets, ok := outputObj["imagePullSecrets"].([]interface{}) require.True(t, ok) assert.Len(t, outputImagePullSecrets, len(tt.expectedImagePullSecrets)) + for i, expected := range tt.expectedImagePullSecrets { + secretMap := outputImagePullSecrets[i].(map[string]interface{}) + assert.Equal(t, expected.Name, secretMap["name"]) + }velero-plugins/persistentvolume/restore_test.go (1)
80-147: Good table-driven test for storageClassName handling.The test correctly validates that storageClassName is set when PvCopyAction annotation is present and absent otherwise. The table-driven approach is clean and the assertions properly check both value and presence.
Consider adding test scenarios for:
- PV with
PvCopyActionbut noMigrateStorageClassAnnotation- PV with existing storageClassName being overwritten
This would further strengthen the test coverage, though current scenarios cover the main paths.
velero-plugins/horizontalpodautoscaler/restore_test.go (2)
7-12: Consider removing unusedexpectedScaleTargetRef(and its type import).
expectedScaleTargetRef v2beta1.CrossVersionObjectReferenceis never set or asserted in the tests, so the only effect is to keep theautoscaling/v2beta1import alive. That’s a bit misleading for readers and slightly hurts maintainability.If you don’t plan to assert on a fully-typed
CrossVersionObjectReference, consider dropping this field and thev2beta1import to keep the test focused and minimal.Also applies to: 27-27
22-164: Table-driven Execute tests look solid; consider an explicitexpectErrorflag.The scenarios cover the key behaviors of the restore plugin (DC v1→apps.openshift.io/v1, already-correct DC, Deployment, missing
scaleTargetRef, and invalid API version) and the assertions onspec.scaleTargetRef.apiVersionvs. unchanged items look good.One small maintainability tweak: instead of special-casing the error-path with
if tt.name == "HPA with invalid API version should return error", add anexpectError boolfield to the table and branch on that. It’s less brittle if names change or new error cases are added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
velero-plugins/buildconfig/restore_test.go(1 hunks)velero-plugins/clusterrolebindings/restore_test.go(1 hunks)velero-plugins/configmap/restore_test.go(1 hunks)velero-plugins/cronjob/restore_test.go(1 hunks)velero-plugins/daemonset/restore_test.go(1 hunks)velero-plugins/deployment/restore_test.go(1 hunks)velero-plugins/deploymentconfig/restore_test.go(1 hunks)velero-plugins/horizontalpodautoscaler/restore_test.go(2 hunks)velero-plugins/imagetag/restore_test.go(1 hunks)velero-plugins/job/restore_test.go(1 hunks)velero-plugins/persistentvolume/restore_test.go(1 hunks)velero-plugins/pod/restore_test.go(2 hunks)velero-plugins/pvc/restore_test.go(1 hunks)velero-plugins/replicaset/restore_test.go(1 hunks)velero-plugins/replicationcontroller/restore_test.go(1 hunks)velero-plugins/rolebindings/restore_test.go(4 hunks)velero-plugins/route/restore_test.go(2 hunks)velero-plugins/scc/restore_test.go(1 hunks)velero-plugins/secret/restore_test.go(1 hunks)velero-plugins/service/restore_test.go(1 hunks)velero-plugins/serviceaccount/restore_test.go(1 hunks)velero-plugins/statefulset/restore_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
velero-plugins/deploymentconfig/restore_test.govelero-plugins/statefulset/restore_test.govelero-plugins/daemonset/restore_test.govelero-plugins/clusterrolebindings/restore_test.govelero-plugins/pod/restore_test.govelero-plugins/horizontalpodautoscaler/restore_test.govelero-plugins/replicationcontroller/restore_test.govelero-plugins/route/restore_test.govelero-plugins/secret/restore_test.govelero-plugins/job/restore_test.govelero-plugins/pvc/restore_test.govelero-plugins/configmap/restore_test.govelero-plugins/serviceaccount/restore_test.govelero-plugins/persistentvolume/restore_test.govelero-plugins/scc/restore_test.govelero-plugins/deployment/restore_test.govelero-plugins/service/restore_test.govelero-plugins/cronjob/restore_test.govelero-plugins/buildconfig/restore_test.govelero-plugins/imagetag/restore_test.govelero-plugins/rolebindings/restore_test.govelero-plugins/replicaset/restore_test.go
🧬 Code graph analysis (12)
velero-plugins/deploymentconfig/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/daemonset/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/pod/restore_test.go (3)
velero-plugins/pod/restore.go (3)
RestorePlugin(31-34)PodHasVolumesToBackUp(44-51)PodHasRestoreHookAnnotations(61-73)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)velero-plugins/common/types.go (2)
PostRestoreHookAnnotation(111-111)InitContainerRestoreHookAnnotation(113-113)
velero-plugins/horizontalpodautoscaler/restore_test.go (2)
velero-plugins/route/restore_test.go (1)
TestRestorePluginExecute(20-148)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/secret/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/job/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/configmap/restore_test.go (2)
velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)velero-plugins/common/types.go (1)
SkipBuildConfigConfigMapRestore(29-29)
velero-plugins/scc/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/cronjob/restore_test.go (16)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)
velero-plugins/buildconfig/restore_test.go (4)
velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/imagetag/restore_test.go (17)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicaset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/util/test/test_logger.go (1)
NewLogger(10-14)
velero-plugins/replicaset/restore_test.go (16)
velero-plugins/buildconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/clusterrolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/configmap/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/cronjob/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/daemonset/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deployment/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/deploymentconfig/restore_test.go (1)
TestRestorePluginAppliesTo(12-17)velero-plugins/horizontalpodautoscaler/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/imagetag/restore_test.go (1)
TestRestorePluginAppliesTo(14-19)velero-plugins/job/restore_test.go (1)
TestRestorePluginAppliesTo(15-20)velero-plugins/persistentvolume/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/pod/restore_test.go (1)
TestRestorePluginAppliesTo(177-182)velero-plugins/pvc/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/replicationcontroller/restore_test.go (1)
TestRestorePluginAppliesTo(16-21)velero-plugins/rolebindings/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)velero-plugins/route/restore_test.go (1)
TestRestorePluginAppliesTo(13-18)
🔇 Additional comments (43)
velero-plugins/cronjob/restore_test.go (2)
12-17: LGTM! Clean test implementation.The AppliesTo test correctly validates the resource selector for cronjobs. The test structure is clear and uses appropriate assertions.
19-22: Good documentation of Execute() test coverage.The comment clearly explains where Execute() functionality is validated through component tests, which helps maintainers understand the complete test strategy.
velero-plugins/statefulset/restore_test.go (1)
12-17: LGTM! Consistent test pattern.The test correctly validates the statefulsets.apps resource selector, maintaining consistency with other plugin tests in this PR.
velero-plugins/daemonset/restore_test.go (1)
12-17: LGTM! Proper resource selector validation.The test correctly validates the daemonsets.apps resource selector with appropriate assertions.
velero-plugins/buildconfig/restore_test.go (2)
12-17: LGTM! Buildconfig resource selector test is correct.The test properly validates the buildconfigs resource selector.
19-24: Helpful documentation of test strategy.The comment provides good context about how Execute() is validated through build.UpdateCommonSpec() and related helper tests, including specific details about secret updates and image reference handling.
velero-plugins/job/restore_test.go (2)
15-20: LGTM! AppliesTo test is correct.The test properly validates the jobs resource selector.
22-89: Excellent Execute() test coverage.The table-driven test comprehensively validates the skip logic for Jobs owned by CronJobs. The three test cases correctly cover:
- Independent Jobs (should restore)
- CronJob-managed Jobs (should skip - CronJob will recreate)
- Jobs with other owners (should restore)
The test structure is clean and maintainable.
velero-plugins/replicaset/restore_test.go (1)
12-17: LGTM! Replicaset resource selector test is correct.The test properly validates the replicasets.apps resource selector.
velero-plugins/clusterrolebindings/restore_test.go (2)
12-17: LGTM! ClusterRoleBinding resource selector test is correct.The test properly validates the OpenShift-specific clusterrolebinding resource selector.
19-24: Clear documentation of shared test coverage.The comment effectively explains that Execute() behavior is validated through rolebindings tests, documenting the shared helper functions for namespace mapping.
velero-plugins/replicationcontroller/restore_test.go (2)
16-21: LGTM! AppliesTo test is correct.The test properly validates the replicationcontrollers resource selector.
23-147: Excellent Execute() test coverage with important edge cases.The table-driven test comprehensively validates the skip logic for ReplicationControllers, including the critical paused annotation handling:
- Independent RCs (should restore)
- DeploymentConfig-managed RCs (should skip - DC will recreate)
- Paused DeploymentConfig-managed RCs (should restore - DC won't recreate)
- RCs with other owners (should restore)
The test correctly uses the
common.PausedOwnerRefconstant and includes realistic object structures with container specs.velero-plugins/deploymentconfig/restore_test.go (1)
12-17: LGTM!The test correctly follows the established pattern for
AppliesTo()tests across the codebase, verifying that the plugin targets"deploymentconfigs"resources.velero-plugins/secret/restore_test.go (2)
14-19: LGTM!The
AppliesTo()test follows the established pattern, correctly verifying the plugin targets"secrets"resources.
21-78: Well-structured table-driven test.The test comprehensively covers all
SkipRestorescenarios for secrets with originating service annotations. The four test cases appropriately cover: no annotations, only the originating service annotation, other annotations, and mixed annotations.velero-plugins/deployment/restore_test.go (1)
12-17: LGTM!The test correctly uses
"deployments.apps"for the resource selector, consistent with other.appsgroup resources likedaemonsets.appsandreplicasets.apps.velero-plugins/imagetag/restore_test.go (2)
14-19: LGTM!The
AppliesTo()test correctly verifies the plugin targets"imagetags"resources.
21-38: LGTM!The test validates the expected behavior that ImageTag resources are always skipped during restore. The test setup correctly uses the OpenShift
image.openshift.io/v1API version.velero-plugins/service/restore_test.go (1)
15-20: LGTM!The
AppliesTo()test correctly verifies the plugin targets"services"resources.velero-plugins/pod/restore_test.go (4)
176-182: LGTM!The test follows the established pattern used across other plugin test files, correctly validating that the plugin targets
podsresources.
184-245: LGTM!Good test coverage for
PodHasVolumesToBackUpwith meaningful scenarios: no volumes, PVC-backed volume (should backup), and ConfigMap volume (should not backup).
247-304: LGTM!The test cases correctly cover the annotation detection logic, including nil annotations, both hook annotation types, and unrelated annotations.
306-310: LGTM!Good documentation explaining why certain functions aren't unit tested and suggesting integration tests as the appropriate approach.
velero-plugins/serviceaccount/restore_test.go (1)
15-20: LGTM!Standard AppliesTo test pattern, correctly validates the plugin targets
serviceaccounts.velero-plugins/configmap/restore_test.go (2)
14-19: LGTM!Standard AppliesTo test following the established pattern.
21-88: LGTM!Comprehensive test coverage for the Execute method, covering all meaningful annotation scenarios including edge cases like invalid values.
velero-plugins/scc/restore_test.go (2)
14-19: LGTM!Standard AppliesTo test correctly validates the plugin targets
securitycontextconstraints.
56-94: LGTM!Good test coverage for namespace mapping scenarios including service account transformation, regular users, mixed cases, and empty mapping.
velero-plugins/route/restore_test.go (3)
13-18: LGTM!Standard AppliesTo test correctly validates the plugin targets
routes.
20-148: LGTM!Comprehensive test coverage for the Execute method with well-structured test cases covering the host stripping logic based on the
openshift.io/host.generatedannotation.
150-152: LGTM!Helpful documentation explaining the testing limitations for error paths.
velero-plugins/pvc/restore_test.go (3)
16-21: LGTM! AppliesTo validation is correct.The test properly validates that the plugin targets PersistentVolumeClaim resources.
23-47: LGTM! Non-migration test coverage is solid.The test correctly validates that non-migration restores pass through unchanged with SkipRestore=false.
49-80: LGTM! Stage restore skip behavior is correctly tested.The test properly validates that PVCs with snapshot copy method are skipped during stage restore.
velero-plugins/persistentvolume/restore_test.go (3)
16-21: LGTM! AppliesTo validation is correct.The test properly validates that the plugin targets PersistentVolume resources.
23-46: LGTM! Non-migration test is correct.The test properly validates that non-migration restores return the item unchanged without skipping.
48-78: LGTM! Stage restore skip test is correct.The test properly validates that PVs with snapshot copy method are skipped during stage restore.
velero-plugins/rolebindings/restore_test.go (5)
13-18: LGTM! AppliesTo test is correct.The test properly validates that the plugin targets rolebinding resources.
20-24: Excellent documentation of test coverage approach.This comment clearly explains that Execute() functionality is tested through the helper functions, which is a valuable addition for future maintainers.
26-109: LGTM! Comprehensive test coverage for subject namespace swapping.The test cases thoroughly cover different subject kinds, successful mappings, missing mappings, and namespace-less subjects. Well-structured table-driven test.
111-150: LGTM! Thorough test coverage for UserNames namespace swapping.The test scenarios cover service account usernames, regular usernames, missing mappings, and mixed inputs. The assertions correctly validate the expected namespace transformations.
152-185: LGTM! Complete test coverage for GroupNames namespace swapping.The test scenarios appropriately cover service accounts groups, regular groups, and missing mappings. The assertions validate the correct behavior.
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, sseago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kaovilai
added
ai-generated-test
lgtm
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@kaovilai: new pull request created: #370 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@kaovilai: new pull request created: #371 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
Co-authored-by: Claude-Code