SpecFlowOSS / SpecFlow

#1 .NET BDD Framework. SpecFlow automates your testing & works with your existing code. Find Bugs before they happen. Behavior Driven Development helps developers, testers, and business representatives to get a better understanding of their collaboration
https://www.specflow.org/
Other
2.25k stars 754 forks source link

Enums with DescriptionAttribute are not transforming properly by SpecFlow. #2669

Closed DmitryLegostaev closed 1 year ago

DmitryLegostaev commented 2 years ago

SpecFlow Version

4.0.16-beta and 3.9.74

Which test runner are you using?

xUnit

Test Runner Version Number

2.4.2

.NET Implementation

.NET 6.0

Project Format of the SpecFlow project

Sdk-style project format

.feature.cs files are generated using

SpecFlowSingleFileGenerator custom tool

Test Execution Method

Visual Studio Test Explorer

SpecFlow Section in app.config or content of specflow.json

No specflow.json file configured No app.config file configured

Issue Description

Enums with DescriptionAttribute are not transforming properly by SpecFlow.

If we set step definition argument type as particular Enum, it will be counted by Specflow as the case where Standart Conversion should be used. Standart Conversion method call stack is the following(I used a stack trace, so read direction is from bottom to top):

   at System.Enum.TryParseInt32Enum(RuntimeType enumType, ReadOnlySpan1 value, Int32 minInclusive, Int32 maxInclusive, Boolean ignoreCase, Boolean throwOnFailure, TypeCode type, Int32& result)
   at System.Enum.TryParse(Type enumType, ReadOnlySpan1 value, Boolean ignoreCase, Boolean throwOnFailure, Object& result)
   at System.Enum.Parse(Type enumType, String value, Boolean ignoreCase)
   at TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertToAnEnum(Type enumType, String value)
   at TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertSimple(Type typeToConvertTo, Object value, CultureInfo cultureInfo)
   at TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertSimple(IBindingType typeToConvertTo, Object value, CultureInfo cultureInfo)
   at TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertAsync(Object value, IBindingType typeToConvertTo, CultureInfo cultureInfo)
   at TechTalk.SpecFlow.Infrastructure.TestExecutionEngine.ConvertArg(Object value, IBindingType typeToConvertTo)
   at TechTalk.SpecFlow.Infrastructure.TestExecutionEngine.GetExecuteArgumentsAsync(BindingMatch match)
   at TechTalk.SpecFlow.Infrastructure.TestExecutionEngine.ExecuteStepAsync(IContextManager contextManager, StepInstance stepInstance)

As you can see, TestExecutionEngine uses TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter instance. However, StepArgumentTypeConverter uses some its methods which leads to run System.Enum.Parse method, which is not working properly with Enums with DescriptionAttributes on its members, throwing an error at some cases:

System.ArgumentException: Requested value 'SecurityQ&A' was not found.

