Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that subtest name is set in SetupSubTest/TearDownSubTest #1393

Closed
wants to merge 1 commit into from

Conversation

thekidder
Copy link

@thekidder thekidder commented Jun 1, 2023

Summary

Ensure that subtest name is set in SetupSubTest/TearDownSubTest

Changes

  • Ensures that suite.SetT() (with the subtest T) is called before SetupSubTest
  • Ensures that the original T is restored after TearDownSubTest

Motivation

One common usecase of SetupSubTest is to set up mocks and expectations for a table test. In this usecase, the current behavior does not set the test name until after SetupSubTest is run. This makes test failure reporting substantially less informative.

@dolmen dolmen added Changes Requested pkg-suite Change related to package testify/suite bug labels Jul 5, 2023
Comment on lines 263 to 265
subTestT := suite.T()
suite.NotEqual(subTestT, suite.SuiteT)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, something like this would allow you to add your assertions alongside the others and call assert.Contains with the specific values we expect to see:

Suggested change
subTestT := suite.T()
suite.NotEqual(subTestT, suite.SuiteT)
suite.TearDownSubTestNames = append(suite.SetupSubTestNames, suite.T().Name())

@thekidder
Copy link
Author

Thanks for the review @tomhutch -- is this similar to what you were thinking?

@thekidder
Copy link
Author

Hey @tomhutch -- this is ready for re-review

@dolmen
Copy link
Collaborator

dolmen commented Oct 16, 2023

This will be fixed by #1471.

@thekidder I'm sorry your own MR didn't get the attention it needed. Could you help the community effort by adding your review to #1471?

@dolmen
Copy link
Collaborator

dolmen commented Oct 16, 2023

@tomhutch You had also reviewed this MR. Could you review #1471?

@thekidder
Copy link
Author

#1471 looks like a great fix, thanks for pointing me toward that. Will close this request.

@thekidder thekidder closed this Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pkg-suite Change related to package testify/suite rejected/duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants