Fix step reporting on mono #1009
Conversation
| @@ -5,5 +5,6 @@ public enum ScenarioBlock | |||
| Given = 1, | |||
| When = 2, | |||
| Then = 3, | |||
| StepDefinition = 4, | |||
There was a problem hiding this comment.
What is this StepDefinition value?
A scenario has Given, When and Then parts in the scenario and thats what this enum should represent.
Please don't change this enum!
There was a problem hiding this comment.
I've used a similar train of thought like for the StepDefinition value on the StepKeyword enum. Still, I'm open to other approaches.
| @@ -6,6 +6,7 @@ public enum StepKeyword | |||
| When = 2, | |||
| Then = 3, | |||
| And = 4, | |||
| But = 5 | |||
| But = 5, | |||
| StepDefinition = 6 | |||
There was a problem hiding this comment.
Same as ScenarioBlock. StepDefinition is no keyword of Gherkin.
Please change it back.
There was a problem hiding this comment.
I've added the StepDefinition value since I need it on line 185 StepDefinitionReportGenerator.cs (the StepDefinition case).
I need the StepDefinition case in order for the steps that use the StepDefinition attribute to be included in the report and correctly marked as used / implemented.
I can't pass null there, so it seemed like a decent solution. I don't necessarily see anything wrong with having a "none" / "undefined" (which I've wrongly named it StepDefinition) value on this enum.
There was a problem hiding this comment.
[StepDefinition] attribute is just a shortcut for adding [Given], [When] and [Then] with the same regex to the same method. You can also do that manually, or even partially (only adding [Given] and [When]), so this case has to be resolved somehow else. But I think this is anyway a different problem from the Mono path issue, so I would recommend to make a separate PR for that.
There was a problem hiding this comment.
That's the thing, I want some steps to work with [Given], [When] and [Then] - I don't want one or the other.
I'll open a separate PR for including the steps with the [StepDefinition] attribute in the step usage report.
Unfortunately I'll be out on holidays next week, so I'll do that sometime after that.
I've built my own specflow.exe from this branch, so at least our problem is solved for now, but I'll spend some time into making it cleaner after I get back from holiday.
I still think that [StepDefinition] steps should be included in the usage report - so we'll have to figure a clean way to do that.
There was a problem hiding this comment.
@cosminstirbu I am not against it at all, but maybe we could calculate this back during report generation time (ie, if the method is included as all G/W/T, we display it as [StepDefinition]). A separate PR is anyway good because we can have a better place to discuss the options there. Have a good holiday!
There was a problem hiding this comment.
I've created a separate PR - #1012
I'll close this one for now an try to come up with a better approach. If I'll be stuck on finding one, I'll reach out.
Thank you,
Cosmin
Fixes #975