proposal: testing: testing.T.IsNil

[glossd]

Proposal Details

Go Wiki explains that assertions are bad because they either

1. stop the tests early
2. omit useful information.

Often, in tests, we need to check the errors on nil. If it's not, it indicates a function failure.
This is a classic way of handling errors in tests.

func TestDo(t *testing.T) {
    out, err := Do()
    if err != nil {
        t.Fatalf("Do error: %s", err)
    }
    ...
}

1. If the error happens there's no point in going further
2. Errors carry a description of what went wrong.

I propose to add the IsNil method to the common testing type.
It would result in code like this:

func TestDo(t *testing.T) {
    out, err := Do()
    t.IsNil(err)
    ...
}

I would also like to point out that the nil or zero value doesn't carry any information and perhaps IsZero method should be added as well.
However, to prevent any possible abuse of assertions, we could limit the parameter type to the error interface.
E.g.

func (c *common) IsErrorNil(err error) {
    if !reflect.ValueOf(err).IsNil() {
        c.checkFuzzFn("IsErrorNil")
        c.log(fmt.Sprintf("error: %s", err))
        c.FailNow()
    }
}

[gabyhelp]

Related Issues

• testing: Assert(...error) #7009 (closed)
• proposal: testing: add conditional-error helpers to T and B #45650 (closed)
• proposal: spec: permit type assertion to nil #11635 (closed)
• proposal: testing: verify failure (rather than skip) #28488 (closed)
• proposal: testing: add assertion methods #52555 (closed)
• proposal: errors: Is resolve nil error problem #40674 (closed)
• proposal: assert of errors #32997 (closed)
• proposal: testing: Collect test failure logs on the testing.common struct and expose collected logs via a new method on testing.TB #61860

Related Documentation

• Go Wiki: Go Code Review Comments > Useful Test Failures

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

I don't like that it encourages checks / tests to fail without context

[Jorropo]

For context you can find third party implementations of this and more:

• NoError
• NotNil
• NotZero

They all use ...any and unpack it into fmt.Sprintf to provide context.

[glossd]

@seankhliao what about the .IsErrorNil(error)? You would see the name of the test and the error that failed it.

[thepudds]

Hi @glossd, I suspect this has already been proposed, discussed, and declined.

Would you mind at least skimming the discussions in the issues linked by the @gabyhelp robot above (for example, #7009) to help check if that suspicion is true, as well as review this FAQ:

https://go.dev/doc/faq#testing_framework

Also note that in the interests of efficiency and to help avoid re-hashing old discussions, the Go project generally does not reconsider prior proposal decisions unless there is significant new information (as described for example in the "Reconsideration" section here).

[glossd]

@thepudds, the person asked for assertions. I feel this is a special case because the nil/zero value does not carry any information which you can report in the tests.

[earthboundkid]

I wrote my own assertion library, and I don't think this makes sense for the standard library. In my own code, I'm happy to follow my own philosophy about test errors and how they should be reported. For the standard library though, I feel like there should really just be universal, low level tools people can use to build their own systems instead of something that imposes its own opinion on users.

[seankhliao]

I think this is a dupe of #7009, and for the same reason we encourage error wrapping with details, we encourage failing tests with sufficient context. a test failure with a non nil error like "unexpected eof" is not very informative on its own.

[glossd]

@seankhliao that Issuer is talking about helper assertion functions and has zero arguments.
The errors should be descriptive by themselves, if it says "unexpected eof" and you don't understand the context, then your function has poor error handling.
t.IsErrorNil(err) would actually enforce writing descriptive errors.

[thepudds]

Hi @glossd, did you have a chance to at least skim through all the prior issues that the @gabybot found, as requested above?

Also, would you say that there is overlap with your proposal and comments like #45650 (comment):

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

Would you say that the rationale for closing #45650 in #45650 (comment) for example would apply to your proposal here as well? In other words, even if you view some of the details as being different, does that rationale still apply despite any differences?

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.

In any event, I do believe your proposal has been discussed before and that Sean is correct to close this.

[glossd]

@thepudds my proposal is about checking values that do not cary any information with IsNil, IsZero methods.
Method checking error on nil seems like something highly desired by other developers, and I agree with them.