Defer 'after' block before any hook execution

[cbron]

Currently if anything in the before block calls t.FailNow (like t.Fatal), the after block never executes because it never gets to defer. This is a problem if your before block sets up multiple objects and you need to clean all of them up if any fail.

Example:

func TestObject(t *testing.T) {
    spec.Run(t, "object", func(t *testing.T, when spec.G, it spec.S) {
        it.Before(func() {
            fmt.Println("Before")
            t.Fatal("Custom Fail")
        })
        it.After(func() {
            fmt.Println("After")
        })
        it("should have some default", func() {
            fmt.Println("Test")
        })
    })
}

Output, notice that After isn't printed:

=== RUN   TestObject
=== RUN   TestObject/object
=== RUN   TestObject/object/should_have_some_default
Before
--- FAIL: TestObject (0.00s)
    --- FAIL: TestObject/object (0.00s)
        --- FAIL: TestObject/object/should_have_some_default (0.00s)
            main_test.go:13: Custom Fail
FAIL
FAIL	tmp/spec	0.007s

Thanks for the library @sclevine , really enjoying working with it so far.

[sclevine]

Good catch!
Glad you like spec 😄

[sclevine]

Released: https://github.com/sclevine/spec/releases/tag/v1.3.0

[sclevine]

@cbron Looks like this change introduced an interesting bug:

func TestObject(t *testing.T) {
    spec.Run(t, "object", func(t *testing.T, when spec.G, it spec.S) {
        it.Before(func() {
            fmt.Println("Before")
            t.Skip()
        })
        it.After(func() {
            fmt.Println("After")
        })
        it("should have some default", func() {
            fmt.Println("Test")
        })
        when("group",  func() {
            it.Before(func() {
               fmt.Println("Skipped before")
            })
            it.After(func() {
                fmt.Println("This after should be skipped too")
            })
            it("should have some other default", func() {
                fmt.Println("blah")
            })
        })
    })
}

One might expect After to display, but not This after should be skipped too.

Here's a real-world example of where v1.3.0 introduced a test failure: https://github.com/buildpacks/pack/blob/master/internal/paths/paths_test.go#L24

I'd prefer to fix this for the next release rather than revert it. Let me know if you disagree about the correct behavior / have suggestions.

[cbron]

@sclevine interesting, I would also assume that only "Before" and "After" would print but not the others. I'd still want "After" to print because you might setup some objects in the before block and then skip/fail on a conditional halfway through, and after should clean that situation up.

Can't think of a simple solution off top of my head. Probably needs something more complex like grouping execution in blocks or adding in a separator. Really what we want is

execute [defer after, before, run, (after)],
execute [defer after, before, run, (after)],
...

[sclevine]

Working on a fix now -- just wanted to make sure that we agree on the behavior. 😄

[sclevine]

@cbron Fixed (v1.3.0...v1.4.0) and released:
https://github.com/sclevine/spec/releases/tag/v1.4.0

[cbron]

@sclevine nice solution, a good ol linked list. I bumped our version and its working well. Thanks!