Gomega not catching nils for pointers to structs

[tnine]

Occasionally my tested code will return nil. This is expected, since it is an eventually consistent system. I have the following function within my test.

func assertSameOrg(expected *model.Organization, actual *model.Organization) {
	log.Printf("Actual org %+v", actual)
	log.Printf("Expected org %+v", expected)
	Expect(actual).NotTo(BeNil())
	Expect(actual).NotTo(PointTo(BeNil()))
	Expect(expected).NotTo(BeNil())
	Expect(expected).NotTo(PointTo(BeNil()))
	Expect(actual.ID).Should(Equal(expected.ID), "Id should match")
	Expect(actual.Domain).Should(Equal(expected.Domain))
	Expect(actual.TwilioAppSid).Should(Equal(expected.TwilioAppSid))
	Expect(actual.TwilioSecret).Should(Equal(expected.TwilioSecret))
	Expect(actual.TwilioSID).Should(Equal(expected.TwilioSID))
	Expect(actual.Created.Unix()).Should(Equal(expected.Created.Unix()))

}

I receive the following stack trace with output.

2017/02/20 16:15:56 Actual org <nil>
2017/02/20 16:15:56 Expected org &{ID:5137410467823616 Name:886cfd8b-f7c2-11e6-bb67-784f4350cdad Domain:886cfd8b-f7c2-11e6-bb67-784f4350cdad.talklikehumans.com TwilioSID:886cfd8e-f7c2-11e6-bb67-784f4350cdad TwilioSecret:bar TwilioAppSid:baz Created:2017-02-20 16:15:56.593910669 -0700 MST}

------------------------------
•! Panic [0.253 seconds]
User
/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/organization_test.go:254
  Search By Sid [It]
  /Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/organization_test.go:156

  Test Panicked
  runtime error: invalid memory address or nil pointer dereference
  /usr/local/Cellar/go/1.7.4_2/libexec/src/runtime/panic.go:458

  Full Stack Trace
  	/usr/local/Cellar/go/1.7.4_2/libexec/src/runtime/panic.go:458 +0x243
  github.com/cradlekiwi/api/dataaccess_test.assertSameOrg(0xc42029c4d0, 0x0)
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/organization_test.go:263 +0x4a7
  github.com/cradlekiwi/api/dataaccess_test.glob..func3.5.1()
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/organization_test.go:148 +0xa0
  github.com/cradlekiwi/api/test.TryAssertions.func1()
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/test/utils.go:56 +0x18
  github.com/cradlekiwi/api/vendor/github.com/onsi/gomega.InterceptGomegaFailures(0xc42016f400, 0x0, 0x0, 0x0)
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/vendor/github.com/onsi/gomega/gomega_dsl.go:81 +0xde
  github.com/cradlekiwi/api/test.TryAssertions(0xa, 0xee6b280, 0xc42016f4d0)
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/test/utils.go:57 +0x52
  github.com/cradlekiwi/api/dataaccess_test.glob..func3.5()
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/organization_test.go:155 +0x2fc
  github.com/cradlekiwi/api/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc4202625a0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
  	/Users/toddnine/develop/go/src/github.com/cradlekiwi/api/dataaccess/suite_test.go:12 +0x64
  testing.tRunner(0xc420016cc0, 0x68cc20)
  	/usr/local/Cellar/go/1.7.4_2/libexec/src/testing/testing.go:610 +0x81
  created by testing.(*T).Run
  	/usr/local/Cellar/go/1.7.4_2/libexec/src/testing/testing.go:646 +0x2ec

As you can see actual is nil, and expected is not. I'm using revision c463cd2. Can you provide me with a suggestion on why this nil check is failing with both a NotTo(BeNil()) and a NotTo(PointTo(BeNil()))? I'm at a loss to explain this behavior.

Note that the line 263 referenced in the code is this line in the function.

Expect(actual.ID).Should(Equal(expected.ID), "Id should match")

[onsi]

This is strange - the error appears to come out of gomega.InterceptGomegaFailures. Are you calling that somewhere in your code?

