test: add more scheduler integration tests (2/) #539
Conversation
| if err := hubClient.Get(ctx, types.NamespacedName{Name: policySnapshotName}, policySnapshot); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
compare the full struct of the policy status?
If your function returns a struct, don't write test code that performs an individual comparison for each field of the struct. Instead, construct the struct that you're expecting your function to return, and compare in one shot using [diffs](https://github.com/golang/go/wiki/TestComments#print-diffs) or [deep comparisons](https://github.com/golang/go/wiki/TestComments#equality-comparison-and-diffs). The same rule applies to arrays and maps.
https://github.com/golang/go/wiki/TestComments#compare-full-structures
There was a problem hiding this comment.
Again and again, any Go guideline is just a general guideline, not a must. I don't particularly have a strong opinion on this one, but the check here is done separately to a) avoid having a large bloc of data and b) avoid stuffing the Diff() call with tons of options for sorting/ignoring nested fields, which is even more difficult to reason about.
There was a problem hiding this comment.
you can always construct the options using,
snapshotStatusCmpOptions = cmp.Options{
ignoreClusterDecisionReasonField,
cmpopts.SortSlices(lessFuncClusterDecision),
...
}
and build the wantStatus like
wantStatus := SchedulingPolicySnapshotStatus{
ObservedCRPGeneration: wantCRPGeneration
Conditions: []metav1.Condition{
wantScheduledCondition,
}
ClusterDecisions: wantClusterDecisions,
}
Then compare the status in one Diff call, the code will be cleaner than comparing the field one by one.
The amount of the data is the same as the current one.
There was a problem hiding this comment.
Stuffing all the options into one struct/array does not make them disappear -> it still targets a number of nested fields that is difficult to track. And building this want struct with blocks IMO does not help much with readability.
Description of your changes
This PR features some more integration tests for the scheduler.
It also includes some minor fixes for the prev. PR that was not rebased into the branch in time.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
N/A
Special notes for your reviewer