fix: optional regex groups

[olegKoshmeliuk]

🤔 What's changed?

Fix to return old binding regex behavior from Specflow v3.9.74 link
During implementation of Cucumber expressions support in Specflow 4.0 beta this behaviour was changed by adding Where(g => g.Success) here

In this PR I added condition to apply old way of CalculateArguments only for regex bindings.

⚡️ What's your motivation?

This PR must fix different parameters for optional regex groups:
regex Task finished(| in (\d+) seconds) based on input is resolved with 1-2 parameters

• Task finished - 1 parameter with emtpy string
• Task finished in 10 seconds - 2 parameters "in 10 seconds" and "10"

In Specflow 3.9.74 this regex was always resolved with 2 parameters.

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[gasparnagy]

I see and understand the motivation and thanks for analyzing this and providing a PR, but I'm not sure it is a good idea to fix the problem this way (see my alternative suggestion below).

Basically my arguments against this is twofold:

1. The concept of supporting different expression types (regex, cucumber ex) was that they all translate to regex that can be "blindly" applied. This behavior is also used by the Visual Studio extension: It asks Reqnroll for the regexes of the expressions and match them. If we introduce "if"-s in the regex processing, we basically break this concept and as a result, we either need to fix this in Visual Studio extension (code dupl!) or they will behave differently.

2. The behavior of SpecFlow was incorrect. It was passing arbitrary null values for parameters whose regex groups were not even matching. Of course the incorrectness was only minor, but there are examples that shows it (this is how it was found). You (and maybe some others) have used this behavior for "optional parameter" support, but this was definitely not the intended usage and optional parameters were never officially supported. With this change, we are basically restoring an incorrect behavior for backwards compatibility and that is not generally a good strategy.

My suggestion would be the following:

1. Extract the logic of the CalculateArguments method to an interface, e.g.

public interface IMatchArgumentCalculator {
  object[] CalculateArguments(Match match, StepInstance stepInstance, IStepDefinitionBinding stepDefinitionBinding);
}

In cases where backwards compatibility is needed, anyone can locally override the implementation with a local runtime plugin (just one file added to the project), like you can see in https://docs.reqnroll.net/latest/guides/how-to-configure-cucumber-expression-behavior.html, and implement the SpecFlow v3 behavior. (We could provide a sample implementation in the linked documentation page even.)

2. In parallel to that, we can think of a proper support of optional parameters - that would be basically another implementation of the interface that could take the default values from the actual .NET parameters instead of using null.

What do you think?

[Code-Grump]

I agree with gasparnagy here. There are knock-on effects to this change that mean it probably can't be included in our codebase, but you can absolutely extend the runtime to achieve what you want for your project.

In the meantime, if you'd like to open or contribute to an issue to add support optional parameter, I think we could get something done.

[olegKoshmeliuk]

@gasparnagy @Code-Grump
I will create new PR to allow change CalculateArguments by using plugin override.
For legacy BDD override CalculateArguments will be good idea. And new code better not use optional regex groups at all 😄

I agree sometimes is better to split one regex binding with optional parameter into two regex bindings:
[Given(@"API(| with timeout (\d+) seconds) called on (.*)")] to:

• [Given(@"API called on (.*)")]
• [Given(@"API with timeout (\d+) seconds called on (.*)")]

[gasparnagy]

@olegKoshmeliuk Great, thx!

[gasparnagy]

FYI: @drosenba had a working sample here: #181 (comment)