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: add conditional-error helpers to T and B #45650

Open
seebs opened this issue Apr 20, 2021 · 8 comments
Open

proposal: testing: add conditional-error helpers to T and B #45650

seebs opened this issue Apr 20, 2021 · 8 comments

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Apr 20, 2021

$ go version
1.16

Does this issue reproduce with the latest release?

sure

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

if err != nil {
  t.Fatalf("explanation: %v", err)
}

Hundreds of times. Maybe thousands by now.

Also, I keep finding helper functions like

func PanicOn(err error) {
  if err != nil { panic(err.Error()) }
}

in various other people's code, which all seem focused on the problem of "it is inconvenient to write three lines of code and you might prefer to write one".

What did you expect to see?

I don't know about expected, but it occurs to me: In general, you can't call a function which makes the caller return early, but in the specific case of tests, we actually do have a way to express "return early from this entire test". And you are likely to want to do so fairly often. Extremely often, even.

Obviously, I can write a helper function that takes all the parameters, but this seems like a case where the reduced verbosity really helps code clarity a lot.

Approximate proposal:

func (t *testing.T) Check(err error, msg string) {
  if err != nil {
    t.Fatalf("%s: %v", msg, err)
  }
}

It's harder to express this as a *f function, like Fatalf, because then the parameter which is The Error is less obvious if you use message-parameter ordering for formatting arguments, and which argument to bind it to is less obvious if you put the parameter first, and if you put it in both places it stutters, but it might be worth trying to get that right.

What did you see instead?

A twisty maze of little test helper functions, all very slightly different and many sort of bad.

@mvdan
Copy link
Member

@mvdan mvdan commented Apr 20, 2021

I think if we're going to have checks/assertions, we should cover all cases and not just err != nil. See https://pkg.go.dev/github.com/frankban/quicktest#Assert for example.

One could argue that would be far more complex, but if #45200 gets accepted, we've already got most of the internal machinery; all that would be necessary is the high-level "checker" API.

@mvdan
Copy link
Member

@mvdan mvdan commented Apr 20, 2021

@ianlancetaylor ianlancetaylor changed the title testing: add conditional-error helpers to T and B proposal: testing: add conditional-error helpers to T and B Apr 20, 2021
@gopherbot gopherbot added this to the Proposal milestone Apr 20, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Apr 20, 2021
@akupila
Copy link

@akupila akupila commented Apr 21, 2021

I think this can be only about err != nil. As mentioned in the original proposal, that's very common in tests. A lot of time you don't care about the error, just that it didn't happen.

I think the message may not be needed, maybe it's ok to just stop at that line?

func (t *testing.T) NoError(err error) {
  if err != nil {
    t.Fatalf("Unexpected error: %v", err)
  }
}

When I used testify in the past, I used require.NoError(t, err) a lot for the same purpose

@rsc
Copy link
Contributor

@rsc rsc commented Apr 21, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Apr 21, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 28, 2021

If you find yourself using these five lines often, then it's easy to put them in your own library:

func (t *testing.T) Check(err error, msg string) {
  if err != nil {
    t.Fatalf("%s: %v", msg, err)
  }
}

Personally, I would do something different, to make sure that the arguments to Check and t.Fatalf are different. And others would do different things too, as you've noted. The diversity in helpers is fine. Package testing only needs to provide the shared abstractions on which they all build.

@seebs
Copy link
Contributor Author

@seebs seebs commented Apr 28, 2021

// ./prog.go:8:6: cannot define new methods on non-local type testing.T
func (t *testing.T) Check(err error, msg string) {
  if err != nil {
    t.Fatalf("%s: %v", msg, err)
  }
}

func Check(t *testing.T, err error, msg string) { ... }

// and now, instead of
t.Check(err, msg)
// i have to write
mylib.Check(t, err, msg)

I think putting it in another library makes it a lot less clear.

The reason I wanted the message is mostly that it makes it easier to tell the errors apart visually without having to look at the source, but knowing the line number is also usually fine.

@bvisness
Copy link

@bvisness bvisness commented Apr 29, 2021

Regarding this specific example:

// and now, instead of
t.Check(err, msg)
// i have to write
mylib.Check(t, err, msg)

If this aesthetic detail is an issue for you, it's easy to write a helper that wraps t for you:

checker := mylib.Checker(t)
checker.Check(err, msg)

In fact, testify already provides an API like this.

I agree with @mvdan that this proposal alone wouldn't provide a rich enough API for asserting things about errors. Error values in Go are a lot richer than just "nil or not nil" anyway, especially with Unwrap, Is, and As as of 1.13.

I think a high-level assert API is clearly out of scope for the testing package, even one as "simple" as the Check proposed here. It's probably out of scope for the standard library in general. Test helpers should be left to community libraries, where iteration can happen more quickly and people can freely use the tools that feel best to them.

@rsc
Copy link
Contributor

@rsc rsc commented May 5, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Decline
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants