error checking: accept Error() as alternative to String()

[pohly]

In Kubernetes, we (not often) check for specific errors:

              gomega.Expect(err).To(gomega.HaveOccurred())
              gomega.Expect(err.Error()).To(gomega.ContainSubstring(`Invalid value: "foo-"`))
              gomega.Expect(err.Error()).To(gomega.ContainSubstring(`Invalid value: "bar.."`))
              gomega.Expect(err.Error()).NotTo(gomega.ContainSubstring(`safe-and-unsafe`))
              gomega.Expect(err.Error()).NotTo(gomega.ContainSubstring("kernel.shmmax"))

I can currently write this as two assertions:

              gomega.Expect(err).To(gomega.HaveOccurred())
              gomega.Expect(err.Error()).gomega.ContainSubstring(`Invalid value: "foo-"`),
			gomega.ContainSubstring(`Invalid value: "bar.."`),
			gomega.Not(gomega.ContainSubstring(`safe-and-unsafe`)),
			gomega.Not(gomega.ContainSubstring("kernel.shmmax")),
	      ))

I would like to reduce this to one assertion:

		gomega.Expect(err).To(gomega.SatisfyAll(
			gomega.HaveOccurred(),
			gomega.ContainSubstring(`Invalid value: "foo-"`),
			gomega.ContainSubstring(`Invalid value: "bar.."`),
			gomega.Not(gomega.ContainSubstring(`safe-and-unsafe`)),
			gomega.Not(gomega.ContainSubstring("kernel.shmmax")),
		))

This currently fails:

  [FAILED] ContainSubstring matcher requires a string or stringer.  Got:
      <*errors.errorString | 0xc00153ebb0>: {
          s: "Invalid value: \"foo-\"",
      }

I think would be consistent with the gomega design to allow error as an alternative for a fmt.Stringer and to call Error() to obtain a string when given an error.

The advantages are:

• in case of a failure, the full error object gets dumped
• IMHO clearer code (one statement about err instead of many)

[onsi]

hey there - obviously I'm open to this but want to point out what currently exists first. You can do all this with:

