Using assert or require from stretchr/testify in godog

[mirogta]

Hi all!

This has been bogging me for a while; why we cannot simply use assert and require funcs in the BDD steps funcs?

TL;DR

It can be done with a very little of boilerplate and it's not difficult at all.

I wish this was built-in into godog and available via some kind of a flag, or property in the suite setup, so that this boilerplate is not needed.

I've put together some analysis with some proposals.

---

More below…

Why?

As a engineering manager
I want my team move some existing non-BDD test suites to BDD (using godog)
So that we improve the quality of the tests and spend the least possible effort in doing so

As a software engineer
I want godog BDD test steps, specifically the Then assertions, to support stretch/testify's assert and require functions out-of-the-box
So that I can easily migrate existing non-BDD tests to BDD (using godog)

I've looked here and here for some ideas.

Happy to be told this has been discussed before, rejected, disregarded, or anything else, but I haven't really found much useful about this topic anywhere. It's kinda big deal to me, possibly a blocker from using godog professionally on very large projects.

Seems like there are a couple of obvious reasons for this issue:

How to get access to the *testing.T instance?

As a developer
I want easy built-in access to the *testing.T instance within my test suite (godog test steps specifically)
So that I can use the assert module, write less code, spend less time doing so, and thus save money

This seems to be an easy one. We can use the test struct pattern as described e.g. in the API example:

type apiFeature struct {
  resp *httptest.ResponseRecorder
}
…

func (a *apiFeature) theResponseCodeShouldBe(code int) error {
	if code != a.resp.Code {
		return fmt.Errorf("expected response code to be: %d, but actual is: %d", code, a.resp.Code)
	}
	return nil
}

… and add the *testing.T field to it. That way it would be easily accessible in all steps:

type apiFeature struct {
  t *testing.T                     // <= HERE
  resp *httptest.ResponseRecorder
}
…

func (a *apiFeature) theResponseCodeShouldBe(code int) {
  assert.Equal(a.t, code, a.resp.Core, fmt.Sprintf("expected response code to be: %d, but actual is: %d", code, a.resp.Code))
}

Note the difference to the original code - it's now much cleaner, less code.

🤔 IMHO, this is the pattern I would like to see being supported Out-Of-the-Box or even suggested by default. When developers are looking at how to use godog for the first time, they may run into three ways of accessing common variables inside their step definitions:

1. Using global variables - I'd argue that it's generally a bad practice to use global variables, and would love these examples to be removed from the examples and docs.
2. Passing values between steps via the context - slightly better, but I'd argue that it's too verbose, complex and error-prone.
3. Using a struct - I'd argue that this is the best pattern, but some may see it as an anti-pattern, because it may lead to a badly written code which couples steps to features - see here

But there's a gotcha. 🚧

Assertion funcs like assert.Equal return a bool and not an error.

The theResponseCodeShouldBe does not return an error anymore and therefore the error from the assertion is somewhat swallowed and this step would actually PASS even in cases where it should FAIL. The godog test statistics would show happily something like 3 scenarios (3 passed, 0 failed) even if a step was failing. It may just log an error in the output. Not good.

Workaround: We could put the error return type back at the expense of more code:

func (a *apiFeature) theResponseCodeShouldBe(code int) error {
  if !assert.Equal(a.t, code, a.resp.Core, fmt.Sprintf("expected response code to be: %d, but actual is: %d", code, a.resp.Code)) {
    return fmt.Errorf("expected response code to be: %d, but actual is: %d", code, a.resp.Code)
  }
  return nil
}

Yuck ;-) This is obviously very naïve, with a duplicated code and should be refactored - producing even more code. Not good…

Another example - loosely following the example from the godog homepage:

func thereShouldBeRemaining(ctx context.Context, remaining int) error {
  available, ok := ctx.Value(godogsCtxKey{}).(int)
  if !ok {
    return errors.New("there are no godogs available")
  }

  if available != remaining {
    return fmt.Errorf("expected %d godogs to be remaining, but there is %d", remaining, available)
  }

  return nil
}

With the pattern supporting assert (and require), we would save so much code and make it even more readable!

type testContext struct {
  t         *testing.T
  available int
}

func (ctx *testContext) thereShouldBeRemaining(remaining int) {
  require.NotEmpty(t, ctx.available, "there are no godogs available")

  assert.Equal(t, ctx.available, remaining, fmt.Sprintf("expected %d godogs to be remaining, but there is %d", remaining, g.available))
}

From 12 lines of code down to 5 (for the step func, including an empty line). I call that a success!

But it doesn't work…

Yeah. I've tried to sum up and visually show the issues on this branch.

Patterns used:

• Test values always passed via a strongly-typed test context struct.
• This test context struct embeds another struct which provides access to *testing.T

See

• feature file with 3 scenarios
  • the 1st scenario should PASS ✅
  • the 2nd scenario should FAIL due to a failing assert, but it's passing ❌
  • the 3rd scenario should FAIL due to invalid pre-condition (using require) even before it gets to the assert - this is failing but the output is still "green" and not "red" ❓
