kubeadm: Add UT for cmd/kubeadm/app/cmd/util/join.go #128283

bzsuni · 2024-10-23T09:48:09Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Add UT for cmd/kubeadm/app/cmd/util/join.go

Running tool: /usr/local/go/bin/go test -timeout 30s -coverprofile=go-code-cover k8s.io/kubernetes/cmd/kubeadm/app/cmd/util

ok  	k8s.io/kubernetes/cmd/kubeadm/app/cmd/util	0.028s	coverage: 81.7% of statements

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

none

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

none

k8s-ci-robot · 2024-10-23T09:48:18Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

neolit123 · 2024-10-23T11:10:22Z

cmd/kubeadm/app/cmd/util/join_test.go

+			kubeConfig:  generateValidKubeConfig(),
+			token:       "test-token",
+			expectError: false,
+		},


can there be another negative test case here?

GetJoinWorkerCommand and GetJoinControlPlaneCommand both call getJoinCommand,
The TestGetJoinControlPlaneCommand below includes the testing scenario, so only one forward case is written here

neolit123 · 2024-10-23T11:10:50Z

cmd/kubeadm/app/cmd/util/join_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir, err := os.MkdirTemp("", "kubeadm-images-test")


Suggested change

tmpDir, err := os.MkdirTemp("", "kubeadm-images-test")

tmpDir, err := os.MkdirTemp("", "kubeadm-join-test")

Down, thanks

neolit123 · 2024-10-23T11:12:43Z

cmd/kubeadm/app/cmd/util/join_test.go

+			err = writeKubeConfigToFile(tt.kubeConfig, configFilePath)
+			require.NoError(t, err)
+
+			_, err = GetJoinWorkerCommand(configFilePath, tt.token, false)


for err != nil should we verify the output string to be the expected one?

Yes, fixed, thanks

neolit123 · 2024-10-23T11:13:14Z

cmd/kubeadm/app/cmd/util/join_test.go

+				require.NoError(t, err)
+			}
+
+			_, err = GetJoinControlPlaneCommand(configFilePath, tt.token, "", true, true)


should we verify the output string for err != nil?

neolit123 · 2024-10-23T11:13:34Z

cmd/kubeadm/app/cmd/util/join_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir, err := os.MkdirTemp("", "kubeadm-images-test")


Suggested change

tmpDir, err := os.MkdirTemp("", "kubeadm-images-test")

tmpDir, err := os.MkdirTemp("", "kubeadm-join-test")

Down, thanks

neolit123

instead of testing the two exported functions

kubernetes/cmd/kubeadm/app/cmd/util/join.go

Lines 40 to 50 in 5ffb052

    
           // GetJoinWorkerCommand returns the kubeadm join command for a given token and 
        
           // Kubernetes cluster (the current cluster in the kubeconfig file) 
        
           func GetJoinWorkerCommand(kubeConfigFile, token string, skipTokenPrint bool) (string, error) { 
        
           	return getJoinCommand(kubeConfigFile, token, "", false, skipTokenPrint, false) 
        
           } 
        
           // GetJoinControlPlaneCommand returns the kubeadm join command for a given token and 
        
           // Kubernetes cluster (the current cluster in the kubeconfig file) 
        
           func GetJoinControlPlaneCommand(kubeConfigFile, token, key string, skipTokenPrint, skipCertificateKeyPrint bool) (string, error) { 
        
           	return getJoinCommand(kubeConfigFile, token, key, true, skipTokenPrint, skipCertificateKeyPrint) 
        
           }

just test this function:

kubernetes/cmd/kubeadm/app/cmd/util/join.go

