Skip to content

Conversation

renzoarreaza
Copy link
Contributor

@renzoarreaza renzoarreaza commented May 25, 2025

Summary

Adds support for users to provide extra filtering on which tests to run.

Changes

A new Exported function was needed in the suite package to support this.

func RunWithSkip(t *testing.T, suite TestingSuite, skip func(string, string) bool)

This function is the same as the existing Run function with an extra call to the skip function passed by the user.
I now let the Run function call this one.

Another change was to move the running of the SetupSuite to outside the iteration of the (test) methods.
If the skip function logs the skipped tests, this change makes sure all of these logs are together, at the top of the testrun, instead of having some skips, followed by the suite setup and more skips afterwards.

Motivation

The main motivation of this change is to allow for test registration. This allows tests to be skipped, as defined by the user, without repeatedly running the SetupTest/TeardownTest repeatedly for skipped tests.

Related

#1050 (issue) #1051 (PR)

Copy link
Collaborator

@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

I see some value in the refactoring of Run that moves the suite setup out of the loop. It would be worth submitting it as a separate cleanup PR.

However at this point I don't understand the use case. So I didn't take time to review the test suite.

@dolmen
Copy link
Collaborator

dolmen commented May 27, 2025

The use case is not clear to me. I don't see why I would need this.

So far this is 👎 for me.

@renzoarreaza If you are serious about adding this feature, you'll have to convince us (Testify maintainers) better (first requirement), improve the provided documentation (2nd requirement). A runnable example would also be helpful as additional documentation (3rd requirement).

@renzoarreaza
Copy link
Contributor Author

Thanks for the feedback, I will take a look. I have some ideas to improve it (apart from your comments) and I will work on a clearer example of the use case.

@dolmen dolmen added the pkg-suite Change related to package testify/suite label May 30, 2025
@renzoarreaza
Copy link
Contributor Author

renzoarreaza commented Jun 8, 2025

I have made some changes to the proposal.
The idea is the same, but the implementation has changed.
Instead of having RunWithSkip and a function that the user has to pass I've added a new interface that the user can optionally implement as part of their suite struct.

Motivation:

The goal of this proposal is for the user to have a way to define a filter for which tests should or shouldn't run.
(you could think of it as an extension of -testify.m)
I use testify for integration tests and SetupSuite and SetupTest are on the slower side.
Depending on how I build the src code, (daily/beta builds, specific feature flags enabled or not etc...) or on cli flags (e.g runLevel express, regular, extreme) I might want to not run some tests. Right now, the only way to achieve this is to have some checks at the beginning of the test and decide if s.T().Skip() needs to be called or not. This however results in SetupTest running unnecessarily.
These changes allow the user to skip tests at the collection phase and skipping the SetupTest for skipped tests (and even the SetupSuite is skipped if all tests are).

dolmen pushed a commit that referenced this pull request Jun 19, 2025
## Summary
Improve readability of suite.Run by moving the running of SetupSuite
outside of the loop iterating over the (test) methods.
This also allows for other simplifications further down in the code.

## Changes
- Move SetupSuite to outside the loop
- Don't run Setup/TeardownSuite if no tests are found (not new
behaviour, but new check)
- Remove variable to keep track of wether SetupSuite was executed or not

## Motivation
This is a subset of the changes I made under PR #1749. It was suggested
by @dolmen to open a separate PR for this part.

## Related issues
N/A
@dolmen
Copy link
Collaborator

dolmen commented Jun 19, 2025

@renzoarreaza I have just merged #1769 so a rebase would be welcome.

@brackendawson
Copy link
Collaborator

brackendawson commented Aug 24, 2025

I think it would be better to make sure that calling testing.T.Skip in the setup function stops that function and prevents the test and the teardown form being run too.

Edit: Skip will stop both the rest of BeforeTest, and the test, but not the call to AfterTest. I raised #1781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg-suite Change related to package testify/suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants