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 tests for ensureNamespace #3217

Merged
merged 1 commit into from
Jun 19, 2020
Merged

🌱 Add tests for ensureNamespace #3217

merged 1 commit into from
Jun 19, 2020

Conversation

hazbo
Copy link
Contributor

@hazbo hazbo commented Jun 19, 2020

What this PR does / why we need it:
#2252 requires a test for ensureNamespaces. This PR includes a test for ensureNamespace (which is called inside ensureNamespaces) and also I think a bug fix regarding the scope of an err.

In writing new tests for ensureNamespaces, I've found some odd behavior there which needs addressing. A test for that function is not in this PR.

Edit: this is in relation to my previous PR #2984

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 19, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @hazbo. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra 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 Jun 19, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 19, 2020
@hazbo
Copy link
Contributor Author

hazbo commented Jun 19, 2020

@vincepri @maelvls much cleaner this time round. Apologies for the last PR.

@hazbo hazbo changed the title This PR was initially to write a test for the ensureNamespaces func… 🌱 Add tests for ensure namespaces Jun 19, 2020
@hazbo hazbo changed the title 🌱 Add tests for ensure namespaces 🌱 Add tests for ensure namespace Jun 19, 2020
@hazbo hazbo changed the title 🌱 Add tests for ensure namespace 🌱 Add tests for ensureNamespace Jun 19, 2020
@vincepri
Copy link
Member

/assign @fabriziopandini @wfernandes

fabriziopandini
fabriziopandini reviewed Jun 19, 2020
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@hazbo thanks!
only few minors to simplify the test and make them more readable/maintainable

cmd/clusterctl/client/cluster/mover.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/mover_test.go Outdated Show resolved Hide resolved
@fabriziopandini
Copy link
Member

I've found some odd behavior there which needs addressing. A test for that function is not in this PR.

Could you provide more context?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 19, 2020
@hazbo hazbo force-pushed the add-test-for-ensureNamespace branch from facd118 to 232cae9 Compare June 19, 2020 15:10
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 19, 2020
@fabriziopandini
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 19, 2020
@hazbo
Copy link
Contributor Author

hazbo commented Jun 19, 2020 •
edited
Loading

Could you provide more context?

Yes I'll go into a bit more detail here. Originally I was writing a test for ensureNamespaces. But I'm not sure that function is working correctly (at all?). I was unable to make a call to that function in the tests in a way where it'd create a namespace on the target, that I could verify using a client and a Get(). I've not yet found the main cause, I've so far only observed how it behaves.

So then I started digging deeper, and that is where this PR comes from.

EDIT: I've since written some tests for ensureNamespaces in #3224 which are passing. I think in my original work I must have had an error in the test code.

@fabriziopandini
Copy link
Member

fabriziopandini commented Jun 19, 2020 •
edited
Loading

only one tiny nit, then lgtm for me; please squash commits

@hazbo hazbo force-pushed the add-test-for-ensureNamespace branch from 1487224 to a2194a3 Compare June 19, 2020 15:29
@hazbo
This PR was initially to write a test for the ensureNamespaces func…
fd43486 
…tion,

not to be confused with the `ensureNamespace` test I have in this commit.

Upon testing `ensureNamespaces`, I think I've come across a few bugs. This
function itself makes a call to `ensureNamespace` and it seems like there
is potentially a scoping issue with where `err` is being set.

It is my understanding that the following pattern:

if err := something(); err != nil {

}

implies that this particular error is available within the scope locally.
You can test out the differece by reverting the small change made in
`mover.go` to see the different in behaviour.

`ensureNamespaces` still needs a test written.

Simplified test code to be more readable

As per the kind suggestions, I have made changes to reflect those
comments in this commit.

Update cmd/clusterctl/client/cluster/mover.go

Co-authored-by: Vince Prignano <vince@vincepri.com>

Updated misleading comment

We're no longer passing extra params here.

Updated comment again

Removed TypeMeta in namespace
@hazbo hazbo force-pushed the add-test-for-ensureNamespace branch from a2194a3 to fd43486 Compare June 19, 2020 15:32
@fabriziopandini
Copy link
Member

/approve
over to @wfernandes for final lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Jun 19, 2020
@wfernandes
Copy link
Contributor

Thanks for this PR!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit 2dd6943 into kubernetes-sigs:master Jun 19, 2020
maelvls
maelvls reviewed Jun 19, 2020
View reviewed changes
cmd/clusterctl/client/cluster/mover_test.go
Comment on lines +832 to +834
mover := objectMover{
fromProxy: test.NewFakeProxy(),
}
Copy link
Contributor

@maelvls maelvls Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi! Great work! Just wondering, is fromProxy required here or can we leave it as a zero value? It doesn't seem to be used in these test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can confirm that the tests pass with fromProxy being removed here

@hazbo hazbo deleted the add-test-for-ensureNamespace branch June 19, 2020 16:57
@hazbo hazbo mentioned this pull request Jun 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maelvls maelvls maelvls left review comments

@wfernandes wfernandes wfernandes left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@wfernandes wfernandes

@fabriziopandini fabriziopandini

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hazbo @k8s-ci-robot @vincepri @fabriziopandini @wfernandes @maelvls