Tech Debt: Simplify unit tests by using UnstructuredSet & ObjMetadataSet with AssertEqual deep comparison

[karlkfi]

Follow up after https://github.com/kubernetes-sigs/cli-utils/pull/419 and https://github.com/kubernetes-sigs/cli-utils/pull/426

/assign @karlkfi

[ash2k]

I've just bumped the version of cli-utils I use and found the UnstructuredSet change. I'm wondering how exactly it's going to help with testing? I'm also using cmp to do diffing and I don't see how the new Equal method is helpful.

p.s. Also, I think ideally methods for testing should live in a separate package and not on a type that is used in production code.

[karlkfi]

The Equal ignores order and duplicates when comparing the set. Having an Equal on the list also allows comparison of structs that contain the list.

It also indicates to callers that input/output order isn’t important while still allowing cli-utils to sort it without a huge refactor.

[ash2k]

The Equal ignores order and duplicates when comparing the set. Having an Equal on the list also allows comparison of structs that contain the list.

Ok, I see the description of Equal() here https://pkg.go.dev/github.com/google/go-cmp/cmp#pkg-overview. Can it show the diff in this case (i.e. why the sets are different)? I suspect it cannot, but I haven't tried.

When using cmp, this can be done via cmp.FilterPath()+cmp.Transformer(). I'm using this technique to transform a runtime.Object into an Unstructured to compare object regardless if they are typed or not. We can use this approach to convert UnstructuredSet into e.g. a []map[string]interface{} after deduplicating and sorting it (sort must be deterministic - perhaps GVK+namespace+name+hash of the object to resolve ordering conflicts).

The benefit of this approach is that cmp would be able to show a meaningful diff and that there is no method for tests on the type.

It also indicates to callers that input/output order isn’t important while still allowing cli-utils to sort it without a huge refactor.

I don't mind the alias per se, I think it makes sense to have it as a way to carry the semantics (and doc) on it.

[ash2k]

So if we build a cmp.Option that would do that transformation, we and library users will be able to use it like this:

a := ... // a is a struct/slice/map, containing UnstructuredSet
b := ... // b is a struct/slice/map, containing UnstructuredSet
assert.Empty(t, cmp.Diff(a, b, cliUtilsTesting.TransformUnstructuredSet()))

I use assert.Empty(t, cmp.Diff()) because it prints the diff between objects, not just says they are not equal and fails, forcing me to run the test under a debugger.

[karlkfi]

I added a testutil.assertEqual/assertNotEqual already that prints a diff on failure. We’re using it for event comparison already in the e2e tests. It doesn’t currently sort the elements for the diff, but we could make it use the transformer pattern before diffing. There’s already a sorter in the ordering package.

I’m hoping to just get rid off the ExpEvent objects all together and compare actual events, but it requires some more enhancements to make things in the event structs more comparable first. There’s some annoyingly deep recursion happening in them that probably shouldn’t have been exposed in a public facing API.

Anyway, I’ll take a stab at using the new sets for the unit tests. It’s should make it clear what other enhancements are needed. But yes, getting a clear and actionable error on failure is definitely one of the goals.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[karlkfi]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[karlkfi]

/remove-lifecycle rotten /lifecycle frozen