Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Rule Backgrounds. #2668

Merged

Conversation

clrudolphi
Copy link
Contributor

@clrudolphi clrudolphi commented Oct 28, 2022

This PR adds support for Rule Backgrounds.

Most of the additional logic has been added to the ScenarioPartHelper class:

  • logic to examine the Feature AST for Rules that have Backgrounds and generate the statement codes for each.
  • logic to identify which rule (if any) a given scenario belongs to (during scenario generation to find out which, if any, rule background steps to include)
  • The UnitTestMethodGenerator is modified to include the relevant rule background statements in the scenario

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • [X ] 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)

Checklist:

  • [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
Copy link
Contributor Author

Open to suggestions for improvements.
I will be adding tests to the Specs project. Considering adding a dedicated unit test project for this -- let me know whether you think that worthwhile.

@clrudolphi clrudolphi changed the title Add support for Rule Backgrounds. Preliminary commit. Lacks tests. Add support for Rule Backgrounds. Oct 29, 2022
Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is generally fine, thx! Please check my smaller notes below.

…ethodBody to accept a ScenarioDefinitionInFeatureFile instead of an AST Scenario. This allows simpler tracking of the relationship between Scenario and Rule.

Refactored Rule Background support in ScenarioPartHelper to simplify the code and accept changes suggested during PR review.
…h multiple Rules, each having a Background; the Background steps should only be executed for the Scenario(s) within their respective Rule.
@clrudolphi
Copy link
Contributor Author

@gasparnagy Thank you so much for the review comments and suggestions! I've incorporated your suggestions, resulting in a greatly simplified implementation (largely isolated to the SPH).
Would welcome any further suggestions you might have.

I am having a problem with adding a Feature file to the SpecFlow.Specs tests however. Pls see my latest commit to this PR (#565afff). When executed the test fails with a TechTalk.SpecFlow.BindingException "Ambiguous step definitions..."
The exact error message related to the first scenario of the feature file is:
Error Message:
TechTalk.SpecFlow.BindingException : Ambiguous step definitions found for step 'Given something first as background': DefaultTestProject:RuleSteps1.GivenSomethingFirst(), DefaultTestProject:BindingsClass_76ec255a.StepBinding()
Stack Trace:
at TechTalk.SpecFlow.Infrastructure.TestExecutionEngine.GetStepMatch(StepInstance stepInstance)

Any suggestions?

@clrudolphi
Copy link
Contributor Author

@gasparnagy never mind on the problem with ambiguous step definitions; I figured it out. My problem :).

This PR ready for final review/approval.

@clrudolphi clrudolphi marked this pull request as ready for review November 8, 2022 20:05
Copy link
Contributor

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove that one scenario and then we are ready to go, i think.

public class RuleSteps1
{

[Given("something first as background")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this formatting only broken on GitHub or also in VS?
If so, please fix it. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is really hard to read for me in this style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SabotageAndi I have rewritten this feature file in an attempt to improve its readability. Please let me know what you think.
RuleBackgroundExecution.feature

@clrudolphi
Copy link
Contributor Author

Please remove that one scenario and then we are ready to go, i think.

Removed. This feature file now reverted to what is in Master.

@clrudolphi
Copy link
Contributor Author

@gasparnagy @SabotageAndi - checking in with you on this. I believe all prior suggestions/concerns are addressed. Are there any other changes you would like to see?

@gasparnagy gasparnagy merged commit 8e0e7d4 into SpecFlowOSS:master Dec 15, 2022
@gasparnagy
Copy link
Contributor

@clrudolphi Thx! It took a bit more time... sry

@praticandobdd
Copy link

@SabotageAndi great news! awesome feature! Congrats.

@GondilOndro
Copy link

GondilOndro commented Mar 21, 2023

Has this already been released? Can't see it working in latest SpecFlow for Visual Studio 2022 released Dec 20 2022

@clrudolphi
Copy link
Contributor Author

Has this already been released? Can't see it working in latest SpecFlow for Visual Studio 2022 released Dec 20 2022

It was incorporated into the v4.0.31-beta build (built on 15 Dec 22).

Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
* Add support for Rule Backgrounds. Preliminary commit. Lacks tests.

* Add support for Rule Backgrounds.Revised Commit to simplify approach and code. Lacks tests.

* Refactored signature of method: UnitTestMethodGenerator.GenerateTestMethodBody to accept a ScenarioDefinitionInFeatureFile instead of an AST Scenario. This allows simpler tracking of the relationship between Scenario and Rule.
Refactored Rule Background support in ScenarioPartHelper to simplify the code and accept changes suggested during PR review.

* Adding a feature file in the Specs tests to demonstrate a Feature with multiple Rules, each having  a Background; the Background steps should only be executed for the Scenario(s) within their respective Rule.

* Further simplification of Rule support in SPH by merging two private methods together.

* Corrected feature's binding code. Tests now pass.

* Updating changelog

* Reverting this feature back to the version present in SpecFlowOSS/SpecFlow.

* Refactored/revised to improve readabliity and formatting

Co-authored-by: Gáspár Nagy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants