Run before step hooks before matching step

[nhatthm]

Description

There is an breaking change in v0.12.0, the beforeStep hooks are executed after s.matchStep(step). Because of that, any changes in the Step won't be applied and taken into consideration in matchStep()

godog/suite.go

Lines 137 to 142 in afaebf2

	match = s.matchStep(step)
	s.storage.MustInsertStepDefintionMatch(step.AstNodeIds[0], match)
	s.fmt.Defined(pickle, step, match.GetInternalStepDefinition())
	// run before step handlers
	ctx, err = s.runBeforeStepHooks(ctx, step, err)

This behavior is newly introduced in v0.12.0, in the previous version, the hooks are executed before matchStep()

godog/suite.go

Lines 52 to 59 in 2b426f8

	// run before step handlers
	for _, f := range s.beforeStepHandlers {
	f(step)
	}
	match := s.matchStep(step)
	s.storage.MustInsertStepDefintionMatch(step.AstNodeIds[0], match)
	s.fmt.Defined(pickle, step, match.GetInternalStepDefinition())

I see this (breaking) change is unnecessary and would like to bring back the old behavior.

I don't know if we should consider this as a bug or not.

Motivation & context

I have some logic to dynamically replace the value in the step using beforeStep hook. For a (stupid) example

    Scenario: Env var is replaced
        Given env var FOOBAR is replaced in step text: $FOOBAR

And the step handler will be like

func assertFoobar(v string) {
	// v should be replaced beforehand by the hook.
	assert.NotEqual(t, "$FOOBAR", v)
}

In v0.12.0, v is still $FOOBAR because the change is too late, matchStep is executed with the original step definition

Type of change

• Refactoring/debt (improvement to code design or tooling without changing behaviour)
• Bug fix (non-breaking change which fixes an issue)

Note to other contributors

n/a

Update required of cucumber.io/docs

n/a

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[vearutop]

This change was indeed unintentional, thank you for fixing this! 👍

[vearutop]

I wonder why CircleCI did not start for this PR. 🤔

[codecov]

Codecov Report

Merging #424 (afaebf2) into main (afaebf2) will not change coverage.
The diff coverage is n/a.

❗ Current head afaebf2 differs from pull request most recent head 8f61688. Consider uploading reports for the commit 8f61688 to get more accurate results

@@           Coverage Diff           @@
##             main     #424   +/-   ##
=======================================
  Coverage   82.66%   82.66%           
=======================================
  Files          26       26           
  Lines        2533     2533           
=======================================
  Hits         2094     2094           
  Misses        341      341           
  Partials       98       98           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afaebf2...8f61688. Read the comment docs.

[aslakhellesoy]

Hi @nhatthm,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[mpkorstanje]

I wonder why CircleCI did not start for this PR. 🤔

People were using GitHub CI to mine Bitcoin. So now maintainers have to authorize CI for pull requests from first time contributors.

[vearutop]

@mpkorstanje thanks for clarification. Could you share how to authorize CI? I worked around the issue by rebasing PR into a local branch, but that felt a bit unnecessary. :)

[mpkorstanje]

@mattwynne I can't work out which group @vearutop should be in. I don't think we wrote down any guidelines for that. Could be we add some docs to the groups?

[mpkorstanje]

Wait, godog is still on circle ci. I don't know who can trigger that at all. But probably the same kinda permissions problem.

[vearutop]

@mpkorstanje would it make sense to migrate godog pipeline to github actions (I can do that easily)?

[mpkorstanje]

Yes. That would make sense. Github actions provides better integration with GitHub and Matt is building release automation based on Github Actions.

[mattwynne]

As far as permissions goes, what I see is that we have a @cucunmber/go team with Admin rights on this repo. @vearutop is alreayd in that team so I'm not sure there's much more rights we could give him, short of making him an org admin.

@mpkorstanje docs sound like a good idea. What do you think we need to write down, and where?