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

Added test case for minikube config unset #4262

Merged
merged 4 commits into from
May 23, 2019
Merged

Added test case for minikube config unset #4262

merged 4 commits into from
May 23, 2019

Conversation

hpandeycodeit
Copy link

Added Test Case for minikube config unset.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 15, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hpandeycodeit
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: afbjorklund

If they are not already assigned, you can assign the PR to them by writing /assign @afbjorklund in a comment when ready.

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

tstromberg
tstromberg suggested changes May 15, 2019
View reviewed changes
cmd/minikube/cmd/config/unset_test.go Outdated
import "testing"

func TestUnsetConfig(t *testing.T) {
err := Set("dashboard", "true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to check this err too.

cmd/minikube/cmd/config/unset_test.go Outdated
err := Set("dashboard", "true")
err = unset("dashboard")
if err != nil {
t.Fatalf("Failed to unset property ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer t.Errorf over t.Fatalf whenever possible. That way you can see more failed test information without rerunning it later. In this particular case, it doesn't change the behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. I have updated it.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2019
tstromberg
tstromberg suggested changes May 16, 2019
View reviewed changes
cmd/minikube/cmd/config/unset_test.go Outdated Show resolved Hide resolved
cmd/minikube/cmd/config/unset_test.go Outdated Show resolved Hide resolved
@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

Test failed with:

E0516 20:45:27.894418   15408 console.go:120] no error file has been set
FAIL	k8s.io/minikube/cmd/minikube/cmd/config	0.032s

The error mentioned is new, but shouldn't be fatal. It's curious that the actual name of the failing test isn't being displayed though? I wonder if this is unintended fall-out from #4253

Do you mind merging your PR from head? I'm curious if make test fails for you.

@hpandeycodeit
Copy link
Author

@tstromberg It's working fine in my local even after merging changes from the HEAD.


= go test ===============================================================
ok  	k8s.io/minikube/cmd/minikube/cmd	0.436s	coverage: 17.5% of statements
ok  	k8s.io/minikube/cmd/minikube/cmd/config	1.296s	coverage: 25.9% of statements
ok  	k8s.io/minikube/cmd/util	0.077s	coverage: 7.1% of statements
ok  	k8s.io/minikube/pkg/drivers	0.100s	coverage: 19.2% of statements
ok  	k8s.io/minikube/pkg/drivers/hyperkit	0.319s	coverage: 15.6% of statements
ok  	k8s.io/minikube/pkg/minikube/assets	0.171s	coverage: 61.9% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper	3.020s	coverage: 21.9% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper/kubeadm	0.413s	coverage: 30.5% of statements
ok  	k8s.io/minikube/pkg/minikube/cluster	0.796s	coverage: 52.6% of statements
ok  	k8s.io/minikube/pkg/minikube/config	0.204s	coverage: 17.6% of statements
ok  	k8s.io/minikube/pkg/minikube/console	0.047s	coverage: 70.7% of statements
ok  	k8s.io/minikube/pkg/minikube/cruntime	0.044s	coverage: 63.8% of statements
ok  	k8s.io/minikube/pkg/minikube/logs	0.147s	coverage: 1.5% of statements
ok  	k8s.io/minikube/pkg/minikube/machine	0.172s	coverage: 12.0% of statements
ok  	k8s.io/minikube/pkg/minikube/notify	0.152s	coverage: 76.9% of statements
ok  	k8s.io/minikube/pkg/minikube/problem	0.072s	coverage: 42.9% of statements
ok  	k8s.io/minikube/pkg/minikube/registry	0.123s	coverage: 81.8% of statements
ok  	k8s.io/minikube/pkg/minikube/service	0.512s	coverage: 41.0% of statements
ok  	k8s.io/minikube/pkg/minikube/sshutil	0.397s	coverage: 75.0% of statements
ok  	k8s.io/minikube/pkg/minikube/tunnel	2.193s	coverage: 59.0% of statements
ok  	k8s.io/minikube/pkg/util	1.268s	coverage: 50.4% of statements

@hpandeycodeit hpandeycodeit force-pushed the master branch 2 times, most recently from e22f863 to 558444d Compare May 17, 2019 21:40
@hpandeycodeit
Copy link
Author

/retest

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@hpandeycodeit
Copy link
Author

@minikube-bot OK to test

@hpandeycodeit
Copy link
Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 23, 2019
@tstromberg
Copy link
Contributor

tstromberg commented May 23, 2019 •
edited
Loading

Thank you for improving our test coverage!

@tstromberg tstromberg merged commit 2e7b8b5 into kubernetes:master May 23, 2019
@hpandeycodeit
Copy link
Author

Thank you @tstromberg! :)

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

@tstromberg tstromberg tstromberg approved these changes

@balopat balopat Awaiting requested review from balopat

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.
4 participants
@hpandeycodeit @minikube-bot @k8s-ci-robot @tstromberg