Expect(err).To(MatchError(SatisfyAll(
	ContainSubstring(`Invalid value: "foo-"`),
	ContainSubstring(`Invalid value: "bar.."`),
	Not(ContainSubstring(`safe-and-unsafe`)),
	Not(ContainSubstring("kernel.shmmax")),
))

(note you don't need the HaveOccurred as MatchError performs a nil check for you).

I could imagine expandingMatchError to take multiple matchers that it implicitly Ands together with SatisfyAll - but nothing else does that right now so it would end up being the odd one out. Also the output you get for a failure given a composite matcher like SatisfyAll might not be as helpful as we'd like - but I'd be happy to invest effort fixing that.

[pohly]

That looks promising. I think it can satisfy all cases that I had in mind for this issue, so we can (almost) close it.

The output could be a bit better, though:

  err := fmt.Errorf("foo")

  [FAILED] Expected
      <*errors.errorString | 0xc001517520>: {s: "foo"}
  to match error
      <*matchers.AndMatcher | 0xc00401f7d0>: {
          Matchers: [
              <*matchers.ContainSubstringMatcher | 0xc00401f710>{
                  Substr: "Invalid value: \"foo-\"",
                  Args: nil,
              },
              <*matchers.ContainSubstringMatcher | 0xc00401f740>{
                  Substr: "Invalid value: \"bar..\"",
                  Args: nil,
              },
              <*matchers.NotMatcher | 0xc001517550>{
                  Matcher: <*matchers.ContainSubstringMatcher | 0xc00401f770>{
                      Substr: "safe-and-unsafe",
                      Args: nil,
                  },
              },
              <*matchers.NotMatcher | 0xc001517580>{
                  Matcher: <*matchers.ContainSubstringMatcher | 0xc00401f7a0>{
                      Substr: "kernel.shmmax",
                      Args: nil,
                  },
              },
          ],
          firstFailedMatcher: <*matchers.ContainSubstringMatcher | 0xc00401f710>{
              Substr: "Invalid value: \"foo-\"",
              Args: nil,
          },
      }

Dumping the error as struct makes sense because it may contain additional details. However, I would also expect to see the result of Error() because (depending on the type of error), the actual error string might not be in the struct at all.

[thediveo]

you can even avoid the "broken pavement" _, _, _, err := foo() boilerplate (in case you needed it before) using Expect(foo()).Error().To(...)

[onsi]

woof - yeah that failure message could use some work. will need to think about it - one areA i've been bumping into the last couple of days is the general problem of consistent and useful reporting when a (hierarchy of)submatcher(s) is involved.

right now most composite matchers just print the format.Object of the submatcher(s) but we can surely do better.

i'm knee deep in another project right now but will add this to my todo list for when i free up.

[pohly]

It's not just the matchers: the Expected <*errors.errorString | 0xc001517520>: {s: "foo"} would also be more informative when it included the Error() result.

I'm thinking of this case:

package test

import (
	"testing"

	"github.com/onsi/gomega"
)

type myError struct{}

func (m myError) Error() string {
	return "some error"
}

func TestSomething(t *testing.T) {
	g := gomega.NewGomegaWithT(t)

	err := error(myError{})
	// g.Expect(err).ToNot(gomega.HaveOccurred())
	g.Expect(err).To(gomega.MatchError(gomega.ContainSubstring("foo")))
}

I found that HaveOccurred includes the Error output:

    Unexpected error:
        <test.myError>: {}
        some error
    occurred

MatchError does not:

    Expected
        <test.myError>: {}
    to match error
        <*matchers.ContainSubstringMatcher | 0xc0000c3620>: {Substr: "foo", Args: nil}

This seems to be solved as a special case in HaveOccurred. Would it perhaps be better to let format.Object always include the Error result when it encounters an object that implements error?

[onsi]

Yes that makes sense - doing it at the format.Object level in particular.

[onsi]

hey sorry for the delay - i havent worked on the submatchers yet but the error output seemed like a higher priority anyway. it's now fixed on v1.27.3 - now anytime format.Object sees an error it will add on the Error() message.

[pohly]

Note that gomega.Eventually now prints the error string twice, once before the format.Object() result and once inside it:

https://github.com/kubernetes/kubernetes/pull/116539/files#diff-89fa7733f32f68c3149970d6cc759c4bbb2e2e849629cca3b4d352f2ac59ee3a

[FAILED] Timed out after <after>.
The function passed to Eventually returned the following error:
pods "no-such-pod" not found
    <framework.transientError>: {
        error: <*errors.StatusError>{
            ErrStatus: {
                TypeMeta: {Kind: "", APIVersion: ""},
                ListMeta: {
                    SelfLink: "",
                    ResourceVersion: "",
                    Continue: "",
                    RemainingItemCount: nil,
                },
                Status: "Failure",
                Message: "pods \"no-such-pod\" not found",
                Reason: "NotFound",
                Details: {Name: "no-such-pod", Group: "", Kind: "pods", UID: "", Causes: nil, RetryAfterSeconds: 0},
                Code: 404,
            },
        },
    }
    pods "no-such-pod" not found
In [It] at: wait_test.go:66 <time>

[onsi]

hey good catch. i've fixed this on master and rearranged things so format.Object() prints the err.Error() representation first and then the struct. please take a look and i'll cut a release if it looks better to you.

[pohly]

That looks better, please cut a release.

[onsi]

thanks for taking a look - 1.27.4 is out now with this change

[thediveo]

Looks like the godoc documentation for MatchError https://github.com/onsi/gomega/blob/5129b5cf7d54245b130b241a4ace8a0d12442ab1/matchers.go#LL90C4-L90C4 needs an update: it only mentions matching an error or a sting, but not using other matchers. Will work on this.

[onsi]

❤️ thank you!On Mar 24, 2023, at 1:28 PM, TheDiveO ***@***.***> wrote: Looks like the godoc documentation for MatchError https://github.com/onsi/gomega/blob/5129b5cf7d54245b130b241a4ace8a0d12442ab1/matchers.go#LL90C4-L90C4 needs an update: it only mentions matching an error or a sting, but not using other matchers. Will work on this. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>