Perhaps you can throw in a bit more context (e.g. the It in organization_test.go and it's parent Describe and any BeforeEaches - also any global BeforeEaches and BeforeSuites?).

[onsi]

Closing this out - please let me know if you're still stuck @tnine

[pohly]

Can we reopen?

This issue still exists (gomega v1.10.5), I ran into it myself. I think I can explain it (after quite some head scratching...). InterceptGomegaFailures installs a failure handler which doesn't abort after a failure:

gomega/gomega_dsl.go

Lines 101 to 110 in dbc6ecd

	func InterceptGomegaFailures(f func()) []string {
	originalHandler := globalFailWrapper.Fail
	failures := []string{}
	RegisterFailHandler(func(message string, callerSkip ...int) {
	failures = append(failures, message)
	})
	f()
	RegisterFailHandler(originalHandler)
	return failures
	}

Therefore a nil pointer test with Expect(expected).NotTo(BeNil()) records a failure, but then allows the test to continue to those lines which use the pointer, which triggers the panic. The panic itself is then not caught by InterceptGomegaFailures. A retry loop that checks the result of InterceptGomegaFailures doesn't help because it typically checks for an error, not a panic.

[pohly]

This is kind of consistent with the documentation. It just should be improved to document this caveat.

It might also be useful to add a InterceptGomegaFailureAndStop which only catches the first failure and then returns immediately.

[blgm]

Re-opening as requested

[onsi]

hey @pohly - InterceptGomegaFailures was originally introduced to make it easier to test matches - and wasn't intended for use in actual "production" test code precisely because it doesn't halt after the first failure. The default behavior if you're using Ginkgo's Fail handler is for test execution to stop after the first failure. What's the reason for using InterceptGomegaFailures?

[pohly]

I was using InterceptGomegaFailures as wrapper around a function inside an Eventually:
https://github.com/intel/pmem-csi/blob/f1944e12f629540c776df60e6ad06af885d82ab7/test/e2e/metrics/metrics.go#L106-L108

The function then uses Gomega to get nicer reports on failures compared to hand-written checks that return a normal error.

[onsi]

ok got it - it's not possible for InterceptGomegaHandler to know, in full generality, which failed assertions it should ignore and continue with and which failed assertions should count and abort the test suite.

I think I see a few options:

• add a defer recover to the function you pass in to Eventually to catch and ignore the panic.
• replace your nil resp.Body assertion with an if statement that breaks out of the loop.

I'd lean toward the second option as it will be the cleanest to reason about.

[pohly]

I already have a PR pending which implements the second option.

I think the current behavior of InterceptGomegaFailures is okay, it just could be documented a bit more explicitly - I wasn't the only one who didn't get it completely.

Manually catching the panic is not a good solution because that doesn't provide any meaningful report of what went wrong when the Eventually times out. I prefer a InterceptGomegaFailureAndStop that reports the first failed assertion.

[onsi]

I'd happily take a PR for InterceptGomegaFailureAndStop :)

[onsi]

Would you be up for working on one? I think that's a fine solution for this usecase. If not I can work on something later this week.

[onsi]

Durp and now I see your second comment:

#198 (comment)

where you are proposing exactly this. Sorry @pohly - my bad for missing that. If you're up for a PR that would be great. Perhaps func InterceptFirstGomegaFailure() string would capture the intent?

Or maybe even func InterceptFirstGomegaFailure() error so that users can write Eventually(...).Should(Succeed())

[pohly]

I don't think I'll have time for this this week, so feel free to go ahead.

func InterceptFirstGomegaFailure() error sounds good to me.

Should it also catch a panic? Hmm, probably not.

[onsi]

Ok, sounds good. I'll try to find some time to work on this.

Thinking on it a bit more deeply - the root issue here is that Eventually doesn't behave the way one would expect if it is passed a function that makes assertions. So I'm also going to look at the viability of having Eventually capture that first assertion failure and try again. This way you could simply write Eventually(test).Should(Succeed()) which is the actual intent.

[onsi]

hey @pohly - the latest commit (2f04e6e) implements func InterceptFirstGomegaFailure() error.

It also improves the behavior of Eventually and Consistently. Both now allow you to pass in a function that makes assertions and you can now pass in functions that have no return value. You should now be able to write Eventually(test, "10s", "1s").Should(Succeed()) - no need to intercept the errors yourself.

I'd love feedback on the commit before I cut a release. If you'd be up for running a go get github.com/onsi/gomega@master and giving this a try I'd be grateful!

[pohly]

I tried Eventually(test, "10s", "1s").Should(Succeed()) and it worked. I couldn't be 100% sure because the original assertion does not always trigger, but I also tested with a fake assertion that always fails and that worked.

Thanks for adding this.

[onsi]

hey @pohly - an issue has surfaced around the improved Eventually and Consistently behavior that shipped with 1.14.0

I'm working on a fix. Unfortunately, it will entail changing the contract for async callbacks that want to make assertions. In 1.14.0 you could pass Eventually a function that uses the global Gomega DSL to make assertions. Under the hood Eventually was rewiring the global DSL to intercept errors. This breaks when you are running multiple Eventuallys concurrently in the same process.

The fix entails passing Eventually a callback that explicitly opt-into receiving a Gomega object:

Eventually(func(g Gomega) {
    g.Expect(...).Should(...)
}).Should(...)

I'm going to ship the fix in 1.15.0 but wanted to give you a heads up that it's coming.

[pohly]

Thanks for the heads up.

How will Eventually(func() {...}) behave? Can you make that fail with an obvious error?

[onsi]

yes - it’s going to fail immediately and let you know that it needs to be changed.

…

On Aug 2, 2021, at 6:13 AM, Patrick Ohly ***@***.***> wrote:  Thanks for the heads up. How will Eventually(func() {...}) behave? Can you make that fail with an obvious error? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

[onsi]

Just pushed out 1.15.0 - PTAL if you've got the time.

[pohly]

it’s going to fail immediately and let you know that it needs to be changed.

Confirmed: "The function passed to Gomega's async assertions should either take no arguments and return values, or take a single Gomega interface that it can use to make assertions within the body of the function. When taking a Gomega interface the function can optionally return values or return nothing. The function you passed takes 0 arguments and returns 0 values."

I'm going to ship the fix in 1.15.0 but wanted to give you a heads up that it's coming.

Works.

[blgm]

From the messages above, it looks like this is working and can now be closed.