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.24k stars 754 forks source link

Background not executing at Rule level #2664

Closed aacuadras closed 1 year ago

aacuadras commented 1 year ago

SpecFlow Version

3.9.74

Which test runner are you using?

NUnit

Test Runner Version Number

3.13.1

.NET Implementation

.NET 6.0

Project Format of the SpecFlow project

Classic project format using <PackageReference> tags

.feature.cs files are generated using

SpecFlowSingleFileGenerator custom tool

Test Execution Method

Command line – `dotnet test -l "console;verbosity=normal"

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

    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.5.0" />
    <PackageReference Include="SpecFlow.Plus.LivingDocPlugin" Version="3.9.*" />
    <PackageReference Include="SpecFlow.NUnit" Version="3.9.74" />
    <PackageReference Include="nunit" Version="3.13.1" />
    <PackageReference Include="NUnit3TestAdapter" Version="3.17.0" />
    <PackageReference Include="FluentAssertions" Version="5.10.3" />

Issue Description

While trying to run scenarios under a rule, I have steps that repeat across those scenarios and want to create a background to avoid duplication. When I insert a background section inside the Rule it gets ignored, currently only backgrounds at the Feature level are supported.

Steps to Reproduce

  1. Create a new SpecFlow + NUnit project
  2. Create a feature file with Background inside a Rule section and a sample scenario inside the Rule
  3. Run dotnet test -l "console;verbosity=normal" command
  4. Verify if background step got recognized by parser

Link to Repro Project

No response

clrudolphi commented 1 year ago

I can reproduce this. Rule Backgrounds are skipped during test method generation. The parser seems to recognize these and the VS add-in properly color codes them (leading me to believe that this is valid syntax per Gherkin).

@SabotageAndi @gasparnagy - is this omission by design or simply not yet implemented yet?

I can take a stab at adding this feature and would propose the following approach:

gasparnagy commented 1 year ago

@clrudolphi Thx for looking into this.

The Rule-level background is a black sheep. ~5 years ago someone brought it up on a Cucumber conf, we discussed it and decided to drop the idea, because the complexity it causes overweighs the benefits, but someone in the meanwhile implemented that in Cucumber as a PR and got merged. So finally it was supported by Cucumber and people started to use it so we decided not to remove it from Cucumber anymore.

For compatibility, it would be good if it would also work for SpecFlow, but was never a super priority because of the background story.

If you are happy to play with it, it is definitely appreciated.

For the proposed design: that sounds good, but feel free to make it simpler by simply injecting the rule-background steps into the scenario methods directly (without generating an own method for them). We currently generate a separate method for feature-level backgrounds, but mainly for historical reasons (the code-behind files were not auto generated). If I would do it now, probably I would just inject the background steps into all scenarios and generate the test method where each includes the code for the background steps. It is anyway a generated code, so we don't need normalized form.

I don't know if that would make the design easier, but if it is feel free to go that way.

Technical note/suggestion: you can start with a PR right from the beginning. You can keep it as "draft" so that no one cares what you do there and when you feel like ready you can switch it to a ready PR. It makes it easier for anyone to have preliminary look.

clrudolphi commented 1 year ago

Certainly. Thanks for the suggestions. On a tangential note - ScenarioPartHelper method SetupFeatureBackground() has the following code and I'm confused by it - hoping you can clarify: `

   public void SetupFeatureBackground(TestClassGenerationContext generationContext)
    {
        if (!generationContext.Feature.HasFeatureBackground())
        {
            return;
        }

        var background = generationContext.Feature.Background;

        var backgroundMethod = generationContext.FeatureBackgroundMethod;

        backgroundMethod.Attributes = MemberAttributes.Public;
        backgroundMethod.Name = GeneratorConstants.BACKGROUND_NAME;

        _codeDomHelper.MarkCodeMemberMethodAsAsync(backgroundMethod);

        var statements = new List<CodeStatement>();
        using (new SourceLineScope(_specFlowConfiguration, _codeDomHelper, statements, generationContext.Document.SourceFilePath, background.Location))
        {
        }

        foreach (var step in background.Steps)
        {
            GenerateStep(generationContext, statements, step, null);
        }
        backgroundMethod.Statements.AddRange(statements.ToArray());
    }

'

Why is the content of the 'using (new SourceLineScope)' statement empty? Should the foreach statement be inside of the using statement?

clrudolphi commented 1 year ago

@gasparnagy PR2668 submitted. Welcome your thoughts.

gasparnagy commented 1 year ago

@clrudolphi On the SetupFeatureBackground() method. This is a strange thing. The method has been extracted from UnitTestFeatureGenerator in commit 91c77da3 (2019) and the original method was:

        private void SetupFeatureBackground(TestClassGenerationContext generationContext)
        {
            if (!HasFeatureBackground(generationContext.Feature))
                return;

            var background = generationContext.Feature.Background;

            var backgroundMethod = generationContext.FeatureBackgroundMethod;

            backgroundMethod.Attributes = MemberAttributes.Public;
            backgroundMethod.Name = BACKGROUND_NAME;

            AddLineDirective(backgroundMethod.Statements, background);

            foreach (var step in background.Steps)
                GenerateStep(backgroundMethod, step, null);

            AddLineDirectiveHidden(backgroundMethod.Statements);
        }

I guess the change was to introduce a "using-style" handling of the source information that we did earlier using the AddLineDirective methods.

Since the GenerateStep methods also set the source line number, it is not a problem that the loop is not inside the using. So the more interesting question is whether the empty using statement is needed at all?

With the current code, SpecFlow generates something like this:

        public virtual void FeatureBackground()
        {
#line 5
#line hidden
#line 6
 testRunner.Given("I have entered 5 into the calculator", ((string)(null)), ((TechTalk.SpecFlow.Table)(null)), "Given ");
#line hidden
        }

The first two lines of the method are generated by the empty using. As the referred line (in my case line 5) is the "Background" header line, the result is that we can put a breakpoint to the header line as well. So I don't know if it was intentional, but it looks useful.

Maybe we should add a comment about this at the empty using, so that it does not confuse us again.

Technical note: if you refer to an issue or PR in a comment with a # and then directly a number (e.g. #2668), GitHub automatically converts it to a clickable link: #2668.

gasparnagy commented 1 year ago

Fixed by #2668

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.