add unit tests for cmd/bundle #3247

jberkhahn · 2020-06-16T23:35:35Z

Description of the change:
Add unit tests for the bundle validate and bundle create commands.

Motivation for the change:
Golang unit tests are another layer of robust CI when paired with the e2e and script testing we currently have.

jberkhahn · 2020-06-16T23:37:11Z

@estroz started work on this. I've had to refactor a couple things to make them accessible from the tests. I'm leaving the Run() tests as pending for the moment as I'm not sure how to test that stuff without doing the counterfeiter refactor, which we should probably wait for the KB integration before we do that. Please tag anyone else you think might be interested in this.

jberkhahn · 2020-06-18T00:34:39Z

this should be good to go

camilamacedo86 · 2020-06-18T09:54:32Z

cmd/operator-sdk/bundle/validate_test.go

+	PDescribe("Run", func() {
+	})


why it is required?

I put in pending tests just as a reminder that they need to be implemented, but was going to wait until after kubebuilder is merged in because it'll require some additional refactoring. I can remove them if you think that's not needed.

could you please add // todo(jberkhahn): {desc of what should be done} or remove them ?

camilamacedo86 · 2020-06-18T09:55:22Z

cmd/operator-sdk/bundle/create_test.go

+	PDescribe("Run", func() {
+	})


Why it is required?

camilamacedo86 · 2020-06-18T09:56:41Z

cmd/operator-sdk/bundle/cmd.go

 	channels       string
-	generateOnly   bool
+	GenerateOnly   bool


why we need these changes?

I had to make a bunch of fields public to be able to set them in the tests. It's because cobra normally populated the fields set via flag when you initialize the command struct, but I can't do it that way because I'm not cobra. So I have to make them public so I can manually set them.

camilamacedo86 · 2020-06-18T09:57:45Z

cmd/operator-sdk/bundle/cmd.go

-		newCreateCmd(),
-		newValidateCmd(),
+		NewCreateCmd(),
+		NewValidateCmd(),


Why we need to export the NewCreateCmd and NewValidateCmd?

So i can call them from the tests.

cmd/operator-sdk/bundle/create_test.go

camilamacedo86 · 2020-06-18T10:00:57Z

cmd/operator-sdk/bundle/create_test.go

+		})
+	})
+
+	Describe("Validate", func() {


Why the validate is in the create_test.go and not in the validate_test.go?

this is testing the argument validation method of the create command, not the bundle validate command

camilamacedo86 · 2020-06-18T10:01:52Z

cmd/operator-sdk/bundle/create_test.go

+)
+
+var _ = Describe("Running a bundle validate command", func() {
+	Describe("NewCreateCmd", func() {


Suggested change

Describe("NewCreateCmd", func() {

Describe("NewCreateCmd should", func() {

I prefer to leave those describes as just the name of the method they're testing

cmd/operator-sdk/bundle/create_test.go

estroz · 2020-06-18T21:05:38Z

cmd/operator-sdk/bundle/cmd_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package bundle_test


Are you unable to write these tests in the same package? I've done this before without issues.

Suggested change

package bundle_test

package bundle

You can, but I didn't on purpose. I'm trying to test the functionality of the package as a unit, so I don't want to be able to reach inside and use private stuff, as other packages won't be able to do that.

I would agree if these packages were intended to be libraries. For now can you keep tests in the same package so we don't have to export a bunch of stuff? Before exporting private stuff we should have a conversation about general CLI refactoring.

ok, I'll move everything into a single package

jberkhahn · 2020-06-18T23:02:13Z

updated. hopefully I didn't screw up squashing the commits

camilamacedo86 · 2020-06-19T23:21:10Z

cmd/operator-sdk/bundle/create.go

@@ -32,7 +32,7 @@ import (
 	"github.com/spf13/pflag"
 )

-type bundleCreateCmd struct {
+type createCmd struct {


I agree that the name is redundant and we could/should change but it is not part of the scope of this PR.
Could we do a PR just to do the renames?

woops, forgot to revert this when I moved everything back to private. fixed.

camilamacedo86

IMO: we are not testing here what we should test. See my comments.

cmd/operator-sdk/bundle/bundle_suite_test.go

jberkhahn · 2020-06-22T22:19:18Z

Updated. Cut out most of the simple reflective tests, had to do some actual refactoring because something touched validate and added a new image builder, so I incorporated it into the validate func there and added a test for it.

estroz

/lgtm

camilamacedo86 · 2020-06-23T23:52:25Z

cmd/operator-sdk/bundle/validate.go

+	if c.imageBuilder != containertools.DockerTool.String() &&
+		c.imageBuilder != containertools.PodmanTool.String() &&
+		c.imageBuilder != containertools.NoneTool.String() {
+		return fmt.Errorf("unrecognized image-builder option: %s", c.imageBuilder)
 	}
+
 	return nil


It should be another PR it is not part of the ad unit test scope. Could we please split it?

camilamacedo86

Just 2 nits. otherwise, it shows great.
Really tks for your terrific contribution 🥇

Co-authored-by: Camila Macedo <[email protected]>

jberkhahn · 2020-06-24T17:17:51Z

@camilamacedo86 updated

jberkhahn · 2020-06-30T20:50:45Z

Is there anything else that needs to be changed here or is it good to go? @camilamacedo86 @estroz

camilamacedo86

/lgtm
/approve

👍

openshift-ci-robot requested review from camilamacedo86 and joelanford June 16, 2020 23:35

jberkhahn added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2020

jberkhahn mentioned this pull request Jun 16, 2020

Add unit tests for OperatorSDK cmd layer #3246

Closed

jberkhahn force-pushed the bundle_tests branch 3 times, most recently from 44fbc81 to 156ab49 Compare June 18, 2020 00:19

jberkhahn requested a review from estroz June 18, 2020 00:34

jberkhahn removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2020

camilamacedo86 reviewed Jun 18, 2020

View reviewed changes

cmd/operator-sdk/bundle/create_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jun 18, 2020

View reviewed changes

cmd/operator-sdk/bundle/create_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jun 18, 2020

View reviewed changes

cmd/operator-sdk/bundle/create_test.go Outdated Show resolved Hide resolved

estroz reviewed Jun 18, 2020

View reviewed changes

jberkhahn force-pushed the bundle_tests branch from 278426b to 7e5cadd Compare June 18, 2020 23:01

camilamacedo86 reviewed Jun 19, 2020

View reviewed changes

jberkhahn force-pushed the bundle_tests branch from 7e5cadd to 4d9af22 Compare June 20, 2020 03:34

camilamacedo86 suggested changes Jun 20, 2020

View reviewed changes

camilamacedo86 reviewed Jun 20, 2020

View reviewed changes

cmd/operator-sdk/bundle/bundle_suite_test.go Show resolved Hide resolved

jberkhahn force-pushed the bundle_tests branch from 4d9af22 to 5f85f92 Compare June 22, 2020 22:03

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2020

jberkhahn force-pushed the bundle_tests branch from 5f85f92 to a6bc557 Compare June 22, 2020 22:18

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2020

jberkhahn force-pushed the bundle_tests branch 2 times, most recently from 308a77e to 5c73f15 Compare June 22, 2020 22:36

estroz approved these changes Jun 23, 2020

View reviewed changes

openshift-ci-robot assigned estroz Jun 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

camilamacedo86 reviewed Jun 23, 2020

View reviewed changes

add unit tests for cmd/bundle

6370862

Co-authored-by: Camila Macedo <[email protected]>

jberkhahn force-pushed the bundle_tests branch from 5c73f15 to 6370862 Compare June 24, 2020 17:17

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020

camilamacedo86 approved these changes Jul 1, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Jul 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2020

camilamacedo86 merged commit 80c1672 into operator-framework:master Jul 1, 2020

jberkhahn mentioned this pull request Sep 28, 2020

OWNERS: add jberkhahn as reviewer #3954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unit tests for cmd/bundle #3247

add unit tests for cmd/bundle #3247

jberkhahn commented Jun 16, 2020

jberkhahn commented Jun 16, 2020 •

edited

Loading

jberkhahn commented Jun 18, 2020

camilamacedo86 Jun 18, 2020

jberkhahn Jun 18, 2020

camilamacedo86 Jun 23, 2020

camilamacedo86 Jun 18, 2020

camilamacedo86 Jun 18, 2020

jberkhahn Jun 18, 2020

camilamacedo86 Jun 18, 2020

jberkhahn Jun 18, 2020

camilamacedo86 Jun 18, 2020

jberkhahn Jun 18, 2020

camilamacedo86 Jun 18, 2020

jberkhahn Jun 18, 2020

estroz Jun 18, 2020

jberkhahn Jun 18, 2020

estroz Jun 18, 2020

jberkhahn Jun 18, 2020

jberkhahn commented Jun 18, 2020

camilamacedo86 Jun 19, 2020

jberkhahn Jun 20, 2020

camilamacedo86 left a comment •

edited

Loading

jberkhahn commented Jun 22, 2020

estroz left a comment

camilamacedo86 Jun 23, 2020 •

edited

Loading

camilamacedo86 left a comment

jberkhahn commented Jun 24, 2020

jberkhahn commented Jun 30, 2020

camilamacedo86 left a comment

	Describe("NewCreateCmd", func() {
	Describe("NewCreateCmd should", func() {

add unit tests for cmd/bundle #3247

add unit tests for cmd/bundle #3247

Conversation

jberkhahn commented Jun 16, 2020

jberkhahn commented Jun 16, 2020 • edited Loading

jberkhahn commented Jun 18, 2020

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

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

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

Choose a reason for hiding this comment

jberkhahn commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

jberkhahn commented Jun 22, 2020

estroz left a comment

Choose a reason for hiding this comment

camilamacedo86 Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

jberkhahn commented Jun 24, 2020

jberkhahn commented Jun 30, 2020

camilamacedo86 left a comment

Choose a reason for hiding this comment

jberkhahn commented Jun 16, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Jun 23, 2020 •

edited

Loading