Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Refactor e2e test #2058

Merged
merged 5 commits into from
May 22, 2019
Merged

Refactor e2e test #2058

merged 5 commits into from
May 22, 2019

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented May 16, 2019

  • To reuse artifacts, run e2e sequentially, in the same CircleCI task.
  • Support running e2e test locally (both in Linux and Darwin systems) through make e2e. An existing Kubernetes cluster will be used if available, otherwise a cluster will be created with Kind.

Supersedes #1895

@2opremio 2opremio force-pushed the refactor-e2e-tests branch 5 times, most recently from 3c1874a to cd958bb Compare May 17, 2019 00:31
@2opremio 2opremio changed the title Refactor e2e test [WIP] Refactor e2e test May 17, 2019
@2opremio 2opremio force-pushed the refactor-e2e-tests branch 16 times, most recently from c99bf09 to bd8750d Compare May 17, 2019 09:17
@2opremio 2opremio force-pushed the refactor-e2e-tests branch 2 times, most recently from fc81ed8 to e3f0555 Compare May 17, 2019 11:32
@2opremio 2opremio changed the title [WIP] Refactor e2e test Refactor e2e test May 17, 2019
@2opremio
Copy link
Contributor Author

The e2e-testing testing step doesn't exist anymore, so we should considered this build as green 💚

@hiddeco
Copy link
Member

hiddeco commented May 17, 2019

I tried to run this on my machine and I noticed a couple of things.

  1. If you have an existing minikube instance running which already has the CRD installed, it will fail.
  2. After I created a fresh minikube instance and ran make e2e again, it seems to fail on the Installing Flux with Helm step.
    <snip>
    deployment "gitsrv" successfully rolled out
    >>> Installing Flux with Helm
    Error: release flux failed: timed out waiting for the condition
    
    Running deferred items, please do not interrupt until they are done:
    deferred: rm -f /home/hidde/go/src/github.com/weaveworks/flux/test/e2e/id_rsa /home/hidde/go/src/github.com/weaveworks/flux/test/e2e/id_rsa.pub
    deferred: kubectl delete namespace flux-e2e
    namespace "flux-e2e" deleted
    deferred: helm reset --force
    Tiller (the Helm server-side component) has been uninstalled from your Kubernetes Cluster.
    deferred: kubectl delete clusterrolebinding tiller-cluster-rule
    clusterrolebinding.rbac.authorization.k8s.io "tiller-cluster-rule" deleted
    deferred: kubectl --namespace kube-system delete sa tiller
    serviceaccount "tiller" deleted
    make: *** [Makefile:59: e2e] Error 1
    
  3. The CRDs aren't truncated when it fails, making it hard to rerun the e2e tests given point 1.

@2opremio
Copy link
Contributor Author

2opremio commented May 17, 2019

  1. If you have an existing minikube instance running which already has the CRD installed, it will fail

helm install fails if they exist, and I thought preventively removing them is too invasive.

2. After I created a fresh minikube instance and ran make e2e again, it seems to fail on the Installing Flux with Helm step.

Huh, that's odd, I tested it both with Kind and my local Docker for Mac cluster. I will install minikube and give it a try.

3. The CRDs aren't truncated when it fails, making it hard to rerun the e2e tests given point

This means helm fails but gets to create the CRDs, right? I will look into this.

@hiddeco
Copy link
Member

hiddeco commented May 17, 2019

Helm install fails if they exist, and I thought preventively removing them is too invasive.

Removing them would probably be too invasive, maybe we can detect them and add a --no-crd-hook to helm install?

This means helm fails but gets to create the CRDs, right?

Correct, please note that purging a release with helm del --purge will not remove any CRDs installed by the chart.

@2opremio
Copy link
Contributor Author

Removing them would probably be too invasive, maybe we can detect them and add a --no-crd-hook to helm install?

I didn't know about that, will do.

@2opremio
Copy link
Contributor Author

2opremio commented May 17, 2019

Huh, that's odd, I tested it both with Kind and my local Docker for Mac cluster. I will install minikube and give it a try.

@hiddeco I tried with a Minikube instance in my Mac and it worked just fine, could you please try again?

test/e2e/run.sh Outdated Show resolved Hide resolved
@squaremo
Copy link
Member

Hrm. Part of my aim in #1895 was to make it easier to add more tests, by keeping the set-up separate from the test (and to some extent modular). What was the motivation for lumping them all together?

@2opremio
Copy link
Contributor Author

Hrm. Part of my aim in #1895 was to make it easier to add more tests, by keeping the set-up separate from the test (and to some extent modular). What was the motivation for lumping them all together?

It simply made the cleanup much easier. I can split them up, but I would like to drive that by an additional test. Would you mind if we do that later?

@squaremo
Copy link
Member

squaremo commented May 20, 2019

I can split them up, but I would like to drive that by an additional test. Would you mind if we do that later?

How much do you reckon would that end up looking like the prior state (or that in #1895)? If "not much", then OK sure let's do it later. If "somewhat", then I feel like that particular change in this PR is taking a step backwards, for a short term convenience.

I suppose that boils down to: is it better to roll forward from changes made to the bits of script here, or port those changes back to the scripts as they were. I have no especial attachment to the work in #1895 except in so far as it made it possible to run some a subset of the setup scripts.

@2opremio
Copy link
Contributor Author

2opremio commented May 20, 2019

@squaremo no strong opinions, the hardest part by far here was reusing the artifacts from the build in the e2e test (which isn't directly related to the scripts, it was mostly moving everything to a machine executor since Kind needs that). Also cleaning things up during local execution was tricky.

I think splitting things up again shouldn't be too hard. It may end up looking (in structure) like before, but I am not sure that should be a show stopper. I think the crucial bit is what parts we want to reuse between tests, which I want to drive by adding actual tests.

@squaremo
Copy link
Member

I think splitting things up again shouldn't be too hard. It may end up looking (in structure) like before, but I am not sure that should be a show stopper. I think the crucial bit is what parts we want to reuse between tests, which I want to drive by adding actual tests.

👍 You're in the best position to make that call, so all good by me

@2opremio
Copy link
Contributor Author

@hiddeco Can I have a rubber-stamp? (I will remove the e2e-testing check after merging)

@hiddeco
Copy link
Member

hiddeco commented May 21, 2019

(I will remove the e2e-testing check after merging)

I was waiting for this before stamping it, there you go 📮

* Run sequentially after build and tests in CircleCI to reuse artifacts
* Support running it locally (both in Linux and Darwin systems)
  through `make e2e`. An existing Kubernetes cluster will be used if available,
  otherwise a cluster will be created with Kind.
@2opremio 2opremio added this to the v1.12.3 milestone May 22, 2019
@2opremio 2opremio merged commit c15480f into fluxcd:master May 22, 2019
@2opremio 2opremio deleted the refactor-e2e-tests branch May 22, 2019 09:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants