Skip to content
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

Add unit tests for Kubernetes apiclient waiter functions #128592

Closed

Conversation

gnarayan1337
Copy link

@gnarayan1337 gnarayan1337 commented Nov 6, 2024

What type of PR is this?
/kind cleanup /kind testing

What this PR does / why we need it:
This PR introduces unit tests for key functions in the apiclient package, specifically within wait.go. These tests enhance the reliability and maintainability of the package by verifying the behavior of core waiter functions used in managing Kubernetes control plane components. The tests cover essential functionality, ensuring that functions perform as expected under various scenarios, including both normal and edge cases.

Which issue(s) this PR fixes:
Added unit tests

Special notes for your reviewer:
The tests use client-go/fake to simulate the Kubernetes API server, and httptest to mock HTTP health check responses. Each test includes detailed assertions for expected outcomes, enabling robust validation of each function’s behavior. Please see individual test cases for specific scenarios covered.

Does this PR introduce a user-facing change? No

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

This commit introduces comprehensive unit tests for critical functions in the `apiclient` package, specifically within `wait.go`. These tests are designed to ensure the reliability and correctness of various waiter functions that are essential for monitoring and managing Kubernetes control plane components.

### Added Tests:
1. **TestGetControlPlaneComponents**: Validates the `getControlPlaneComponents` function by checking that it correctly constructs control plane component URLs based on configuration inputs.
   
2. **TestGetStaticPodSingleHash**: Ensures that `getStaticPodSingleHash` retrieves the correct hash value from pod annotations.

3. **TestGetStaticPodSingleHash_PodNotFound**: Verifies that `getStaticPodSingleHash` returns an appropriate error when the specified pod does not exist.

4. **TestWaitForStaticPodSingleHash**: Confirms that `WaitForStaticPodSingleHash` waits for and retrieves the correct hash value for a static pod.

5. **TestWaitForPodsWithLabel**: Checks that `WaitForPodsWithLabel` only returns successfully when all pods with a specific label are in the `Running` phase, simulating different pod states.

6. **TestWaitForStaticPodHashChange**: Validates that `WaitForStaticPodHashChange` accurately detects changes in the hash of a static pod, which is critical for determining if the pod has been restarted.

7. **TestWaitForStaticPodHashChange_Timeout**: Ensures that `WaitForStaticPodHashChange` times out appropriately if the pod hash does not change.

8. **TestWaitForStaticPodControlPlaneHashes**: Verifies that `WaitForStaticPodControlPlaneHashes` successfully retrieves hashes for all control plane static pods.

### Implementation Notes:
- **Mock Kubernetes Client**: These tests use `client-go/fake` to simulate interactions with the Kubernetes API server, allowing for isolated and predictable testing without requiring an actual cluster.
- **HTTP Server Simulation**: `httptest` is used to create mock HTTP responses for the health endpoints, testing the HTTP polling logic in functions like `WaitForControlPlaneComponents` and `WaitForAPI`.
- **Timeout Handling**: All tests are written with specific timeouts to avoid hanging indefinitely. Short timeouts are used in certain cases to simulate realistic polling intervals.

These tests enhance the robustness of the `apiclient` package by ensuring the core waiter functions handle expected scenarios and edge cases effectively. The coverage gained by these tests will assist in catching regressions and improve confidence in code modifications going forward.
This commit introduces comprehensive unit tests for critical functions in the `apiclient` package, specifically within `wait.go`. These tests are designed to ensure the reliability and correctness of various waiter functions that are essential for monitoring and managing Kubernetes control plane components.

### Added Tests:
1. **TestGetControlPlaneComponents**: Validates the `getControlPlaneComponents` function by checking that it correctly constructs control plane component URLs based on configuration inputs.
   
2. **TestGetStaticPodSingleHash**: Ensures that `getStaticPodSingleHash` retrieves the correct hash value from pod annotations.

3. **TestGetStaticPodSingleHash_PodNotFound**: Verifies that `getStaticPodSingleHash` returns an appropriate error when the specified pod does not exist.

4. **TestWaitForStaticPodSingleHash**: Confirms that `WaitForStaticPodSingleHash` waits for and retrieves the correct hash value for a static pod.

5. **TestWaitForPodsWithLabel**: Checks that `WaitForPodsWithLabel` only returns successfully when all pods with a specific label are in the `Running` phase, simulating different pod states.

6. **TestWaitForStaticPodHashChange**: Validates that `WaitForStaticPodHashChange` accurately detects changes in the hash of a static pod, which is critical for determining if the pod has been restarted.

7. **TestWaitForStaticPodHashChange_Timeout**: Ensures that `WaitForStaticPodHashChange` times out appropriately if the pod hash does not change.

8. **TestWaitForStaticPodControlPlaneHashes**: Verifies that `WaitForStaticPodControlPlaneHashes` successfully retrieves hashes for all control plane static pods.

### Implementation Notes:
- **Mock Kubernetes Client**: These tests use `client-go/fake` to simulate interactions with the Kubernetes API server, allowing for isolated and predictable testing without requiring an actual cluster.
- **HTTP Server Simulation**: `httptest` is used to create mock HTTP responses for the health endpoints, testing the HTTP polling logic in functions like `WaitForControlPlaneComponents` and `WaitForAPI`.
- **Timeout Handling**: All tests are written with specific timeouts to avoid hanging indefinitely. Short timeouts are used in certain cases to simulate realistic polling intervals.

These tests enhance the robustness of the `apiclient` package by ensuring the core waiter functions handle expected scenarios and edge cases effectively. The coverage gained by these tests will assist in catching regressions and improve confidence in code modifications going forward.
@k8s-ci-robot
Copy link
Contributor

@gnarayan1337: The label(s) kind//kind, kind/testing cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?
/kind cleanup /kind testing

What this PR does / why we need it:
This PR introduces unit tests for key functions in the apiclient package, specifically within wait.go. These tests enhance the reliability and maintainability of the package by verifying the behavior of core waiter functions used in managing Kubernetes control plane components. The tests cover essential functionality, ensuring that functions perform as expected under various scenarios, including both normal and edge cases.

Which issue(s) this PR fixes:
Fixes # (replace with relevant issue number if applicable)

Special notes for your reviewer:
The tests use client-go/fake to simulate the Kubernetes API server, and httptest to mock HTTP health check responses. Each test includes detailed assertions for expected outcomes, enabling robust validation of each function’s behavior. Please see individual test cases for specific scenarios covered.

Does this PR introduce a user-facing change? No

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

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.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 6, 2024
Copy link

linux-foundation-easycla bot commented Nov 6, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gnarayan1337
Once this PR has been reviewed and has the lgtm label, please assign sataqiu for approval. For more information see the Kubernetes Code Review Process.

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

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

@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @gnarayan1337. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 6, 2024
@gnarayan1337 gnarayan1337 changed the title Gnarayan1337 patch 1 Add unit tests for Kubernetes apiclient waiter functions Nov 6, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 6, 2024
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks for the PR, please use the sub test approach.

Comment on lines +87 to +89
// Call the function under test
actual := getControlPlaneComponents(tc.cfg)
// Compare the expected and actual results
Copy link
Member

Choose a reason for hiding this comment

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

these comments are not needed


kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
)

// TestGetControlPlaneComponents tests that getControlPlaneComponents returns the correct control plane components and their URLs.
Copy link
Member

Choose a reason for hiding this comment

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

don't add comments above test functions


// TestGetStaticPodSingleHash tests that getStaticPodSingleHash correctly retrieves the static pod hash from annotations.
func TestGetStaticPodSingleHash(t *testing.T) {
// Initialize a fake clientset
Copy link
Member

Choose a reason for hiding this comment

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

all of these comments are not needed as the test code is rather simple.

}

// TestGetStaticPodSingleHash_PodNotFound tests that getStaticPodSingleHash returns an error when the pod is not found.
func TestGetStaticPodSingleHash_PodNotFound(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

we use subtests with testcases instead of seperate functions that do a different test.
check how TestGetControlPlaneComponents is doing it.

podName := fmt.Sprintf("%s-%s", component, nodeName)
expectedHash := "abc123"

// Create annotations with the expected hash
Copy link
Member

Choose a reason for hiding this comment

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

same here and everywhere else.

@neolit123
Copy link
Member

/release-note-none

to test locally type:

go test ./cmd/kubeadm/app/util/apiclient/... -count=1

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 6, 2024
@neolit123
Copy link
Member

please limit the number of commits to 1.

@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024
@neolit123
Copy link
Member

@gnarayan1337 are you going to continue work on this PR?

@neolit123
Copy link
Member

/close
please reopen if you continue work on this PR.

@k8s-ci-robot
Copy link
Contributor

@neolit123: Closed this PR.

In response to this:

/close
please reopen if you continue work on this PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants