PO to GMP Migration Tool: Unit Test Cleanup - #2059
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and consolidates several test suites in the pkg/migrate package into table-driven tests, specifically in helpers_test.go and migrate_test.go. While the refactoring simplifies the test structure, the review feedback highlights several regressions in test coverage due to the removal of critical test cases, including corrupted secret data handling, ConfigMap-to-Secret deduplication, cluster-scoped ClusterPodMonitoring logic, and static target label conversion. Additionally, the reviewer notes that errors from os.WriteFile are ignored in the refactored tests and should be properly handled.
| } | ||
|
|
||
| func TestExtractSecretKey(t *testing.T) { | ||
| func TestExtractResourceKey(t *testing.T) { |
There was a problem hiding this comment.
This refactoring removed the test case Secret exists but data corrupted from the old TestExtractSecretKey function. This test case was valuable as it verified the error handling for malformed base64 data within a Secret, which is an important edge case. Its removal reduces test coverage. Please consider re-adding this test case to the new TestExtractResourceKey table.
References
- When a resource converter or migration tool encounters an invalid, empty, or broken configuration that prevents successful conversion, it should return an explicit error rather than silently skipping the resource or returning an empty result.
| } | ||
| } | ||
|
|
||
| func TestConvertConfigMapToSecretSelectorDeduplication(t *testing.T) { |
There was a problem hiding this comment.
The removed test TestConvertConfigMapToSecretSelectorDeduplication verified an important behavior: that multiple calls to convertConfigMapToSecretSelector for the same ConfigMap would result in only one Secret being generated (deduplication). The new table-driven tests for TestConvertConfigMapToSecretSelector run in isolation and do not cover this scenario. This is a regression in test coverage. Please add a new test case that verifies this deduplication logic.
References
- Keep the input resource cache separate from generated output resources. Use an output map at the migrator level to detect collisions and handle deduplication of generated resources (such as generated Secrets).
| _ = os.WriteFile(p1, []byte(svcYAML), 0644) | ||
| _ = os.WriteFile(p2, []byte(pmYAML), 0644) |
There was a problem hiding this comment.
The errors returned by os.WriteFile are being ignored here. While this is in a test, it's best practice to handle these errors. If writing the test files fails, the test will fail later with a less clear message. Please check the errors and fail the test immediately using t.Fatalf for better test reliability and clearer failure reporting, as is done in other test cases in this file.
if err := os.WriteFile(p1, []byte(svcYAML), 0644); err != nil {
t.Fatalf("failed to write service file: %v", err)
}
if err := os.WriteFile(p2, []byte(pmYAML), 0644); err != nil {
t.Fatalf("failed to write podmonitor file: %v", err)
}| }, | ||
| }, | ||
| { | ||
| name: "Scope-aware Metadata: namespace metadata mapping dropped in namespaced PodMonitoring", |
There was a problem hiding this comment.
This PR removes several test cases for ClusterPodMonitoring, stating they are redundant with the namespaced PodMonitoring tests. However, some helper functions like resolveMetadata and resolveAttachMetadata have distinct logic paths for cluster-scoped resources (when isCluster is true). Removing all tests for ClusterPodMonitoring leaves these specific paths untested, which is a regression. Please consider re-instating at least one key test case for ClusterPodMonitoring to ensure the cluster-scoped logic is still covered, for example, a test that verifies namespace metadata handling.
| return addServiceWithSelectorToCache(cache, namespace, name, labels, selector, ports) | ||
| } | ||
|
|
||
| // TestConvertStaticTargetLabels tests that label keys are sanitized and protected labels are renamed to exported_<label>. |
There was a problem hiding this comment.
The test TestConvertStaticTargetLabels has been removed. The PR description mentions it's covered in helpers_test.go, but a replacement test doesn't seem to exist there. This unit test was valuable as it directly tested the convertStaticTargetLabels helper function, including important edge cases like label sanitization and collisions. The integration-style tests in TestServiceMonitorConverter_Convert may not cover all these specific scenarios. Please consider re-adding this unit test to prevent regressions.
a9b5efb to
0ca8b33
Compare
1 participant
This PR refactors the unit test suite across
pkg/migrateto adhere strictly to table-driven testing standards, consolidating overlapping test suites and removing duplicate assertions.Key Consolidations & Changes
TestExtractSecretKeyandTestExtractConfigMapKeyinto a single, unifiedTestExtractResourceKeytable test.TestConvertConfigMapToSecretSelectorDeduplicationandTestDecoupledNamespacesintoTestExtractResourceKeyandTestMergeFromPodtable cases.ClusterPodMonitoringtest cases inpodmonitor_test.gothat duplicatedconvertMonitorSpecassertions already verified in namespaced test cases.TestConvertStaticTargetLabelsfromservicemonitor_test.go(covered byhelpers_test.go).migrate_test.gopipeline tests (Migrator.Run, directory scanning, error reporting) into table-driven cases.