Fix for 81 - Cucumber Expression using Enums errors when two enums exist with the same short name

[clrudolphi]

This fix causes the CucumberExpressionParameterTypeRegistry to use the FQN of the enum Types used as Cucumber Expression parameters.

Types of changes

• [X ] Bug fix (non-breaking change which fixes an issue).

• [ ] New feature (non-breaking change which adds functionality).

• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected).

• [ ] Performance improvement

• [ ] Refactoring (so no functional change)

• [ ] Other (docs, build config, etc)

• [X ] I've added tests for my code. (most of the time mandatory)

• [X ] I have added an entry to the changelog. (mandatory)

• [ ] My change requires a change to the documentation.

• [ ] I have updated the documentation accordingly.

[clrudolphi]

@gasparnagy , this is regarding #81 . We discussed two possible sub-scenarios. This fix addresses the most immediate one (two enums that have the same short name but exist in different namespaces). Let me know if the testing is adequate. I have a short unit test, but nothing added yet in Specs or elsewhere to demonstrate no adverse affect to using enums.

[clrudolphi]

@gasparnagy Argh. NM. Already see where this breaks everything. I'll rethink this.

[gasparnagy]

@clrudolphi Yes, unfortunately the problem is conceptual here. When I did the first version of CukeEx support, I had to solve somehow that enums work. Finally the workaround was to go through all possible parameter types and register them. But obviously this is not working if there is a name duplication...

Maybe what we could do is the following: when we realize that there is a duplication (we are about to add one with a name, but the dictionary already has sg with the same name), we could replace the existing value with a "proxy". This proxy would "remember" (have a list) of all ambiguous types and if (and only if) someone would try to use it actually (i.e. use a placeholder like "... {Color} ...") we would throw an error, suggesting that this is ambiguous (write out the type names from its list) and if you want to use this enum, you need to create two StepArgumentTransformations and give them unique name (names are case sensitive).

Note that @kipusoep was not actually using the registered name in #81, so in his case the "proxy" would never be actually used, so he would not see this error, but could just use the enums as he planned to.

What do you think?

[Code-Grump]

Creating some kind of "aggregate" transformer is an interesting idea. How would we detect when a transform is actually ambiguous? It feels like a really clever result if Reqnroll can slip around the problem when it's just theoretical and only throw an exception when there is a practical problem. 👍

[gasparnagy]

How would we detect when a transform is actually ambiguous?

There is a point where we would register the name to a dictionary. Is somebody is already in it is ambiguous.

[clrudolphi]

Identifying colliding Enum shortnames at the time of discovery is difficult as the Dictionary is built Lazily.

Instead, please review the latest change(cba2383). At execution time, if there is no match in the Dictionary based upon the name given, then I attempt to match enums just on short name (assuming that the Dict key values are FullNames of enum types). Then, if 1 match is found, return that; if more than 1, then we have an ambiguous situation.

[gasparnagy]

@clrudolphi good idea. I will check it.

[clrudolphi]

Can you make a quick manual check in a sample project to verify if it really works with this the two cases?

Please take a look at this repo with a set of projects that demonstrate behavior. I used two separate projects because the 'ambiguous' error will be thrown for EVERY test in the project as each Scenario execution is asking the registry for parameter bindings... Not great.

[gasparnagy]

@clrudolphi could you paste the new stack trace of the errors?

[gasparnagy]

error will be thrown for EVERY test in the project as each Scenario execution is asking the registry for parameter bindings

BTW, I think this is the usual behavior if there is any binding error. Like an invalid regex. So it is not that horrible.

[clrudolphi]

@clrudolphi could you paste the new stack trace of the errors?

In the VisualStudio Output window, the VS RnR Extension will output:

