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

Adding context to steps #360

Closed
jlorgal opened this issue Dec 2, 2020 · 21 comments · Fixed by #409
Closed

Adding context to steps #360

jlorgal opened this issue Dec 2, 2020 · 21 comments · Fixed by #409
Labels
⚡ enhancement Request for new functionality

Comments

@jlorgal
Copy link

jlorgal commented Dec 2, 2020

Is your feature request related to a problem? Please describe.
We want to generate a library with some common steps. For example, a step that sends a REST request, a step that checks the status code, a step that validates the response, and a step that checks a HTTP header from the response. In this way, we could reuse some common steps.
However, godog only passes the regex parameters as arguments. We could use a struct and create methods for that type. But we wouldn't get reusable steps in any scenario.

Describe the solution you'd like
I'd like that each scenario creates a context.Context instance (or custom context) and the context is passed as first argument in each step function. Instead of:

func thereAreGodogs(available int) error {

we would have:

func thereAreGodogs(context.Context ctx, available int) error {

Note that context.Context is immutable. So we would need to create a map to store all context changes related to godog, to return the context to chain the context for the next step, or to use a custom context type.

Describe alternatives you've considered
We could use InitializeScenario for this purpose but it would require a lot of repetitive code for each step:

func InitializeScenario(ctx *godog.ScenarioContext) {
	var stepCtx godog.Context
	ctx.Step(`^there are (\d+) godogs$`, func(available int) {
		thereAreGodogs(stepCtx, available)
	})
	...
}

Additional context
See GoBDD that follows this approach. See also the motivations to write an alternative to Godog which I consider to be very good feedback for possible improvements in Godog.

@jlorgal jlorgal added the ⚡ enhancement Request for new functionality label Dec 2, 2020
@lonnblad
Copy link
Member

lonnblad commented Dec 5, 2020

Hi @jlorgal, I like the idea!

@lonnblad
Copy link
Member

Hi @jlorgal would you have time to write a code-example using your example for the steps making a REST request, etc. from the perspective that you are free to change the API and we can use that as a base for the discussion?

@radtriste
Copy link
Contributor

Hello @jlorgal
You can already have some context with godog.
in kogito-cloud-operator, we use a structure and setup step methods on the structure.
See https://github.com/kiegroup/kogito-cloud-operator/blob/master/test/steps/data.go#L38

is that similar to your idea ?

@jlorgal
Copy link
Author

jlorgal commented Jan 28, 2021

Hi @jlorgal would you have time to write a code-example using your example for the steps making a REST request, etc. from the perspective that you are free to change the API and we can use that as a base for the discussion?

I was working on a project to provide common steps for HTTP and DNS (in the future it could be enhanced) with this philosophy of passing a context. My idea is to release it as open source but, due to other priorities, I had to delay it (I expect to retake it and have something public in a couple of months).

@jlorgal
Copy link
Author

jlorgal commented Jan 28, 2021

Hello @jlorgal
You can already have some context with godog.
in kogito-cloud-operator, we use a structure and setup step methods on the structure.
See https://github.com/kiegroup/kogito-cloud-operator/blob/master/test/steps/data.go#L38

is that similar to your idea ?

Yes, it's exactly the same approach. The only difference is that I've used the standard Context (and this context is immutable). I've also delegated the registration of the steps by functional component (HTTP, DNS, ...) as you've done. So, I would say that we are pretty aligned.
However, I consider that it would simplify the use of this framework if this context is passed by default.

@ograycode
Copy link

I support this. My initial thought is the framework would expose a SetContext method either at the Feature or Scenario level. The SetContext method would take an interface of something along the lines of:

type Context interface {
  func GetValue(key interface{}) (interface{}, bool)
  func SetValue(key interface{}, value interface{})
  func Meta() map[string]interface{}
  func SetMeta(map[string]interface{})
}

Functions or methods whose first arg fulfills the interface contract are provided either a context supplied by the user or Scenario one provided by godog.

The Set and GetValue methods are strictly for user data passing and the Meta method is for godog supplied metadata about what is running.

This enables some key use cases in my mind:

  • Easy and safe concurrency
  • Reuse of steps across boundaries instead of having a global TestContext struct
  • Users get the benefit of metadata that the framework can provide, such as feature name, scenario name, etc. This is also a requested feature.

Should also consider a different name then Context as it's not the same as the one in stdlib.

@jlorgal
Copy link
Author

jlorgal commented Feb 22, 2021

We've already released an initial version of Golium. It is published as open source with Apache license.
It extends godog with the following features:

  • Support for context to share information among steps
  • Common steps that we consider that are reusable in many test suites.
  • Some utilities to parse tables, configuration parameters, logs, ...

It is not a fork of godog but the main dependency.
We can adapt this library if godog include context support in the future.

@lonnblad
Copy link
Member

lonnblad commented Mar 5, 2021

Hi

How would you feel about this API?

type TestSuiteContext struct{}

func (ctx *TestSuiteContext) BeforeSuite(h BeforeSuiteHook)
type BeforeSuiteHook func(ctx context.Context) context.Context

func (ctx *TestSuiteContext) AfterSuite(h AfterSuiteHook)
type AfterSuiteHook func(ctx context.Context) context.Context

type ScenarioContext struct{}

func (ctx *ScenarioContext) BeforeScenario(h BeforeScenarioHook)
type BeforeScenarioHook func(ctx context.Context, sc Scenario) context.Context

func (ctx *ScenarioContext) AfterScenario(h AfterScenarioHook)
type AfterScenarioHook func(ctx context.Context, sc Scenario, err error) (context.Context, error)

func (ctx *ScenarioContext) BeforeStep(h BeforeStepHook)
type BeforeStepHook func(ctx context.Context, st Step) context.Context

func (ctx *ScenarioContext) AfterStep(h AfterStepHook)
type AfterStepHook func(ctx context.Context, st Step, err error) (context.Context, error)

func (ctx *ScenarioContext) Step(expr, stepFunc interface{})

func stepFuncExample1(arg1 string) error
func stepFuncExample2(ctx context.Context, arg1 string) (context.Context, error)

Basically adding the stdlib context to all hooks and the stepFuncs will have it as an optional argument and return value.

We would also be able to build an Attachment API on top of the context.

@lonnblad
Copy link
Member

lonnblad commented Mar 5, 2021

It would also be good to consider

@wedothings
Copy link

Updating and adding a comment as requested from #378

I'd like to get the TestStepFinished.TestStepResult.Status for each step, from an AfterStep func.

By doing the below within the InitializeScenario.

func InitializeScenario(s *godog.ScenarioContext) {

    s.AfterStep(func(st *messages.TestStepFinished, err error) {
		    stepResultStatus = st.TestStepResult.Status
	    })

I'd like the AfterStep method to return the possible status like the below on step execution.

	TestStepResult_UNKNOWN TestStepResult_Status = 0
	TestStepResult_PASSED TestStepResult_Status = 1
	TestStepResult_SKIPPED TestStepResult_Status = 2

It'd also be good to get the Keyword in a similar fashion. i.e.

st.GherkinDocument_Feature_Step.Keyword

@erikdubbelboer
Copy link

I'm curious if there has been any progress here? The proposed API by @lonnblad seems good. But I understand that it will be yet again a breaking change which might not be what is wanted right now?

@inluxc
Copy link

inluxc commented Apr 14, 2021

On the contrary, I think these changes should be done now, before v1.0.
I know it's a bit difficult for us to maintain, I alone got about 15 projects depending on GoDog.

@vearutop
Copy link
Member

vearutop commented May 30, 2021

I think all hook functions (including BeforeStepHook, BeforeScenarioHook and BeforeSuiteHook) would benefit from an error return, so that end-user can nicely fail execution based some global (environmental?) condition.

Also returning (context.Context, error) from all hooks would be consistent and less surprising.

As for breaking changes, maybe we could avoid them (or at least allow gradual refactoring with deprecation) by using different names for hook functions.

Given that hook is defined on particular entity, we could drop that entity name from function name:

type ScenarioContext struct{}

func (ctx *ScenarioContext) Before(h BeforeScenarioHook)
type BeforeScenarioHook func(ctx context.Context, sc Scenario) context.Context

func (ctx *ScenarioContext) After(h AfterScenarioHook)
type AfterScenarioHook func(ctx context.Context, sc Scenario, err error) (context.Context, error)

though this appoach does not work well for step hooks

func (ctx *ScenarioContext) BeforeStep(h BeforeStepHook)
type BeforeStepHook func(ctx context.Context, st Step) context.Context

func (ctx *ScenarioContext) AfterStep(h AfterStepHook)
type AfterStepHook func(ctx context.Context, st Step, err error) (context.Context, error)

I'm personally mostly interested in failing scenarios with AfterScenario, so func (ctx *ScenarioContext) After(h AfterScenarioHook) would work for me.

@mattwynne
Copy link
Member

To clarify my understanding: currently godog does not have any support for sharing state between steps of a scenario - is that right?

Do we have a question to resolve about whether that context should be immutable or not?

@vearutop
Copy link
Member

vearutop commented Jul 19, 2021

User can build step handlers in a way that allows capturing and using state, but there is no standard way to share state with 3rd party steps which are out of control of the user.

Step handlers are invoked with reflection, we can easily support ctx context.Context as a first argument without breaking changes. We also can add backwards compatible support for step to return context.

Go's context.Context is immutable and updates are made by wrapping previous context with extra value.

func thereAreGodogs(context.Context ctx, available int) (context.Context, error) {

The flow could be that each step takes output context of previous step as own input.

Together with contextualized hooks this can provide great flexibility with a simple and familiar model of context.Context.

For the hooks I'd like to elaborate my previous comment, this way we can support new API for hooks together with old one, which can be gradually deprecated:

type ScenarioContext struct {
	suite *suite
}

type StepContext struct {
	suite *suite
}

func (ctx *ScenarioContext) Before(h BeforeScenarioHook) { /* TODO */ }

type BeforeScenarioHook func(ctx context.Context, sc *Scenario) (context.Context, error)

func (ctx *ScenarioContext) After(h AfterScenarioHook) { /* TODO */ }

type AfterScenarioHook func(ctx context.Context, sc *Scenario, err error) (context.Context, error)

func (ctx *ScenarioContext) StepContext() StepContext {
	return StepContext{suite: ctx.suite}
}

func (ctx *StepContext) Before(h BeforeStepHook) { /* TODO */ }

type BeforeStepHook func(ctx context.Context, st *Step) (context.Context, error)

func (ctx *StepContext) After(h AfterStepHook) { /* TODO */ }

type AfterStepHook func(ctx context.Context, st *Step, err error) (context.Context, error)

Implementing these changes seems like a quite feasible effort, I can work on that if we agree on API.

@vearutop
Copy link
Member

Please check #409 as a proposed solution to this and other related issues, if this solution seems good, I'll finish the PR with more documentation and additional fixes (#370, #378).

@vearutop
Copy link
Member

The PR #409 is finished from my point of view and is ready for review. Please check.

@Johnlon
Copy link
Member

Johnlon commented May 2, 2024

@lonnblad > We would also be able to build an Attachment API on top of the context.

Did anyone manage to get attachments working in godog?

@fgm
Copy link

fgm commented May 25, 2024

The way I ended up solving this recently was to define a custom type with everything I needed to share, and making it implement context.Context. Then the first line in any step function stepFoo(ctx context.Context, ...whatever) is to get it back with a type assertion to the actual type , like state := ctx.(AppState).

@Johnlon
Copy link
Member

Johnlon commented May 25, 2024

@lonnblad > We would also be able to build an Attachment API on top of the context.

Did anyone manage to get attachments working in godog?

I have forked godog and added a pretty clean API for attachments. There is a pr but not been reviewed. This is a blocker for me.

@Johnlon
Copy link
Member

Johnlon commented May 25, 2024

I wrote up my approach to shared state in detail in this blog post..

https://johnlonergan.blogspot.com/2024/05/recommented-pattern-for-shared-state-in.html?m=1

Custom context type suggestion above looks Interesting

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

Successfully merging a pull request may close this issue.