Skip to content

Commit 00a4ce0

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#4705 from kersten/chore/machinery-test-var-initialization
🌱 (chore): refactor unit tests to isolate mutable state with BeforeEach in pkg/machinery tests
2 parents 10576d0 + 8e59376 commit 00a4ce0

File tree

4 files changed

+116
-63
lines changed

4 files changed

+116
-63
lines changed

‎pkg/machinery/errors_test.go‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,30 @@ import (
2626

2727
var _ = Describe("Errors", func() {
2828
var (
29-
path = filepath.Join("path", "to", "file")
30-
testErr = errors.New("test error")
29+
path string
30+
testErr error
3131
)
3232

33+
BeforeEach(func() {
34+
path = filepath.Join("path", "to", "file")
35+
testErr = errors.New("test error")
36+
})
37+
3338
DescribeTable("should contain the wrapped error",
34-
func(err error) {
39+
func(getErr func() error) {
40+
err := getErr()
41+
Expect(err).To(HaveOccurred())
3542
Expect(errors.Is(err, testErr)).To(BeTrue())
3643
},
37-
Entry("for validate errors", ValidateError{testErr}),
38-
Entry("for set template defaults errors", SetTemplateDefaultsError{testErr}),
39-
Entry("for file existence errors", ExistsFileError{testErr}),
40-
Entry("for file opening errors", OpenFileError{testErr}),
41-
Entry("for directory creation errors", CreateDirectoryError{testErr}),
42-
Entry("for file creation errors", CreateFileError{testErr}),
43-
Entry("for file reading errors", ReadFileError{testErr}),
44-
Entry("for file writing errors", WriteFileError{testErr}),
45-
Entry("for file closing errors", CloseFileError{testErr}),
44+
Entry("for validate errors", func() error { return ValidateError{testErr} }),
45+
Entry("for set template defaults errors", func() error { return SetTemplateDefaultsError{testErr} }),
46+
Entry("for file existence errors", func() error { return ExistsFileError{testErr} }),
47+
Entry("for file opening errors", func() error { return OpenFileError{testErr} }),
48+
Entry("for directory creation errors", func() error { return CreateDirectoryError{testErr} }),
49+
Entry("for file creation errors", func() error { return CreateFileError{testErr} }),
50+
Entry("for file reading errors", func() error { return ReadFileError{testErr} }),
51+
Entry("for file writing errors", func() error { return WriteFileError{testErr} }),
52+
Entry("for file closing errors", func() error { return CloseFileError{testErr} }),
4653
)
4754

4855
// NOTE: the following test increases coverage

‎pkg/machinery/injector_test.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,14 @@ func (t *templateWithResource) InjectResource(res *resource.Resource) {
9393
}
9494

9595
var _ = Describe("injector", func() {
96-
tmp := templateBase{
97-
path: "my/path/to/file",
98-
ifExistsAction: Error,
99-
}
96+
var tmp templateBase
97+
98+
BeforeEach(func() {
99+
tmp = templateBase{
100+
path: "my/path/to/file",
101+
ifExistsAction: Error,
102+
}
103+
})
100104

101105
Context("injectInto", func() {
102106
Context("Config", func() {

‎pkg/machinery/mixins_test.go‎

Lines changed: 60 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,17 @@ var _ = Describe("TemplateMixin", func() {
4545
body = "content"
4646
)
4747

48-
tmp := mockTemplate{
49-
TemplateMixin: TemplateMixin{
50-
PathMixin: PathMixin{path},
51-
IfExistsActionMixin: IfExistsActionMixin{ifExistsAction},
52-
TemplateBody: body,
53-
},
54-
}
48+
var tmp mockTemplate
49+
50+
BeforeEach(func() {
51+
tmp = mockTemplate{
52+
TemplateMixin: TemplateMixin{
53+
PathMixin: PathMixin{path},
54+
IfExistsActionMixin: IfExistsActionMixin{ifExistsAction},
55+
TemplateBody: body,
56+
},
57+
}
58+
})
5559

5660
Context("GetPath", func() {
5761
It("should return the path", func() {
@@ -75,11 +79,15 @@ var _ = Describe("TemplateMixin", func() {
7579
var _ = Describe("InserterMixin", func() {
7680
const path = "path/to/file.go"
7781

78-
tmp := mockInserter{
79-
InserterMixin: InserterMixin{
80-
PathMixin: PathMixin{path},
81-
},
82-
}
82+
var tmp mockInserter
83+
84+
BeforeEach(func() {
85+
tmp = mockInserter{
86+
InserterMixin: InserterMixin{
87+
PathMixin: PathMixin{path},
88+
},
89+
}
90+
})
8391

8492
Context("GetPath", func() {
8593
It("should return the path", func() {
@@ -97,7 +105,11 @@ var _ = Describe("InserterMixin", func() {
97105
var _ = Describe("DomainMixin", func() {
98106
const domain = "my.domain"
99107

100-
tmp := mockTemplate{}
108+
var tmp mockTemplate
109+
110+
BeforeEach(func() {
111+
tmp = mockTemplate{}
112+
})
101113

102114
Context("InjectDomain", func() {
103115
It("should inject the provided domain", func() {
@@ -110,7 +122,11 @@ var _ = Describe("DomainMixin", func() {
110122
var _ = Describe("RepositoryMixin", func() {
111123
const repo = "test"
112124

113-
tmp := mockTemplate{}
125+
var tmp mockTemplate
126+
127+
BeforeEach(func() {
128+
tmp = mockTemplate{}
129+
})
114130

115131
Context("InjectRepository", func() {
116132
It("should inject the provided repository", func() {
@@ -123,7 +139,11 @@ var _ = Describe("RepositoryMixin", func() {
123139
var _ = Describe("ProjectNameMixin", func() {
124140
const name = "my project"
125141

126-
tmp := mockTemplate{}
142+
var tmp mockTemplate
143+
144+
BeforeEach(func() {
145+
tmp = mockTemplate{}
146+
})
127147

128148
Context("InjectProjectName", func() {
129149
It("should inject the provided project name", func() {
@@ -134,7 +154,11 @@ var _ = Describe("ProjectNameMixin", func() {
134154
})
135155

136156
var _ = Describe("MultiGroupMixin", func() {
137-
tmp := mockTemplate{}
157+
var tmp mockTemplate
158+
159+
BeforeEach(func() {
160+
tmp = mockTemplate{}
161+
})
138162

139163
Context("InjectMultiGroup", func() {
140164
It("should inject the provided multi group flag", func() {
@@ -147,7 +171,11 @@ var _ = Describe("MultiGroupMixin", func() {
147171
var _ = Describe("BoilerplateMixin", func() {
148172
const boilerplate = "Copyright"
149173

150-
tmp := mockTemplate{}
174+
var tmp mockTemplate
175+
176+
BeforeEach(func() {
177+
tmp = mockTemplate{}
178+
})
151179

152180
Context("InjectBoilerplate", func() {
153181
It("should inject the provided boilerplate", func() {
@@ -158,14 +186,21 @@ var _ = Describe("BoilerplateMixin", func() {
158186
})
159187

160188
var _ = Describe("ResourceMixin", func() {
161-
res := &resource.Resource{GVK: resource.GVK{
162-
Group: "group",
163-
Domain: "my.domain",
164-
Version: "v1",
165-
Kind: "Kind",
166-
}}
167-
168-
tmp := mockTemplate{}
189+
var (
190+
res *resource.Resource
191+
tmp mockTemplate
192+
)
193+
194+
BeforeEach(func() {
195+
res = &resource.Resource{GVK: resource.GVK{
196+
Group: "group",
197+
Domain: "my.domain",
198+
Version: "v1",
199+
Kind: "Kind",
200+
}}
201+
202+
tmp = mockTemplate{}
203+
})
169204

170205
Context("InjectResource", func() {
171206
It("should inject the provided resource", func() {

‎pkg/machinery/scaffold_test.go‎

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -115,12 +115,12 @@ var _ = Describe("Scaffold", func() {
115115
)
116116

117117
var (
118-
testErr = errors.New("error text")
119-
120-
s *Scaffold
118+
testErr error
119+
s *Scaffold
121120
)
122121

123122
BeforeEach(func() {
123+
testErr = errors.New("error text")
124124
s = &Scaffold{fs: afero.NewMemMapFs()}
125125
})
126126

@@ -168,29 +168,36 @@ var _ = Describe("Scaffold", func() {
168168
)
169169

170170
DescribeTable("file builders related errors",
171-
func(errType interface{}, files ...Builder) {
171+
func(setup func() (interface{}, []Builder)) {
172+
errType, files := setup()
173+
172174
err := s.Execute(files...)
175+
173176
Expect(err).To(HaveOccurred())
174177
Expect(errors.As(err, &errType)).To(BeTrue())
175178
},
176-
Entry("should fail if unable to validate a file builder",
177-
&ValidateError{},
178-
fakeRequiresValidation{validateErr: testErr},
179-
),
180-
Entry("should fail if unable to set default values for a template",
181-
&SetTemplateDefaultsError{},
182-
&fakeTemplate{err: testErr},
183-
),
184-
Entry("should fail if an unexpected previous model is found",
185-
&ModelAlreadyExistsError{},
186-
&fakeTemplate{fakeBuilder: fakeBuilder{path: path}},
187-
&fakeTemplate{fakeBuilder: fakeBuilder{path: path, ifExistsAction: Error}},
188-
),
189-
Entry("should fail if behavior if-exists-action is not defined",
190-
&UnknownIfExistsActionError{},
191-
&fakeTemplate{fakeBuilder: fakeBuilder{path: path}},
192-
&fakeTemplate{fakeBuilder: fakeBuilder{path: path, ifExistsAction: -1}},
193-
),
179+
Entry("should fail if unable to validate a file builder", func() (interface{}, []Builder) {
180+
return &ValidateError{}, []Builder{
181+
fakeRequiresValidation{validateErr: testErr},
182+
}
183+
}),
184+
Entry("should fail if unable to set default values for a template", func() (interface{}, []Builder) {
185+
return &SetTemplateDefaultsError{}, []Builder{
186+
&fakeTemplate{err: testErr},
187+
}
188+
}),
189+
Entry("should fail if an unexpected previous model is found", func() (interface{}, []Builder) {
190+
return &ModelAlreadyExistsError{}, []Builder{
191+
&fakeTemplate{fakeBuilder: fakeBuilder{path: path}},
192+
&fakeTemplate{fakeBuilder: fakeBuilder{path: path, ifExistsAction: Error}},
193+
}
194+
}),
195+
Entry("should fail if behavior if-exists-action is not defined", func() (interface{}, []Builder) {
196+
return &UnknownIfExistsActionError{}, []Builder{
197+
&fakeTemplate{fakeBuilder: fakeBuilder{path: path}},
198+
&fakeTemplate{fakeBuilder: fakeBuilder{path: path, ifExistsAction: -1}},
199+
}
200+
}),
194201
)
195202

196203
// Following errors are unwrapped, so we need to check for substrings

0 commit comments

Comments
 (0)