Add a boolean return value to test functions

[jli-cparta]

I find it useful to be able to control the flow of execution in my tests depending on the outcome of previous tests. This PR adds a bool return value from test test functions.

[breml]

I like this and I wanted to have something like this in the past myself. It is especially useful in combination with NewRelaxed.

[breml]

@jli-cparta Can you provide an example, how you would use this new functionality. I can see the value e.g. if one wants to print additional debug information in the case of a failing test. That being said, one of my use cases would still not be possible with this, because if an assertion fails, the test case will be considered as failed even if NewRelaxed is used.

So something like this will not work if one wants to test the error and the non-error case in the same test function:

is := is.NewRelaxed(t)
result, err := FuncUnderTest()
if is.NoErr(err) {
  // do some more checks on the result
  is.Equal(wantResult, result)
  ...
}

[jli-cparta]

I want to use it elide subsequent tests that are known to fail if a precondition fails. The test case should be failed; this is just to cut down on chaff so that only the most relevant errors are shown. Like, if we fail to open a required file that we want to verify the contents of, no point in trying to read the contents and check them if the open failed - but we still want to see a error for the missing file.

[jli-cparta]

Ping @breml

[breml]

First of all, I am not the maintainer of this package but only a frequent user myself, so in the end, you will not have to convince me but @matryer. Second, I am not yet sure, if I understand your example. Can you provide a code snipped similar to the one I provided above?

[jli-cparta]

@breml Sorry! I'll ping @matryer here instead.

Sure, here is some untested (ironically enough) code:

func Test_Sample_Parses_OK(t *testing.T) {
  is := is.New(t)
  if f, err := os.Open("test.sample.data"); is.NoErr(err) {
    defer f.Close()
    if b, err := io.ReadAll(f); is.NoErr(err) {
      if result, err := FuncUnderTest(b); is.NoErr(err) {
        is.Equal(expected, result)
      }
    }
  }
}

So if the sample data isn't readable, the test fails but instead of several errors, only one (the relevant one) is reported.

[matryer]

@jli-cparta this seems fair enough, and it isn't a breaking change (since they don't return anything at the moment.)

But it only makes sense with NewRelaxed I suppose, since the rest will stop immediately. Right?

[breml]

But if you only care about the relevant error, why don't you write the code like this:

func Test_Sample_Parses_OK(t *testing.T) {
  is := is.New(t)

  f, err := os.Open("test.sample.data")
  is.NoErr(err)
  defer f.Close()

  b, err := io.ReadAll(f)
  is.NoErr(err)

  result, err := FuncUnderTest(b)
  is.NoErr(err)
  is.Equal(expected, result)
}

With is.New(t), the is package fails on the first error and the processing is not continued so only the relevant error is reported.

Personally, I prefer this form of test code instead of the (deeply) nested form.

As @matryer already mentioned, the return value for is.NoErr(...) and is.Equal(...) is only relevant, if used with is.NewRelaxed(t).

[jli-cparta]

As @matryer already mentioned, the return value for is.NoErr(...) and is.Equal(...) is only relevant, if used with is.NewRelaxed(t).

You're right. And so it's only useful if you want to do multiple tests in one function. And doing more than one test in a function may be considered a bad thing, or at least something not encouraged by Go best practices.

I'll leave it up to @matryer then - please feel free to close the PR or merge it - I will adapt my tests accordingly.

[matryer]

Sorry everyone, I dropped the ball on this. @flowchartsman Can you have a look and let me know what you think?

[flowchartsman]

I think ergonomics are a founding principle of the module as I understand it. so readability is paramount, and the nested code is much harder to read. Plus, there's already existing functionality to exit immediately by simply not using NewRelaxed. I think we'll close this one, but thank you so much for the contribution!

If it turns out there's a similar need that the package doesn't address, we can try and come up with something that's a little more is-y.