first-in-last-out order for AfterEach and DeferCleanup

[pohly]

In the Kubernetes E2E framework, we have tests like this:

Describe(..., func() {
   f := framework.NewDefaultFramework()
})

NewDefaultFramework calls BeforeEach to set up the content of f and AfterEach to clean up for each test.

The problem now is that currently AfterEach seems to be called in first-in-first-out order inside the same node.

Therefore this code is slightly wrong because although the f pointer is valid, its content no longer is (already cleaned up).

Describe(..., func() {
   f := framework.NewDefaultFramework()

   AfterEach(func() {
      // use f
   })
})

The workaround is this:

Describe(..., func() {
   f := framework.NewDefaultFramework()

   Context("", func() {
      AfterEach(func() {
         // use f
      })
   })
})

But that causes additional indention and the reason for it is not obvious without comments.

A better solution would be to execute AfterEach in first-in-last-out order. My reading of the current documentation is that only the order of nested nodes is specified (inner first), while the behavior inside the same node is undefined. Therefore defining and implementing it as first-in-last-out would be a clarification, not an API break.

The same order would be needed for DeferCleanup.

[onsi]

hey @pohly

I'm going to update the documentation to better describe the ordering for clean-up nodes so that it can be clearer going forward; however the current ordering behavior for AfterEach isn't something that I'll be changing. Though it hasn't been documented - the behavior has been consistent since Ginkgo's inception and I suspect changing it will cause significant issues for users.

AfterEach run in first-in-first-out order within a given container node - but they run from the most deeply nested container node out. While this generally breaks the use case you have in mind with framework.NewDefaultFramework it is the natural order users typically expect things to run when they write out their nested tests.

This ordering has the side-effect of making it hard to write support structures like framework that need to do their own cleanup. In Ginkgo V1 I would say that you need to tell consumers of the framework to do something like:

Describe("...", func() {
	var f *framework.Framework

	BeforeEach(func() {
		f = framework.NewFramework()
		f.Setup()
	})

	AfterEach(func() {
	    f.Cleanup()
	})

	// ...tests
})

This is, admittedly, more boilerplate and error-prone.

With V2, however, you can keep what you want:

Describe(..., func() {
   f := framework.NewDefaultFramework()

   AfterEach(func() {
      // use f
   })
})

...you just need to use DeferCleanup instead of AfterEach in NewDefaultFramework:

func NewDefaultFramework() *framework.Framework {
	var f *framework.Framework

	BeforeEach(func() {
	    f = &framework.Framework{}
	    //...

	    DeferCleanup(func() {
	    	f.Cleanup()
	    	//etc...
	    })
	})

	return f
})

This will work as desired because of how Ginkgo orders DeferCleanups: they will always run after any After* nodes and will run in first-in-last-out order.

All of this needs to be better documented but DeferCleanup was created in part to solve precisely the problem you are bumping into.

[pohly]

I agree that DeferCleanups solves the problem.

[onsi]

ok great. i'll plan to close this issue after i update the docs, then.