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

bugfix: ensure no panics when calling t.Name() inside a generator #58

Conversation

swist
Copy link

@swist swist commented Jul 10, 2023

This is a fairly annoying footgun. While rapid.T is guaranteed to never be nil pretty much, we offer no such guarantees as to the presence of testing.T inside of it. As a result calling APIs present on rapid.T interface can result in panics if invoked via .Example() instead of .Draw()

Attached test case would fail prior to this change and illustrates the problem

@swist swist force-pushed the bugfix/support-calling-name-inside-generators-invoked-via-example branch 2 times, most recently from cd0306d to f744d8e Compare July 10, 2023 16:03
@swist
Copy link
Author

swist commented Jul 10, 2023

Perhaps when testing.T is not available we should print some other name?

This is a fairly annoying footgun. While rapid.T is guaranteed to
never be nil pretty much, we offer no such guarantees as to
the presence of testing.T inside of it. As a result
calling APIs present on rapid.T interface can result
in panics if invoked via `.Example()` instead of .Draw()

Attached test case would fail prior to this change and
illustrates the problem
@swist swist force-pushed the bugfix/support-calling-name-inside-generators-invoked-via-example branch from f744d8e to 38d7864 Compare July 10, 2023 16:04
@flyingmutant
Copy link
Owner

What is the scenario in which a generator is calling t.Name()? I'd say the contract is that the generator should only use *T to Draw, plus maybe call Logf.

@swist
Copy link
Author

swist commented Jul 11, 2023

Sometimes I use rapid to generate ids matching some schema. It's useful to be able to pass in the test name to the ID as part of the emitted string to be able to debug cases where for whatever reason the test fixture leaked (we sometimes use rapid to produce mock responses from other services and use that to manipulate the state in our test, kind of a state machine test but a bit more awkward)

[EDIT]: thinking about this harder, this is a problem with my test suite, but having this working would probably make unfurling the problem a bit easier

@flyingmutant
Copy link
Owner

The thing I am worried about is T.Helper(). It can't be wrapped (because it calls runtime.Callers() with a fixed skip), and because of that rapid just forwards it directly to tb. I think to have a proper fix, newT should enforce an invariant that tb is always non-nil (using a nilT instead of nil with noop Helper(), Name() etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants