Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Johnlon opened this issue Jun 21, 2024 · 0 comments
Labels
⚡ enhancement Request for new functionality

Comments

@Johnlon
Copy link
Member

Johnlon commented Jun 21, 2024

See PR #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

No branches or pull requests

1 participant