Skip to content

Revert "test: use maps.Clone in TestPrepareMountArgs to prevent mutating shar…"#1401

Open
chrisThePattyEater wants to merge 2 commits into
mainfrom
revert-1398-fix-test-flakiness-main
Open

Revert "test: use maps.Clone in TestPrepareMountArgs to prevent mutating shar…"#1401
chrisThePattyEater wants to merge 2 commits into
mainfrom
revert-1398-fix-test-flakiness-main

Conversation

@chrisThePattyEater
Copy link
Copy Markdown
Collaborator

Reverts #1398

@google-oss-prow
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chrisThePattyEater

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors test setups. In injection_test.go, it switches from directly adding objects to the informer's store to creating them via a fake client and waiting for cache synchronization. In sidecar_mounter_config_test.go, it removes the use of maps.Clone and directly mutates tc.expectedArgs. However, feedback points out that this direct mutation re-introduces a test pollution bug because tc.expectedArgs references a shared global map, and suggests manually copying the map instead.

Comment on lines 529 to 539
found := slices.Contains(tc.mc.Options, util.DisableMetricsForGKE+":true")
if !found {
expectedArgs["prometheus-port"] = strconv.Itoa(testPrometheusPort)
tc.expectedArgs["prometheus-port"] = strconv.Itoa(testPrometheusPort)
}
// Increase port value to match behavior of prepareMountArgs()
testPrometheusPort++

tc.mc.prepareMountArgs()
if !reflect.DeepEqual(tc.mc.FlagMap, expectedArgs) {
t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, expectedArgs)
if !reflect.DeepEqual(tc.mc.FlagMap, tc.expectedArgs) {
t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, tc.expectedArgs)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Reverting the use of maps.Clone re-introduces a test pollution bug. tc.expectedArgs points to the shared global map defaultFlagMap for multiple test cases. Mutating tc.expectedArgs directly via tc.expectedArgs["prometheus-port"] = ... mutates defaultFlagMap for all subsequent test cases that reference it.

To prevent this test pollution without relying on the maps package (if that was the reason for the revert), we can manually copy the map for each test run.

Suggested change
found := slices.Contains(tc.mc.Options, util.DisableMetricsForGKE+":true")
if !found {
expectedArgs["prometheus-port"] = strconv.Itoa(testPrometheusPort)
tc.expectedArgs["prometheus-port"] = strconv.Itoa(testPrometheusPort)
}
// Increase port value to match behavior of prepareMountArgs()
testPrometheusPort++
tc.mc.prepareMountArgs()
if !reflect.DeepEqual(tc.mc.FlagMap, expectedArgs) {
t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, expectedArgs)
if !reflect.DeepEqual(tc.mc.FlagMap, tc.expectedArgs) {
t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, tc.expectedArgs)
}
expectedArgs := make(map[string]string, len(tc.expectedArgs))
for k, v := range tc.expectedArgs {
expectedArgs[k] = v
}
found := slices.Contains(tc.mc.Options, util.DisableMetricsForGKE+":true")
if !found {
expectedArgs["prometheus-port"] = strconv.Itoa(testPrometheusPort)
}
// Increase port value to match behavior of prepareMountArgs()
testPrometheusPort++
tc.mc.prepareMountArgs()
if !reflect.DeepEqual(tc.mc.FlagMap, expectedArgs) {
t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, expectedArgs)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant