🌱 Refactor testenv and remove unused ones #3159

vincepri · 2020-06-08T17:02:10Z

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR makes it so we can reuse one envtest across all our packages.

/assign @detiber @fabriziopandini

vincepri · 2020-06-08T18:08:55Z

/milestone v0.3.7

fabriziopandini · 2020-06-08T18:51:34Z

Nice cleanup!
/approve

k8s-ci-robot · 2020-06-08T18:51:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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~~ [fabriziopandini,vincepri]

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

vincepri · 2020-06-08T18:53:09Z

@fabriziopandini @benmoss pls lgtm :)

fabriziopandini · 2020-06-08T19:06:39Z

/lgtm

fabriziopandini · 2020-06-08T19:07:21Z

/hold
just noticed the breaking change.

detiber · 2020-06-08T19:07:36Z

/hold

detiber · 2020-06-08T19:08:00Z

holding, in the middle of a review, I think there are potential issues outside of the api change.

vincepri · 2020-06-08T19:09:34Z

~~The test/ package isn't under API contract~~ This hasn't been documented I guessed

detiber

If I'm understanding these changes correctly, this should allow for re-use of the same envtest environment across tests that are running under the same test process.

One gotcha is that Ginkgo-based parallel tests are actually running under separate processes, so if the goal is to re-use the same envtest for parallel ginkgo tests, then the calls to start/stop the test environment in BeforeSuite/AfterSuite will need to be moved into SyncronizedBeforeSuite/SynchronizedAfterSuite.

controlplane/kubeadm/controllers/suite_test.go

exp/controllers/suite_test.go

test/helpers/envtest.go

vincepri · 2020-06-08T21:53:36Z

/retitle 🌱 Refactor testenv and remove unused ones

Addressed all comments, I've opted to clean everything up and avoid finding a complex solution to reuse envtest across packages, few things I've noticed/discovered:

A full test environment was running in places where it shouldn't have been.
Go may run tests from different packages in parallel.
Go runs tests in a separate binary / context, it's not possible to use the same global package.

Cleaning everything up resulted in faster local runs. In general, we should be mindful that each time we spin up a new test environment in a new package it might affect the overall runtime of tests.

vincepri · 2020-06-08T21:53:53Z

/hold cancel

controllers/remote/suite_test.go

vincepri · 2020-06-08T22:54:10Z

scripts/ci-test.sh

@@ -24,9 +24,10 @@ cd "${REPO_ROOT}" || exit 1
 # shellcheck source=./hack/ensure-go.sh
 source "${REPO_ROOT}/hack/ensure-go.sh"

-make generate test


I've removed the generate pre-req from here, given that we're already checking it in pr-verify presubmit. This reduced the runtime for tests in CI from ~12 minutes to ~3 mins

vincepri · 2020-06-08T23:14:28Z

/test pull-cluster-api-test

Signed-off-by: Vince Prignano <[email protected]>

k8s-ci-robot · 2020-06-08T23:24:11Z

@vincepri: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`07db329`	link	`/test pull-cluster-api-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

benmoss · 2020-06-09T14:38:17Z

Doesn't look like this has much of an impact on my machine (8 cores, 32gb ram). I ran make test but with it modified to add -count=1 to avoid go test caching.

== master

real    0m51.821s
user    5m7.616s
sys     0m31.552s

real    0m54.547s
user    5m13.911s
sys     0m32.933s

real    0m53.169s
user    5m4.569s
sys     0m31.689s

== reuse-testenv

real    0m48.349s
user    4m40.661s
sys     0m29.396s

real    0m53.823s
user    4m40.636s
sys     0m29.350s

real    0m51.977s
user    4m40.644s
sys     0m30.269s

benmoss · 2020-06-09T14:40:26Z

still a good cleanup though!
/approve

fabriziopandini · 2020-06-09T15:22:28Z

/lgtm

detiber · 2020-06-09T15:17:35Z

controlplane/kubeadm/controllers/controller_test.go

@@ -60,7 +60,7 @@ var _ = Describe("KubeadmControlPlaneReconciler", func() {

 	Context("Reconcile a KubeadmControlPlane", func() {
 		It("should return error if owner cluster is missing", func() {
-			clusterName, clusterNamespace := "foo", "default"
+			clusterName, clusterNamespace := "foo-1", "default"


It seems like this should be using a generated namespace rather than "default", possibly one created in BeforeEach and deleted in AfterEach

detiber · 2020-06-09T15:20:23Z

controlplane/kubeadm/controllers/controller_test.go

+			Eventually(func() error {
+				_, err := r.Reconcile(ctrl.Request{NamespacedName: util.ObjectKey(kcp)})
+				return err
+			}, 10*time.Second).Should(HaveOccurred())


Why the switch to eventually here? Would it be better to have an eventually on the deletion being fully completed and then retrying the reconcile call instead?

detiber · 2020-06-09T15:35:04Z

test/helpers/envtest.go

 }

 func (t *TestEnvironment) StartManager() error {
 	return t.Manager.Start(t.doneMgr)
 }

 func (t *TestEnvironment) Stop() error {
-	close(t.doneMgr)
-	return t.Environment.Stop()
+	t.doneMgr <- struct{}{}


Does this actually stop the Manager? It looks like based on the docs that the manager is expected to stay running until the channel is closed: https://godoc.org/sigs.k8s.io/controller-runtime/pkg/manager#Runnable

k8s-ci-robot assigned detiber and fabriziopandini Jun 8, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 8, 2020

k8s-ci-robot requested review from JoelSpeed and ncdc June 8, 2020 17:02

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2020

vincepri changed the title ~~Reuse testenv across all our packages~~ 🌱 Reuse testenv across all our packages Jun 8, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 8, 2020

vincepri changed the title ~~🌱 Reuse testenv across all our packages~~ 🌱 Reuse envtest across all our packages Jun 8, 2020

vincepri force-pushed the reuse-testenv branch 2 times, most recently from 1278ec0 to 8d0239d Compare June 8, 2020 17:24

k8s-ci-robot added this to the v0.3.7 milestone Jun 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020

detiber reviewed Jun 8, 2020

View reviewed changes

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

vincepri force-pushed the reuse-testenv branch from 8d0239d to 87bb607 Compare June 8, 2020 21:50

vincepri force-pushed the reuse-testenv branch from 87bb607 to 776dffd Compare June 8, 2020 21:50

k8s-ci-robot changed the title ~~🌱 Reuse envtest across all our packages~~ 🌱 Refactor testenv and remove unused ones Jun 8, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020

vincepri force-pushed the reuse-testenv branch 4 times, most recently from 9ad1c3c to 4f97eed Compare June 8, 2020 22:43

sedefsavas reviewed Jun 8, 2020

View reviewed changes

controllers/remote/suite_test.go Show resolved Hide resolved

vincepri commented Jun 8, 2020

View reviewed changes

vincepri force-pushed the reuse-testenv branch 2 times, most recently from c7f1638 to a8e2a42 Compare June 8, 2020 23:04

🌱 Refactor testenv and remove unused ones

07db329

Signed-off-by: Vince Prignano <[email protected]>

vincepri force-pushed the reuse-testenv branch from a8e2a42 to 07db329 Compare June 8, 2020 23:22

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2020

k8s-ci-robot merged commit 61debe3 into kubernetes-sigs:master Jun 9, 2020

detiber reviewed Jun 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Refactor testenv and remove unused ones #3159

🌱 Refactor testenv and remove unused ones #3159

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

vincepri commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

detiber commented Jun 8, 2020

detiber commented Jun 8, 2020

vincepri commented Jun 8, 2020 •

edited

Loading

detiber left a comment

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

vincepri Jun 8, 2020

vincepri commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

benmoss commented Jun 9, 2020

benmoss commented Jun 9, 2020 •

edited

Loading

fabriziopandini commented Jun 9, 2020

detiber Jun 9, 2020

detiber Jun 9, 2020

detiber Jun 9, 2020

🌱 Refactor testenv and remove unused ones #3159

🌱 Refactor testenv and remove unused ones #3159

Conversation

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

vincepri commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

detiber commented Jun 8, 2020

detiber commented Jun 8, 2020

vincepri commented Jun 8, 2020 • edited Loading

detiber left a comment

Choose a reason for hiding this comment

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

vincepri Jun 8, 2020

Choose a reason for hiding this comment

vincepri commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

benmoss commented Jun 9, 2020

benmoss commented Jun 9, 2020 • edited Loading

fabriziopandini commented Jun 9, 2020

detiber Jun 9, 2020

Choose a reason for hiding this comment

detiber Jun 9, 2020

Choose a reason for hiding this comment

detiber Jun 9, 2020

Choose a reason for hiding this comment

vincepri commented Jun 8, 2020 •

edited

Loading

benmoss commented Jun 9, 2020 •

edited

Loading