-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fixed TestGetCoreClient unit test failure on Windows #21737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"context" | ||
"fmt" | ||
"os" | ||
"path/filepath" | ||
"reflect" | ||
"strings" | ||
"testing" | ||
|
@@ -712,50 +713,49 @@ preferences: {} | |
users: | ||
- name: minikube | ||
` | ||
var tests = []struct { | ||
description string | ||
kubeconfigPath string | ||
config string | ||
err bool | ||
tests := []struct { | ||
description string | ||
config string | ||
err bool | ||
}{ | ||
{ | ||
description: "ok", | ||
kubeconfigPath: "/tmp/kube_config", | ||
config: mockK8sConfig, | ||
err: false, | ||
description: "ok", | ||
config: mockK8sConfig, | ||
err: false, | ||
}, | ||
{ | ||
description: "empty config", | ||
kubeconfigPath: "/tmp/kube_config", | ||
config: "", | ||
err: true, | ||
description: "empty config", | ||
config: "", | ||
err: true, | ||
}, | ||
{ | ||
description: "broken config", | ||
kubeconfigPath: "/tmp/kube_config", | ||
config: "this**is&¬: yaml::valid: file", | ||
err: true, | ||
description: "broken config", | ||
config: "this**is&¬: yaml::valid: file", | ||
err: true, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.description, func(t *testing.T) { | ||
mockK8sConfigByte := []byte(test.config) | ||
mockK8sConfigPath := test.kubeconfigPath | ||
err := os.WriteFile(mockK8sConfigPath, mockK8sConfigByte, 0644) | ||
defer os.Remove(mockK8sConfigPath) | ||
if err != nil { | ||
t.Fatalf("Unexpected error when writing to file %v. Error: %v", test.kubeconfigPath, err) | ||
// Each sub-test gets its own temp dir & file for isolation and portability. | ||
tmpDir := t.TempDir() | ||
mockK8sConfigPath := filepath.Join(tmpDir, "kube_config") | ||
|
||
// Only write the file if we have any content; an empty string still creates a 0-byte file, | ||
// which is what we want to simulate "empty config". | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment contradicts the code, we always write a file, even if we don't have any content. |
||
if err := os.WriteFile(mockK8sConfigPath, []byte(test.config), 0644); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no reason to create the kubeconfig world readable. This is the default kubeconfig permissions on macOS:
It will be better to use real world permissions. This will break the code if it has bad assumptions about being able to read the config from another user in the same group. |
||
t.Fatalf("failed to write kubeconfig: %v", err) | ||
} | ||
t.Setenv("KUBECONFIG", mockK8sConfigPath) | ||
|
||
k8s := K8sClientGetter{} | ||
_, err = k8s.GetCoreClient("minikube") | ||
_, err := k8s.GetCoreClient("minikube") | ||
|
||
if err != nil && !test.err { | ||
t.Fatalf("GetCoreClient returned unexpected error: %v", err) | ||
} | ||
if err == nil && test.err { | ||
t.Fatal("GetCoreClient expected to return error but got nil") | ||
t.Fatalf("GetCoreClient expected to return error but got nil") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why t.Fatalf() if we don't format anything? |
||
} | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is standard practice, I don't think we ned to justify it. All tests should work like this.