reqnroll / Reqnroll

Open-source Cucumber-style BDD test automation framework for .NET.
https://reqnroll.net
BSD 3-Clause "New" or "Revised" License
356 stars 38 forks source link

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

Closed kipusoep closed 5 months ago

kipusoep commented 6 months ago

Reqnroll Version

1.0.1

Which test runner are you using?

xUnit

Test Runner Version Number

2.5.6

.NET Implementation

.NET 6.0

Test Execution Method

Visual Studio Test Explorer

Content of reqnroll.json configuration file

{
  "$schema": "https://schemas.reqnroll.net/reqnroll-config-latest.json",
  "generator": {
    "addNonParallelizableMarkerForTags": [ "NoParallel" ]
  }
}

Issue Description

I am migrating a Specflow project to Reqnroll. All is working fine, except for a single step definition which I can't "migrate" to cucumber expressions. I have tried this in a clean solution and can't reproduce it, also I am unable to figure out in what scenario exactly the issue is happening. Anyway, it's the following step definition:

[Given("the organisation with status '(.*)' is stored")]
[Then("the organisation with status '(.*)' should be stored")]
public async Task GivenThenTheOrganisationIsStoredWithStatus(OrganisationStatusEnum status)
{
    <redacted>
}

As shown above it's working fine, but when I change one of the RegEx's I get a lot of errors saying: An item with the same key has already been added. Key: OrganisationStatusEnum

I don't have a clue why and I'm also not sure how to look into it, there's no stacktrace or whatever. Any thoughts?

Steps to Reproduce

I don't know unfortunately :-(

Link to Repro Project

No response

gasparnagy commented 6 months ago

@kipusoep If I understand well, as long as you keep using the regex it works, but when you try to change it to cucumber expression, you get the error.

Two questions:

kipusoep commented 6 months ago

If I understand well, as long as you keep using the regex it works, but when you try to change it to cucumber expression, you get the error.

Correct. Cucumber expressions are working fine on other step definitions though.

Could you please share the cucumber expression that causes the error?

Sure, this will trigger the issue for me:

[Given("the organisation with status {string} is stored")]
[Then("the organisation with status '(.*)' should be stored")]
public async Task GivenThenTheOrganisationIsStoredWithStatus(OrganisationStatusEnum status)
{
    <redacted>
}

Where do you see the error? In Visual Studio or during test execution? If it is reproducible during test execution as well, please share the full stack trace.

It's a compilation error in VS. So test execution won't even run at all. There's no stack trace unfortunately.

gasparnagy commented 6 months ago

@kipusoep Could you please try to run the tests from command line, like with dotnet test? Or disable the Reqnroll VS integration temporarily and run the tests from VS?

kipusoep commented 6 months ago

Yes ofcourse, I will do so tomorrow.

kipusoep commented 6 months ago

