onsi / ginkgo

A Modern Testing Framework for Go
http://onsi.github.io/ginkgo/
MIT License
8.22k stars 650 forks source link

AfterAll runs before DeferCleanup of last It in Ordered block #1284

Open birdayz opened 11 months ago

birdayz commented 11 months ago

I noticed the following behavior:

It looks a bit like AfterAll runs before the last Ordered It block, but after all others..

jtarchie commented 11 months ago

From the documentation,

Under the hood DeferCleanup is generating a dynamic AfterEach node and adding it to the running spec. This detail isn't important - you can simply assume that code in DeferCleanup has the identical runtime semantics to code in an AfterEach.

AfterEach happen before AfterAll.

I think your extra It block might be a red herring. It could be causing enough of delay in execution that it appears that behavior is different.

Could you clarify what the expected behavior was for the above?

birdayz commented 11 months ago

"AfterEach happen before AfterAll"

This is not what I am observing. I would expect this, however After all runs before After each, unless I add that dummy It as last block in the ordered container.

onsi commented 11 months ago

hey - the introduction of Ordered containers added a lot of complexity to the codebase and in this case you have hit an edge-case. In reality Ginkgo runs the After* family followed by the Defer* family. And so the AfterAll does in fact run before the DeferCleanup in the final It.

Ideally I'd fix this - but I'm not going to be able to get to it for a while. For now the workaround is to use a DeferCleanup in your BeforeAll in lieu of the AfterAll. It will behave like an AfterAll (it will only run after all the Its but it will join the Defer* family and therefore run at the very end.

Sorry I don't have a cleaner answer for you right now!

onsi commented 11 months ago

Perhaps just to say a bit more - my sense is that Ordered was a misstep and that I need to revisit the problem more generally. What I've learned since implementing it is what folks seem to really want is the ability to define a sub-suite of steps that have a shared setup and shared teardown that is expensive and, therefore, only runs once. And folks want to have some degree of control over how this sub-suite is parallelized (or not) and randomized (or not) with respect to the rest of the suite.

Obviously this isn't the issue you're raising - but I'm just trying to rationalize why I'm not super keen to invest more in Ordered but, instead, to step back and think about what Ginkgo actually needs to solve the various usecases I keep seeing folks wrestle with.

birdayz commented 11 months ago

Thanks for the thorough answer!

In my case (large e2e infra provisioning setup) i make use of Ordered because i have a list of distinct test steps, and the ability to focus individual steps (i.e. using FIt) is very helpful for debugging failures on already existing infrastructure. The alternative is having one huge It block, which is not super appealing. By does not allow focusing and has a weak semantic meaning.

Having some kind of block below It that can be focused would do the trick for me. But i'm pretty new to ginkgo so i basically have no idea :)

onsi commented 10 months ago

In case there's interest, please take a look at the new proposal for managing shared resources and having finer-grained control over parallelism here: https://github.com/onsi/ginkgo/issues/1292

gabolaev commented 3 months ago

@onsi hi!

We have encountered a relatively similar problem to the one discussed here. We have a self-written focus logic built on Skips (we had to, too long to explain).

package e2e

import (
    "context"
    . "github.com/onsi/ginkgo/v2"
)

var _ = BeforeEach(func(ctx context.Context) {
    if CurrentSpecReport().FullText() != "[foo-bar] sub sub test" {
        Skip("skip", 1)
    }
})

var _ = Describe("[foo-bar]", Ordered, func() {
    AfterAll(func(ctx context.Context) {
        println("after all")
    })
    Describe("sub", Ordered, func() {
        BeforeEach(func(ctx context.Context) {
            println("before each")
        })
        It("sub test", func(ctx context.Context) {
            println("sub test")
        })
    })
    It("root test", func(ctx context.Context) {
        println("root test")
    })
})

So am I right that in this case we're losing the AfterAll because it is "tied" to the last It, that is going to be skipped?

How would you solve this problem? Considering that we need to save our BeforeEach-based skipper. BeforeAll(DeferCleanup( works perfect btw, but I just want to make sure there is nothing more "elegant".