Adding e2e make target, makefile adjustments, and e2e test framework #81
Conversation
Signed-off-by: dtfranz <dfranz@redhat.com>
dtfranz
changed the title
There was a problem hiding this comment.
Nice! I'm a big fan of getting this setup early so that there's a place for contributors to fairly easily add tests.
One other thing I think we should try to do early on is encourage more unit testing than we did in rukpak for the core functionality. Right now, much of rukpak's functional tests are in e2e's. In e2e's we can't (easily) get coverage metrics, so it's difficult to ensure we're covering critical sections in tests, e2e's generally take an order of magnitude more time to run (making dev cycles take longer way longer), and it's basically impossible to test individual state transitions.
Signed-off-by: dtfranz <dfranz@redhat.com>
Signed-off-by: dtfranz <dfranz@redhat.com>
dtfranz
changed the title
| test -s $(LOCALBIN)/kind || GOBIN=$(LOCALBIN) go install sigs.k8s.io/kind@v0.15.0 | ||
|
|
||
| .PHONY: ginkgo | ||
| ginkgo: $(GINKGO) ## Download ginkgo locally if necessary. | ||
| $(GINKGO): $(LOCALBIN) | ||
| test -s $(LOCALBIN)/ginkgo || GOBIN=$(LOCALBIN) go install github.com/onsi/ginkgo/v2/ginkgo@v2.1.4 |
There was a problem hiding this comment.
The test -s $(LOCALBIN)/<binName> tests are sufficient for CI because they'll always fail and we'll always get the correct version.
But locally, these can be annoying when we bump the version of one of these and you still have an old version from an earlier commit. If there's some diff in behavior between your local version and the one specified here, you'll be scratching your head trying to figure out why.
There are two solutions I've seen for this:
- Rukpak has a separate
toolsgo module with a separate go.mod that lists all the tool versions with make targets that alwaysgo build - A somewhat curated script that knows how to check the presence and version of the existing binary to see if a new one is needed called like this. It's mostly set and forget.
Option 1 is good if all of our tools are Go-based
Option 2 is probably slightly faster when actually using the tools, but requires some tweaks every now and then when a tool changes its release artifacts.
There was a problem hiding this comment.
This could be a follow-up for sure.
There was a problem hiding this comment.
I did notice number 1 there in rukpak's makefile. I initially was working with that but it ended up turning into a bit of a rabbit-hole with the amount of changes needed, since it appeared as though it would require re-working every built-in make target we got from kubebuilder, and then at that point it felt like I was going to end up with just a copy-paste of rukpak's makefile which would require re-testing all of the targets. We definitely need to take care of it at some point, if you think it's a good thing to take care of now I could give it another go. It just felt like it was getting a bit out-of-scope to me.
There was a problem hiding this comment.
I've created issue #85 to track this.
| } | ||
|
|
||
| var _ = BeforeSuite(func() { | ||
| cfg = ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
Could these tests go under suite_test.go? Just because we have the test framework already scaffolded by the tool?
There was a problem hiding this comment.
Just saw Joe's comment #81 (comment), please feel free to ignore my review :)
Only one nit (we could do in a follow up), we could use envtest even for the unit (case 1) tests (though it is mainly intended for integration) if we foresee the interaction of multiple individual components, and would have to test "Reconcile" anyway. This in the case (1) where we need deppy entity source, and essentially mock the functioning of controller. This is just because previously we had faced version incompatibility issues by using locally available binaries and had a hard time figuring the cause.
There was a problem hiding this comment.
Thanks Varsha! I have to admit, my knowledge on the envtest stuff is lacking here, so if it's alright with you I'd like to reserve that for a follow up.
There was a problem hiding this comment.
No worries, I have created an issue to follow up #86. We can start using envtest when we get to writing tests for reconciler depending on the complexity of those. Till then its good :)
| kind-load: kind ## Loads the currently constructed image onto the cluster | ||
| $(KIND) load docker-image $(IMG) --name $(KIND_CLUSTER_NAME) | ||
|
|
||
| kind-cluster: kind kind-cluster-cleanup ## Standup a kind cluster | ||
| $(KIND) create cluster --name ${KIND_CLUSTER_NAME} | ||
| $(KIND) export kubeconfig --name ${KIND_CLUSTER_NAME} | ||
|
|
||
| kind-cluster-cleanup: kind ## Delete the kind cluster | ||
| $(KIND) delete cluster --name ${KIND_CLUSTER_NAME} |
There was a problem hiding this comment.
Nit: Looking at consistency, we might want to rename these. If we wanted to go with k8s' V-O verbiage:
e.g.
kind-cluster -> kind-create-cluster
kind-cluster-cleanup -> kind-delete-cluster
kind-load -> kind-load-image
There was a problem hiding this comment.
I agree with your reasoning for these, but I also named them this way for the sake of consistency with rukpak, so we'd need to change them in both places if we also wanted to be internally consistent. If it's alright with you, I'd prefer to leave these at least for now.
Signed-off-by: dtfranz <dfranz@redhat.com>
|
/lgtm |
Adds the
e2etarget formakealong with the associated github action. Also adjustsrunto be more in-line with what we do in RukPak, running theoperator-controllerinside a kind cluster instead of the kubebuilder generated default behavior (run locally)To keep the changes to a minimum I've not fully adjusted the makefile to match RukPak's, so there are many functional and stylistic differences which we may want to resolve in the near future.
Signed-off-by: dtfranz dfranz@redhat.com