onsi / ginkgo

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

DeferCleanup() blocks are run after all AfterEach() blocks, regardless of nesting #1360

Closed akrejcir closed 4 months ago

akrejcir commented 4 months ago

In one of our projects, we've noticed that DeferCleanup() blocks are run after all AfterEach() blocks, without considering nesting. It may lead to a hard to find bug, when one of the top-level AfterEach() cleans up some resources, that a lower-level DeferCleanup() uses.

Here is a simple code for illustration:

var _ = Describe("test suite", func() {
    BeforeEach(func() {
        fmt.Println("- BeforeEach() 1")
        DeferCleanup(func() {
            fmt.Println("- BeforeEach() 1 - DeferCleanup()")
        })
    })

    AfterEach(func() {
        fmt.Println("- AfterEach() 1")
    })

    Context("context", func() {
        BeforeEach(func() {
            fmt.Println("-- BeforeEach() 2")
            DeferCleanup(func() {
                fmt.Println("-- BeforeEach() 2 - DeferCleanup()")
            })
        })

        AfterEach(func() {
            fmt.Println("-- AfterEach() 2")
        })

        It("Test", func() {
            fmt.Println("--- Test")
            DeferCleanup(func() {
                fmt.Println("--- Test - DeferCleanup()")
            })
        })
    })
})

It prints:

- BeforeEach() 1
-- BeforeEach() 2
--- Test
-- AfterEach() 2
- AfterEach() 1
--- Test - DeferCleanup()
-- BeforeEach() 2 - DeferCleanup()
- BeforeEach() 1 - DeferCleanup()

I would expect this order:

- BeforeEach() 1
-- BeforeEach() 2
--- Test
--- Test - DeferCleanup()
-- AfterEach() 2
-- BeforeEach() 2 - DeferCleanup()
- AfterEach() 1
- BeforeEach() 1 - DeferCleanup()

I've tested it on the latest master of this repository (cd418b74c1e8).

Related Issue: https://github.com/onsi/ginkgo/issues/1284

onsi commented 4 months ago

hey there - I appreciate the surprise factor here isn't great - but this is operating as intended. I should probably make the docs much clearer on this point but my intent with DeferCleanup is to provide a cleaner and simpler mechanism for cleanup and want to nudge users in the direction of favoring it over After* - you can mix the two in a given spec but then you'll end up with these edge cases.

Ginkgo could track things at the nesting level as you're describing but this will add complexity to the code (particularly some of the gnarly complexity around Ordered). Also, at this point, it would constitute a change in behavior so I'm not inclined to change the cleanup ordering.

akrejcir commented 4 months ago

That makes sense, thank you for the explanation.