Enum is very handy entity to work with code, which is very wide used along the projects. DescriptionAttribute and its similarities are also pretty wide used, as sometimes, you need to store/use human-readable version of Enum member name(which is limited by C#). But if we would like to use particular Enum as argument type in Step Definition, we became limited to the implementation of System.Enum.Parse method, which is not working properly with DescriptionAttribute. That means Scenario Steps are limited to basic Enum members rules limitation(except for spaces and letter case) - we are not able to use special characters in steps, we are not allowed to start with numbers(maybe there are more limitations which I don't know about). However, numbers at start and special characters could be easily used in project at places in user interface, or other human-related/natural things, as example, on the website.

Of course, you can easily create a Step Definition with string argument type, and do whatever you want in the method body with that string value. But for me, it looks like a workaround, and not a normal use of Gherkin, C# and Enum.

So, I've tried to find a resolution for it. There are some ideas that I've tested:

1. Use custom value retrievers for enum.

It turned out that value retrievers are not working for standart conversions. They are working only for tables conversion.

2. Create a StepArgumentTransformation for all Enums.

Not working, as Enum is not just a generic or something like that. It turned out that I am able to create transformations only for particular Enum. Code issue of this solution: I can interact with string argument inside step definition in any way. Idea issue of this solution: All Enums are implementations of the same entity and should be interacted with one way. Possible profits of this solution:: If you have multiple step definitions with one particular Enum used as type argument, you will not need to interact with string argument in particular steps.

3. Use custom StepArgumentTypeConverter.

Not working. I've tried to use IObjectContainer instance inside BeforeTestRun Hook to register my custom converter, but got an error BoDi.ObjectContainerException: An object has been resolved for this interface already. Even if it worked - there is an issue, where we should create our own implementation of IStepArgumentTypeConverter for all standart types, instead of just re-defining only Enum type conversion.

My suggestion to resolve the issue is:

Change method TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertToAnEnum to work with DescriptionAttribute

OR(a better, more scalable and customizable way)

Create an ability in the SpecFlow Framework to re-define TechTalk.SpecFlow.Bindings.StepArgumentTypeConverter.ConvertToAnEnum method which is used for default transformation to Enum, as example, using Dependency Injection.

P.S. I'm still afraid I am missed something in there, and will be happy to read your suggestions for the issue solution.

Steps to Reproduce

Repro project was attached and you need just to run scenarios in feature files. However, steps to reproduce by your own project will be the following:

  1. Create enum.
  2. Add DescriptionAttribute to enum members. Descriptions and members name should be different.
  3. Create a scenario with step using string passed to enum DescriptionAttribute.
  4. Create a step definition for the step which will take the enum as input argument. It will cause SpecFlow to run a standart conversion from the step argument to the enum
  5. Run scenario

Actual result: System.ArgumentException: Requested value 'Special!Character' was not found.

Expected result: Scenario run without any errors and step argument was successfully transformed to an enum member by matching to it's DescriptionAttribute value.

Link to Repro Project

https://github.com/Dlegostaev/SpecFlowEnumIssue

clrudolphi commented 2 years ago

You were very close to success. I've modified your approach by creating a SpecFlow run-time plugin that overrides the default StepArgumentTransformer to use the code you posted in your repro.

Please take a look at this sample project: https://github.com/clrudolphi/EnumDescriptionStepArgumentTransformer

HTH

DmitryLegostaev commented 2 years ago

@clrudolphi Tried your solution, adapted to project, used POC and it worked! Thank you!

To someone who will face the issue: Chris helped me to implement my 3rd idea - "3. Use custom StepArgumentTypeConverter."

Here I created a new branch with plugin added, which is using Enum processing method from my repro repo. https://github.com/Dlegostaev/SpecFlowEnumIssue/tree/plugin

Pay attention that an issue which I've mentioned in original topic for this idea still persists:

Even if it worked - there is an issue, where we should create our own implementation of IStepArgumentTypeConverter for all standard types, instead of just re-defining only Enum type conversion.

So, in order to implement this solution, you need to implement your own CustomStepArgumentTypeConverter to process all basic types done by SpecFlow StepArgumentTypeConverter. Please note that if your custom conversion method is throwing an exception(probably, it should do it during an attempt of wrong conversion), exception should match catch blocks exceptions in CanConvertSimple method. Otherwise, exception will not be handled, which will lead to the test fail during StepDefinition parsing.

In order to demonstrate this issue, I've created this branch from plugin branch: https://github.com/Dlegostaev/SpecFlowEnumIssue/tree/plugin_error One of the tests will fail by unhandled exception(handling of it was commented by me), and this is not an expected result

I'm not sure should I close this issue or not, so I leave it open for some time, then close it if there are no further discussion regarding this issue.

clrudolphi commented 2 years ago

I see your point. I made an unfounded assumption about the behavior of StepArgumentTypeConverters. I'm working on fixing that problem. I don't think a full reimplementation will be required, but I'm still investigating the full API surface area of the built in argument type converter.

clrudolphi commented 2 years ago

Please take another look at my example project here

The EnumStepArgumentTypeConverter is enhanced to encapsulate the built-in StepArgumentTypeConverter and it delegates all non-Enum type conversions to it. The demonstration feature file was enhanced to show existing type conversions working, along with use of the Descriptive enums in Tables and Quote text.

My first attempt at this was to sub-class the built-in type converter but that failed to work for reasons I couldn't figure out. The encapsulation/delegation technique works just fine.

If you proceed with this you will have to take on the maintenance burden of upgrading it as SpecFlow publishes new releases.

Good luck!

DmitryLegostaev commented 2 years ago

Thank you for your response, Chris!

I've also thought about solution like yours, but there were some issues in my attempt. I've tried it in my repro project, and one test in my solution is failing - it's name is "Check first definition" in "SimilarStepDefinitionSignatures" feature file.

Looks like the root of cause is that new code over StepArgumentTypeConverter is not returning false in any cases. However, it should. Failed test is a good representation why it is happening - we have a string that is "Similar Definition"(is string), we have Enum "FirstDefinition", which is RuntimeBindingType, IsEnum and it have DescriptionAttribute in it. So your code return "true". But this Enum doesn't have proper member to match this string. So CanConvert method should return "false" in that case, but it don't.

If we try to put our checks after execution of _baseConverter.CanConvert or _baseConverter.Convert, we will face an issue where we can't determine why we got "false" instead of "true", so it looks like we can't properly handle it to return correct result of CanConvert or Convert methods("false" from base methods could be returned by different reasons, and we can't determine in which cases we could continue our processing of Enum Description and not to cause situation with false "true" result).

I've tried to improve your solution and put my investigation results in this branch: https://github.com/Dlegostaev/SpecFlowEnumIssue/tree/plugin_v2 All tests in my solution marked as passed on this branch.

If you have some free time, I would really appreciate it if you could take a look into it and make suggestions if you have.

Still, the best solution in my opinion would be from my original post and it is - to implement an ability to re-define ConvertToAnEnum method implementation inside StepArgumentTypeConverter class.

clrudolphi commented 2 years ago

Happy to continue the conversation, Diego.

I am not understanding the problem you are trying to highlight. I have copied your feature files, enums, and step bindings in to my solution. See here

One potential snag is that you're working with the SpecFlow 4 beta, while my solution is back on 3.9.74. As such, I had to eliminate the bindings that were using Gherkin expressions and limit the bindings to only regex expressions. I'm working this way to avoid the effort to make this compliant with 4.x's use of Async. I'll get around to it, but I don't think it's relevant for working out how this feature should work (for now).

When I removed the use of gherkin expressions (using only regex), I ran into a problem where the "Check first definition" resulted in an ambiguous step definition. The phrase "Similar Definition" in the When statement matches both binding methods. Which did you intend to match? I inserted the use of apostrophes to make the matches explicit.

Do I understand your statement of the problem correctly in that the scenario should be testing a string value against an Enum that happens to have Description attributes but that matches neither the Description attribute value nor the Enum field name value for all members of the Enum? I have that demonstrated as the fourth scenario in my feature file (attempting to match the string Green as a Color where the Colors enum does not contain that value). In this situation, the plug-in CanConvert() method returns true. I consider this to be correct behavior as the target enum does have a Description attribute on its field values, therefore this type converter should be used to attempt the conversion. The Convert() method then examines each field of the enum and finds no match and then throws the ArgumentException.

Is that not the behavior you would expect? Or would you expect that it would delegate to the built-in type converter instead when it does not match a value?

clrudolphi commented 2 years ago

So, I created a branch that upgrades to SpecFlow v4 beta and updated my type converter to match the async signatures required. I reverted the changes I had made to your SimilarStepDefinitionSignatures feature file so that it was using gherkin expressions.

Everything that I expected to pass, does pass. The SimilarStepDefinitionSignatures feature still has that ambiguous step definition problem mentioned above.

Help me better understand your intent with this feature file and we'll go from there.

Branch: here

DmitryLegostaev commented 2 years ago

I'm happy to continue it too.

Regarding this point:

One potential snag is that you're working with the SpecFlow 4 beta, while my solution is back on 3.9.74. As such, I had to eliminate the bindings that were using Gherkin expressions and limit the bindings to only regex expressions. I'm working this way to avoid the effort to make this compliant with 4.x's use of Async. I'll get around to it, but I don't think it's relevant for working out how this feature should work (for now).

I've tried it once on both versions, for me it looks like there are no significant changes between these versions in terms of the issue. Regarding Async conversion in v4, for me it looks like there is nothing special about it, same methods from v3 but in Task wrapper

Regarding that statement:

When I removed the use of gherkin expressions (using only regex), I ran into a problem where the "Check first definition" resulted in an ambiguous step definition. The phrase "Similar Definition" in the When statement matches both binding methods. Which did you intend to match? I inserted the use of apostrophes to make the matches explicit.

If I'm not mistaken in my past investigation, v4 treats Cucumber Expressions as regex expressions to process them, so, in v4, step definition signature firstly will be converted to regex expression(explicit Enum definition inside signature will be just a string *(.)** in regex", and only then it will process further with matching definitions, conversions, etc... So again, no significant differences here compared to v3.

So, if my investigation is correct, all of the following step definition signatures will be treated by the same way inside SpecFlow:

SpecFlow v3:

[When(@"I use (.*)")] 

SpecFlow v4:

[When(@"^I use (.*)")] 
[When(@"^I use (.*)$")] 
[When(@"I use {FirstEnum}")]
[When(@"I use {SecondEnum}")]

But to make it simple, let's stick with the [When(@"I use (.*)")] of SpecFlow v3.

In this situation, the plug-in CanConvert() method returns true. I consider this to be correct behavior as the target enum does have a Description attribute on its field values, therefore this type converter should be used to attempt the conversion. The Convert() method then examines each field of the enum and finds no match and then throws the ArgumentException.

I'm not sure that it's the correct behaviour. Let's have a look into this example:

    [When(@"I use (.*)")]
    public void WhenIUse(FirstEnum p0)
    {
    }

    [When(@"I use (.*)")]
    public void WhenIUse(SecondEnum p0)
    {
    }

As you can see, signatures are the same, but argument types are different.

So, when you will try to run the code above with 2 step definitions, you will NOT get ambiguous step definition. If string can be converted to FirstEnum, then first step definition will be executed. If it can be converted to SecondEnum, second step definition will be executed. If it can't be converted to any of them, then this error should be shown Multiple step definitions found, but none of them have matching parameter count and type for step

So, we can make a conclusion, that SpecFlow determines the difference between steps with similar definition by their argument types, at least in this exact case. The same approach should be applied to Enum with Descriptions - if it can be converted to exact Enum, then CanConvert should return "true". If not - "false". Another point is - CanConvert method calls CanConvertSimple method, which is basically running a conversion method - ConvertSimple, and returns "false" if exception was caught. But in yours code, CanConvert returns "true" to any Enum with description attribute, even if there are no members matching this string as argument. It happens because your method does not running a conversion as original CanConvert method does.

Could you tell me please, is it clear enough?

clrudolphi commented 2 years ago

I understand now. I've modified the branch of my code that is compatible with v4. CanConvert now confirms that the target type is an Enum and that the Enum has the Description attribute applied to its fields. If so, it invokes the Convert method and returns true if that succeeds. If the target type is not an Enum, or the enum doesn't have any Description attributes, then it falls back to built-in behavior.

My confusion stems from the use of the CanXXX/DoXXX pattern. That pattern is typically leveraged when there are a collection of agents that could act upon an item and a framework would test each agent in turn to find the one that should be invoked. But in this situation, there is only the single StepArgumentTypeConverter. I'm not sure why this was built using the CanXXX/DoXXX pattern.

Every test that I expect to pass is now passing, including yours. I have one failing test in which I use a value not found in any enum and the type converters throw an exception (as expected). The exception thrown is not the 'value not found' exception but the 'Multiple steps definitions found..." b/c I have multiple step definitions that use the same regex (but target different enum types).

I can't commit that this functionality would find its way in to the base StepArgumentTypeConverter. The project leaders would have to make that decision. In the meantime I hope you find this helpful.

DmitryLegostaev commented 2 years ago

My confusion stems from the use of the CanXXX/DoXXX pattern. That pattern is typically leveraged when there are a collection of agents that could act upon an item and a framework would test each agent in turn to find the one that should be invoked. But in this situation, there is only the single StepArgumentTypeConverter. I'm not sure why this was built using the CanXXX/DoXXX pattern.

I'm not an expert in the SpecFlow, but for me it looks quite logical, CanConvert method have exception handling logic inside during execution of (Do)Convert method. Besides that, in some cases it returns a result without calling a (Do)Convert method, so it sounds as yours and a framework would test each agent in turn to find the one that should be invoked, and could provide some structuration and summarization during SpecFlow steps parsing, so possibly SpecFlow could get some benefits of it.

Regarding your latest plugin version - I've reviewed it and looks like we are came to pretty similar solution, just with slight code differences. I don't have any objections or suggestions regarding it at the moment, looks like both our solutions works as expected now, thank you for your help, Chris!

I will close this ticket within 1-2 weeks if there are no further discussions regarding the original issue and solutions we're implemented.

DmitryLegostaev commented 1 year ago

Closed as solution was implemented and there was no further discussion.

github-actions[bot] commented 1 year ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.