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

Move clusterdeployer_test.go into clusterdeployer package #439

Merged
merged 3 commits into from
Jul 25, 2018
Merged

Move clusterdeployer_test.go into clusterdeployer package #439

merged 3 commits into from
Jul 25, 2018

Conversation

ashish-amarnath
Copy link
Contributor

@ashish-amarnath ashish-amarnath commented Jul 23, 2018 •
edited
Loading

Signed-off-by: Ashish Amarnath ashish.amarnath@gmail.com

What this PR does / why we need it:
This PR makes clusterdeployer.go more testable by moving clusterdeployer_test.go into the same package as clusterdeployer.go

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

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
    No image versions being changed in the PR.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2018
@ashish-amarnath
Copy link
Contributor Author

CLA signed

@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 Jul 23, 2018
@roberthbailey
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2018
@roberthbailey
Copy link
Contributor

@ashish-amarnath - first of all, thank you for sending this PR, and especially for writing tests and working to help improve our test coverage.

In June during the weekly working group meeting we discussed removing machine role from the machine spec (notes). As I result, I opened #338 and created #405 (which hasn't merged yet, but should merge soon). That PR removes the two functions that you wrote tests for in this PR.

@ashish-amarnath
Copy link
Contributor Author

@roberthbailey Thanks for the explanation. I will remove the tests that I added. But I would like to preserve the changes to the Makefile and also the package name for the clusterdeployer_test.go.
WDYT?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 25, 2018
@ashish-amarnath ashish-amarnath changed the title Add unit tests to clusterdeployer.go Move clusterdeployer_test.go into clusterdeployer package Jul 25, 2018
@roberthbailey
Copy link
Contributor

@ashish-amarnath - the package rename looks good.

As for the makefile, I'm not sure why you'd want the makefile at the root level to only run clusterctl tests / coverage when there is lots of other code in pkg and cmd.

https://github.com/kubernetes-sigs/cluster-api/blob/master/scripts/ci-test.sh runs all unit and integration tests in the repo. Maybe we could move that into the makefile (and have the script just call make test-integration)?

If we really want make targets that are specific to the clusterctl directory, what about adding a makefile to that dir?

@karan
Copy link
Contributor

karan commented Jul 25, 2018

+1 to @roberthbailey's point about only testing clusterctl in the root level. We should test everything (minus vendor) in the root.

@ashish-amarnath
Copy link
Contributor Author

ashish-amarnath commented Jul 25, 2018 •
edited
Loading

@roberthbailey I didn't know the existence of that script. Thanks for pointing that out :)
The reason for adding the make target just for clusterctl was that I wasn't able to get go test from the repo root to pass on my laptop and I didn't want to open a PR which has a make target that fails. I was planning on opening an issue for that.

Now that I know of the test script, which runs successfully, I will not open that issue and will also undo the changes to the makefile.

@ashish-amarnath
Make clusterdeployer.go more testable
c33a09b 
Move clusterdeployer_test into clusterdeployer package

Signed-off-by: Ashish Amarnath <ashish.amarnath@gmail.com>
@ashish-amarnath
Copy link
Contributor Author

Also, just realized that we can use the tests that I wrote for splitMachineRoles function to test extractMasterMachine. Pushing those changes in a bit.

@roberthbailey
Copy link
Contributor

If you'd like a make target instead of the script, the makefile successfully runs go test at the root level so you could use that to help figure out why your invocation was failing.

Adding test coverage for the new methods would be greatly appreciated.

@ashish-amarnath
* add tests for extractMasterMachine()
320a632 
* remove unused PHONY make targets
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 25, 2018
@ashish-amarnath
Copy link
Contributor Author

I take back my initial comment that the script runs successfully.
All the tests pass except for this one

=== RUN   TestMachineSet
[::]:58989
[::]:58990
[::]:58991
panic: exec: "etcd": executable file not found in $PATH

goroutine 131 [running]:
sigs.k8s.io/cluster-api/vendor/github.com/kubernetes-incubator/apiserver-builder/pkg/test.(*TestEnvironment).startEtcd(0xc420704000, 0xc4206c8de0)
	/Users/bht5/Work/kubernetes/go/src/sigs.k8s.io/cluster-api/vendor/github.com/kubernetes-incubator/apiserver-builder/pkg/test/server.go:167 +0x472
created by sigs.k8s.io/cluster-api/vendor/github.com/kubernetes-incubator/apiserver-builder/pkg/test.(*TestEnvironment).Start
	/Users/bht5/Work/kubernetes/go/src/sigs.k8s.io/cluster-api/vendor/github.com/kubernetes-incubator/apiserver-builder/pkg/test/server.go:85 +0xb5
FAIL	sigs.k8s.io/cluster-api/pkg/controller/machineset	0.077s

The reason, as the error suggests, I don't have etcd on my laptop. So this would be an expected failure. Am I missing anything in my dev-setup to get this to pass?

@roberthbailey
Copy link
Contributor