Sorry for the delay, running from CLI also fails:

  Failed <redacted> [236 ms]
  Error Message:
   Reqnroll.BindingException : Binding error(s) found:
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
An item with the same key has already been added. Key: OrganisationStatusEnum
  Stack Trace:
     at Reqnroll.ErrorHandling.ErrorProvider.GetInvalidBindingRegistryError(IEnumerable`1 errors)
   at Reqnroll.Infrastructure.TestExecutionEngine.GetStepMatch(StepInstance stepInstance)
   at Reqnroll.Infrastructure.TestExecutionEngine.ExecuteStepAsync(IContextManager contextManager, StepInstance stepInstance)
   at Reqnroll.Infrastructure.TestExecutionEngine.OnAfterLastStepAsync()
   at Reqnroll.TestRunner.CollectScenarioErrorsAsync()
   at <redacted>.Features.<redacted>Feature.ScenarioCleanupAsync()
   at <redacted>.Features.<redacted>Feature.<redacted>() in C:\<redacted>\OrganisationService.feature:line 18
--- End of stack trace from previous location ---
  Standard Output Messages:
 Given an organisation with status 'Active'
 -> binding error: Binding error(s) found:
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
 An item with the same key has already been added. Key: OrganisationStatusEnum
clrudolphi commented 6 months ago

I can't recreate this. As a simplified example, I created the following feature file:

Feature: Color Enum Testing

Scenario: Name a color enum Given a color of 'Red' Then the color of 'Red' should be OK

and the corresponding step definition:

namespace RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.StepDefinitions
{
    public enum Colors
    {
        Red,
        Blue, 
        Green, 
        Yellow
    }
    [Binding]
    public sealed class ColorEnumTestStepDefinitions
    {
        [Given("a color of {string}")]
        [Then("the color of '(.*)' should be OK")]
        public void TheColorOfShouldBeOK(Colors color)
        {

        }
    }
}

Without the single-quotes surrounding the enum value in the scenario steps, the VS IDE plugin would not colorize the steps correctly (meaning it was not binding the step to the definition). Running the tests as shown above succeeds. Without the single-quotes, the tests fail with the usual error message about not finding a matching step definition.

Do you have a custom ValueRetriever registered for your enum?

kipusoep commented 6 months ago

No custom ValueRetriever registered. I tried to reproduce it in a clean project as well, but failed. So I guess I have to find a way to get more information about what's happening using this specific project, but I'm not sure how.

clrudolphi commented 6 months ago

Can you share the definition of the enum you are using? Perhaps there is something there that is causing issues with the default conversion done by Reqnroll.

clrudolphi commented 6 months ago

I have managed to recreate this, although in a somewhat forced way. I don't think most users would stumble upon this as enum parsing is already provided out of the box.

The issue does occur when a StepArgumentTransformation is present that is given a name the same as the enum Type.

Given the same feature file as given as example above (using a Colors enum) and the following Binding code:

namespace RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.StepDefinitions
{
    public enum Colors
    {
        Red,
        Blue, 
        Green, 
        Yellow,
        Black,
        abc
    }
    [Binding]
    public sealed class ColorEnumTestStepDefinitions
    {

        [Given("a color of {string}")]
        [Then("the color of {string} should be OK")]
        public void TheColorOfShouldBeOK(Colors color)
        {

        }
    }

    [Binding]
    public class MistakenColorEnumTransformer
    {
        [StepArgumentTransformation("(.*)", Name = "Colors")]
        public Colors Transform(string color)
        {
            Colors result = Colors.abc;
            if (Enum.TryParse( color, out result)) return result;
            return Colors.abc;
        }
    }
}

Results in the following error:

Name a color enum
   Source: ColorEnumTest.feature line 3
   Duration: 13 ms

  Message: 
Reqnroll.BindingException : Binding error(s) found: 
An item with the same key has already been added. Key: Colors
An item with the same key has already been added. Key: Colors

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

  Standard Output: 
Given a color of 'Red'
-> binding error: Binding error(s) found: 
An item with the same key has already been added. Key: Colors
An item with the same key has already been added. Key: Colors
Then the color of 'Black' should be OK
-> binding error: Binding error(s) found: 
An item with the same key has already been added. Key: Colors
An item with the same key has already been added. Key: Colors

The error does not occur when the StepArgumentTransformation is given a unique name (such as 'mycolor') and that is used as the Cucumber expression in the Binding pattern.

So, when I asked earlier about a ValueRetriever, I should have asked about the presence of any StepArgumentTransformer. Do you have any of those in your project?

kipusoep commented 6 months ago

No I don't 😅 But I'm curious if the underlying issue is the same.

gasparnagy commented 6 months ago

@kipusoep I was also able to reproduce the problem by having enums with the same name defined multiple times (in different namespaces). Could you please check if this might be the case for you (ie having multiple OrganisationStatusEnum defined)?

kipusoep commented 5 months ago

Yes I do, although I use both of them with a different name via the usings, like this:

using OrganisationStatusOriginal = Some.Namespace.OrganisationStatusEnum;
using OrganisationStatusAlternative = Some.Other.Namespace.OrganisationStatusEnum;

So I think it is caused by the same issue.

gasparnagy commented 5 months ago

@kipusoep Thx for the confirmation. We can act now.

The C# using alias is only for that C# file, the Reqnroll processing will not receive it and from Reqnroll point of view these are two enums with the same short name. I need to figure out what problems this might cause and how should Reqnroll handle this better.

kipusoep commented 5 months ago

Yeah that makes sense. It would be ace if you are able to find a way to handle this, as I honestly can't come up with a work-around other than having to rename the enums.

gasparnagy commented 5 months ago

@kipusoep Agree. This afternoon I'm quite busy, but tomorrow I'll check it out.

kipusoep commented 5 months ago

Trust me, there's no hurry at all from my end :-) So whenever you have a spare moment would be fine. Thanks so far!

clrudolphi commented 5 months ago

@gasparnagy how valid is the situation I identified of a named StepArgumentTransformer that collides with the short name of an enum? I could see that happening when the values of the enum aren't all that useful to the business and the user wants to use a SAT to map from business values to enum values. Or, for instance, when enum values are coded in one language by developer convention/standard, but the business wants to use a different language in their scenarios (eg, French words for colors vs English words for colors). Should we allow for named StepArgumentTransformers that overload the short name of a built-in transformer?

gasparnagy commented 5 months ago

@clrudolphi I fear that is also valid. I need to reconsider the approach we have chosen and find a way to support those situations as well.

gasparnagy commented 5 months ago

Fixed by #100

clrudolphi commented 5 months ago

Fixed by #100

@gasparnagy The same problem exists when using custom type names as cucumber expression parameters. How likely do you think is to occur and is it worth it to attempt a fix?

namespace RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.StepDefinitions
{
    public class B
    {

    }
    public enum Shade
    {
        Light,
        Dark,
        Grey
    }

    [Binding]
    public sealed class ShadeEnumTestStepDefinitions
    {
        [StepArgumentTransformation("of ([A-Z][a-z]+)")]
        public static B ShadeOf(string shade) => new B();

        [Given("a shade of {B}")]
        public void ThisWillResultInAnError(B shade) { }
    }
}
namespace RnRGH81CucumberExpr_ItemWithSameKeyAlreadyAdded.AnotherNamespace
{
    public class B
    {

    }
    public enum Shade
    {
        Light,
        Dark,
        Grey
    }
    [Binding]
    public sealed class AnotherShadeEnumTestStepDefinitions
    {
        [StepArgumentTransformation("of ([A-Z][a-z]+)")]
        public static B ShadeOf(string shade) => new B();

        [Given("a shade of {B}")]
        public void ThisWillResultInAnError(B shade) { }
    }

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

  Message: 
Reqnroll.BindingException : Binding error(s) found: 
An item with the same key has already been added. Key: B
An item with the same key has already been added. Key: B

  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: 
An item with the same key has already been added. Key: B
An item with the same key has already been added. Key: B
gasparnagy commented 5 months ago

@clrudolphi I see. Yes, let's try. Maybe we just need to remove the "IsEnum" condition as the fix.

clrudolphi commented 5 months ago

@gasparnagy I have a fix written. It's a bit more involved than simply removing the IsEnum condition as the problem location is not there, but in the way the custom transforms are handled.

I'm not sure how to proceed from a github perspective. Since the PR is closed, should I create a new one against the same branch? Or create an entirely new branch and PR? What will work best for you and not ruin the build and commit history?

gasparnagy commented 5 months ago

You have to create a new PR on a new branch started from the main. The most convenient way is if you simply clone the main repo (not your fork) and create the branch there. You have right to push the branch up and create a PR from it. This has two benefits: 1. you don't need to hassle with synchronizing your fork. 2. if someone else need to help, they can add additional commits to your PR.

clrudolphi commented 5 months ago

Added as you suggested. PR created.

gasparnagy commented 5 months ago

Second case is also fixed by #104