franela / goblin

Minimal and Beautiful Go testing framework
MIT License
887 stars 79 forks source link

Test hooks should not execute if all tests are skipped #103

Open shakefu opened 3 years ago

shakefu commented 3 years ago

For example:

func TestSkipped(t *testing.T) {
    g := goblin.Goblin(t)
    g.Before(func () {
        panic("Tests are skipped, this shouldn't happen")
    })
    g.Xit(func (){ 
        g.Assert(false)
    })
}

This test suite should be a no-op, but will take action in the hooks (Before, After, etc.) even so.

This would be parity with the behavior when no tests are declared but there are hooks present.

My use case is using Before hooks to set up external services for the tests. We also allow a "fast" test mode when the services aren't present (by wrapping It and Xit from an environment flag).

Alternatively: Add a g.SkipIf(bool) to skip entire test suites if the passed value is true/false.

shakefu commented 3 years ago

This is the work-around we're currently using, a bit clumsy:

// Goblin is a partial interface for grabbing the funcs we need
type Goblin interface {
    It(string, ...interface{})
    Xit(string, ...interface{})
    Before(func())
    BeforeEach(func())
    JustBeforeEach(func())
    After(func())
    AfterEach(func())
    // Goblin funcs
    Assert(interface{}) *goblin.Assertion
    Fail(interface{})
    FailNow()
    Failf(string, ...interface{})
    Fatalf(string, ...interface{})
    Errorf(string, ...interface{})
    Helper()
}

type Skip struct {
    Goblin
}

func (this *Skip) It(name string, h ...interface{}) {
    this.Goblin.Xit(name, h...)
}
func (this *Skip) Before(f func()) {
}
func (this *Skip) BeforeEach(f func()) {
}
func (this *Skip) JustBeforeEach(f func()) {
}
func (this *Skip) After(f func()) {
}
func (this *Skip) AfterEach(f func()) {
}

// SkipIf returns a wrapped Goblin object if noSkip is false, otherwise it
// returns the normal Goblin instance passed in.
func SkipIf(g *goblin.G, noSkip bool) Goblin {
    if noSkip {
        return g
    }
    // Skip holds noop functions for skipping tests
    skip := &Skip{g}
    return skip
}
marcosnils commented 3 years ago
func TestSkipped(t *testing.T) {
    g := goblin.Goblin(t)
    g.Before(func () {
        panic("Tests are skipped, this shouldn't happen")
    })
    g.Xit(func (){ 
        g.Assert(false)
    })
}

This is not a valid example since it doesn't contain a Describe block

This would be parity with the behavior when no tests are declared but there are hooks present.

What do you mean with this? Parity with what exactly?

I think an alternative here would be to also provide a Xdescribe option that will skip all block altogether along with their hooks and tests. WDYT?

shakefu commented 3 years ago

This is not a valid example since it doesn't contain a Describe block Bad example... hastily typed. Imagine there is one. xD

This would be parity with the behavior when no tests are declared but there are hooks present. What do you mean with this? Parity with what exactly?

func MyTest(...){
g := Goblin(...)
g.Describe("no tests", func (){
g.Before(func () {
fmt.Println("This never runs because there are no tests.")
})
})
g.Describe("one skipped test", func (){
g.Before(func (){ 
fmt.Println("This will run because there is a declared test, even tho it's empty and skipped")
})
g.Xit("placeholder test")
})
}

... having gotten into the code more this is because notifyParents is always called in (g *G)It and (g *G)Xit... and hence hasTests will be set in the 2nd example (but not the first) and it'll run the hooks.

So this was unexpected behavior/non-parity between what I assumed were similar test suite states (e.g. no tests, or just skipped tests).

I think an alternative here would be to also provide a Xdescribe option that will skip all block altogether along with their hooks and tests. WDYT?

I've gone ahead and implemented an alternative in #106 that handles a variety of cases with tests... I'm using that successfully from my fork but would love to see it make it back upstream.

/cc @marcosnils