Find Unused Step Definitions command

[clrudolphi]

This commit duplicates code from FindStepDefinitionUsageCommand. Not tested.

This is a preliminary PR to track the POC for adding a feature to the Reqnroll.VisualStudio extension that provides a context menu command that will find all StepDefinitions in the current project that are not bound to a feature file Step.

• [ ] 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)
• [ ] I have added an entry to the changelog. (mandatory)
• [X] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.

[clrudolphi]

Pardon my ignorance of VS Extensions... I have this new command running in Unit tests. But I can't figure out how to launch a second instance of VS that would use my version of the extension so that it can be tested via the UI. What's the procedure?

[gasparnagy]

Trying things out in VS:

• You need to enable "Visual Studio Extension Development" in VS installer

• This will add a separate registry hive for Visual Studio that you can use for testing, so that you can separately add/remove extensions to that without affecting your main VS. This is called the "Ext" hive. The ext hive can be run from a dev console with devenv /rootSuffix Exp, but the VS installer also adds a new command to the start menu called "Start Experimental Instance of Visual Studio 2022" (I usually pin that to the start menu somewhere).
• Every time you build the VS extension in Visual Studio, the build will install the just-compiled package into the ext hive. So if you open the ext hive version of VS, you will be able to use the latest version of your extension.
• Once the exp hive is running, you can attach to that "devenv" process from your main VS and you can also debug.
• You can even "run" the package project (F5), in that case VS will automatically start the exp hive with the debugger attached (and your extension will be loaded of course). I normally does not use that, but run my exp hive manually and attach to it later because otherwise you need to wait for a long time until the VS exp hive boots up with the debugger attached.

[gasparnagy]

Comments on the code:

• In order to make the actual command visible (for example in the context menu), you need to register it in the ReqnrollVsPackage.vsct file. It's an XML file. You can see my commit about the "go to hook" command about what changes are needed.

• The unit tests are good, but usually the most end-to-end help you get is from the Reqnroll tests. Take a look at the scenarios in the FindStepDefinitionUsagesCommand.feature (and the other editor command features) to get an inspiration.

• I think otherwise you seem to go to the right direction. At the end we don't have to collect the step definitions and make a temporary binding registry, like you do here, but you can simply access the latest binding registry with var bindingRegistry = _discoveryService.BindingRegistryCache.Value;, like we do at the syntax coloring here

[gasparnagy]

One more dev note: As compiling the "package" project is pretty slow (as it needs to install the package to the exp hive), during development I usually select the "TestDebug" configuration (instead of "Debug") in VS. That does not include the package project to the build by default, so I can work with unit and specs tests much faster. If I want to try this out in VS, I just right click on the package project in the solution explorer and invoke build from there - that builds it even though it is not included in the configuration.

[gasparnagy]

