Add envtest controller tests for DevWorkspaces #957

amisevsk · 2022-10-22T04:56:02Z

What does this PR do?

Sets up tooling and configuration to allow running controller tests via envtest. These tests work by launching a control plane and allowing the controller to "interact" with it -- i.e. creating DevWorkspaces and related objects.

Currently a draft PR as this is a revival of something I started back in January and hadn't touched again until a few weeks ago; I'm not sure the overall quality.

Tests can still be run without installing the ginkgo CLI, but the output is nicer if it's present.

Controller-gen assumes a newer version of ginkgo/gomega than was used previously; these packages are updated and that requires updates to existing e2e test code.

Docs:

Kubebuilder controller tests: https://book.kubebuilder.io/cronjob-tutorial/writing-tests.html
Envtest setup: https://book.kubebuilder.io/reference/envtest.html
Ginkgo docs: https://onsi.github.io/ginkgo/
Gomega docs: https://onsi.github.io/gomega/

What issues does this PR fix or reference?

Closes #930

Is it tested? How?

They're tests :)

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

openshift-ci · 2022-10-22T04:56:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2022-10-22T05:20:43Z

Codecov Report

Base: 54.11% // Head: 52.36% // Decreases project coverage by -1.74% ⚠️

Coverage data is based on head (b54b578) compared to base (255d699).
Patch coverage: 16.66% of modified lines in pull request are covered.

❗ Current head b54b578 differs from pull request most recent head e1128bc. Consider uploading reports for the commit e1128bc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
- Coverage   54.11%   52.36%   -1.75%     
==========================================
  Files          44       46       +2     
  Lines        2770     4289    +1519     
==========================================
+ Hits         1499     2246     +747     
- Misses       1135     1841     +706     
- Partials      136      202      +66

Impacted Files	Coverage Δ
pkg/config/sync.go	`65.21% <0.00%> (-6.05%)`	⬇️
controllers/workspace/status.go	`75.58% <100.00%> (ø)`
pkg/library/overrides/containers.go	`63.15% <0.00%> (-12.52%)`	⬇️
pkg/provision/storage/ephemeralStorage.go	`28.00% <0.00%> (-7.30%)`	⬇️
pkg/library/flatten/flatten.go	`77.82% <0.00%> (-3.54%)`	⬇️
pkg/library/container/mountSources.go	`89.83% <0.00%> (-2.86%)`	⬇️
pkg/provision/storage/shared.go	`59.39% <0.00%> (-2.59%)`	⬇️
pkg/library/flatten/merge.go	`75.13% <0.00%> (-1.61%)`	⬇️
pkg/config/migrate.go	`85.57% <0.00%> (-1.61%)`	⬇️
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

AObuchow

Looks awesome to me so far :)

One additional note:
The README should probably be updated to specify that go v1.17 is required, as well as Ginkgo CLI v2.0.0 (I initially tried v.2.4.0, and it wouldn't work).

AObuchow · 2022-10-25T19:54:56Z

controllers/workspace/util_test.go

+	By("Loading DevWorkspace from test file")
+	devworkspace := &dw.DevWorkspace{}
+	err := loadObjectFromFile(name, devworkspace, fromFile)
+	Expect(err).NotTo(HaveOccurred())


This is purely stylistic, but error checks for loadObjectFromFile like this one could be written: Expect(loadObjectFromFile(name, devworkspace, fromFile)).Should(Succeed())

Obviously, there's a tradeoff: you lose a line of code and don't care have to declare err, but it's a bit more verbose of a line to write out.

Good idea -- updated the PR.

AObuchow · 2022-10-25T20:38:35Z

controllers/workspace/util_test.go

+	}
+}
+
+func namespacedName(name, namespace string) types.NamespacedName {


In a future PR, we probably should add a similar function to use elsewhere in the codebase 👍

Let me know if you think this is worthwhile, and I'll create an issue about it.

Eh, I think it's probably not worth a separate package that has to be imported everywhere. Golang is a language that includes code duplication by design 🤷

probably not worth a separate package that has to be imported everywhere

Agreed +1

Golang is a language that includes code duplication by design 🤷

Very true ;P

AObuchow · 2022-10-25T20:42:10Z

.github/workflows/pr.yml

        make test
+    -
+      name: Build Codecov report


Is there a particular reason why this step was added to the PR CI action? The code-coverage CI action already occurs on pull-requests.

I know codecov was acting weird for this PR - I assume it's because there's no controller.cover.out being referenced by codecov on the main branch, so it has nothing to compare the coverage against.

Note that the Codecov action was also disabled for PRs; previously we were running the tests as part of the PR check and generating coverage files, and then running them again as part of the Codecov action; it's easier to just upload the already-generated files.

I should break this out into a separate PR perhaps; maybe that would fix the codecov weirdness?

it's easier to just upload the already-generated files

Agreed, it'll speed things up. If we do this, it's probably worth removing the PR trigger for the code coverage action (and only have it run on push to main), cause otherwise it will get run twice for PR's (I would imagine?).

I should break this out into a separate PR perhaps; maybe that would fix the codecov weirdness?

It should fix the weirdness because then there will be a reference point of coverage % for controller.cover.out on the main branch. But if this causes other problems, then it's totally fine to leave it. Future PR's should not have this issue once the controller.cover.out is uploaded to codecov.

Agreed, it'll speed things up. If we do this, it's probably worth removing the PR trigger for the code coverage action (and only have it run on push to main), cause otherwise it will get run twice for PR's (I would imagine?).

I thought I had already done this -- https://github.com/devfile/devworkspace-operator/pull/957/files#diff-49708f979e226a1e7bd7a68d71b2e91aae8114dd3e9254d9830cd3b4d62d4303L7-L8. Is anything else required?

My understanding is that codecov merges all uploaded files: https://docs.codecov.com/docs/merging-reports. I think this means that it should correctly compute a diff even if we were to e.g. split coverage reports per package.

I thought I had already done this -- #957 (files). Is anything else required?

Oops I forgot about that when I wrote this - that should be all that's needed for the code coverage action.

My understanding is that codecov merges all uploaded files: docs.codecov.com/docs/merging-reports. I think this means that it should correctly compute a diff even if we were to e.g. split coverage reports per package.

Yes, that seems to be the case. My theory about the controller.cover.out not being present on master doesn't seem to hold up then.

I think it's okay to just include the CI changes in this PR and if codecov continues to display incorrect coverage reports after this PR is merged, I will look into it.

openshift-ci · 2022-12-06T14:04:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow, ibuziuk

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

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

Update to testing packages Ginkgo and Gomega to version expected by kubebuilder bootstrapped test files. This requires minor updates to the previous end to end tests as Ginkgo v2 does not support custom reporters in the same way. Signed-off-by: Angel Misevski <[email protected]>

Bootstrap test files for DevWorkspace controller and add a basic DevWorkspace creation test. Signed-off-by: Angel Misevski <[email protected]>

Add controller tests to cover creation of various objects used workspaces (roles, rolebindings, devworkspaceroutings, deployments, etc.) Verify that each created object is owned by the DevWorkspace that defines it. Signed-off-by: Angel Misevski <[email protected]>

Introduce a fake http.Transport to allow simulating responses to HTTP calls in DevWorkspace Operator tests. This is required for testing that DevWorkspaces with mainUrls can enter the Running state. Signed-off-by: Angel Misevski <[email protected]>

Add test to verify that workspaces can enter the running state Signed-off-by: Angel Misevski <[email protected]>

Fix issue where workspaces with a mainUrl with path that did not end in a '/' resulted in the incorrect path for the '/healthz' endpoint due to DWO only concatenating strings. Signed-off-by: Angel Misevski <[email protected]>

Extract common test functionality to functions that can be reused to make test code less painfully long. Signed-off-by: Angel Misevski <[email protected]>

Add test cases covering automatic mounting of image pull secrets, configmaps/secrets, git credentials. Signed-off-by: Angel Misevski <[email protected]>

Rework makefile rules to automatically download binaries needed for running envtest tests. New versions of envtest ship a binary instead of shell script, making the install process different, so the new rules are copied from the Makefile bootstrapped by recent versions of controller-gen Signed-off-by: Angel Misevski <[email protected]>

Add fixups for tests (originally written in January 2022) to accommodate changes to DevWorkspace controller functionality since then. Signed-off-by: Angel Misevski <[email protected]>

Regenerate DeepCopy files for CRDs after switching how controller-gen is downloaded. Previously, we would download + build controller-gen locally and use that in scripts. Now we `go install` it to ./bin and use that, which is a lot simpler. However, despite both instances using controller-gen v0.6.1, the output of deepcopy-gen is different: Previously, locally-built controller gen included the line //go:build !ignore_autogenerated at the top of deepcopy files The new, `go install`ed version omits this line. ¯\_(ツ)_/¯ Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Creating multiple workspaces during a test is required for testing functionality that only triggers when multiple workspaces exist (e.g. PVC cleanup) Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

* Update make rule to run envtest tests using Ginkgo CLI if available * Update github actions to install ginkgo and upload both coverage files * Allow skipping envtest tests by setting env var Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

openshift-ci · 2022-12-12T17:29:53Z

New changes are detected. LGTM label has been removed.

* Test new ServiceAccount behavior (role binds to individual serviceaccounts rather than all SAs in namespace) * Set default PodSecurityContext when using a config for testing to accomodate changes around how PodSecurityContext is handled in OpenShift Signed-off-by: Angel Misevski <[email protected]>

amisevsk requested review from AObuchow and dkwon17 October 22, 2022 04:56

openshift-ci bot added the do-not-merge/work-in-progress label Oct 22, 2022

openshift-ci bot added the approved label Oct 22, 2022

amisevsk force-pushed the test-coverage branch 2 times, most recently from 970f556 to 755db23 Compare October 22, 2022 05:16

amisevsk force-pushed the test-coverage branch from 755db23 to a5fcf48 Compare October 22, 2022 05:34

AObuchow reviewed Oct 25, 2022

View reviewed changes

amisevsk mentioned this pull request Oct 27, 2022

Cherry-pick minor fixes from open PRs #960

Merged

3 tasks

amisevsk force-pushed the test-coverage branch from a5fcf48 to b54b578 Compare October 28, 2022 17:11

amisevsk marked this pull request as ready for review November 18, 2022 16:32

amisevsk requested a review from ibuziuk as a code owner November 18, 2022 16:32

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 18, 2022

AObuchow approved these changes Dec 1, 2022

View reviewed changes

openshift-ci bot assigned AObuchow Dec 1, 2022

openshift-ci bot added the lgtm label Dec 1, 2022

ibuziuk approved these changes Dec 6, 2022

View reviewed changes

openshift-ci bot assigned ibuziuk Dec 6, 2022

amisevsk added 9 commits December 12, 2022 12:27

Bootstrap controller tests and add basic test cases

542e853

Bootstrap test files for DevWorkspace controller and add a basic DevWorkspace creation test. Signed-off-by: Angel Misevski <[email protected]>

Test workspaces entering Running state

d030de4

Add test to verify that workspaces can enter the running state Signed-off-by: Angel Misevski <[email protected]>

Rework controller test utilities to reduce repetition

40924a8

Extract common test functionality to functions that can be reused to make test code less painfully long. Signed-off-by: Angel Misevski <[email protected]>

Add DevWorkspace controller tests for automount functionality

dd0fcab

Add test cases covering automatic mounting of image pull secrets, configmaps/secrets, git credentials. Signed-off-by: Angel Misevski <[email protected]>

amisevsk added 11 commits December 12, 2022 12:27

Fix workspace controller tests after rebasing on main

f86dfb7

Add fixups for tests (originally written in January 2022) to accommodate changes to DevWorkspace controller functionality since then. Signed-off-by: Angel Misevski <[email protected]>

Add tests to cover overriding autogenerated workspace IDs

0133d95

Signed-off-by: Angel Misevski <[email protected]>

Add devworkspace tests to cover stopping DevWorkspaces

d982f0d

Signed-off-by: Angel Misevski <[email protected]>

Extract test functionality to avoid repetition and reduce verbosity

7b1fbde

Signed-off-by: Angel Misevski <[email protected]>

Update test utilities to allow creating multiple workspaces

f41c66e

Creating multiple workspaces during a test is required for testing functionality that only triggers when multiple workspaces exist (e.g. PVC cleanup) Signed-off-by: Angel Misevski <[email protected]>

Add controller tests to cover deleting workspaces and finalizers

44f6dd4

Signed-off-by: Angel Misevski <[email protected]>

Move PR code coverage upload to PR check job to avoid rerunning tests

a23b75d

Signed-off-by: Angel Misevski <[email protected]>

Update Go in GitHub actions to 1.17.12 to match Dockerfiles

4b73526

Signed-off-by: Angel Misevski <[email protected]>

Clean up some error checking in controller tests

490987d

Signed-off-by: Angel Misevski <[email protected]>

amisevsk force-pushed the test-coverage branch from b54b578 to 490987d Compare December 12, 2022 17:28

openshift-ci bot removed the lgtm label Dec 12, 2022

amisevsk merged commit 4bb7a56 into devfile:main Dec 14, 2022

amisevsk deleted the test-coverage branch December 14, 2022 16:10

AObuchow mentioned this pull request Feb 10, 2023

Add DevWorkspaceRouting controller envtests #1042

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add envtest controller tests for DevWorkspaces #957

Add envtest controller tests for DevWorkspaces #957

amisevsk commented Oct 22, 2022

openshift-ci bot commented Oct 22, 2022

codecov bot commented Oct 22, 2022 •

edited

Loading

AObuchow left a comment •

edited

Loading

AObuchow Oct 25, 2022

amisevsk Oct 28, 2022

AObuchow Oct 25, 2022

AObuchow Oct 25, 2022

amisevsk Oct 28, 2022

AObuchow Oct 28, 2022

AObuchow Oct 25, 2022

amisevsk Oct 28, 2022

AObuchow Oct 28, 2022

amisevsk Oct 31, 2022

AObuchow Oct 31, 2022

openshift-ci bot commented Dec 6, 2022

openshift-ci bot commented Dec 12, 2022

Add envtest controller tests for DevWorkspaces #957

Add envtest controller tests for DevWorkspaces #957

Conversation

amisevsk commented Oct 22, 2022

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Oct 22, 2022

codecov bot commented Oct 22, 2022 • edited Loading

Codecov Report

AObuchow left a comment • edited Loading

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

openshift-ci bot commented Dec 6, 2022

openshift-ci bot commented Dec 12, 2022

codecov bot commented Oct 22, 2022 •

edited

Loading

AObuchow left a comment •

edited

Loading