suite: Skipping a test in BeforeTest prevents test execution but not AfterTest

[brackendawson]

Description

Related to #473 and #1722. If testing.T.Skip is called in Before test, we should skip that test and its teardowns. If it's called in the suite setup, we should skip the entire suite.

Step To Reproduce

package kata_test

import (
	"testing"

	"github.com/stretchr/testify/suite"
)

type My struct {
	suite.Suite
}

func (m *My) BeforeTest(s, t string) {
	m.T().Skip()
	panic("before")
}

func (m *My) AfterTest(s, t string) {
	panic("after")
}

func (m *My) TestA() {
	panic("during")
}

func TestIfy(t *testing.T) {
	suite.Run(t, &My{})
}

Expected behavior

No panics

Actual behavior

panic: "after"

[segogoreng]

Hi @brackendawson
I'm new to this project and open source projects in general, and I'm looking for "beginner" issues to contribute to. Would you say if this is beginner's friendly issue? 😃

[segogoreng]

Let me check my understanding of the issue.
If testing.T.Skip is called in BeforeTest or SetupTest, we should skip both AfterTest and TearDownTest?
This one is not the current behavior.

if testing.T.Skip is called in SetupSuite, we should skip all tests and the hooks?
This one is already the current behavior.

[brackendawson]

I'd say this is not a trivial issue due to the bad API in the suite package.

It's clear that if testing.T.Skip() is called in a setup method then all lesser setup methods, test and lesser teardown methods should not be called. But it's unclear if the corresponding teardown should be called.

package kata_test

import (
	"testing"

	"github.com/stretchr/testify/suite"
)

type MySuite struct {
	suite.Suite
}

func (m *MySuite) BeforeTest(_, _ string) {
	// setup dependency here?
	m.T().Skip()
	m.T().Log("Before")
	// setup dependency here?
}

func (m *MySuite) TestIfy() {
	m.T().Log("During")
}

func (m *MySuite) AfterTest(_, _ string) {
	// teardown dependency?
	m.T().Log("After")
	m.T().Log(m.T().Skipped())
}

func TestIfy(t *testing.T) {
	suite.Run(t, &MySuite{})
}

Whether the corresponding teardown method needs to be run depends on if the user called skip at the start or end of the setup method. In the absence of documentation saying what is supposed to happen we probably have to keep the existing behavior where there teardown does run. This is supported by the fact that you can tell whether the test is skipped in the teardown.

However SutupSuite and TearDownSuite (why is setup one word but tear down two?) behave in the opposite manor. Probably this has to stay the same too otherwise it would be a breaking change.

The best we can do in this situation is to test the behavior of calling skip in all of the setup/before methods and document what the behavior is.

Skip in BeforeTest really should skip the test, though. It's amazing that we have the full trinity of possible behaviour today.

[brackendawson]

One of the most helpful things you could do is to find a PR with changes requested where the author hasn't replied in a few months and finish it for them. Make sure you base your PR off their branch to preserve their attribution.

#1723 perhaps.

[segogoreng]

Hi @brackendawson , thanks for the answer.

Whether the corresponding teardown method needs to be run depends on if the user called skip at the start or end of the setup method. In the absence of documentation saying what is supposed to happen we probably have to keep the existing behavior where there teardown does run. This is supported by the fact that you can tell whether the test is skipped in the teardown.

However SutupSuite and TearDownSuite (why is setup one word but tear down two?) behave in the opposite manor. Probably this has to stay the same too otherwise it would be a breaking change.

Yeah, that makes sense. So a bug (or undefined requirement) might have become a feature for some users.

Skip in BeforeTest really should skip the test, though

It's already the current behavior, isn't it?

One of the most helpful things you could do is to find a PR with changes requested where the author hasn't replied in a few months and finish it for them. Make sure you base your PR off their branch to preserve their attribution.

#1723 perhaps.

Thank you so much for the advice. I'll take a look at the PR