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.

Sign up for GitHub

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

Jump to bottom

Would like for *rapid.T to implement the Cleanup function like *testing.T #62

Closed
nbgraham opened this issue Aug 28, 2023 · 2 comments · Fixed by #79
Closed

Would like for *rapid.T to implement the Cleanup function like *testing.T #62

nbgraham opened this issue Aug 28, 2023 · 2 comments · Fixed by #79
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@nbgraham
Copy link

From gomock:

// cleanuper is used to check if TestHelper also has the `Cleanup` method. A
// common pattern is to pass in a `*testing.T` to
// `NewController(t TestReporter)`. In Go 1.14+, `*testing.T` has a cleanup
// method. This can be utilized to call `Finish()` so the caller of this library
// does not have to.
type cleanuper interface {
	Cleanup(func())
}

I am using rapid with gomock and found some errors in my tests when I noticed that Finish was not being called on the mock controller after each Check
@flyingmutant flyingmutant added enhancement New feature or request help wanted Extra attention is needed labels Feb 1, 2025
abhinav added a commit to abhinav/rapid-go that referenced this issue Feb 16, 2025
@abhinav
feat: Add T.Context()
96b6002 
*Context*:
Go 1.24 adds a `Context()` method to `testing.TB` that returns
a context bound to the current test's lifetime.
The context becomes invalid _before_ `T.Cleanup` functions are run.

*Details*:
This change adds a similar `Context()` method to `rapid.T`,
except this context is only valid for the duration of one iteration
of a rapid check.

*Implementation notes*:
This changes `newT` to return a `cancel` function
instead of just adding a `cancel` method to `T`
to ensure that all callers of `newT` remember to call `cancel`.

This also uses IIFEs (immediately-invoked function expressions)
in a couple places to rely on well-timed `defer` calls for cleanup
instead of manually calling `cancel`.

*Future work*:
The logic added in this commit will make it relatively straightforward
to add a `Cleanup` method (flyingmutant#62).
@abhinav abhinav mentioned this issue Feb 16, 2025
Merged
flyingmutant pushed a commit that referenced this issue Feb 20, 2025
@abhinav
feat: Add T.Context() (#77)
c3c5e3c 
* feat: Add T.Context()

*Context*:
Go 1.24 adds a `Context()` method to `testing.TB` that returns
a context bound to the current test's lifetime.
The context becomes invalid _before_ `T.Cleanup` functions are run.

*Details*:
This change adds a similar `Context()` method to `rapid.T`,
except this context is only valid for the duration of one iteration
of a rapid check.

*Implementation notes*:
This changes `newT` to return a `cancel` function
instead of just adding a `cancel` method to `T`
to ensure that all callers of `newT` remember to call `cancel`.

This also uses IIFEs (immediately-invoked function expressions)
in a couple places to rely on well-timed `defer` calls for cleanup
instead of manually calling `cancel`.

*Future work*:
The logic added in this commit will make it relatively straightforward
to add a `Cleanup` method (#62).

* lazy init, cleanup in checkOnce, maybeValue, example

Per GitHub comment, delete the `cancel` return value from newT,
instead add a single `cleanup` method.

The method is called for cleanup in three places:

- checkOnce: this is per property
- maybeValue: this is per Custom generator function call
- example: this is per Example call

Context is now initialized lazily:
if there isn't one, it is created.

* fix: data race in t.ctx access
abhinav added a commit to abhinav/rapid-go that referenced this issue Feb 20, 2025
@abhinav
feat: Add T.Cleanup
d5c6847 
Adds a new T.Cleanup method to register cleanup functions.
These will run after each iteration of a Check.

The implementation of how cleanup functions are tracked and run
is borrowed heavily from testing.T's own implementation.
Namely:

- [T.Cleanup puts functions in a slice](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1214-1216)
- [cleanups are run in reverse order](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1433-1446)
  popping functions from the slice one by one
- [panics are not allowed to interrupt cleanup](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1420-1427)

As with `testing.T`, cleanup runs after the context is canceled.
Because Rapid's context is lazily initialized,
it needs an additional check to avoid providing a valid context
during cleanup.

Resolves flyingmutant#62
@abhinav abhinav mentioned this issue Feb 20, 2025
Merged
@flyingmutant
Copy link
Owner

@nbgraham can you please check that Cleanup implemented in #79 works for your use-case?

@nbgraham
Copy link
Author

@nbgraham can you please check that Cleanup implemented in #79 works for your use-case?

yes, looks good!

flyingmutant pushed a commit that referenced this issue Feb 24, 2025
@abhinav
feat: Add T.Cleanup (#79)
7adc026 
* feat: Add T.Cleanup

Adds a new T.Cleanup method to register cleanup functions.
These will run after each iteration of a Check.

The implementation of how cleanup functions are tracked and run
is borrowed heavily from testing.T's own implementation.
Namely:

- [T.Cleanup puts functions in a slice](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1214-1216)
- [cleanups are run in reverse order](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1433-1446)
  popping functions from the slice one by one
- [panics are not allowed to interrupt cleanup](https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1420-1427)

As with `testing.T`, cleanup runs after the context is canceled.
Because Rapid's context is lazily initialized,
it needs an additional check to avoid providing a valid context
during cleanup.

Resolves #62

* doc: try to be clearer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add T.Cleanup abhinav/rapid-go
2 participants
@flyingmutant @nbgraham