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.

Sign up for GitHub

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

Jump to bottom

🏃Add workload cluster unit tests in KCP #2806

Merged
merged 1 commit into from
Mar 30, 2020
Merged

🏃Add workload cluster unit tests in KCP #2806

merged 1 commit into from
Mar 30, 2020

Conversation

wfernandes
Copy link
Contributor

@wfernandes wfernandes commented Mar 27, 2020 •
edited
Loading

What this PR does / why we need it:
This PR backfills unit tests for KCP/workload_cluster components

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Ref #2753
#2779

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 27, 2020
@wfernandes wfernandes force-pushed the workload-cluster-unit-tests branch 2 times, most recently from 2dc4a32 to bc53d5b Compare March 27, 2020 19:58
@wfernandes
Copy link
Contributor Author

/hold
Need to add some more unit tests

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2020
@wfernandes
Copy link
Contributor Author

/hold remove

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2020
@wfernandes
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 27, 2020
@vincepri
Copy link
Member

/milestone v0.3.4

@k8s-ci-robot k8s-ci-robot added this to the v0.3.4 milestone Mar 30, 2020
@vincepri
Copy link
Member

@benmoss FYI, after this PR merges, your changes might need a rebase given that we're moving the etcd methods to a different file

@wfernandes
Copy link
Contributor Author

I'm going to rebase this into a single commit.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2020
@benmoss benmoss mentioned this pull request Mar 30, 2020
Merged
vincepri
vincepri approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/assign @benmoss

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri, wfernandes

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:

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2020
benmoss
benmoss reviewed Mar 30, 2020
View reviewed changes
controlplane/kubeadm/internal/workload_cluster_etcd_test.go
}{
{
name: "does not panic if machine is nil",
expectErr: false,
Copy link

Choose a reason for hiding this comment

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

These "does not panic" tests also seem weird, it would be nice to assert something positive like that the k8s and etcd clients aren't called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...I wasn't sure the best way to test this. I know gomega has the Panic() assertion.
But if the nil checks are removed it's going to panic regardless because it's calling removeMemberForNode(ctx, machine.Status.NodeRef.Name). And I decided that a panic is a failing test.
I added this test case just to document the behavior and not lose it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is, if someone remove the nil checks, these tests gonna blow up.

benmoss
benmoss reviewed Mar 30, 2020
View reviewed changes
controlplane/kubeadm/internal/workload_cluster_etcd_test.go
expectErr bool
}{
{
name: "does not panic if machine is nil",
Copy link

Choose a reason for hiding this comment

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

same as above

@benmoss
Copy link

benmoss commented Mar 30, 2020

Other than those readability nits it seems good 👍

Add unit tests to workload_cluster methods.
d821cd5 
- Move private and helpers to the bottom of file
- Move etcd methods into separate file
- Add test for RemoveEtcdMemberForMachine
- Add test for RemoveMachineFromKubeadmConfigMap
- Add test for UpdateEtcdVersionInKubeadmConfigMap
- Add test for ForwardEtcdLeadership
- Add test for UpdateKubeletConfigMap
- Add test for UpdateKubernetesVersionInKubeadmConfigMap
- Add test for ClusterStatus
- Use kubeadm-config as a const
@benmoss
Copy link

benmoss commented Mar 30, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3f0710b into kubernetes-sigs:master Mar 30, 2020
@wfernandes wfernandes deleted the workload-cluster-unit-tests branch April 6, 2020 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmoss benmoss benmoss left review comments

@vincepri vincepri vincepri approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@benmoss benmoss

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@wfernandes @vincepri @k8s-ci-robot @benmoss