Unit tests for api.go by VenkatRamaraju · Pull Request #27 · operator-framework/java-operator-plugins

VenkatRamaraju · 2021-07-08T18:14:34Z

Added unit tests for v1alpha/api.go

Note: These tests depend on the v1_suite_test.go test suite file from this PR: #24.

laxmikantbpandhare · 2021-07-08T18:40:48Z

Working fine.

Running Suite: V1alpha Suite
============================
Random Seed: 1625769627
Will run 18 of 18 specs

••••••••••••••••••
Ran 18 of 18 Specs in 0.000 seconds
SUCCESS! -- 18 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

Ginkgo ran 1 suite in 1.119981223s
Test Suite Passed

laxmikantbpandhare

/lgtm

jmrodri

A few changes to the test structure.

jmrodri · 2021-07-14T03:00:04Z

pkg/quarkus/v1alpha/api_test.go

+		testAPIOptions := &createAPIOptions{
+			CRDVersion: "testVersion",
+			Namespaced: true,
+		}
+		updateTestResource := resource.Resource{}
+		testAPIOptions.UpdateResource(&updateTestResource)
+
+		It("verify that resource API's CRDVersion was set", func() {
+			Expect(updateTestResource.API.CRDVersion, testAPIOptions.CRDVersion)
+		})
+
+		It("verify that resource API's Namespaced was set", func() {
+			Expect(updateTestResource.API.Namespaced, testAPIOptions.Namespaced)
+		})
+
+		It("verify that resource path is an empty string", func() {
+			Expect(updateTestResource.Path, "")
+		})
+
+		It("verify that resource controller is false", func() {
+			Expect(updateTestResource.Controller).To(BeFalse())
+		})


The UpdateResource method call should be in the It blocks. OR put it in a BeforeEach block.

jmrodri · 2021-07-14T03:09:35Z

pkg/quarkus/v1alpha/api_test.go

+		testAPIOptions := &createAPIOptions{
+			CRDVersion: "testVersion",
+			Namespaced: true,
+		}
+		updateTestResource := resource.Resource{}


I would move these into a BeforeEach block.

Suggested change

testAPIOptions := &createAPIOptions{

CRDVersion: "testVersion",

Namespaced: true,

}

updateTestResource := resource.Resource{}

var (

testAPIOptions createAPIOptions

updateTestResource resource.Resource

)

BeforeEach(func() {

testAPIOptions = &createAPIOptions{

CRDVersion: "testVersion",

Namespaced: true,

}

updateTestResource = resource.Resource{}

})

jmrodri · 2021-07-14T03:11:05Z

pkg/quarkus/v1alpha/api_test.go

+			Namespaced: true,
+		}
+		updateTestResource := resource.Resource{}
+		testAPIOptions.UpdateResource(&updateTestResource)


Make this call inside the It blocks. Or you could put all of the expects in a single It. Either way is fine. I'm okay with it being in multiple Its.

Suggested change

testAPIOptions.UpdateResource(&updateTestResource)

It("...", func() {

testAPIOptions.UpdateResource(&updateTestResource)

Expect(....)

})

jmrodri · 2021-07-14T03:11:32Z

pkg/quarkus/v1alpha/api_test.go

+		testAPIOptions.UpdateResource(&updateTestResource)
+
+		It("verify that resource API's CRDVersion was set", func() {
+			Expect(updateTestResource.API.CRDVersion, testAPIOptions.CRDVersion)


This suggestion applies to all the Expects

Suggested change

Expect(updateTestResource.API.CRDVersion, testAPIOptions.CRDVersion)

Expect(updateTestResource.API.CRDVersion).To(Equal(testAPIOptions.CRDVersion))

jmrodri · 2021-07-14T03:12:17Z

pkg/quarkus/v1alpha/api_test.go

+
+	Describe("BindFlags", func() {
+		flagTest := pflag.NewFlagSet("testFlag", -1)
+		testAPISubcommand.BindFlags(flagTest)


Put in the It

jmrodri · 2021-07-14T03:15:17Z

pkg/quarkus/v1alpha/api_test.go

+	Describe("Run", func() {
+		It("should return nil", func() {
+			Expect(testAPISubcommand.Run(machinery.Filesystem{})).To(BeNil())
+		})
+	})
+
+	Describe("Validate", func() {
+		It("should return nil", func() {
+			Expect(testAPISubcommand.Validate()).To(BeNil())
+		})
+	})
+
+	Describe("PostScaffold", func() {
+		It("should return nil", func() {
+			Expect(testAPISubcommand.PostScaffold()).To(BeNil())
+		})
+	})


These tests are using a testApiSubcommand that you created in another test. You want tests to always have a clean version, they should not depend on what happens in another test. That's why the BeforeEach comes in handy.

jmrodri · 2021-07-14T03:16:23Z

pkg/quarkus/v1alpha/api_test.go

+			Plural: "test-plural",
+		}
+
+		groupErr := failAPISubcommand.InjectResource(&failResource)


These calls should be included with the It blocks. Basically consider the It as a specific test and it should include the call and the expectations.

jmrodri · 2021-07-14T03:17:19Z

pkg/quarkus/v1alpha/api_test.go

+		failResource.GVK.Group = "test-group"
+		versionErr := failAPISubcommand.InjectResource(&failResource)


This has to be inside the It block. Otherwise you are affecting subsequent tests.

jmrodri · 2021-07-14T03:17:31Z

pkg/quarkus/v1alpha/api_test.go

+		failResource.GVK.Version = "v1"
+		kindError := failAPISubcommand.InjectResource(&failResource)


Same comment as above

jmrodri · 2021-07-14T03:17:38Z

pkg/quarkus/v1alpha/api_test.go

+			Expect(kindError).To(HaveOccurred())
+		})
+
+		noErr := testAPISubcommand.InjectResource(&testResource)


jmrodri

/lgtm

laxmikantbpandhare assigned VenkatRamaraju Jul 8, 2021

laxmikantbpandhare requested review from jmrodri and laxmikantbpandhare July 8, 2021 18:41

laxmikantbpandhare approved these changes Jul 8, 2021

View reviewed changes

openshift-ci bot assigned laxmikantbpandhare Jul 8, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021

laxmikantbpandhare assigned VenkatRamaraju and unassigned VenkatRamaraju and laxmikantbpandhare Jul 8, 2021

laxmikantbpandhare linked an issue Jul 8, 2021 that may be closed by this pull request

Meta-issue: Java Operator #1

Closed

26 tasks

jmrodri requested changes Jul 14, 2021

View reviewed changes

Added api unit test

0838cb1

VenkatRamaraju force-pushed the API-Tests branch from b00cc56 to 0838cb1 Compare July 14, 2021 18:04

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021

VenkatRamaraju requested a review from jmrodri July 14, 2021 18:19

jmrodri approved these changes Jul 16, 2021

View reviewed changes

openshift-ci bot assigned jmrodri Jul 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2021

jmrodri merged commit 458fd27 into operator-framework:main Jul 16, 2021

	Expect(updateTestResource.API.CRDVersion, testAPIOptions.CRDVersion)
	Expect(updateTestResource.API.CRDVersion).To(Equal(testAPIOptions.CRDVersion))

		failResource.GVK.Group = "test-group"
		versionErr := failAPISubcommand.InjectResource(&failResource)

		failResource.GVK.Version = "v1"
		kindError := failAPISubcommand.InjectResource(&failResource)

Conversation

VenkatRamaraju commented Jul 8, 2021

Uh oh!

laxmikantbpandhare commented Jul 8, 2021

Uh oh!

laxmikantbpandhare left a comment

Choose a reason for hiding this comment

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants