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

clrudolphi · 2024-04-12T01:39:05Z

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.

…th the same short name exist as parameters to cucumber expressions. This fix causes the CucumberExpressionParameterTypeRegistry to use the FQN of the enum Types used as Cucumber Expression parameters.

clrudolphi · 2024-04-12T01:41:12Z

@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 · 2024-04-12T02:03:14Z

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

gasparnagy · 2024-04-12T13:10:57Z

@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 · 2024-04-12T13:15:58Z

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 · 2024-04-12T13:53:04Z

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.

…ype FullName.

clrudolphi · 2024-04-12T16:06:22Z

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 · 2024-04-12T16:08:17Z

@clrudolphi good idea. I will check it.

gasparnagy

Yes!!! This is a good idea.

I also like the test. You can add another one that checks if the exception is thrown in case of ambiguity.

And the good thing is, that if you still want to use the enum by short name (for one of the types), you can create a [StepArgumentTransformation] and that will register the trafo with the short name and therefore it will not run into the ambiguous lookup exception. So this will also fix your case as well, I think. We can maybe even add this hint to the exception message: [...] Use the enum's full name in the cucumber expression or define a [StepArgumentTransformation] with the chosen type and the short name.

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

Reqnroll/Bindings/CucumberExpressions/CucumberExpressionParameterTypeRegistry.cs

gasparnagy · 2024-04-15T12:36:13Z

Reqnroll/Bindings/CucumberExpressions/CucumberExpressionParameterTypeRegistry.cs

