Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
akrejcir opened this issue Feb 16, 2024 · 2 comments

Comments

@akrejcir
Copy link

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 (cd418b7).

Related Issue: #1284

@onsi
Copy link
Owner

onsi commented Feb 16, 2024

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
Copy link
Author

That makes sense, thank you for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants