reqnroll / Reqnroll.VisualStudio

Visual Studio extension for Reqnroll - open-source .NET BDD framework
https://reqnroll.net
BSD 3-Clause "New" or "Revised" License
10 stars 3 forks source link

'Find Unused Step Definitions' command doesn't handle 'StepDefinition' attributes well #22

Open UL-ChrisGlew opened 1 month ago

UL-ChrisGlew commented 1 month ago

Used Visual Studio

Visual Studio 2022

Are the latest Visual Studio updates installed?

Yes

Content of reqnroll.json (if present)

{ "bindingCulture": { "name": "en-GB" }, "language": { "feature": "en-GB" } }

Issue Description

Step definitions declared with the 'StepDefinition' attribute are showing multiple times inside the new 'Find Unused Step Definitions' menu option. These appear to show Given/Then/When usages, and implies that the Step Definition is unused - which is not always the case.

Is it possible to change this behavior to show which methods decorated with 'StepDefinition' are unused?

Steps to Reproduce

  1. Have a Reqnroll project
  2. Create a new 'Step Definition' file, adding the [Binding] attribute
  3. Create a new step definition, using the 'StepDefinition' attribute. For example: [StepDefinition("I can do the thing")] image
  4. Use this StepDefinition in a feature file. For example: Given I can do the thing image
  5. View the 'Find Unused Step Definitions' context on the Step Definition file.
  6. You will see:
    • [Then("I can do the thing")]
    • [When("I can do the thing")] image

Link to a project repository that reproduces the issue

No response

gasparnagy commented 1 month ago

That makes sense.

Reqnroll internally generates three step definition binding from the [StepDefinition] attribute and this is what the VS extension receives. At that point it is not possible to tell if they were coming from the same attribute or separate ones, but if they belong to the exact same method and have the exact same expression we could assume that these were created using [StepDefinition].

The improvement could be done by modifying the UnusedStepDefinitionsFinder.FindUnused method (plus add tests for it to FindUnusedStepDefinitionsCommandTests).

I think the implementation should:

Do you want to try making a PR for this?

UL-ChrisGlew commented 1 month ago

@gasparnagy Sure I'm happy to contribute, just a couple of questions:

  1. Is it possible to debug the Visual Studio extension? I can't seem to debug/run from the Reqnroll.VisualStudio.Package project without installing a 'preview' package into Visual Studio?
  2. I don't seem to have rights to create a new branch in this repository, is that something that could be sorted?

Thanks!

gasparnagy commented 1 month ago

@UL-ChrisGlew Thank you for taking care of this.

For the development experience & debug experience I have updated now the CONTRIBUTION.md guide. That should contain all information. If anything is unclear, please let me know.

For pushing the branch: normally new contributors fork the project and make a new branch in their own fork and create a Pull Request from there. But I have added you now to the contributors group (you need to accept the invite), so you should be able to create a branch here.

UL-ChrisGlew commented 1 month ago

@gasparnagy Thanks for sorting that, I can push new branches/etc to the repository now.

From the guide it says:

You can also set the Reqnroll.VisualStudio.Package project as startup project. It is configured in a way that if you use the Start Debugging command (F5) it will automatically start the Visual Studio Experimental Instance with the debugger attached. So you can debug the full life-cycle of the extension, right from the loading. (Similarly, the "Start Without Debugging" (Ctrl+F5) is also working that simply starts Visual Studio Experimental Instance and you don't have to find it in the Start menu.)

For me, when loading the extension project, I don't see the option to debug, and get the following popup:

image image

Normally I'd think it was something with my Visual Studio install, but I'm able to debug/launch other extensions fine.

gasparnagy commented 1 month ago

That is strange. But I think you can just set the debug config to debug the devenv.exe process (from the vs install location) with the params /rootSuffix Exp. But I will check it once I am back at my desk.

gasparnagy commented 1 month ago

I have checked and indeed the package project cannot be started by default. I might have created a launch profile earlier. Anyway: I have documented the steps to configure the launch profile: https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/CONTRIBUTION.md#debug-the-visual-studio-package

@UL-ChrisGlew I hope it will work for you as well.

UL-ChrisGlew commented 1 month ago

@gasparnagy That worked great, thanks for writing that - hopefully it helps other contributors in the future as well 😃

UL-ChrisGlew commented 1 month ago

@gasparnagy unrelated to this specific issue, I've noticed that the 'Find Unused Step Definitions' doesn't handle it very well if step definitions are included in a separate project.

I've tried to access the context menu option from this separate project and it throws the error below. Is this intended behavior or something that also needs to be fixed? I'm happy to look into this while I'm in this area.

image

gasparnagy commented 1 month ago

@UL-ChrisGlew Thx. We were going back and forth with @clrudolphi about how the projects should be handled. So you should definitely check the discussion we had from here down. We also created a scenario to cover the cases, but it can happen that we still miss a combination.

clrudolphi commented 1 month ago

IIRC we decided to not look at external assemblies because of the risks of false positives. An external step definition assembly may be used by many different projects/solutions. A step that is not used by one project may be used by another. To mark it as 'unused' would be misleading. We could perhaps fine-tune that with some heuristics. If the external step definition project is incorporated via Nuget for example it would be excluded but if the project containing the steps is a project within the current solution then perhaps we include it.

UL-ChrisGlew commented 1 month ago

Ah okay, it's not as simple as a problem to solve as I first thought - it's not a dealbreaker at the moment for us but would be useful in the future.