How to? InterceptGinkgoFail(ure)?

[thediveo]

With GinkgoHelperGo in place (thanks again for merging!) I've now stumbled on my old nemesis, Ginkgo's Fail: in the past I had to refactor my testing support to use Gomega assertions instead, in order to leverage InterceptGomegaFailure in testing the test helpers. With GinkgoHelperGo this does not work anymore, unless I'm overlooking something.

One important aspect of GinkgoHelperGo that it depends on global.Failer.GetState() as well as global.Failer.Drain() to work its "magic", as well as the usual Fail (which in turn depends on global.Failer.Fail()).

Please note that test code for test helpers using GinkgoHelperGo will be outside the Ginkgo package, so types such as internal.Failer are out of their reach. Since the default Gomaga is usually wired into Fail and that uses whichever global.Failer is set ... would it make sense to temporarily replace the global.Failer, calling a spec-supplied func, and upon returning then returning the types.Failure (which is exported unless I overlooked an "internal") if any?

Or is this a terrible idea and it can hopefully be done much better?

Which unspoken assumptions about global.Failer do I fail, especially regarding potential concurrency?

[onsi]

I think it all depends crucially on whether or not assertions can happen in parallel outside of the helper - in which case overriding the shared global Fail for the duration of the helper will lead to surprising behavior. You could conceivably handle this but I suspect you might not have all the information you need to handle it correctly in all cases.

Ginkgo bends over backwards (too far, perhaps!) to avoid the "noise" of injecting things into each node function. This works in general and is quite convenient - but cases like the one you are working through become tricky.

The best bet might be to bite the bullet and inject Fail where needed and have better control over failure handling that way.

[thediveo]

Thank you for the explanation! How should I do the injection, refactor a new API in? Since these are very specific tests that will use this, I expect them to be guarded by Serial. Yet are we in danger of tripping the race detector, there isn't any mutex in this part of the code.

[onsi]

heyo - can you share a bit more detail. Ginkgo's parallel spec model is multi-process so Serial vs not-Serial is a none-issue for race conditions. Indeed this is why a global fail handler works at all given multiple parallel specs - since the specs run in different processes they don't share the same globals.

[thediveo]

Me mentioning Serial was because I wasn't sure if that is of relevance; based on your reply it's a non-issue in this case.

Me reasoning about the race detector was that, if I got that part correct, Ginkgo runs the node's test code on a separate Go routine. Now, if that node's test code swaps in and out a temporary catch-all fail handler I wasn't sure if that would be something very much disliked by the race detector? Before, the fail handler and associated data were only modified from one of the other Ginkgo go routines, but now the separate node go routine would start to meddle, too.

[onsi]

ah i see. sorry I wasn't tracking! Yes, any call to Fail() (whether directly by the user or implicitly via gomega with Fail registered) will read from global.Failer which could trigger the race detector if there is a concurrent write to global.Failer (say when the handler is registering things). Patching that with locks etc. is doable (we could add a new api to get and set the Failer in the global package) but while that might silence the race detector it won't resolve the deeper issue, which is one of potential misattribution.

Simply put, if the helper is off running asynchronously while user test code is running in parallel then if the user test fails by simply calling Fail it may be calling the overridden fail handler instead of the original global fail handler. That's a misattribution - it should have called the global fail handler but instead it's now masquerading as a failure in the helper.

This is actually why I don't put locks around some of these global getters and setters. Every now and then someone will open an issue saying there's a race condition in Ginkgo and I'll explain that actually the issue is that they've somehow leaked a goroutine and are performing actions in one test that are inadvertently being attributed to a different test.