Enable diff reporting on predicates

[metlos]

This adds additional interface that the Predicate implementations can implement to enable reporting of the differences that caused the predicate to not match. This will be used in the toolchain-e2e wait.For()... methods to aid in debugging the e2e test failures from the logs. The changes in toolchain-e2e are here: codeready-toolchain/toolchain-e2e#897

In toolchain-common, the AssertThat function has also been updated to take advantage of this new diffing capability. Before, the output of the AssertThat function could be specified similarly to assert.* functions using the "message and arguments` array. While this possibility has been removed, the method gained the ability to report the diffs that caused the predicates to fail.

Let's consider the following (fabricated) example:

actual := &corev1.Secret{}
actual.SetName("actual")
actual.SetLabels(map[string]string{"some-other-label": "value"})

AssertThat(t, actual, Has(Name("expected")), Has(Labels(map[string]string{"part-of": "app"})))

The AssertThat will cause a test failure with the following error message:

assertions_test.go:23: 
            Error Trace:    /home/brq/lkrejci/Projects/go/src/github.com/codeready-toolchain/toolchain-common/pkg/test/assertions/assertions.go:48
                                                    /home/brq/lkrejci/Projects/go/src/github.com/codeready-toolchain/toolchain-common/pkg/test/assertions/assertions_test.go:23
            Error:          some predicates failed to match
            Test:           TestExplain/with_diff
            Messages:       failed predicates report:
                            predicate 'assertions.named' didn't match the object because of the following differences:
                              &v1.Secret{
                                    TypeMeta: {},
                                    ObjectMeta: v1.ObjectMeta{
                            -               Name:         "expected",
                            +               Name:         "actual",
                                            GenerateName: "",
                                            Namespace:    "",
                                            ... // 12 identical fields
                                    },
                                    Immutable: nil,
                                    Data:      nil,
                                    ... // 2 identical fields
                              }                         

                            predicate 'assertions.hasLabels' didn't match the object because of the following differences:
                              &v1.Secret{
                                    TypeMeta: {},
                                    ObjectMeta: v1.ObjectMeta{
                                            ... // 8 identical fields
                                            DeletionTimestamp:          nil,
                                            DeletionGracePeriodSeconds: nil,
                                            Labels: map[string]string{
                            -                       "part-of":          "app",
                                                    "some-other-label": "value",
                                            },
                                            Annotations:     nil,
                                            OwnerReferences: nil,
                                            ... // 2 identical fields
                                    },
                                    Immutable: nil,
                                    Data:      nil,
                                    ... // 2 identical fields
                              }

[codecov]

Codecov Report

Merging #362 (2c2e36e) into master (a7f4a3e) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   76.42%   76.42%           
=======================================
  Files          43       43           
  Lines        1998     1998           
=======================================
  Hits         1527     1527           
  Misses        424      424           
  Partials       47       47           

[alexeykazakov]

Very nice!
I have one comment regarding breaking API changes though.

[alexeykazakov]

Since you are changing a signature of an existing function let's be careful with breaking components which uses it. We need to get the other PRs ready or we can consider introducing a new function and keep the current one deprecated while updating all the dependencies.

[metlos]

I have checked the code base in all repositories for this. Since this was introduced only very recently, it was used only on a couple of places, and none of them used the removed vararg parameter. So at this time, this is a "source compatible" change.

[alexeykazakov]

Great. Thanks for checking.

[rajivnathan]

This is really nice!

I'm not totally sure I understand the value of Is and Has other than making the code sound more like a sentence but I guess it's not something that'd need to be changed/maintained often so probably doesn't hurt?

Really nice improvements overall. 🙌

Not sure if I missed it but if there's one thing I'd suggest it's including which of -/+ are actual/expected in the explanations because that always trips me up when it's not stated but maybe that's just me.

[metlos]

So the reason for Is and Has is actually not the readability as much as Golang generics.

I wanted the Predicate interface to be generic over the type it accepts so that we can have specialized implementations for our CRDs. Like for example the predicates for SpaceProvisionerConfig present in this PR. The main advantage of the interface being generic is that the impls declare what type they support in their type signature.

But that, of course, brings problems when you want to mix predicates that work on different types. E.g. if you want to combine, say, the Labels predicate that works on any client.Object with a predicate specialized for your CRD.

The obvious solution to that problem would be to make the common predicates like Labels generic over the type they look at so that you could build an array with a uniform type, e.g. https://play.golang.com/p/6TsZRz_NvY2. But because of the not-so-great type inference of the go compiler that requires you to specify the type with each use of that predicate (as can be seen in the example). That IMHO grows old quite quickly, especially if you need to specify multiple such predicates.

Note that in languages with more advanced type system, it is possible to leave out the type params, e.g. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=02027f0a894b6e3389fb7619cbd5d0b4 and I believe we could implement something similar in Java, too, using wildcards.

I tried quite hard to come up with something that would be both type-safe and ergonomic to write in Go but I failed.

So the "backup plan" is to use the wrappers Is and Has which basically do a cast of T into client.Object. Therefore we get a uniform array of Predicate[client.Object] in the invocations of AssertThat and we don't have to specify any generic types. The cost, of course, is the loss of type safety and possible type casting errors at runtime, if we're not careful and supply incompatible predicates to AssertThat. IMHO, that is a reasonable price to pay for the readability and brevity this brings to the tests.

[metlos]

Not sure if I missed it but if there's one thing I'd suggest it's including which of -/+ are actual/expected in the explanations because that always trips me up when it's not stated but maybe that's just me.

I'm the same! :) I modified the output to include a short note about that.

[mfrancisc]

Nice work 🥇

Thanks a lot for adding diff capabilities to the new generic functions.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues

Measures
0 Security Hotspots
No data about Coverage
3.5% Duplication on New Code

See analysis details on SonarCloud