suite: skip AfterTest when SetupTest or BeforeTest skips (#1781)#1873
Open
barry3406 wants to merge 1 commit intostretchr:masterfrom
Open
suite: skip AfterTest when SetupTest or BeforeTest skips (#1781)#1873barry3406 wants to merge 1 commit intostretchr:masterfrom
barry3406 wants to merge 1 commit intostretchr:masterfrom
Conversation
If SetupTest or BeforeTest calls t.Skip(), the deferred cleanup still invoked AfterTest even though the paired BeforeTest never completed. Track whether BeforeTest finished and only run AfterTest when it did, so AfterTest always has a matching BeforeTest. TearDownTest still runs in all cases to preserve the existing contract that teardown can clean up partially initialized state. Fixes stretchr#1781
dolmen
reviewed
Apr 22, 2026
| // AfterTest has a matching BeforeTest. | ||
| if beforeTestFinished { | ||
| afterTestSuite.AfterTest(suiteName, method.Name) | ||
| } |
Collaborator
There was a problem hiding this comment.
Move the if beforeTestFinished around the if suite.(AfterTest) block: I prefer to keep the cast really close to the method call.
dolmen
requested changes
Apr 22, 2026
|
|
||
| // skipInBeforeTestSuite is used to verify that AfterTest is not called when | ||
| // BeforeTest skips the test (#1781). | ||
| type skipInBeforeTestSuite struct { |
Collaborator
There was a problem hiding this comment.
Wrap SuiteTester that already handles counting.
You'll have to override only the method where Skip() happens.
With this the case will be much shorter and so clearer.
type skipInBeforeTestSuite struct {
SuiteTester
}
func (s *skipInBeforeTestSuite) BeforeTest(suiteName, testName string) {
s.SuiteTester.BeforeTest(suiteName, testName)
if testName == "TestA" {
s.T().Skip()
}
}Also add a TestB and check that its Before/After are correctly called
| // skipInSetupTestSuite is used to verify that AfterTest is not called when | ||
| // SetupTest skips the test (#1781). | ||
| type skipInSetupTestSuite struct { | ||
| Suite |
Collaborator
There was a problem hiding this comment.
Wrap SuiteTester also here.
| // skipInTestBodySuite is used to verify that AfterTest is still called when | ||
| // the test body itself calls Skip (regression guard for existing behavior). | ||
| type skipInTestBodySuite struct { | ||
| Suite |
Collaborator
There was a problem hiding this comment.
Wrap SuiteTester also here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Skip the
AfterTesthook when aSetupTestorBeforeTestcall caused the test to be skipped, soAfterTestalways has a matchingBeforeTest.Changes
suite/suite.go: track whetherBeforeTestfinished via a localbeforeTestFinishedflag; only invokeAfterTestin the deferred cleanup when that flag is set.suite/suite_test.go: add tests covering skip-in-BeforeTest, skip-in-SetupTest, and skip-in-test-body (regression guard).TearDownTestis intentionally left unchanged: it still runs in all cases so users can clean up partially initialized state set up before the skip.Motivation
Per #1781, calling
t.Skip()fromBeforeTestcorrectly prevents the test body from running, butAfterTestis still invoked — leavingAfterTestwithout the pairedBeforeTestit expects. Scenario 3 in the reporter's example causes apanic("after")today even though the test should be skipped cleanly.SetupTesthas the same inconsistency (scenario 2). GatingAfterTestonBeforeTesthaving finished resolves both without changing the test-body-skip path.Related issues
Closes #1781