Add additional unit-tests for Conditions #92
Conversation
Make sure each condition is defined (as best we can) Enhancements to operator-framework#91 Signed-off-by: Todd Short <tshort@redhat.com>
controllers/operator_controller.go
Outdated
| compareOp := reconciledOp.DeepCopy() | ||
| existingOp.Status, compareOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} | ||
| existingOp.Finalizers, compareOp.Finalizers = []string{}, []string{} | ||
| specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp) | ||
| unexpectedFieldsChanged := !equality.Semantic.DeepEqual(existingOp, compareOp) |
There was a problem hiding this comment.
could we create a function that takes the reconciledOp and returns if an unexpected field was changed? it may make things easier and would allow us to update the block above to look like:
updateStatus := !equality.Semantic.DeepEqual(existingOp.Status, reconciledOp.Status)
updateFinalizers := !equality.Semantic.DeepEqual(existingOp.Finalizers, reconciledOp.Finalizers)
unexpectedFieldsChanged := checkForUnexpectedFieldChanges(reconciledOp) # Function name could be different.
There was a problem hiding this comment.
You'd have to also pass existingOp in, but yeah, this seems like a good idea.
controllers/suite_test.go
Outdated
| Expect(conds).To(ContainElement(HaveField("Type", t))) | ||
| } | ||
| }) | ||
| It("has matching gnerations in Conditions", func() { |
There was a problem hiding this comment.
I think there's a misspelling here, but I'm not sure if generations was the intended word to be spelt.
| It("has matching gnerations in Conditions", func() { | |
| It("has matching generations in Conditions", func() { |
api/v1alpha1/operator_types.go
Outdated
| @@ -28,13 +28,20 @@ type OperatorSpec struct { | |||
| } | |||
|
|
|||
| const ( | |||
| // TODO(user): add more Types | |||
| // TODO(user): add more Types, here and into GetTypes() | |||
api/v1alpha1/operator_types.go
Outdated
| func GetTypes() []string { | ||
| // TODO(user): add Types from above | ||
| return []string{ | ||
| TypeReady, | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if there's a way to do this without exporting GetTypes(). This is more of an implementation detail than something that should be part of the API.
This is also only something we'd use in tests, I think?
Maybe something like:
internal/operatorutil/util.go:
// ConditionTypes is the full set of Operator condition types.
//
// NOTE: This is populated by the init function in
// api/v1alpha1/operator_types.go
var ConditionTypes []stringAnd then here:
func init() {
operatorutil.ConditionTypes = append(operatorutil.ConditionTypes,
// Add all Operator condition types to this list.
TypeReady,
)
)WDYT? Is there another way I'm not thinking of that:
- Keeps everything self-contained in the v1alpha1 packagae
- Keeps the list of types internal
- Makes the list of types available to tests, perhaps in both reconciler unit tests and e2es?
controllers/operator_controller.go
Outdated
| compareOp := reconciledOp.DeepCopy() | ||
| existingOp.Status, compareOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} | ||
| existingOp.Finalizers, compareOp.Finalizers = []string{}, []string{} | ||
| specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp) | ||
| unexpectedFieldsChanged := !equality.Semantic.DeepEqual(existingOp, compareOp) |
There was a problem hiding this comment.
You'd have to also pass existingOp in, but yeah, this seems like a good idea.
controllers/suite_test.go
Outdated
|
|
||
| // The ObservedGeneration should match the resource generation | ||
| for _, c := range getOperator.Status.Conditions { | ||
| Expect(c).To(HaveField("ObservedGeneration", getOperator.GetGeneration())) |
There was a problem hiding this comment.
nit: HaveField doesn't give us compile-time checks on the field name, so its susceptible to typos, for example.
| Expect(c).To(HaveField("ObservedGeneration", getOperator.GetGeneration())) | |
| Expect(c.ObservedGeneration).To(Equal(getOperator.GetGeneration())) |
There was a problem hiding this comment.
I'd agree with you 100% if this were not test code. Test code, on the other hand, I could see it either way.
|
/approve |
|
Nice work on this @tmshort |
|
Ping @joelanford, this should address everything. |
Make sure each condition is defined (as best we can)
Enhancements to #91
Signed-off-by: Todd Short tshort@redhat.com