I suspect it will report multiple "unused" for a single Step method for each Given/When/Then Attribute applied to that Method (when those attributes don't match with feature steps). These should probably be deduplicated.

The ProjectStepDefinitionBinding.Implementation gives you a ProjectBindingImplementation object that is unique within a binding registry, so you can use that for deduplication.

It is an interesting question though what the expected behavior is for this two key cases:

Case 1

[When("X")] // unused
[Given("Y")] // unused
public void WhenX() {}

Shall we list WhenX twice (as an unused Given and an unused When) or only once?

My vote would be probably to list it twice... but I'm not fully sure.

(Of course the same problem can happen with two Whens ([When("X"), When("y")]) as well.)

Case 2

[When("X")] // used
[Given("Y")] // unused
public void WhenX() {}

Shall we list WhenX as unused? Because the method itself is used (as When) but not used as Given.

My vote would be probably to list it... but I'm not fully sure.

(Of course the same problem can happen with two Whens ([When("X"), When("y")]) as well.)

[gasparnagy]

I like the concept of "draft pull requests" quite much - they give you early indication of the integration without looking to be "ready"

[gasparnagy]

@clrudolphi could you please do a dummy push to this to see if the CI works better now with external PRs?

[clrudolphi]

@clrudolphi could you please do a dummy push to this to see if the CI works better now with external PRs?

Meaning you want me to execute git commit --no-edit --amend git push -f ?

[gasparnagy]

yes (or you can just change a dummy line and push)

[clrudolphi]

yes (or you can just change a dummy line and push)

Executed.

[clrudolphi]

• In order to make the actual command visible (for example in the context menu), you need to register it in the ReqnrollVsPackage.vsct file. It's an XML file. You can see my commit about the "go to hook" command about what changes are needed.

I see in the vsct file that keyboard shortcuts are defined for each menu command. Should one be defined for this new command, and what should the keybinding be?

[gasparnagy]

I see in the vsct file that keyboard shortcuts are defined for each menu command. Should one be defined for this new command, and what should the keybinding be?

@clrudolphi The <KeyBinding> part is optional, you can skip that.

[clrudolphi]

@gasparnagy Thanks for the help so far. I have this wired in and working. Here is an update on status:

First, an observation: when a Reqnroll solution is first loaded, the context menu items for 'Find Step Definition Usages' and this new one 'Find Unused Step Definitions' fail. The output shows:

Info: OnActivityStarted: Starting Visual Studio Extension... Info: CreateProjectScope: Initializing project: ReqnrollExampleProject Info: OnSettingsInitialized: Project settings initialized: .NETCoreApp,Version=v8.0,Reqnroll:1.0.0 Warning: GetStepDefinitionsAsync: Unable to get step definitions from project 'ReqnrollExampleProject', usages will not be found for this project. Info: FindUsagesInProjectsAsync: Found 0 usages in 0 feature files

Subsequent invocations do work properly. Am I doing anything incorrectly or is this known/expected behavior?

Secondly, I need some guidance on what we want to be displayed in the dynamically built result list in the context menu. Taking a cue from the 'Find Step Definition Usages' command, I have it currently set up to show the Location and Method. For a step definition like this: [Given("this isnt used anywhere")] [When("a second unused binding")] public void ThisIsntUsed() { throw new PendingStepException(); }

The context menu is populated with this info:

Per our previous dialog, it may be more helpful to include both the methodName and the RnR Attribute in that message. What are your thoughts?

My next step is to create Specs to further test this.

[gasparnagy]

First, an observation: when a Reqnroll solution is first loaded, the context menu items for 'Find Step Definition Usages' and this new one 'Find Unused Step Definitions' fail.

I have also observed this, but haven't got time to debug it yet (it is since a last change and somehow related to the async handling, I think). Feel free to ignore it for now.

Per our previous dialog, it may be more helpful to include both the methodName and the RnR Attribute in that message. What are your thoughts?

This is a good question. So basically we have the following information that might be relevant:

• file name & line no
• class & method name
• attribute type (G/W/T)
• attribute parameter

For the most of the cases, the unused method will not have multiple attribute, so IMHO we should find a solution that is somewhat optimized for that, but it also works for the multi-attribute model as well.

Maybe:

StepDefinitions\CalculatorStepdefinitions(47,9): [Given("this isnt used anywhere")]

or

StepDefinitions\CalculatorStepdefinitions(47,9): [Given("this isnt used anywhere")] CalculatorStepdefinitions.ThisIsntUsed()

but this is probably too long (would need to check how it looks).

(We can consider to add G/W/T as icon in addition to that, but I'm not sure.)

[gasparnagy]

Technical side-note: in the PRs it is better not to rewrite the commits (amend/force push), so that reviewers can better see what has been reviewed and what has changed since. At the end we will anyway squash the changes, so the individual commits will not be relevant.

[gasparnagy]

And BTW: it is fantastic that two days after someone bought up an idea, we have a working solution. Congrats!

[clrudolphi]

Technical side-note: in the PRs it is better not to rewrite the commits (amend/force push), so that reviewers can better see what has been reviewed and what has changed since. At the end we will anyway squash the changes, so the individual commits will not be relevant.

Sorry about that. My git setup has something weird about it. Sometimes when I go to Push, git thinks that I have conflicting commits from Origin even though I was the last one to commit and push. Perhaps it's me doing something wrong with my manner of using git (sometimes from the command line, sometimes from within VS, sometimes by using GH Desktop). In this situation, I knew that the last commit was mine (the dummy commit we did earlier today) and so a force push was simpler for me than trying to figure out what it thought was wrong.

[clrudolphi]

Do we have available a large RnR project that this could be tested on? I'm curious about the performance and how long the user will have to wait for the context menu to be populated with the results. Since this is a result of a bunch of set subtraction operations we don't have a convenient means of progressively populating the result. On large solutions, this command might take a while. Perhaps this whole thing should really be a batch operation (command line) and expect users to run this as part of their CI?

[gasparnagy]

With this command line tool, you can generate a big sample project. https://github.com/reqnroll/Reqnroll.VisualStudio/tree/main/Tests/Reqnroll.SampleProjectGenerator

I think for many users, the version integrated to vs is beneficial. Having it as a ci option might also make sense, but that is another story - we don't really have infrastructure for that yet.

[gasparnagy]

Most of the users have <500 scenarios per project.

[clrudolphi]

Maybe:

StepDefinitions\CalculatorStepdefinitions(47,9): [Given("this isnt used anywhere")]

or

StepDefinitions\CalculatorStepdefinitions(47,9): [Given("this isnt used anywhere")] CalculatorStepdefinitions.ThisIsntUsed()

IIRC, you mentioned earlier that the expression in the StepDefinitionBinding was, in the case of Cucumber expressions, a Regex created by transforming the Cucumber expr into a Regex. Would displaying the regex in that situation be confusing for the user (as it doesn't match their own code)?

[gasparnagy]

There are two properties on the loaded step deft: the calculated regex and the original expression that the user entered.

[clrudolphi]

There are two properties on the loaded step deft: the calculated regex and the original expression that the user entered.

OK. It looks like if I use stepDefinition.Expression then I should be safe.

[gasparnagy]

Yes. I think this is it: https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Reqnroll.VisualStudio/Discovery/ProjectStepDefinitionBinding.cs#L20

[clrudolphi]

Current UI:

A potential issue with my approach is that I'm using the stepDefinition.StepDefinitionType property to get the keyword. But that is a simple enum and won't be localized.

[gasparnagy]

Looks good! I like it!

[clrudolphi]

Pushed another commit. This includes testing via Specs. Fixed up unit tests (such as they are).

I tested this with an external assembly and it worked fine.

What other refactoring/clean-up would you like to see?

[clrudolphi]

@gasparnagy thanks for the extensive feedback. I will get to work on those items. I have completed an attempt at using the SampleProjectGenerator; and now I suspect we have a bug to find somewhere. I generated a sample project with 50 feature files and 10 scenarios per file. The rest of the options were defaults. That gave a project with 235 Step definition classes and 1400 Step Definition methods. Ridiculously large, yes, but my intent was to determine the processing rate. The command hung the UI thread for several minutes (I'll rerun it later to get a specific time). When it did return, the output window showed this:

Found 67648 unused step definitions in 50 feature files

That number can't possibly be correct; remember there were only 1,400 step definitions in the project.

You might be able to help me by explaining how the generator works - how many step definitions should be expected to match with scenario steps? In a spot check, I see some that do and some that don't. If the generator is deterministic, I might be able to determine which types of step defs won't match and would then look at the logic of the command for bugs related to those types of step defs.

[gasparnagy]

@clrudolphi Ah. These are good questions. I haven't checked the generator for some years, so I don't really remember. One thing is for sure that it is deterministic. I will refresh my memories next week and give some hints.

For the size: The 235 step definition classes and 1400 step definition methods look too much. I have no idea why it generated that many for 50 feature files. I will check. Whatever we measure, we should also compare with other features, like the "find step definition usages". If that one also hangs, the issue is not with your code (or maybe we need to include a few "yield" calls to give a chance for the UI to refresh).

[clrudolphi]

Refactored per your review suggestions. Open issues are:

• Perf
• Messaging on the UI when no Unused StepDefs found - per project or solution?

[clrudolphi]

Whatever we measure, we should also compare with other features, like the "find step definition usages".

The "Find step definition usages" operates quickly without hanging. But let's acknowledge that the amount of work done by these two commands is not comparable. Find SD Usages starts from a single StepDef method and has to scan for matches against the 500 scenarios. The FindUnusedSD command does that work for ALL step definition methods (1x vs 1400x). I will look into the idea of incrementally yielding context menu items.

[clrudolphi]

Found a bug that may explain the weird results seen in the perf test. For each Project/FeatureFile, the algo subtracts those step defs that are matched and creates a set of "unused" from the remaining. The problem is that the set of step defs spans the entire solution, so stepDefs that don't match for a given feature file are reported as "unused" even when they might match a different feature file. Each stepDef is getting found as "unused" multiple times, at least as many as the number of featureFiles minus 1. I'll look for ways to address this in the coming days.

[clrudolphi]

Found a bug that may explain the weird results seen in the perf test.

I think I have this fixed. 17:45:07:301 Info: FindUnusedStepDefinitionsInProjectsAsync: Found 282 unused step definitions in 1 Projects Instead of the 67K unused step definitions found in this morning's test, the function is now reporting 282, which seems more reasonable. To examine the 1400 step definitions and report back the 282 found as unused took only about 2 seconds. Pushing code changes shortly.

[gasparnagy]

I think I have this fixed.

Nice! This is yet another good example of the importance of exploratory testing. We did specs, we did unit tests, we did review - these are good, but they all driven by our own thinking, so they cannot detect such out of the box problems. It's good that you went after the strange perf test results!

[gasparnagy]

I have checked it and indeed now it finds the unused step definitions from all, but in my opinion it is not convenient this way (I tested it in a real project and it found some totally unimportant unused one in an included sample). But I'm not 100% sure.

The reason that it works this way is because you ask for the binding registry from the projects of the feature files here. If you would move up this call, and would only get the binding registry from the project of the editor in here (and pass down the binding registry) then it would only show the ones from the editor, I think.

[clrudolphi]

I have checked it and indeed now it finds the unused step definitions from all, but in my opinion it is not convenient this way (I tested it in a real project and it found some totally unimportant unused one in an included sample). But I'm not 100% sure.

The reason that it works this way is because you ask for the binding registry from the projects of the feature files here. If you would move up this call, and would only get the binding registry from the project of the editor in here (and pass down the binding registry) then it would only show the ones from the editor, I think.

I've tried that, but it resulted in a bug of duplicate and erroneous findings. The reason was because the list of projects is being enumerated in PreExec resulting in all feature files (and scenarios) for all projects in the solution. When the StepDefs for the current project are compared against that list of scenario steps, the results are incorrect as the SDs are not matching against scenarios from other projects.

Instead, the approach I took was to eliminate the enumeration of all project files, but simply used the current Project as the source of both SDs (via the Binding Registry) and scenarios (via the feature file loading and parsing done in FindUnused and FindUsagesFromContent). Changes made in this commit

[clrudolphi]

Updated the original PR form to reflect that Tests are now included. Still need to update Documentation and to update the Change Log.

Before those get done, is there anything else code-wise or logic yet to do?

[gasparnagy]

Instead, the approach I took was to eliminate the enumeration of all project files, but simply used the current Project as the source of both SDs (via the Binding Registry) and scenarios (via the feature file loading and parsing done in FindUnused and FindUsagesFromContent).

I think I have confused you now (sorry) and I don't know what is the best option. Let me explain the complexity that causes the problem, because this is going to be useful info anyway.

In the majority of the cases, the step definitions are in the same project as the feature files, but this is not always like that. You can include step definitions of another project as well with the Bindings from External Assemblies feature. You can include bindings from other projects that also have feature files (the projects that have feature files are called the ReqnrollTestProjects) or you can include bindings from projects that are just used to implement shared step definitions (these are called ReqnrollProjects). These ReqnrollProjects that are not ReqnrollTestProjects cannot have a reqnroll.json config file, so they are not "standalone".

Actually you can only get the binding registry of ReqnrollTestProjects. The binding registry of these projects will contain the step definitions of the project itself and also the step definitions of the used external step definitions as well.

Technically you can probably get the binding registry of a ReqnrollProject that is not a ReqnrollTestProjects as well, but that is not really a valid operation (they might be dependent on some plugins or config settings that is only available in ReqnrollTestProjects).

This altogether means that if we would like to list all step definitions in the current project that are unused, we cannot get the editor's binding registry as I suggested earlier, because that might be just a normal ReqnrollProject (without feature files), but we would need to enumerate all ReqnrollTestProjects that "use" the current project as an external binding source, get the unused step definitions of them and filter out these unused step definitions to show only the ones from the current project. Pretty complicated. (I think there is no function to list ReqnrollTestProjects that "use" the current project, but of course we could just list all of them the filtering will anyway ignore all their unused step definitions if they don't "use" the current project.)

I'm not sure that we want this.

The alternative is to revert the last change and rater implement this feature in a way that it finds all step definitions from all projects (as you did it anyway). The most of the users have anyway only one Reqnroll project, and we can still change this feature later to filter for the current project's step definitions if we will see big problems with this approach.

Sorry again for misleading you...

[gasparnagy]

Still need to update Documentation and to update the Change Log.

We don't yet have a documentation of the VS integration, so you cannot extend that (OK for now), but please add a line to the change log. I don't see anything else to be done. (Except solving the confusion that I caused as described above.)

[clrudolphi]

I'd like to ensure I correctly understand the nuances of what you've explained and ultimately the requirements for this feature. Please review the following and correct it/add to it so that we have a well thought out statement of our objectives/requirements.

Feature: Reqnroll StepDefinition Finding

Scenario Outline: ReqnRoll Finds Unused Step Definitions
    Given a project of <type> that has <features> features and <stepDefs> step definitions
    And a sibling project of <siblingProjectType> with <siblingFeatures> and <siblingStepDefs> step definitions
    When the Find Unused Step Definitions command is executed in the <project>
    Then the command will report back unused Step Definitions from <scope> project(s)

Examples:
    | type           | features | stepDefs | siblingProjectType | siblingFeatures | siblingStepDefs | project | scope   | comment                                                    |
    | RnrTestProject | some     | some     | none               | none            | none            | main    | main    |                                                            |
    | RnrTestProject | some     | some     | RnRTestProject     | some            | some            | main    | main    |                                                            |
    | RnrTestProject | some     | some     | RnRTestProject     | some            | some            | sibling | sibling |                                                            |
    | RnrTestProject | some     | some     | ExternalAssembly   | none            | some            | main    | both    |                                                            |
    | RnrTestProject | some     | some     | RnRProject         | none            | some            | main    | main    |                                                            |
    | RnrTestProject | some     | some     | RnRProject         | none            | some            | sibling | sibling |                                                            |
    | RnRProject     | none     | some     | RnRTestProject     | some            | some            | main    | main    | mirror of line above; really restating the same thing      |
    | RnrTestProject | some     | none     | none               | none            | none            | main    | invalid | the FUSD command can only be executed from a binding class |

[gasparnagy]

Essentially your understanding is correct, my thoughts on the expected result only differs in the "ExternalAssembly" case, but that is something we haven't discussed anyway and the difference is not important.

I have tried to rephrase the specs in a way that it is better fitting to my way of thinking. Instead of using the "sibling" term I simply used project "A" and "B" and added an extra condition whether the B is listed as additional binding assembly for A or not. This is what I got. As I said at the end the result is the same, the only diff is marked with (*) .

Feature: Reqnroll StepDefinition Finding

Scenario Outline: ReqnRoll Finds Unused Step Definitions
    Given a project A of <typeA> that has <featuresA> features and <stepDefsA> step definitions
    And a project B of <typeB> with <featuresB> and <stepDefsB> step definitions
    And project B is <B listed as bindigns of A> as binding assembly of A
    When the Find Unused Step Definitions command is executed in the <project>
    Then the command will report back unused Step Definitions from <scope> project(s)

Examples:
    | description                                                  | typeA   | featuresA | stepDefsA | typeB   | featuresB | stepDefsB | B listed as bindigns of A | project | scope         | comment                                                                                                          |
    | single RnR Test project                                      | Project | some      | some      | none    | none      | none      | not listed                | A       | A             |                                                                                                                  |
    | two independent RnR Test project                             | Project | some      | some      | Project | some      | some      | not listed                | A       | A             |                                                                                                                  |
    | two independent RnR Test project, invoked from B             | Project | some      | some      | Project | some      | some      | not listed                | B       | B             | same case as 'two independent RnR Test project'                                                                  |
    | RnR Test project uses bindings from a DLL                    | Project | some      | some      | DLL     | none      | some      | listed                    | A       | A (*)         | the unused step defs of the external DLL are not managed in this solution, but in the one where it is maintained |
    | RnR Test project uses bindings from a shared library project | Project | some      | some      | Project | none      | some      | listed                    | A       | A             |                                                                                                                  |
    | feature invoked from a shared binding library project        | Project | some      | some      | Project | none      | some      | listed                    | B       | B             |                                                                                                                  |
    | feature invoked from a shared binding library project 2      | Project | none      | some      | Project | some      | some      | listed                    | A       | A             | mirror of line above; really restating the same thing                                                            |
    | single RnR Test project, without any bindings                | Project | some      | none      | none    | none      | none      | not listed                | A       | not available | the FUSD command can only be executed from a binding class                                                       |

[clrudolphi]

@gasparnagy I've modified the Command to limit the set of UnUsed SDs to those within the current project. In doing so, I made a change to VsProjectScope.GetProjectFiles(string extension). While that method would take any string as an extension, it did not use it and instead was only returning feature files. I modified it such that it used the extension in the filtering process it ran. Let me know if that change is too intrusive or risky.

I am a noob with git and not quite sure how to proceed here. I've sync'd my fork on GH to bring in the other changes that have been merged, and pulled them down to my machine. I then rebased these changes from main. My GH Desktop app is now reporting that my branch has 7 commits to push (6 from me of the work done already, which seems to me that it duplicates what has already been pushed to GH; and one that is a merge commit ("Merge branch 'reqnroll:main' into RNR70_UnusedStepDefinitions {the branch name}). that merge commit seems suspicious to me too. What did I do wrong?

Any suggestions on how to proceed? I am guessing that a squash is needed. Should I have done that before rebasing? Should I delete my local git repo and pull it back down fresh from GH?

[gasparnagy]

I've modified the Command to limit the set of UnUsed SDs to those within the current project. In doing so, I made a change to VsProjectScope.GetProjectFiles(string extension).

Good idea! I like it.

[...] that is a merge commit ("Merge branch 'reqnroll:main' into RNR70_UnusedStepDefinitions {the branch name}). that merge commit seems suspicious to me too. What did I do wrong?

Nothing. This is good. When GH runs the PR checks, it automatically tries to merge your changes to the reqnroll:main and runs the tests on the merged result. So basically it checks the "merge compatibility" or "integration". If this succeeds, there is nothing else to do.

In some cases your changes cannot be merged with the reqnroll:main because there has been changes there that are not compatible. This happens for example when you add a new line to the changelog while others have also added some, so git cannot decide which line should go first. In this cases, you need to "resolve the conflict" on the PR. This can be done in multiple ways. It could be done by rebasing the entire PR to reqnroll:main and force push, but it is not practical if people have done reviews (as we discussed earlier). The better option is to simply merge the reqnroll:main into your PR branch. (This might look ugly, because all the changes others have done will appear in this PR, but don't worry GH and git knows that those are not belong to this PR.) This is exactly what you did, so all ok.

You do not need to do squashing, this will be done by GH once I "merge" the PR.

Now as you are part of the core-team, it will be a bit easier, because you don't need to use & sync your own fork, you can work on the main repo directly: in the cloned main repo, just create a new branch and push it - on the GH Reqnroll project page it will ask you to make that a PR.

Side note: As you work with "draft" PRs now, at the point where you think the PR is OK from your point of view, you can click on the "Ready for review" button. This is just an signal for the potential reviewers, does not really change anything. (I did that now.)