-
Notifications
You must be signed in to change notification settings - Fork 277
Refactor - Making testing & writing generators more easy #26
Changes from all commits
426343b
93e0a10
9e25d45
0a5c584
a038baa
d08efb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ package controllers | |
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"github.com/argoproj-labs/applicationset/pkg/generators" | ||
argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/mock" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/client-go/tools/record" | ||
|
@@ -16,7 +19,149 @@ import ( | |
argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1" | ||
) | ||
|
||
func TestCreateOrUpdateApplications(t *testing.T) { | ||
type generatorMock struct { | ||
mock.Mock | ||
} | ||
|
||
func (g *generatorMock) GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) ([]map[string]string, error) { | ||
args := g.Called(appSetGenerator) | ||
|
||
return args.Get(0).([]map[string]string), args.Error(1) | ||
} | ||
|
||
type rendererMock struct { | ||
mock.Mock | ||
} | ||
|
||
func (r *rendererMock) RenderTemplateParams(tmpl *argov1alpha1.Application, params map[string]string) (*argov1alpha1.Application, error) { | ||
args := r.Called(tmpl, params) | ||
|
||
if args.Error(1) != nil { | ||
return nil, args.Error(1) | ||
} | ||
|
||
return args.Get(0).(*argov1alpha1.Application), args.Error(1) | ||
|
||
} | ||
|
||
func TestExtractApplications(t *testing.T) { | ||
scheme := runtime.NewScheme() | ||
argoprojiov1alpha1.AddToScheme(scheme) | ||
argov1alpha1.AddToScheme(scheme) | ||
|
||
client := fake.NewFakeClientWithScheme(scheme) | ||
|
||
for _, c := range []struct { | ||
name string | ||
params []map[string]string | ||
template argoprojiov1alpha1.ApplicationSetTemplate | ||
generateParamsError error | ||
rendererError error | ||
}{ | ||
{ | ||
name: "Generate two applications", | ||
params: []map[string]string{{"name": "app1"}, {"name": "app2"}}, | ||
template: argoprojiov1alpha1.ApplicationSetTemplate{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "name", | ||
Namespace: "namespace", | ||
Labels: map[string]string{ "label_name": "label_value"}, | ||
}, | ||
Spec: argov1alpha1.ApplicationSpec{ | ||
|
||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Handles error for the generator", | ||
generateParamsError: errors.New("error"), | ||
}, | ||
{ | ||
name: "Handles error from the render", | ||
params: []map[string]string{{"name": "app1"}, {"name": "app2"}}, | ||
template: argoprojiov1alpha1.ApplicationSetTemplate{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "name", | ||
Namespace: "namespace", | ||
Labels: map[string]string{ "label_name": "label_value"}, | ||
}, | ||
Spec: argov1alpha1.ApplicationSpec{ | ||
|
||
}, | ||
}, | ||
rendererError: errors.New("error"), | ||
}, | ||
}{ | ||
cc := c | ||
app := argov1alpha1.Application{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "test", | ||
}, | ||
} | ||
|
||
t.Run(cc.name, func(t *testing.T) { | ||
|
||
generatorMock := generatorMock{} | ||
generator := argoprojiov1alpha1.ApplicationSetGenerator{ | ||
List: &argoprojiov1alpha1.ListGenerator{}, | ||
} | ||
|
||
generatorMock.On("GenerateParams", &generator). | ||
Return(cc.params, cc.generateParamsError) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
rendererMock := rendererMock{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
expectedApps := []argov1alpha1.Application{} | ||
|
||
if cc.generateParamsError == nil { | ||
for _, p := range cc.params { | ||
|
||
if cc.rendererError != nil { | ||
rendererMock.On("RenderTemplateParams", getTempApplication(cc.template), p). | ||
Return(nil, cc.rendererError) | ||
} else{ | ||
rendererMock.On("RenderTemplateParams", getTempApplication(cc.template), p). | ||
Return(&app, nil) | ||
expectedApps = append(expectedApps, app) | ||
} | ||
} | ||
} | ||
|
||
r := ApplicationSetReconciler{ | ||
Client: client, | ||
Scheme: scheme, | ||
Recorder: record.NewFakeRecorder(1), | ||
Generators: []generators.Generator{ | ||
&generatorMock, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is more E2E test, I will think about how to do it and open an issue |
||
}, | ||
Renderer: &rendererMock, | ||
} | ||
|
||
got := r.generateApplications(argoprojiov1alpha1.ApplicationSet{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "name", | ||
Namespace: "namespace", | ||
}, | ||
Spec: argoprojiov1alpha1.ApplicationSetSpec{ | ||
Generators: []argoprojiov1alpha1.ApplicationSetGenerator{generator}, | ||
Template: cc.template, | ||
}, | ||
},) | ||
|
||
assert.Equal(t, expectedApps, got) | ||
generatorMock.AssertNumberOfCalls(t, "GenerateParams", 1) | ||
|
||
if cc.generateParamsError == nil { | ||
rendererMock.AssertNumberOfCalls(t, "RenderTemplateParams", len(cc.params)) | ||
} | ||
|
||
}) | ||
} | ||
|
||
|
||
} | ||
|
||
func TestCreateOrUpdateInCluster(t *testing.T) { | ||
|
||
scheme := runtime.NewScheme() | ||
argoprojiov1alpha1.AddToScheme(scheme) | ||
|
@@ -203,7 +348,7 @@ func TestCreateOrUpdateApplications(t *testing.T) { | |
|
||
} | ||
|
||
func TestDeleteApplications(t *testing.T) { | ||
func TestDeleteInCluster(t *testing.T) { | ||
|
||
scheme := runtime.NewScheme() | ||
argoprojiov1alpha1.AddToScheme(scheme) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling this out into the controller seems like a really great simplification. 👍