10:04:38:688    Info: OnActivityStarted: Starting Visual Studio Extension...
10:04:38:695    Info: CreateProjectScope: Initializing project: RnR81Exp2
10:04:38:745    Info: OnSettingsInitialized: Project settings initialized: .NETCoreApp,Version=v6.0,Reqnroll:1.0.2-local
10:04:39:145    Info: ThenImportBindings: 2 step definitions and 0 hooks discovered for project RnR81Exp2
10:04:39:148    Warning: ReportInvalidStepDefinitions: Invalid step definitions found: 
10:04:39:148      [Given(a shade of '{Shade}')]: ShadeEnumTestStepDefinitions.ThisWillResultInAnError(RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.StepDefinitions.Shade): Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name. at C:\Users\clrud\source\repos\RnR81Exp\RnR81Exp2\StepDefinitions\ColorEnumTestStepDefinition2s.cs(15,58)
10:04:39:148      [Given(a shade of '{Shade}')]: AnotherShadeEnumTestStepDefinitions.ThisWillResultInAnError(RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.AnotherNamespace.Shade): Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name. at C:\Users\clrud\source\repos\RnR81Exp\RnR81Exp2\StepDefinitions\ColorEnumTestStepDefinition2s.cs(31,58)
10:05:00:421    Info: ThenImportBindings: 2 step definitions and 0 hooks discovered for project RnR81Exp2
10:05:00:422    Warning: ReportInvalidStepDefinitions: Invalid step definitions found: 
10:05:00:422      [Given(a shade of '{Shade}')]: ShadeEnumTestStepDefinitions.ThisWillResultInAnError(RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.StepDefinitions.Shade): Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name. at C:\Users\clrud\source\repos\RnR81Exp\RnR81Exp2\StepDefinitions\ColorEnumTestStepDefinition2s.cs(15,58)
10:05:00:422      [Given(a shade of '{Shade}')]: AnotherShadeEnumTestStepDefinitions.ThisWillResultInAnError(RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.AnotherNamespace.Shade): Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name. at C:\Users\clrud\source\repos\RnR81Exp\RnR81Exp2\StepDefinitions\ColorEnumTestStepDefinition2s.cs(31,58)

In the Test Explorer View of VS, the Test Detail Summary is:

Using a short name of an enum with conflicting enum names will result in an Ambiguous Error
   Source: ColorEnumTest2.feature line 4
   Duration: 14 ms

  Message: 
Reqnroll.BindingException : Binding error(s) found: 
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.

  Stack Trace: 
ErrorProvider.GetInvalidBindingRegistryError(IEnumerable`1 errors)
TestExecutionEngine.GetStepMatch(StepInstance stepInstance)
TestExecutionEngine.ExecuteStepAsync(IContextManager contextManager, StepInstance stepInstance)
TestExecutionEngine.OnAfterLastStepAsync()
TestRunner.CollectScenarioErrorsAsync()
ColorEnumTestingFeature.ScenarioCleanupAsync()
ColorEnumTestingFeature.UsingAShortNameOfAnEnumWithConflictingEnumNamesWillResultInAnAmbiguousError() line 5
--- End of stack trace from previous location ---

  Standard Output: 
Given a shade of 'Grey'
-> binding error: Binding error(s) found: 
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.

[gasparnagy]

I think this is OK.

Could you please check if you get the same behavior if you add an attribute like [When("^invalid regex ( $")]

[clrudolphi]

I think this is OK.

Could you please check if you get the same behavior if you add an attribute like [When("^invalid regex ( $")]

 Using a short name of an enum with conflicting enum names will result in an Ambiguous Error
   Source: ColorEnumTest2.feature line 4
   Duration: 14 ms

  Message: 
Reqnroll.BindingException : Binding error(s) found: 
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Invalid pattern '^invalid regex ( $' at offset 18. Not enough )'s.

  Stack Trace: 
ErrorProvider.GetInvalidBindingRegistryError(IEnumerable`1 errors)
TestExecutionEngine.GetStepMatch(StepInstance stepInstance)
TestExecutionEngine.ExecuteStepAsync(IContextManager contextManager, StepInstance stepInstance)
TestExecutionEngine.OnAfterLastStepAsync()
TestRunner.CollectScenarioErrorsAsync()
ColorEnumTestingFeature.ScenarioCleanupAsync()
ColorEnumTestingFeature.UsingAShortNameOfAnEnumWithConflictingEnumNamesWillResultInAnAmbiguousError() line 5
--- End of stack trace from previous location ---

  Standard Output: 
Given a shade of 'Grey'
-> binding error: Binding error(s) found: 
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Ambiguous Enum Parameters share the same short name 'Shade'. Use the Enum's FullName in the Cucumber Expression or define a [StepArgumentTransformation] with the chosen type and the short name.
Invalid pattern '^invalid regex ( $' at offset 18. Not enough )'s.

[gasparnagy]

@clrudolphi ok. so that works the same way. i think this is good.

So I think what is missing is:

• You can add another test that checks if the exception is thrown in case of ambiguity.
• The error message still has capital case phrases, please also fix that

[clrudolphi]

@gasparnagy Had to make a modification to the logic that identifies ambiguity - in addition to enums that are children of namespaces ("." + name) we also need to account for enums that are embedded in classes ("+" + name). Please confirm that the wording of the error message is acceptable.

[gasparnagy]

@clrudolphi yes, this is fine. Is it ready to merge once the CI is green then?

[clrudolphi]

Yes. I'm not aware of any other remaining action items.

[gasparnagy]

Thx!