proposal: Replace errors with logging

[bkielbasa]

Describe the solution you'd like Right now, every step returns the context and error.

func step(ctx context.Context) (context.Context, error) {
   return ctx, errors.New("the math doesn't work for you")
}

My proposal is about removing the error from the step definition and replace them with logging. We can add them to the context

func step(ctx context.Context) context.Context {
   ctx.Error("the math doesn't work for you")
   return ctx
}

or

func step(TestingT t, ctx context.Context) context.Context {
   t.Error("the math doesn't work for you")
   return ctx
}

Available methods would be:

• t.Error
• t.Errorf
• t.Log
• t.Logf
• t.Fatal
• t.Fatalf

This proposal would resolve the issue in #63 . We can get read of panics and replace them with just regular function calls.

[bkielbasa]

@sagikazarmark, @arberiii, @sblundy WDYT?

[sagikazarmark]

Interesting proposal.

IMHO there are two types of error in a test:

• unexpected error that should stop the test (Fatal if you like)
• expected/business errors that belong to the result of a business call.

Business errors should probably checked in a Then step.

For example:

When I do a business call
Then I should see a business result

The error created in When gets checked in Then.

From this perspective, it doesn't really matter if the error is returned or Fatal is called. On the other hand, I might want to continue running the test (eg. to further evaluate certain results).

Sorry, this was kind of a brain dump. I will try to give it more thought.

[bkielbasa]

I proposed the change because it will help with debugging. Handling panics, returned errors etc is more complicated than I thought. Unexpected (for example runtime) errors will behave the same.

Another argument which promotes the proposal is that the standard testing.T/B has similar API so the end-user will have known experience.

It's a huge change so I want to discuss it with others :)

[bkielbasa]

about business errors, you can do what you do now. Just save the error in the context and check it in another step. There will be no changes in that flow.

The question is how about the context? Should it stay as it is or we should add the error there and the error should be checked in the step. Here's a proposal: https://github.com/go-bdd/gobdd/issues/77

[arberiii]

I'm in dilemma, I liked the proposition of adding error to the context because it adds no more complexity since no more parameters are added. But the second proposition it gets user the same feeling as usual testing.

As I user, I would prefer the second proposition as I'm used more to it and its more flexible (can use fatal, log, etc.)

[bkielbasa]

I have the dilemma as well. Personally, I'd prefer the second version but it's only my opinion.

[bkielbasa]

Do you have any arguments against the change? If now, I think I'll go for it (the second example).

[sblundy]

Debugging failed tests is a pain. But I worry these proposals make it too easy to create overly large steps. Also good failure messages are hard. Don’t think either will relieve that burden.

Informative and succinct failure messages is a core function of an assertion library. I think adding one, or a mechanism for users to use their favorite, would be more helpful.

[bkielbasa]

yeah, and because it's very hard to provide a good error message I proposed the change. Here's an example:

func someStep(ctx) (context, error) {
// ...
doSomething, err := service.Do(ctx)
if err != nil {
   return ctx, err
}
// more code goes here
}

How can I know on which line the error was created/occurred? It's almost impossible to track it down using reflection etc. We can force the user to use OUR errors and wrap those from their domain code but in my opinion, the code below looks a bit hacky.

func someStep(ctx) (context, error) {
// ...
doSomething, err := service.Do(ctx)
if err != nil {
   return ctx, gobdd.Err(err)
}
// more code goes here
}

What if the user forgets to add the wrapper? It's complicated with panics which are very heavily used in the context.Context. In this scenario, we can get the stack trace from the runtime package but it sounds like a hacky as well, at least IMO.

We still have to handle panics but it would be much more rare and not the default way of dealing with errors. And, what's more, the code will follow the best Go practices.