🌱 Added the test-coverage for completion.go and version.go inside cli pkg #3233

yashsingh74 · 2023-02-19T15:53:38Z

This PR introduces the files changes in completion.go and version.go that will enhance the test coverage.

Result -

> go test pkg/cli/suite_test.go                                                                                                                                   3s
ok      command-line-arguments 

> go test -coverprofile=coverage.out ./...
ok      sigs.k8s.io/kubebuilder/v3/pkg/cli      0.168s  coverage: 84.1% of statements

> go tool cover -func=coverage.out
sigs.k8s.io/kubebuilder/v3/pkg/cli/completion.go:26:    newBashCmd                      100.0%
sigs.k8s.io/kubebuilder/v3/pkg/cli/completion.go:45:    newZshCmd                       100.0%
sigs.k8s.io/kubebuilder/v3/pkg/cli/completion.go:64:    newFishCmd                      100.0%
sigs.k8s.io/kubebuilder/v3/pkg/cli/completion.go:80:    newPowerShellCmd                100.0%
sigs.k8s.io/kubebuilder/v3/pkg/cli/completion.go:90:    newCompletionCmd                100.0%`
sigs.k8s.io/kubebuilder/v3/pkg/cli/version.go:25:       newVersionCmd                   100.0%

k8s-ci-robot · 2023-02-19T15:53:46Z

Hi @yashsingh74. 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.

yashsingh74 · 2023-02-19T15:54:15Z

cc: @camilamacedo86

camilamacedo86

This one needs to add the tests.
Also, should I change the runE?

The idea of #3211 would not ensure that we cover all (100%) with the unit test but ensure that we increase the tests by covering the methods/func that can easily be covered with unit tests. The commands do not seem a good fit for this one.

Also, in this case, the emoji is 🌱 because it does not change anything for end users, and we should not add those changes in the release notes, which are generated automatically based on the emojis.

yashsingh74 · 2023-02-19T16:52:48Z

Understand. I will add the test case for the files.
Thanks for the suggestion.

yashsingh74 · 2023-03-02T13:55:48Z

cc: @varshaprasad96

varshaprasad96 · 2023-03-02T16:48:29Z

pkg/cli/completion_test.go

@@ -0,0 +1,74 @@
+/*


The header needs to be fixed

pkg/cli/completion_test.go

pkg/cli/version_test.go

camilamacedo86 · 2023-03-13T10:49:45Z

Hi @yashsingh74,

It is failing in the tests, could you please:

squash the commits
rebase it with the master

and the push again for we check out its latest state?

varshaprasad96 · 2023-03-13T14:29:10Z

pkg/cli/completion_test.go

+			Expect(cmd.Short).To(ContainSubstring("Load bash completions"))
+			Expect(cmd.Example).NotTo(Equal(""))
+			Expect(cmd.Example).To(ContainSubstring("# To load completion for this session"))
+			Expect(cmd.Execute()).Error()


Do we need to assert here and in the following tests?:

Suggested change

Expect(cmd.Execute()).Error()

Expect(cmd.Execute()).Error().ShouldNot(HaveOccured())

I ran this Execute locally, which returns the error because it returns the multiple possible values. So, checking whether the Execute runs as expected will be good.

• [FAILED] [0.001 seconds] Version newVersionCmd [It] Test the version /Users/syash/Desktop/Sample/examples/version/version_test.go:18 [FAILED] Expected an error-type. Got: <*cobra.Command | 0x140000e2000>: { Use: "version", Aliases: nil, SuggestFor: nil, Short: "Print the version", GroupID: "", Long: "Print the version", Example: " version", ValidArgs: nil,

Does this work:

err := cmd.Execute() Expect(err).To(BeNil())

Expect(cmd.Execute()).Error() does not assert anything if I'm not wrong. If there is no error, we should assert that error did not occur.
If we expect an output of type *cobra.Command, we should assert exactly the same.

I ran the following workaround with Expect(err).To(BeNil())

• [FAILED] [0.001 seconds] Version newVersionCmd [It] Test the version /Users/syash/go/src/k8s.io/kubebuilder/pkg/cli/version_test.go:34 [FAILED] Expected <*errors.errorString | 0x1400061d390>: { s: "unknown flag: --test.timeout", } to be nil In [It] at: /Users/syash/go/src/k8s.io/kubebuilder/pkg/cli/version_test.go:47 @ 03/20/23 21:48:32.276

and with Expect(err).NotTo(BeNil()) the test are passing.
I am confused that the Execute command should return nil.

This means that the command does return an error, with an unknown flag: --test.timeout. Could you check where it is coming from? It seems to be a flag being passed by ginkgo tests. If that's an inevitable case, I'd say to remove this check.

yashsingh74 · 2023-03-13T15:30:11Z

@camilamacedo86 I rebased the branch with master but still the test are failing.

git log

commit 732270950993a799c6dff90a5b3744af1c1190d4 (HEAD -> yash-unit-test, origin/yash-unit-test)
Author: Yash Singh <syash@vmware.com>
Date:   Sun Feb 19 21:19:15 2023 +0530

    Added the test coverage for completion.go and version.go

commit 1dc75e33d0e9f3a500a20f4d9a9bf87cbabdda00 (upstream/master, origin/master, origin/HEAD)
Merge: 958cda20 66aa98b1
Author: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Date:   Sun Mar 12 13:00:40 2023 -0700

    Merge pull request #3281 from camilamacedo86/update-cronjob-tutorial

    :book: update cronjob tutorial sample to use go/v4

commit 66aa98b1a3ef172ce895abdc4590454a34d3aece
Author: Camila Macedo <camilamacedo86@gmail.com>
Date:   Sat Mar 11 12:49:23 2023 +0000

    :book: update cronjob tutorial sample to use go/v4

commit 958cda208be4db41b5adf20742fdc780b7000bbd
Author: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com>
Date:   Sat Mar 11 12:07:43 2023 +0000

    🌱 : fix sample project-v4-grafana (#3279)

    :bug: fix sample project-v4-grafana

varshaprasad96 · 2023-04-11T15:28:48Z

pkg/cli/completion_test.go

+			Expect(cmd.Short).To(ContainSubstring("Load bash completions"))
+			Expect(cmd.Example).NotTo(Equal(""))
+			Expect(cmd.Example).To(ContainSubstring("# To load completion for this session"))
+			Expect(cmd.Execute()).Error()


This means that the command does return an error, with an unknown flag: --test.timeout. Could you check where it is coming from? It seems to be a flag being passed by ginkgo tests. If that's an inevitable case, I'd say to remove this check.

yashsingh74 · 2023-04-28T04:25:21Z

@camilamacedo86 @varshaprasad96 can you review and help to mark this completed.

camilamacedo86

/ok-to-test
/hold cancel

camilamacedo86 · 2023-05-04T07:44:31Z

@yashsingh74,

Could you please squash the commits so we can get this one merged now that the suggestion in the review are addressed?

yashsingh74 · 2023-05-16T14:50:47Z

@camilamacedo86 I have squashed the commit. Please provide your approval.

camilamacedo86

/lgtm

k8s-ci-robot · 2023-05-16T20:57:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, yashsingh74

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:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

All your requests shows addressed so I am moving forward with this one

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 19, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2023

yashsingh74 changed the title ~~(:sparkles:) Added the test to cover the test-coverage for cli~~ ✨ Added the test to cover the test-coverage for cli Feb 19, 2023

k8s-ci-robot requested review from pwittrock and varshaprasad96 February 19, 2023 15:53

yashsingh74 changed the title ~~✨ Added the test to cover the test-coverage for cli~~ ✨ Added the test-coverage for completion.go and version.go inside cli pkg Feb 19, 2023

camilamacedo86 reviewed Feb 19, 2023

View reviewed changes

camilamacedo86 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2023

yashsingh74 changed the title ~~✨ Added the test-coverage for completion.go and version.go inside cli pkg~~ 🌱 Added the test-coverage for completion.go and version.go inside cli pkg Feb 19, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2023

yashsingh74 requested review from camilamacedo86 and removed request for pwittrock and varshaprasad96 February 28, 2023 14:58

varshaprasad96 requested changes Mar 2, 2023

View reviewed changes

varshaprasad96 requested changes Mar 13, 2023

View reviewed changes

yashsingh74 force-pushed the yash-unit-test branch 2 times, most recently from a59746c to 7322709 Compare March 13, 2023 15:27

yashsingh74 requested review from varshaprasad96 and camilamacedo86 and removed request for camilamacedo86 and varshaprasad96 March 29, 2023 12:42

varshaprasad96 previously requested changes Apr 11, 2023

View reviewed changes

camilamacedo86 reviewed May 4, 2023

View reviewed changes

yashsingh74 force-pushed the yash-unit-test branch from 3fc4358 to 9ed16e7 Compare May 16, 2023 14:48

Added the test coverage for completion.go and version.go

3811dd7

yashsingh74 force-pushed the yash-unit-test branch from 9ed16e7 to 3811dd7 Compare May 16, 2023 14:50

camilamacedo86 approved these changes May 16, 2023

View reviewed changes

k8s-ci-robot assigned camilamacedo86 May 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2023

k8s-ci-robot merged commit a6fca17 into kubernetes-sigs:master May 16, 2023
9 checks passed

yashsingh74 deleted the yash-unit-test branch May 17, 2023 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Added the test-coverage for completion.go and version.go inside cli pkg #3233

🌱 Added the test-coverage for completion.go and version.go inside cli pkg #3233

yashsingh74 commented Feb 19, 2023 •

edited

Loading

k8s-ci-robot commented Feb 19, 2023

yashsingh74 commented Feb 19, 2023

camilamacedo86 left a comment •

edited

Loading

yashsingh74 commented Feb 19, 2023

yashsingh74 commented Mar 2, 2023

varshaprasad96 Mar 2, 2023

yashsingh74 Mar 13, 2023

camilamacedo86 commented Mar 13, 2023

varshaprasad96 Mar 13, 2023

yashsingh74 Mar 13, 2023

varshaprasad96 Mar 20, 2023

yashsingh74 Mar 20, 2023

varshaprasad96 Apr 11, 2023

yashsingh74 commented Mar 13, 2023

varshaprasad96 Apr 11, 2023

yashsingh74 commented Apr 28, 2023

camilamacedo86 left a comment

camilamacedo86 commented May 4, 2023

yashsingh74 commented May 16, 2023

camilamacedo86 left a comment

k8s-ci-robot commented May 16, 2023

	Expect(cmd.Execute()).Error()
	Expect(cmd.Execute()).Error().ShouldNot(HaveOccured())

🌱 Added the test-coverage for completion.go and version.go inside cli pkg #3233

🌱 Added the test-coverage for completion.go and version.go inside cli pkg #3233

Conversation

yashsingh74 commented Feb 19, 2023 • edited Loading

k8s-ci-robot commented Feb 19, 2023

yashsingh74 commented Feb 19, 2023

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

yashsingh74 commented Feb 19, 2023

yashsingh74 commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashsingh74 commented Mar 13, 2023

Choose a reason for hiding this comment

yashsingh74 commented Apr 28, 2023

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented May 4, 2023

yashsingh74 commented May 16, 2023

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 16, 2023

yashsingh74 commented Feb 19, 2023 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading