test: add tests for custom status readers #316
Conversation
📝 WalkthroughWalkthroughThis pull request introduces three new test files within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (8)
internal/kstatus/statusreaders/deployment_test.go (2)
28-51: Consider improving test data maintainabilityWhile the test data is comprehensive, consider these improvements:
- Add comments explaining the significance of specific fields in the test deployment
- Consider using heredoc syntax (``) with proper YAML indentation for better readability
Example improvement:
var ( + // currentDeployment represents a fully available deployment with: + // - 1 replica + // - All conditions met (Available and Progressing) + // - Matching generation and observedGeneration currentDeployment = strings.TrimSpace(` apiVersion: apps/v1 kind: Deployment # ... rest of the YAML with proper indentation `) )
54-87: Consider adding more test cases for comprehensive coverageThe current test cases cover the basic scenarios well, but consider adding tests for:
- Invalid deployment status conditions
- Partial availability (not all replicas ready)
- Generation mismatch scenarios
- Invalid resource version
internal/kstatus/statusreaders/replicaset_test.go (3)
28-69: Consider simplifying the test data YAMLWhile the test data is comprehensive, consider removing fields that aren't essential for testing the status reader (e.g.,
creationTimestamp,terminationMessagePath, etc.). This would make the test data more focused and easier to maintain.currentReplicaset = strings.TrimSpace(` apiVersion: apps/v1 kind: ReplicaSet metadata: labels: app: guestbook - plural.sh/managed-by: agent tier: frontend name: frontend namespace: test-do resourceVersion: "4869207" uid: 437e2329-59e4-42b9-ae40-48da3562d17e spec: replicas: 3 selector: matchLabels: tier: frontend template: metadata: - creationTimestamp: null labels: tier: frontend spec: containers: - image: us-docker.pkg.dev/google-samples/containers/gke/gb-frontend:v5 - imagePullPolicy: IfNotPresent name: php-redis - resources: {} - terminationMessagePath: /dev/termination-log - terminationMessagePolicy: File - dnsPolicy: ClusterFirst - restartPolicy: Always - schedulerName: default-scheduler - securityContext: {} - terminationGracePeriodSeconds: 30 status: availableReplicas: 3 fullyLabeledReplicas: 3 observedGeneration: 1 readyReplicas: 3 replicas: 3 `)
72-105: Consider adding more test cases for edge scenariosThe current test cases cover the basic scenarios well. Consider adding tests for:
- Invalid resource version
- Generation mismatch between spec and status
- Partial availability (when not all replicas are ready)
- Zero replicas case
Example additional test case:
"Partial availability": { identifier: object.UnstructuredToObjMetadata(testutil.YamlToUnstructured(t, partialReplicaset)), readerResource: testutil.YamlToUnstructured(t, partialReplicaset), expectedResourceStatus: &event.ResourceStatus{ Status: status.InProgressStatus, Message: "ReplicaSet is not available. Ready: 2/3 replicas", }, },
119-126: Consider using testify's require package for error assertionsFor better test failure messages, consider using
require.Errorandrequire.Equalfor error assertions:if tc.expectedErr != nil { - if err == nil { - t.Errorf("expected error, but didn't get one") - } else { - assert.EqualError(t, err, tc.expectedErr.Error()) - } + require.Error(t, err) + require.Equal(t, tc.expectedErr, err) return }internal/kstatus/statusreaders/statefulset_test.go (3)
22-101: Consider simplifying test data and improving organization.The StatefulSet manifest is quite detailed for a unit test. Consider:
- Simplifying the manifest to include only the fields necessary for testing the status reader
- Moving the test data to a separate test fixtures file (e.g.,
testdata/statefulset.yaml) for better maintainabilityExample of a simplified manifest:
apiVersion: apps/v1 kind: StatefulSet metadata: name: web namespace: test-do spec: replicas: 3 selector: matchLabels: app: nginx status: availableReplicas: 0 replicas: 1 currentReplicas: 1
103-163: Consider adding more test coverage and improving test case descriptions.The test implementation is good, but consider these improvements:
Add test cases for edge scenarios:
- StatefulSet with zero replicas
- StatefulSet with status field missing
- StatefulSet with generation mismatch
- StatefulSet with failed pods
Make test case names more descriptive, e.g.:
- "In progress resource" → "should_report_in_progress_when_replicas_not_fully_available"
- "Resource not found" → "should_report_not_found_when_statefulset_doesnt_exist"
Add more specific assertions for the status message format
Example of additional test cases:
"should_report_in_progress_when_status_missing": { identifier: object.UnstructuredToObjMetadata(testutil.YamlToUnstructured(t, statefulsetWithoutStatus)), readerResource: testutil.YamlToUnstructured(t, statefulsetWithoutStatus), expectedResourceStatus: &event.ResourceStatus{ Status: status.InProgressStatus, Message: "Status field not found", }, }, "should_report_in_progress_when_generation_mismatch": { identifier: object.UnstructuredToObjMetadata(testutil.YamlToUnstructured(t, statefulsetWithGenerationMismatch)), readerResource: testutil.YamlToUnstructured(t, statefulsetWithGenerationMismatch), expectedResourceStatus: &event.ResourceStatus{ Status: status.InProgressStatus, Message: "Generation mismatch", }, },
1-163: Consider organizing tests into test suites for better structure.Given that this is part of a larger set of status reader tests (including deployment and replicaset), consider using a test suite structure (e.g., using
suite.Suitefrom testify) to share common test utilities and fixtures across all resource reader tests. This would:
- Reduce code duplication across test files
- Provide consistent test patterns for all resource readers
- Make it easier to add tests for new resource types
Example structure:
type StatusReaderTestSuite struct { suite.Suite fakeReader *fakecr.ClusterReader fakeMapper *fakemapper.FakeRESTMapper } func (s *StatusReaderTestSuite) SetupTest() { // Common setup } func (s *StatusReaderTestSuite) TestStatefulSetReadStatus() { // Current test implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
internal/kstatus/statusreaders/deployment_test.go(1 hunks)internal/kstatus/statusreaders/replicaset_test.go(1 hunks)internal/kstatus/statusreaders/statefulset_test.go(1 hunks)
🔇 Additional comments (4)
internal/kstatus/statusreaders/deployment_test.go (2)
1-20: LGTM! Well-structured imports and package organization.
The package structure and imports follow Go best practices and include all necessary dependencies.
89-114: LGTM! Well-structured test implementation
The test implementation follows best practices:
- Uses table-driven tests for maintainability
- Properly handles error cases
- Uses appropriate test assertions
internal/kstatus/statusreaders/replicaset_test.go (1)
1-20: LGTM!
The package declaration and imports are well-organized and follow Go conventions.
internal/kstatus/statusreaders/statefulset_test.go (1)
1-20: LGTM! Package and imports are well-organized.
The package name follows Go conventions for test files, and imports are properly organized.
Summary by CodeRabbit
New Features
DeploymentResourceReader,ReplicaSetStatusReader, andStatefulSetResourceReaderfunctionalities, enhancing the reliability of resource status reading.Bug Fixes