GinkgoRecover not aborting the current It test node when Expect in goroutine fails

[thediveo]

go.mod:

github.com/onsi/ginkgo/v2 v2.6.1
github.com/onsi/gomega v1.24.2

It("renders user tree with loose threads with CLI -d", func() {
		By("creating a stray task with its own namespace...")
		tidch := make(chan int)
		done := make(chan struct{})
		go func() {
			defer GinkgoRecover()
			// unix.Unshare will fail with err != nil
			Expect(unix.Unshare(unix.CLONE_FS | unix.CLONE_NEWNS)).To(Succeed())

			tidch <- unix.Gettid()
			<-done
		}()

		tid := <-tidch
		// ...
}

When running this test (as a non-root user) the expectation in the separate go routine will fail. However, the test node itself won't be aborted but instead will "hang" until the overall test watchdog barks and bites.

This is probably the, erm, expected behavior (pun intended). What is the proper way to correctly handle failed expectations while the original test node go routine is blocked on some channel read? Somehow I would imagine using a test context and then cancel this? Is there a convenient way to ensure that the context is only cancelled when GinkgoRecover actually meets with a failed expectation (panic)?

[onsi]

I think in this case I would recommend:

It("renders user tree with loose threads with CLI -d", func() {
		By("creating a stray task with its own namespace...")
		tidch := make(chan int)
		done := make(chan struct{})
		go func() {
			defer GinkgoRecover()
			// unix.Unshare will fail with err != nil
			Expect(unix.Unshare(unix.CLONE_FS | unix.CLONE_NEWNS)).To(Succeed())

			tidch <- unix.Gettid()
			<-done
		}()

		var tid int
                Eventually(tidch).Should(Receive(&tid))
		// ...
}

that's probably the cleanest/simplest/most legible way. No doubt there are other potential approaches (e.g. additional defers in the goroutine or using a SpecTimeout to kill the whole test if it hangs) but I think this approach will give you the cleanest failure message.

[thediveo]

Ah, that's a nifty solution!

Alternatively, I was thinking about either allowing GinkgoRecover() to accept an optional conext.CancelFunc? Or instead offering a new GinkgoRecoverAndCancel(). However, this needs instead more test harness code in order to create a cancel-able context and bailing out while waiting for the receive (that never comes).

[onsi]

ah, yes - that would make sense too. though you're write it would take more plumbing!

[austince]

Should we update the docs for this case?

https://onsi.github.io/ginkgo/#mental-model-how-ginkgo-handles-failure suggests that:

It("panics in a goroutine", func() {
  var c chan interface{}
  go func() {
    defer GinkgoRecover()
    Fail("boom")
    close(c)
  }()
  <-c
})

should work but the result is hanging as described here.

[thediveo]

@austince ah, that's probably why I originally fell for this anti-pattern! Yes please, update the documentation to use Eventually!

[austince]

Fell for the same :), though trying to figure out how to do this with a sync.WaitGroup (which seems less straight-forward with Eventually). Will create a new issue and PR once I figure that out

[onsi]

ha. um. yeah that example is borked 😬

for a waitgroup you’ll either want to defer wg.Done() at the top of the goroutine or launch a separate goroutine to wait for wg.Wait() and pass it a channel. The only downside to the latter is you’ll leak the waitgroup and monitoring goroutine if the test fails.

[onsi]

issue + pr would be great though @austince thanks!

[thediveo]

@austince WaitGroup.Wait doesn't have a cancellable context -- I came across this restriction just these days in a test, so I had to rewrite it differently.

[austince]

Finally got around to it here: #1339