• test
• textual output
• colorised output - as you can see, it's all nice and green (not good) and only the last scenario is failing. The second scenario is shown as PASS (not good). I've highlighted the problematic areas with red boxes for you.

Is there a fix?

You be the judge 😉 With the ideas shared in the links at the top and the above issues, I've tried to come up with a proof-of-concept to fix them.

Patterns used:

• Test values always passed via a strongly-typed test context struct.
• This test context struct contains another struct bt *BddTesting which handles failures (from require) and errors (from assert).

What is BddTesting?

• Wrapper around t *testing.T and potential errors
• Used in place of the t in assertion funcs, e.g. assert.Equal(t, …) would become assert.Equal(bt, …)
• Assertions from assert are tracked as errors, so that we get correct statistics and color-coding
• Pre-conditions from require are tracked as failures, but unlike the original t *testing.T module it's using t.Fail() and not t.FailNow(), so that we always get the overall test suite stats.

See

• feature file is the same one with 3 scenarios
  • the 1st scenario should PASS ✅
  • the 2nd scenario should FAIL due to a failing assert ✅
  • the 3rd scenario should FAIL due to invalid pre-condition (using require) even before it gets to the assert ✅
• test
• textual output
• colorised output - as you can see, all stats are correct and all failed steps are in red with the errors explaining the problems

Is it done? Can I use it now?

No, not quite.

There's a little issue with some duplicated output. It's currently too chatty. There's work to be done. If you know how to fix it, please feel free to raise a PR :-)

This is only a PoC and intended as a starting point for a discussion on how to make better use of assert and require effectively in godog.

So why not use assert-godogs?

• The pattern of returning errors is too verbose for testing, compared to single-line assertions. If you want to have multiple assertions, you would need to have multiple if error != nil {return error} conditions and therefore more code.
• I bet for anyone (like me) who has heaps of existing unit tests using the assert.Equal(t, expected, actual, msg) pattern, it would take a lot of effort to rewrite them to the format suggested in assert-godogs - in my case at least, it won't be done, too much work, and it's yet another reason why it's so hard to adopt BDD in a large organisation.

Thanks.

[wichert]

One addition that I would like to see is to run every scenario as a separate test, leveraging T.Run().

[mirogta]

@wichert sounds like a good idea - would you mind creating a new discussion or maybe even a new issue for this separately to this thread?

[vearutop]

This seems to be already the case https://github.com/cucumber/godog/blob/v0.12.6/suite.go#L445-L454.

	if s.testingT != nil {
		// Running scenario as a subtest.
		s.testingT.Run(pickle.Name, func(t *testing.T) {
			ctx, err = s.runSteps(ctx, pickle, pickle.Steps)
			if err != nil {
				t.Error(err)
			}
		})
	} 

[mirogta]

@aslakhellesoy @mattwynne hi, I understand you may have been super busy, but may I get some feedback on the post please?

I was wondering if you would have any objections to replace the existing assert-godogs example with something that more closely resembles how the assert or require are typically being used? IMHO this would make life easier for anyone using stretchr/testify.

Proposal:

1. Add support for t *testing.T in the step function signatures.
   e.g.

func iEat(t *testing.T, arg1 int) {
        // yay, now we can easily use assert or require
	assert.Fail(t, "Not implemented")
}

func InitializeScenario(ctx *godog.ScenarioContext) {
        ctx.Step(`^I eat (\d+)$`, iEat)
 }

And with the context:

func iEat(t *testing.T, ctx context.Context, arg1 int) {
        // yay, now we can easily use assert or require
	assert.Fail(t, "Not implemented")
}

func InitializeScenario(ctx *godog.ScenarioContext) {
        ctx.Step(`^I eat (\d+)$`, iEat)
 }

… but as per the post above, this alone won't be enough, so also:

2. Add a new flag to the suite options, to toggle the stretchr/testify support.
   This would add all the necessary plumbing to make the above actually work as expected.
   e.g.

func RunTests(t *testing.T) {
	suite := &godog.TestSuite{
		Options: &godog.Options{
			TestingT: &godog.AssertT{t: t}
		}
	}
}

[vearutop]

Having access to *testing.T in a step function would be indeed convenient.
A few thoughts:

• Parent *testing.T is optional, so if the step needs the test instance, we need to define what happens if there is no instance available.
• Failure condition can be inferred from a t.Failed() after step,
• Failure message can perhaps be captured by hijacking os.Stdout with os.Pipe,
• Idiomatically, ctx is passed as first argument, however t usually takes first position too, not sure which would be less surprising to users, I'd personally expect to have ctx, t, ...,
• Adding more "magical" (optionally positioned) arguments to step definition increases complexity.

[mirogta]

With a great contribution from @mrsheepuk (#571), this discussion can be closed now, testing.T is now supported. Thanks!