Introduce integration-tests framework #391

qu1queee · 2020-09-14T19:15:09Z

Add test-integration framework. This was the last missing testing level from our side, based on kubernetes sig-testing. This should allow developers to get more confidence in upcoming PRs. Ideally you want a PR to at least pass unit-tests and integration-tests.

Running integration-tests is very simple, is just a single Make target call, no need to set environment variable or similar, only requirement is a k8s cluster and tekton installed. This also should simplify what e2e tests is currently testing, now e2e tests should be only one assertion around if the image was successfully build or not. I added documentation, which explains more in detail everything.

This PR contains 5 commits, first commit contains the main logic, the other commits are generated clients, vendor packages , documentation and one more commit to address fixes on the pr. I recommend to click on each commit to simplify the view of the changes.

integration test generates a new type of test to validate all the different integrations between all components that take place during a build:

builds to buildruns
buildruns to taskruns
resources like secrets and sa to builds and buildruns

and any other interaction one can think of.

This integration framework is implemented on the following way:

an instance of the build operator is generated per test spec
a namespace is generated per test spec
a set of resources are generated per test spec to validate their interactions
cleans the generated resources , operator and namespace after the test is finish

The above implementation is based on https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/integration-tests.md

This framework requires to generate:

clientsets for the Build operator. Based on https://www.openshift.com/blog/kubernetes-deep-dive-code-generation-customresources
With clientsets, other golang projects can use our CRDs resources
With clientsets, we can easily test our CRDs during integration

To run the integration test you only need:

cluster
tekton controller installed
run make test-integration

For e2e tests:

I simplify the asserting, to only assert if an image was successfully build or not, we do not longer need to trigger more assertions around TaskRun and BuildRun status, this is already covered in the integration-tests.

HeavyWombat · 2020-10-05T09:04:51Z

test/integration/build_to_buildruns_test.go

+
+		It("should fail the builRun with a Reason", func() {
+
+			Expect(tb.CreateBuild(buildObject)).To(BeNil())


I do not really have a strong opinion on this, but I like to use HaveOccurred() for an Expect where I know the return value is an error, mostly for readability.

Suggested change

Expect(tb.CreateBuild(buildObject)).To(BeNil())

Expect(tb.CreateBuild(buildObject)).ToNot(HaveOccurred())

Now that I read the suggestion when it is rendered here, it actually reads more confusing with ToNot HaveOccurred.

Would be interested in other feedback on this. I am fine with BeNil here.

I think I was doing this based on the typical go code block that looks like:

if err != nil { }

Im fine keeping it the way it is.

test/integration/utils/builds.go

test/integration/utils/environment.go

HeavyWombat · 2020-10-05T09:55:01Z

cmd/manager/main.go

@@ -102,8 +100,13 @@ func main() {
 	}
 	defer r.Unset()


To be fair, it is very edge case, but it triggered me a little to see a defer in the same function with an os.Exit(). See https://stackoverflow.com/questions/27629380/how-to-exit-a-go-program-honoring-deferred-calls for a discussion about it.

If is fine for you, I will leave it like this for now.

The whole Ready interface usage will go away once we bump the sdk, because is a package they do not longer have. Once we do this update, we can refactor the above to ensure the defer can work with the os.Exit.

docs/development/testing.md

test/catalog.go

HeavyWombat

Finally got around to look through the many changes in this PR. There are a couple of function names I would like to see using the full name, because that seems to be common schema. Other than that, it does look like a nice addition to the tests to me.

This generates a new type of test to validate all the different integrations between all components that take place during a build: - builds to buildruns - buildruns to taskruns - resources like secrets and sa to builds and buildruns This framework will generate for each test spec: - an instance of the build operator - a namespace to run the test - a set of resources to test - cleans the resources and namespace after the test is finish To run the integration test you only need: - cluster - tekton controller installed - run make test-integration

These are needed for generating tekton pipeline objects during integration tests. We might use this for e2e in the future. The vendor update only includes: - tekton clientset dependencies

I needed to refactor the whole docs for testing, and introduced a new testing specific document that containts all related information around the three types of testing we now have.

HeavyWombat

/lgtm

qu1queee · 2020-10-08T12:50:15Z

/approve

openshift-ci-robot · 2020-10-08T12:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HeavyWombat, qu1queee

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~~ [qu1queee]

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

qu1queee added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2020

openshift-ci-robot requested review from sbose78 and zhangtbj September 14, 2020 19:15

qu1queee force-pushed the qu1queee/add_integration_tests branch 3 times, most recently from a0b1d20 to 91fa1a4 Compare September 14, 2020 20:11

HeavyWombat mentioned this pull request Sep 15, 2020

Add genclient tags to build resources #387

Closed

qu1queee force-pushed the qu1queee/add_integration_tests branch 6 times, most recently from 82803ce to e83c066 Compare September 16, 2020 06:35

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

qu1queee force-pushed the qu1queee/add_integration_tests branch from 9a0f7d2 to 9faab5f Compare September 22, 2020 12:14

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

qu1queee force-pushed the qu1queee/add_integration_tests branch 9 times, most recently from 558db0c to 276fb75 Compare September 23, 2020 07:43

qu1queee changed the title ~~WIP: Introduce integration-tests framework~~ Introduce integration-tests framework Sep 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020

qu1queee requested review from adambkaplan and removed request for sbose78 and zhangtbj September 23, 2020 09:52