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.

Sign up for GitHub

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

Fix: Define Steps produces incorrect cucumber expression for step definition (#28) #29

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Fix: Define Steps produces incorrect cucumber expression for step definition (#28) #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91ea625
Proposed fix for GH28
clrudolphi Jun 19, 2024
1194d2a
Revised CucumberExpressionSkeletonProvider and Spec test to reflect c…
clrudolphi Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# [vNext]

* Fix: GH28: Define Steps produces incorrect regex for step definition
clrudolphi marked this conversation as resolved.
Show resolved Hide resolved
## Improvements:

* Find Unused Step Definitions Command: Improved handling of Step Definitions decorated with the 'StepDefinition' attribute. If a Step Definition is used in any Given/Then/When step in a Feature file, the step will no longer show in the 'Find Unused Step Definitions' context menu. (#24)
Expand Down
2 changes: 1 addition & 1 deletion Reqnroll.VisualStudio/Snippets/Fallback/CucumberExpressionSkeletonProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ protected override string GetExpression(AnalyzedStepText stepText)

private string EscapeCucumberExpression(string text)
{
var escapedCukeEx = text.Replace(@"\", @"\\").Replace(@"{", @"\{").Replace("(", @"\");
var escapedCukeEx = Regex.Escape(text).Replace("\"", "\"\"").Replace(@"\ ", @" ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code supposed to produce valid cucumber expressions and cucumber expressions are different from regex from the escaping perspective.

The cucumber expression escaping is defined at https://github.com/cucumber/cucumber-expressions?tab=readme-ov-file#escaping and it only allows escaping for: {, (, / and \.

Therefore we cannot use Regex.Escape(text) as it uses the regex escaping that is far more greedy than necessary. I think we would just need to fix the original code to match the specs: the .Replace("(", @"\") is clearly buggy, as it should be .Replace("(", @"\(") and the escaping for / is missing (so one more replace).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reference.
Two follow-up questions then:

  1. In the original code, lines 31 and 32 both contain a Replace(@"", @"\"). Is that intentional?
  2. When escaping the forward slash ('/'), C# doesn't recognize @"/" as a valid escape sequence. I will attempt testing without escaping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: question 2 above, in my experimentation, I see now that / is meant to convey alternatives and so the use of a / in an example text fed into our snippet provider should treat that as a literal and should be escaped.
Since C# doesn't recognize that as a proper escape sequence, should the snippet provider generate the string as a strictly literal string (with the prepended @ sign)? Such as:

        [Given(@"I have {int} cucumber\(s) in my \/ belly")]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see that number 1 and 2 are related. When I leave line 32 as originally written, the example I'm using in number 2 is generated as

        [Given("I have {int} cucumber\\(s) in my \\/ belly")]

which is handled gracefully by the C# compiler without the prepended @ sign.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clrudolphi I agree. Let's keep it this way. Switching over to @ strings (verbatim string) just complicates the things.

var escapedCSharpString = escapedCukeEx.Replace(@"\", @"\\").Replace(@"""", @"\""");
return escapedCSharpString;
}
Expand Down
36 changes: 36 additions & 0 deletions Tests/Reqnroll.VisualStudio.Specs/Features/Editor/Commands/DefineStepsCommand.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,39 @@ Scenario: DefineSteps command abides by reqnroll.json configuration for regex sk
| type | expression |
| Given | the client added (.*) pcs to the basket |

Scenario: DefineSteps command properly escapes empty brackets when using Cucumber expressions
Given there is a Reqnroll project scope
And the following feature file in the editor
"""
Feature: Scenario using empty brackets

Scenario: Client has a simple basket
When I call the integer.ToString() method
"""
And the project is built and the initial binding discovery is performed
When I invoke the "Define Steps" command
Then the define steps dialog should be opened with the following step definition skeletons
| type | expression |
| When | I call the integer\\.ToString\\(\\) method |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper escaped version of I call the integer.ToString() method would be I call the integer.ToString\() method (the . should not be escaped). See above.

Maybe we can find a better example, so that it shows all escaped characters, like When I use (parenthesis), {curly braces} and/or \ backslash, where the escaped should be When I use \(parenthesis), \{curly braces} and\/or \\ backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Regex snippet provider behaving correctly as-is? It does escape the period, which if I'm not mistaken is required because it could otherwise be mistaken for a Regex pattern character. Correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct.


Scenario: DefineSteps command properly escapes empty brackets when using Regex expressions
Given there is a Reqnroll project scope
And the following feature file in the editor
"""
Feature: Scenario using empty brackets

Scenario: Client has a simple basket
When I call the integer.ToString() method
"""
And the reqnroll.json configuration file contains
"""
{
"trace": { "stepDefinitionSkeletonStyle": "RegexAttribute" }
}
"""
And the project is built and the initial binding discovery is performed
When I invoke the "Define Steps" command
Then the define steps dialog should be opened with the following step definition skeletons
| type | expression |
| When | I call the integer\\.ToString\\(\\) method |

Loading