Refactor - Making testing & writing generators more easy #26
Conversation
OmerKahani
changed the title
| ) | ||
|
|
||
| // Generator defines the interface implemented by all ApplicationSet generators. | ||
| type Generator interface { | ||
| // GenerateApplications interprets the ApplicationSet and generates all relevant Applications. | ||
| // The expected / desired list of Applications is returned, it then needs to be reconciled | ||
| // against the current state of the Applications in the cluster. | ||
| GenerateApplications(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, | ||
| appSet *argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) | ||
| GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) ([]map[string]string, error) |
There was a problem hiding this comment.
Godoc needs updating here.
Be sure to note that generators must safely return if the given appSetGenerator does not match their type. By iterating every generator for every appSet we run a little risk of something getting merged that doesn't and then you've got a crash even if you're not using that particular type of generator. This is slightly more risky than the previous version where our controller explicitly controls who is getting called based on what generator is populated in the object.
| } | ||
|
|
||
| for _, p := range params { | ||
| app, err := r.Renderer.RenderTemplateParams(tmplApplication, p) |
There was a problem hiding this comment.
Pulling this out into the controller seems like a really great simplification. 👍
| generatorMock.On("GenerateParams", &generator). | ||
| Return(cc.params, cc.generateParamsError) | ||
|
|
||
| rendererMock := rendererMock{} |
There was a problem hiding this comment.
Do we need a mock renderer? It seems like we should let it do it's thing so we can get the coverage to know everything is working.
There was a problem hiding this comment.
I agree we need to test the RenderTemplateParams, with unit tests or E2E; but I don't think that the applicationset_controller should test it, it's not part of the application_controller responsbilty
| } | ||
|
|
||
| generatorMock.On("GenerateParams", &generator). | ||
| Return(cc.params, cc.generateParamsError) |
There was a problem hiding this comment.
Actually same question for the list generator, wouldn't it be possible to use that entirely in memory and get better coverage the controller is working?
| Scheme: scheme, | ||
| Recorder: record.NewFakeRecorder(1), | ||
| Generators: []generators.Generator{ | ||
| &generatorMock, |
There was a problem hiding this comment.
Ideally I'd like to see this using the real ListGenerator, and also including all the other generators to help avoid the crash I alluded to earlier if a generator slips in that doesn't properly check it's struct for nil. We can just do list generator testing in this module, but if the others are present we could prevent a full application crash a user can't control.
There was a problem hiding this comment.
I think this is more E2E test, I will think about how to do it and open an issue
Extract common parts from the generators. Now a generator only creates the params for the application, and the rendering is happing outside.
Generator now has only one unique responsibility.
Refactor the if-else generator chooser to a simpler loop, and move the generators list to outside the function.
Making the code more clean, easy to test, and easy to add a new generator
This PR is going to brake every PR, so it will be better to merge: #25 , #24 , #21 before this one