Unittests for cilium k8sd feature #704
Conversation
maci3jka
force-pushed
the
KU-1517/cilium-unittests
branch
from
48d1ff3
to
7cb185b
Compare
|
adresses also KU-1516 |
There was a problem hiding this comment.
Thanks a lot @maci3jka! Great work. Almost LGTM, but left some minor comments.
| g.Expect(status.Message).To(Equal(fmt.Sprintf(cilium.IngressDeployFailedMsgTmpl, | ||
| fmt.Errorf("failed to rollout restart cilium to apply ingress: %v", | ||
| fmt.Errorf("failed to restart cilium-operator deployment after 3 attempts: %w", | ||
| fmt.Errorf("failed to restart cilium-operator deployment: %w", | ||
| fmt.Errorf("failed to get deployment cilium-operator in namespace kube-system: %w", | ||
| errors.New("deployments.apps \"cilium-operator\" not found"))))), |
There was a problem hiding this comment.
This is too tightly coupled to the message structure and contents. I see several instances of testing the specific error message in this PR, can those be reverted? I would much rather use a simple errors.Is, if possible.
There was a problem hiding this comment.
I misunderstood other review comments. I updated the test. Regarding errors when it makes sense i use gomega expect(err).To(ErrorMatch()) which checks if the error is in the chain of errors will that be enough?
| g.Expect(status.Enabled).To(BeTrue()) | ||
| g.Expect(status.Message).To(Equal(cilium.EnabledMsg)) | ||
| g.Expect(status.Version).To(Equal(cilium.CiliumAgentImageTag)) | ||
| g.Expect(helmM.ApplyCalledWith).To(HaveLen(1)) |
There was a problem hiding this comment.
I'm not a fan of the tight coupling to the implementation details here. There is a time and place for testing the arguments or the number of arguments that a function was called with, e.g. when the function doesn't return anything or isn't easily testable. In this case, we have other information that can be validated, like the status, error, and final state/outcome. Since we can validate the final state or outcome more directly, I'd suggest removing the explicit argument checking from the tests.
There are lacks in unittest for cilium feature in k8sd this pr proves test coverage form 42.9% files, 44.4% statements to 85.7% files, 86.7% statements for github.com/canonical/k8s/pkg/k8sd/features/cilium package