Deep Equality Check in /pkg/agent test #1379
Conversation
- Check the content of returned listed releases (not just size comparison). - Complete information about returned apps in test cases. - Add test case for release with same name, but different versions and namespaces.
- Use deep equality for comparison.
go.mod
Outdated
| @@ -8,6 +8,7 @@ require ( | |||
| github.com/Masterminds/semver v1.5.0 // indirect | |||
| github.com/Masterminds/sprig v2.22.0+incompatible // indirect | |||
| github.com/arschles/assert v1.0.0 | |||
| github.com/davecgh/go-spew v1.1.1 | |||
There was a problem hiding this comment.
This one slipped through the cracks 🙁. Needed it for debugging.
There was a problem hiding this comment.
you should be able to remove it now
pkg/agent/agent_test.go
Outdated
| // Map a unique identifier to ptr to AppOverview | ||
| m := make(map[string]*proxy.AppOverview) | ||
|
|
||
| for _, eapp := range tc.expectedApps { | ||
| m[releaseStub{eapp.ReleaseName, eapp.Namespace, strtoi(eapp.Version)}] = true | ||
| for i, eapp := range tc.expectedApps { | ||
| m[getAppIdentity(eapp)] = &tc.expectedApps[i] | ||
| } | ||
|
|
||
| // All attained apps, must have a unique id that is already in the map | ||
| // Attained app and its mapping should be equal in structure not only identifier | ||
| for _, app := range apps { | ||
| rs := releaseStub{app.ReleaseName, app.Namespace, strtoi(app.Version)} | ||
| if _, ok := m[rs]; !ok { | ||
| t.Errorf("got: %v, want: %v", rs, "None") | ||
| appIdentity := getAppIdentity(app) | ||
| if expectedApp, ok := m[appIdentity]; !ok { | ||
| t.Errorf("got: %v, want: %v", &app, "None") | ||
| } else if !cmp.Equal(*expectedApp, app) { | ||
| t.Errorf(cmp.Diff(*expectedApp, app)) | ||
| } | ||
| } |
There was a problem hiding this comment.
you can just do:
if !cmp.Equal(tc.expectedApps, apps) {
t.Errorf(cmp.Diff(tc.expectedApps, apps))
}
If you reorder the expected apps
There was a problem hiding this comment.
Sure thing. It can be done that way with the pair app.ReleaseName and app.Namespace as keys to sort on.
Did I understand correctly?
There was a problem hiding this comment.
nope, I meant that if you reorder the apps in tc.expectedApps to match the apps you don't need the map nor the iteration over the apps, you can just compare the arrays
- Use deep equality for comparison.
pkg/agent/agent_test.go
Outdated
| // getAppIdentity concatenates the release name and namespace of an app | ||
| func getAppIdentity(app proxy.AppOverview) string { | ||
| return fmt.Sprintf("%v %v", app.ReleaseName, app.Namespace) | ||
| } |
There was a problem hiding this comment.
This looks like it was left over from some testing or earlier sorting? I don't see it being used.
|
Hi @latiif, it seems that you have rebased master or something like that and now there is a bunch of commits here that are not related to your change. Can you remove those? Note: When getting the latest changes from master is better to |
|
Sorry for the mess @andresmgot ! Git is not my forté With help from my colleagues, I think the commit history is in better shape now. |
|
So it looks like the memory driver does not return results with consistent ordering? At least, we're seeing an ordering failure in tests occasionally it seems https://circleci.com/gh/kubeapps/kubeapps/14014 |
Description of the change
A follow up PR for #1369, where the comparisons between expected and attained apps are performed on the actual representation rather than a test stub.
Completed fields 'version' and 'icon' for expected results.
Benefits
Making sure the equality checks are performed on the entire structure of the attained results.
Applicable issues
Related PR #1369 based on the discussion that followed with @absoludity.
Additional information
No performance drawbacks noticed.