Line 52 in 5ffb052

    
           func getJoinCommand(kubeConfigFile, token, key string, controlPlane, skipTokenPrint, skipCertificateKeyPrint bool) (string, error) {

bzsuni · 2024-10-23T12:41:58Z

instead of testing the two exported functions

kubernetes/cmd/kubeadm/app/cmd/util/join.go

Lines 40 to 50 in 5ffb052

// GetJoinWorkerCommand returns the kubeadm join command for a given token and

// Kubernetes cluster (the current cluster in the kubeconfig file)

func GetJoinWorkerCommand(kubeConfigFile, token string, skipTokenPrint bool) (string, error) {

return getJoinCommand(kubeConfigFile, token, "", false, skipTokenPrint, false)

}

// GetJoinControlPlaneCommand returns the kubeadm join command for a given token and

// Kubernetes cluster (the current cluster in the kubeconfig file)

func GetJoinControlPlaneCommand(kubeConfigFile, token, key string, skipTokenPrint, skipCertificateKeyPrint bool) (string, error) {

return getJoinCommand(kubeConfigFile, token, key, true, skipTokenPrint, skipCertificateKeyPrint)

}

just test this function:

kubernetes/cmd/kubeadm/app/cmd/util/join.go

Line 52 in 5ffb052

func getJoinCommand(kubeConfigFile, token, key string, controlPlane, skipTokenPrint, skipCertificateKeyPrint bool) (string, error) {

Yes, but I think using two functions as separate entrances would increase the coverage of unit testing, even though it doesn't actually have much effect

neolit123 · 2024-10-23T13:49:40Z

Yes, but I think using two functions as separate entrances would increase the coverage of unit testing, even though it doesn't actually have much effect

please only test the called function. there is no point to test the parent exported ones.

bzsuni · 2024-10-24T07:15:39Z

Yes, but I think using two functions as separate entrances would increase the coverage of unit testing, even though it doesn't actually have much effect

please only test the called function. there is no point to test the parent exported ones.

Fixed, please review

neolit123

LGTM, just a couple more minor comments.

neolit123 · 2024-10-25T14:43:09Z

cmd/kubeadm/app/cmd/util/join_test.go

+func writeKubeConfigToFile(config *clientcmdapi.Config, filePath string) error {
+	configBytes, err := clientcmd.Write(*config)
+	if err != nil {
+		return err
+	}
+	return os.WriteFile(filePath, configBytes, 0644)
+}


just move this code in inside the test (i.e. unroll it) because it's used in only one place now.

neolit123 · 2024-10-25T14:43:44Z

cmd/kubeadm/app/cmd/util/join_test.go

+	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
+)
+
+type kubeConfigSpec struct {


Suggested change

type kubeConfigSpec struct {

type testKubeConfigSpec struct {

to better signify this is a test type.

neolit123 · 2024-10-25T14:44:14Z

cmd/kubeadm/app/cmd/util/join_test.go

+	CertificateAuthorityData string
+	CertificateAuthorityFile string
+	Server                   string
+	ClusterName              string
+	ContextName              string
+	User                     string


all these field names can be lowercase (not exported)

neolit123 · 2024-10-25T14:44:46Z

cmd/kubeadm/app/cmd/util/join_test.go

+	User                     string
+}
+
+func generateKubeConfig(spec kubeConfigSpec) *clientcmdapi.Config {


Suggested change

func generateKubeConfig(spec kubeConfigSpec) *clientcmdapi.Config {

func generateTestKubeConfig(spec kubeConfigSpec) *clientcmdapi.Config {

Signed-off-by: bzsuni <bingzhe.sun@daocloud.io>

neolit123

thanks for the updates
this looks fine.

/lgtm
/approve

k8s-ci-robot · 2024-10-28T06:42:19Z

LGTM label has been added.

Git tree hash: f8fa3b1ac2dfffb8f9663a8103e344d90f380a30

k8s-ci-robot · 2024-10-28T06:42:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bzsuni, neolit123

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 23, 2024

k8s-ci-robot requested review from neolit123 and SataQiu October 23, 2024 09:48

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 23, 2024

bzsuni force-pushed the bz/ut/kubeadm branch 2 times, most recently from 07d2387 to d1aa906 Compare October 23, 2024 10:37

neolit123 reviewed Oct 23, 2024

View reviewed changes

bzsuni force-pushed the bz/ut/kubeadm branch from d1aa906 to 3966693 Compare October 23, 2024 12:32

bzsuni force-pushed the bz/ut/kubeadm branch from 3966693 to 9746203 Compare October 24, 2024 01:50

neolit123 reviewed Oct 25, 2024

View reviewed changes

bzsuni force-pushed the bz/ut/kubeadm branch from 9746203 to c628071 Compare October 28, 2024 02:59

Add UT for cmd/kubeadm/app/cmd/util/join.go

80fe671

Signed-off-by: bzsuni <bingzhe.sun@daocloud.io>

bzsuni force-pushed the bz/ut/kubeadm branch from c628071 to 80fe671 Compare October 28, 2024 03:12

neolit123 reviewed Oct 28, 2024

View reviewed changes

k8s-ci-robot assigned neolit123 Oct 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2024

k8s-ci-robot merged commit 9ec52fc into kubernetes:master Oct 28, 2024
14 checks passed

k8s-ci-robot added this to the v1.32 milestone Oct 28, 2024

	tmpDir, err := os.MkdirTemp("", "kubeadm-images-test")
	tmpDir, err := os.MkdirTemp("", "kubeadm-join-test")

	// GetJoinWorkerCommand returns the kubeadm join command for a given token and
	// Kubernetes cluster (the current cluster in the kubeconfig file)
	func GetJoinWorkerCommand(kubeConfigFile, token string, skipTokenPrint bool) (string, error) {
	return getJoinCommand(kubeConfigFile, token, "", false, skipTokenPrint, false)
	}

	// GetJoinControlPlaneCommand returns the kubeadm join command for a given token and
	// Kubernetes cluster (the current cluster in the kubeconfig file)
	func GetJoinControlPlaneCommand(kubeConfigFile, token, key string, skipTokenPrint, skipCertificateKeyPrint bool) (string, error) {
	return getJoinCommand(kubeConfigFile, token, key, true, skipTokenPrint, skipCertificateKeyPrint)
	}

	type kubeConfigSpec struct {
	type testKubeConfigSpec struct {

	func generateKubeConfig(spec kubeConfigSpec) *clientcmdapi.Config {
	func generateTestKubeConfig(spec kubeConfigSpec) *clientcmdapi.Config {

kubeadm: Add UT for cmd/kubeadm/app/cmd/util/join.go #128283

kubeadm: Add UT for cmd/kubeadm/app/cmd/util/join.go #128283

Uh oh!

Conversation

bzsuni commented Oct 23, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

bzsuni commented Oct 23, 2024

Uh oh!

neolit123 commented Oct 23, 2024

Uh oh!

bzsuni commented Oct 24, 2024

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 28, 2024

Uh oh!

k8s-ci-robot commented Oct 28, 2024

Uh oh!

Uh oh!

Uh oh!