cucumber / godog

Cucumber for golang
MIT License
2.32k stars 254 forks source link

runAfterStepHooks should allow the user to replace an error with a nil or another error #633

Open Johnlon opened 5 months ago

Johnlon commented 5 months ago

See PR https://github.com/cucumber/godog/pull/634

🤔 What's the problem you're trying to solve?

I need the ability to installa hook that either knocks out an error returned from a step, or completely replaced the error returned from a step.

The current runAfterStepHooks function does not make this possible as it has a guard expression that prevents me knocking out a current error, and a separate guard expression that prevents me substituting a different error (does an append instead).

! To be honest I think it would be better for godog just to change AfterStep to make it do what my PostStep does and break compat.

✨ What's your proposed solution?

I can't see a backwards compat solution so I will raise a PR adding a "PostStepHook" and runPostStepHooks() which is a more open api than the existing PostStepHook.

⛏ Have you considered any alternatives or workarounds?

Alternatively, we can break backwards compatibility - we could replace the existing runAfterStepHooks with the impl I have in runPostStepHooks in my PR.

The two impls are below for your convenience....

// Diffs vs runAfterStepHooks
// - my impl allows the handler to 
//    - knock out an error (reset to nil)
//    - inject an error to an otherwise passing test
//    - entirely swap one error for another
//    - or append errors to existing errors if the user so chooses (ie the current AfterStep behaviour)
// - my impl also threads the status thru the handlers so that the user can do whatever they want with it
// 
func (s *suite) runPostStepHooks(ctx context.Context, step *Step, status StepResultStatus, err error) (context.Context, StepResultStatus, error) {
    for _, f := range s.postStepHandlers {
        ctx, status, err = f(ctx, step, status, err)
    }

    return ctx, status, err
}

func (s *suite) runAfterStepHooks(ctx context.Context, step *Step, status StepResultStatus, err error) (context.Context, error) {
    for _, f := range s.afterStepHandlers {
        hctx, herr := f(ctx, step, status, err)

        // Adding hook error to resulting error without breaking hooks loop.
        if herr != nil {
            if err == nil {
                err = herr
            } else {
                err = fmt.Errorf("%v, %w", herr, err)
            }
        }

        if hctx != nil {
            ctx = hctx
        }
    }

    return ctx, err
}

📚 Any additional context?

It is unfortunate that the existing AfterStep hook make the assumptions it does in runAfterStepHooks.

"AfterStep" doco says // It may be convenient to return a different kind of error // in order to print more state details which may help
... but the current impl doesn't allow me to return a "different" error, just an "additional" error.