roberthbailey commented Jul 25, 2018 •
edited
Loading

Am I missing anything in my dev-setup to get this to pass?

It looks like you are missing having etcd in your path. :)

I think most other folks have stuck in there in the past as they were working on other parts of Kubernetes that required it for unit/integration testing so no one noticed that it was required. Could you file an issue and/or send a documentation update to make it part of the developer getting started instructions?

roberthbailey
roberthbailey reviewed Jul 25, 2018
View reviewed changes
clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
{
name: "success_1_master_multiple_nodes",
inputMachines: generateValidExtractMasterMachineInput(singleMasterName, multipleNodeNames),
expectedMasters: generateMasterNode("test-master"),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pass singleMasterName here instead of the raw string.

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 had missed that. Thanks!

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
actualMasters, actualNodes, actualError := extractMasterMachine(tc.inputMachines)

if tc.expectedError == nil && actualError != nil {
t.Fatalf("Expected error=[nil], Actual error=[%v]", actualError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Your test strings are backwards according to the go style (see https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures)

Something like t.Fatalf("extractMasterMachine(%q) = %q; want %q", tc.inputMachines, actualError, tc.expectedError) would be more along the lines of the style recommendation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for that link!

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
master := &clusterv1.Machine{}
master.Name = "test-master"
master.Name = name
Copy link
Contributor

Choose a reason for hiding this comment

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

It was this way before but I don't understand why these parameters are set here rather than when creating the Machine on the above line. Is there a reason we can't write

  return &clusterv1.Machine{
    Name: name,
    Spec: clusterv1.MachineSpec{
      Versions: clusterv1.MachineVersionInfo{
        ControlPlane: "1.10.1",
      },
    },
  }

(or the variation of that which compiles)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. I was just following the way this was before. Now I've moved the setting of fields into the initializer.

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
func generateTestMasterMachine(masterNames []string) []*clusterv1.Machine {
var masters []*clusterv1.Machine
for _, mn := range masterNames {
m := generateMasterNode(mn)
Copy link
Contributor

Choose a reason for hiding this comment

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

combine with the next line: masters = append(masters, generateMasterNode(mn))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
func generateTestNodeMachines(nodeNames []string) []*clusterv1.Machine {
var nodes []*clusterv1.Machine
for _, nn := range nodeNames {
n := &clusterv1.Machine{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nodes = append(nodes, &clusterv1.Machine{Name: nn})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
masters := generateTestMasterMachine(masterNames)
nodes := generateTestNodeMachines(nodeNames)

var inputMachines []*clusterv1.Machine
Copy link
Contributor

Choose a reason for hiding this comment

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

return append(masters, nodes...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
}

func generateValidExtractMasterMachineInput(masterName string, nodeNames []string) []*clusterv1.Machine {
masters := generateMasterNode(masterName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make masters an array (to match the plural name:

masters = []*clusterv1.Machine{generateMasterNode(masterName)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
masters := generateMasterNode(masterName)
nodes := generateTestNodeMachines(nodeNames)

var inputMachines []*clusterv1.Machine
Copy link
Contributor

Choose a reason for hiding this comment

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

return append(masters, nodes...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
}

func generateMachines() []*clusterv1.Machine {
master := generateMasterNode("test-master")
node := &clusterv1.Machine{}
Copy link
Contributor

Choose a reason for hiding this comment

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

node := &clusterv1.Machine{Name: "test.Node"}

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated
}

func generateMachines() []*clusterv1.Machine {
master := generateMasterNode("test-master")
node := &clusterv1.Machine{}
node.Name = "test.Node"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any idea why the syntax for master & node names is different (dash vs. dot)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't. It seems odd though. I am going to use the dash syntax.

@roberthbailey roberthbailey self-assigned this Jul 25, 2018
@ashish-amarnath
Address code-review comments
1ed0be2 
* follow style guidelines for test failure messages
* set fields in test data using initializers
* refactor to simplify test data generation
@roberthbailey
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashish-amarnath, roberthbailey

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 Jul 25, 2018
@k8s-ci-robot k8s-ci-robot merged commit 977b11a into kubernetes-sigs:master Jul 25, 2018
@ashish-amarnath
Copy link
Contributor Author

@roberthbailey Thank you!

@ashish-amarnath ashish-amarnath deleted the write-unit-tests branch July 25, 2018 23:08
@ashish-amarnath ashish-amarnath mentioned this pull request Jul 25, 2018
Merged
@ashish-amarnath ashish-amarnath mentioned this pull request Aug 27, 2018
Closed
6 tasks
jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020
@k8s-ci-robot
Merge pull request kubernetes-sigs#439 from akutz/feature/match-relea…
28645fa 
…se-tag

Match release tag script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey left review comments

@karan karan Awaiting requested review from karan

@medinatiger medinatiger Awaiting requested review from medinatiger

Assignees

@roberthbailey roberthbailey

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ashish-amarnath @k8s-ci-robot @roberthbailey @karan