flyingmutant / rapid

Rapid is a modern Go property-based testing library
https://pkg.go.dev/pgregory.net/rapid
Mozilla Public License 2.0
579 stars 25 forks source link

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

Closed swist closed 1 year ago

swist commented 1 year ago

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 commented 1 year ago

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

flyingmutant commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.).