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

proposal: testing: expose testing.Verbose() as part of testing.TB #53504

Closed
mway opened this issue Jun 22, 2022 · 3 comments
Closed

proposal: testing: expose testing.Verbose() as part of testing.TB #53504

mway opened this issue Jun 22, 2022 · 3 comments
Labels
Milestone

Comments

@mway
Copy link

mway commented Jun 22, 2022

Currently, the testing package exports the package-level Verbose function, which maps to whether or not the -test.v flag was provided.

I'd like to propose that Verbose() bool also be added to testing.TB (and thus be implemented by testing.T and testing.B). AFAICT it would be fine for this to simply call testing.Verbose() under the hood, as the value would not change among tests in a suite or their subtests.

The primary motivation is that it's not possible to mock behavioral changes in libraries (such as test utilities) based on whether -test.v was provided or not. Many libraries provide a pseudo-testing.TB interface for mocking (which intentionally can't be mocked directly due to the unexported TB.private() interface method), but package-level functions cannot be mocked. Thus, to support testing behavior based on testing.Verbose() without a corresponding method on testing.TB, something like the following would be required:

type VerboseReader interface {
  Verbose() bool
}

type verboseReader struct{}

func (verboseReader) Verbose() bool {
  return testing.Verbose()
}

which is, IMO, not ideal. Given that the methods currently on testing.TB are all specific to the currently-running test, testing.Verbose() indirectly influences the output behavior of each test despite being test-agnostic itself, so I think it makes sense to include as part of testing.TB.

Any thoughts, feedback, or suggestions are appreciated!

@mway mway added the Proposal label Jun 22, 2022
@gopherbot gopherbot added this to the Proposal milestone Jun 22, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 22, 2022
@mvdan
Copy link
Member

mvdan commented Jun 23, 2022

TB.Verbose would be unlike all other existing methods on TB, in that it would report a static global value rather than do something different depending on what test or benchmark is running. I know that is exactly what you're asking for, but it's worth noting that it would be a somewhat inconsistent API.

I also wonder why should we add TB.Verbose and not also Short, Coverage, CoverMode, or other global "getter" APIs. Potentially we could end up with quite a bit of duplication across four types - the interface and three implementers.

Finally, I'm a bit confused by the purpose of this proposal, which is to mock the testing package. The testing package already has a mechanism to discourage mocking, like you said. It seems to me like if you want to test a testing library or utility, you need to agree on a small interface to abstract away the testing package, just like you already do for TB. Adding Verbose to that mix, for a total of an extra handful of lines, doesn't seem like a big difference to me :)

@rsc
Copy link
Contributor

rsc commented Jun 23, 2022

There are already much simpler ways to accomplish this.

Instead of

type VerboseReader interface {
  Verbose() bool
}

type verboseReader struct{}

func (verboseReader) Verbose() bool {
  return testing.Verbose()
}

myCode(verboseReader{})

Why not

myCode(testing.Verbose())

?

Or if you really want to pass code and not a value, why not

type verboseReader func() bool

myCode(testing.Verbose)

?

@mway
Copy link
Author

mway commented Jun 23, 2022

All fair points - in particular, agreed that just passing a func() bool is a perfectly fine (and simpler) way to address this as well. Closing this due to the proposal being superfluous.

For posterity, the idea was less about "this is difficult/impossible to accomplish right now" and more about "it would be convenient to access this as part of a testing.T/testing.B", ideally avoiding adding a dedicated "verbose getter" (or similar) parameter to testing utility functions/libraries that users need to care about.

In retrospect, there are better ways to inject and mask this from users by default - functional options or unexported var function manipulation being among the more obvious ways.

Thanks for the feedback!

@mway mway closed this as completed Jun 23, 2022
@ianlancetaylor ianlancetaylor removed this from Incoming in Proposals (old) Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants