Add unit tests for remote.kubernetes components #3770
Conversation
| return fakeClientset, nil | ||
| } | ||
|
|
||
| func Eventually(t *testing.T, min, max time.Duration, retries int, f func() error) { |
There was a problem hiding this comment.
Any reason why not using the other implementations we have available of similar function?
| if !reflect.DeepEqual(expect.Data, actual.Data) { | ||
| return fmt.Errorf("expected %#v, got %#v", expect, actual) | ||
| } |
There was a problem hiding this comment.
Nit: there's a slight advantage of using require or assert packages because they will print a nice diff for many data types when they are not equal that helps to spot where the issue is.
| // Do the initial Update() in Run() instead of in New(). | ||
| // That way New() is stateless and has no side effects. |
There was a problem hiding this comment.
Is it because of the initial poll or does the GetKubernetesClient try to talk to k8s on creation and can return errors if misconfigured? Not sure how New() is stateful, seems to me that multiple calls to New() will not modify internal state, but maybe I'm missing something.
By moving this from New to Run, we change the way errors will be handled, no? Errors in New I think generally give a more detailed information back to the user and can stop applying a new config?
There was a problem hiding this comment.
Sorry, "stateful" is the wrong word. What I meant is that New() won't have side effects. It makes it easier to substitute parts of the component with fake ones. I think it also aligns better with how Alloy components are meant to be designed. Normally you wouldn't want a New function to connect to external services, to create files, etc.
PR Description
Adding unit tests for
remote.kubernetescomponents which use a fake K8s to test whether the correct exports are produced.PR Checklist