+        if (matchingEnums.Length == 1) { return matchingEnums[0].Value; }
+        if (matchingEnums.Length > 1)
+        {
+            throw new ApplicationException($"Ambigous Enum Parameters share the same short name '{name}'. Use the Enum's FullName in the Cucumber Expression.");


throw ReqnrollException instead

I would make the error message more sounding like a normal text (there was also a typo in the Ambiguous word!): $"Ambiguous enum parameters share the same short name '{name}'. Use the enum's full name in the cucumber expression."

gasparnagy · 2024-04-15T12:37:19Z

...ll.RuntimeTests/Bindings/CucumberExpressions/CucumberExpressionParameterTypeRegistryTests.cs

+        public void Should_not_error_on_multiple_enums_of_the_same_name()
+        {
+            var sut = CreateSut();
+            Reqnroll.Bindings.Reflection.IBindingMethod enumUsingBindingMethod1 = new RuntimeBindingMethod(typeof(SampleEnumUsingClass).GetMethod("MethodUsingSampleColorEnum1"));


instead of "MethodUsingSampleColorEnum1" you can use nameof(SampleEnumUsingClass.MethodUsingSampleColorEnum1)

gasparnagy · 2024-04-15T12:37:35Z

...ll.RuntimeTests/Bindings/CucumberExpressions/CucumberExpressionParameterTypeRegistryTests.cs

+            var sut = CreateSut();
+            Reqnroll.Bindings.Reflection.IBindingMethod enumUsingBindingMethod1 = new RuntimeBindingMethod(typeof(SampleEnumUsingClass).GetMethod("MethodUsingSampleColorEnum1"));
+            sut.OnBindingMethodProcessed(enumUsingBindingMethod1);
+            Reqnroll.Bindings.Reflection.IBindingMethod enumUsingBindingMethod2 = new RuntimeBindingMethod(typeof(CucumberAddtionalExpressions.EnumCucumberExpressions).GetMethod("MethodUsingSampleColorEnum2"));


Use nameof also here

reqnroll#100 (review)

clrudolphi · 2024-04-15T14:21:58Z

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 · 2024-04-15T14:27:00Z

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

gasparnagy · 2024-04-15T14:36:53Z

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 · 2024-04-15T15:15:11Z

@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 · 2024-04-15T16:39:52Z

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 · 2024-04-15T18:06:19Z

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 · 2024-04-15T20:31:26Z

@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

…nd BindingRegistry in InValid state when ambiguous enum cucumber parameter expressions are present. Modified Error message for formatting.

clrudolphi · 2024-04-16T12:10:47Z

@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 · 2024-04-16T12:13:58Z

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

clrudolphi · 2024-04-16T12:32:06Z

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

gasparnagy · 2024-04-16T12:40:41Z

Thx!

* origin/main: Fix for 81 - Cucumber Expression using Enums errors when two enums exist with the same short name (#100)

…ons-dependencyinjection-plugin * origin/main: (21 commits) Fix #56 autofac ambiguous stepdef and hook required #127 issue (#139) Reduce target framework of Reqnroll to netstandard2.0 (#130) Fix StackOverflowException when using [StepArgumentTransformation] with same input and output type (#136) MsTest: Replace DelayedFixtureTearDown special case with ClassCleanupBehavior.EndOfClass (#128) Temporarily disabled tests until #132 is resolved Add NUnit & xUnit core tests to portability suite Capture ExecutionContext after every binding invoke (#126) small improvement in CodeDomHelper to be able to chain async calls fix method name sources in UnitTestFeatureGenerator External data plugin, support for JSON files (#118) UnitTests: Check if SDK version is installed and if not ignore the test (#109) Fix 111 ignore attr not inherited from rule (#113) Update README.md (#110) Fix 81 - modified CucumberExpressionParameterTypeRegistry to handle multiple custom types used as cucumber expressions when those types share the same short name. (#104) Update index.md Simplify test project targets (#105) Make SystemTests temp folder configurable and use NUGET_PACKAGES env var to override global NuGet folder Fleshing out Generation System Tests (2) (#99) Fix for 81 - Cucumber Expression using Enums errors when two enums exist with the same short name (#100) Include BoDi to Reqnroll package (#91) (#95) ... # Conflicts: # Reqnroll.sln # Tests/Reqnroll.PluginTests/Reqnroll.PluginTests.csproj

clrudolphi added 2 commits April 11, 2024 20:35

Fix for 81 - Cucumber Expression using Enums errors when two enums wi…

66c479d

…th the same short name exist as parameters to cucumber expressions. This fix causes the CucumberExpressionParameterTypeRegistry to use the FQN of the enum Types used as Cucumber Expression parameters.

Change Log

cdff1f5

Modified CucumberExpressionParameterTypeRegistry to lookup enums by t…

cba2383

…ype FullName.

gasparnagy requested changes Apr 15, 2024

View reviewed changes

clrudolphi added 2 commits April 15, 2024 08:29

Code clean-up per PR code review

19c701d

reqnroll#100 (review)

Clean-up per PR code review.

5daefe0

gasparnagy marked this pull request as ready for review April 15, 2024 20:34

Added to test to confirm creation of InValid StepDefinitionBindings a…

0bea0fe

…nd BindingRegistry in InValid state when ambiguous enum cucumber parameter expressions are present. Modified Error message for formatting.

gasparnagy merged commit 791b0c5 into reqnroll:main Apr 16, 2024
6 checks passed

gasparnagy mentioned this pull request Apr 16, 2024

Cucumber expression: An item with the same key has already been added. #81

Closed

gasparnagy added a commit that referenced this pull request Apr 16, 2024

Merge remote-tracking branch 'origin/main' into system-test-additions

e8bf1c6

* origin/main: Fix for 81 - Cucumber Expression using Enums errors when two enums exist with the same short name (#100)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

clrudolphi commented Apr 12, 2024

clrudolphi commented Apr 12, 2024

clrudolphi commented Apr 12, 2024

gasparnagy commented Apr 12, 2024 •

edited

Loading

Code-Grump commented Apr 12, 2024

gasparnagy commented Apr 12, 2024

clrudolphi commented Apr 12, 2024

gasparnagy commented Apr 12, 2024

gasparnagy left a comment

gasparnagy Apr 15, 2024

clrudolphi Apr 15, 2024

gasparnagy Apr 15, 2024

clrudolphi Apr 15, 2024

gasparnagy Apr 15, 2024

clrudolphi Apr 15, 2024

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024 •

edited

Loading

clrudolphi commented Apr 16, 2024

gasparnagy commented Apr 16, 2024

clrudolphi commented Apr 16, 2024

gasparnagy commented Apr 16, 2024

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

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

Conversation

clrudolphi commented Apr 12, 2024

Types of changes

clrudolphi commented Apr 12, 2024

clrudolphi commented Apr 12, 2024

gasparnagy commented Apr 12, 2024 • edited Loading

Code-Grump commented Apr 12, 2024

gasparnagy commented Apr 12, 2024

clrudolphi commented Apr 12, 2024

gasparnagy commented Apr 12, 2024

gasparnagy left a comment

Choose a reason for hiding this comment

gasparnagy Apr 15, 2024

Choose a reason for hiding this comment

clrudolphi Apr 15, 2024

Choose a reason for hiding this comment

gasparnagy Apr 15, 2024

Choose a reason for hiding this comment

clrudolphi Apr 15, 2024

Choose a reason for hiding this comment

gasparnagy Apr 15, 2024

Choose a reason for hiding this comment

clrudolphi Apr 15, 2024

Choose a reason for hiding this comment

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024

clrudolphi commented Apr 15, 2024

gasparnagy commented Apr 15, 2024 • edited Loading

clrudolphi commented Apr 16, 2024

gasparnagy commented Apr 16, 2024

clrudolphi commented Apr 16, 2024

gasparnagy commented Apr 16, 2024

gasparnagy commented Apr 12, 2024 •

edited

Loading

gasparnagy commented Apr 15, 2024 •

edited

Loading