Proposed Fix for ReqnrollVisualStudio issue 47

[clrudolphi]

(Context menu throws error in project that Step Bindings but no feature files)

Looking for feedback on whether this is a valid path to take to solve the reported issue.

Partial fix; tests not passing and needing update to reflect that with this change we would allow Discovery to proceed on external binding assemblies.

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

• 📖 Documentation (improvements without changing code)
• 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
• 🐛 Bug fix (non-breaking change which fixes a defect)
• ⚡ New feature (non-breaking change which adds new behaviour)
• 💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[gasparnagy]

My thoughts on making the background execution of discovery.

The background execution for discovery is a must, because otherwise I would lock to main (UI) thread causing the UI freeze and also VS would kill our extension.

@Code-Grump said on Discord:

If we were talking in terms of tasks, I would expect to fire-and-forget the discovery (because we don't want to do heavy work on the main thread) by using Task.Run but have components interested in the discovered bindings await the task (because it may still be running.)

This is basically how it works. We trigger the discovery from DiscoveryService here as

_projectScope.IdeScope.FireAndForgetOnBackgroundThread(
            _ => BindingRegistryCache.Update(_ => DiscoveryInvoker.InvokeDiscoveryWithTimer()));

The IdeScope.FireAndForgetOnBackgroundThread is just an interface method to make testing easier, but the real implementation uses a Task.Start:

Task.Factory.StartNew(async () =>
    {
        try
        {
            ThreadHelper.ThrowIfOnUIThread(callerName);
            await action(_backgroundTaskTokenSource.Token);
        }
        catch (Exception e)
        {
            Logger.LogException(MonitoringService, e, $"Called from {callerName}");
        }
    },
    _backgroundTaskTokenSource.Token,
    TaskCreationOptions.LongRunning,
    TaskScheduler.Default
);

About waiting for the binding discovery result in UI commands

The binding discovery is performed whenever you build the project as it is working based on the compiled assemblies, once the feature file has been opened on that project (that initializes the system and performs the first discovery).

Once you build the discovery is finished within a few seconds.

Most of our UI commands are executed from the feature file, so normally the discovery has been anyway performed.

There are a few commands that start from the C# files, so there is a chance that no feature files were executed yet and therefore the discovery is not initialized yet. In that case we should initialize the discovery (this is done when you call project.GetDiscoveryService() like in here) and wait to finish with an await, like here.

        var discoveryService = project.GetDiscoveryService();
        var bindingRegistry = await discoveryService.BindingRegistryCache.GetLatest();

@clrudolphi said on Discord

The second issue is that when discovery IS performed, is launched on a background thread in a Fire&Forget manner. That thread of activity has not yet finished when the Command is looking for the result, and so it finds the original (Invalid) binding registry in the cache.

Based on the description above, this should not be a problem (maybe there is a bug though).

On extending these features to Reqnroll "lib" projects

Now I better remember the problem.

The problem with the Reqnroll "lib" projects (so literally class lib projects that do not contain feature files), that these projects do not have a Reqnroll configuration. To understand this, consider the following case:

• Reqnroll Lib A: defines a step argument transformation for type X
• Reqnroll Lib B: defines a step definition that uses X in the parameter, e.g. with Cucumber expression: I do something with {X}
• Reqnroll Test Project: has a configuration that lists both Lib A and Lib B as binding assembly and has a feature file that uses the step definition from Lib B

This is a fully valid setup. Once the tests are running, the binding discovery is performed for the Test Project, that loads step definitions and transformations from both Lib A and Lib B and run the step successfully.

But if you would consider Lib B alone, that is invalid, because it cannot understand {X} in the cucumber expression and it does not have a configuration that would tell that it depends on Lib A.

Of course this is a special case and in most of the cases, the binding discovery can also be performed for lib projects, but in this case it is hard to separate if the binding is invalid because the user did something wrong or it is invalid because it can only be used in a context of a test project. So we could run into an issue that we keep showing invalid errors for lib project discovery to the users although nothing is wrong.

So I don't know how we could support this easily.

What do you think?

[clrudolphi]

About the threading issues involved; I understand the need to avoid blocking the UI thread, but not sure that what we have in place is sufficient.
In the FindStepDefinitionsCommand class, this entire sequence is kicked off with the following line of code:

            var bindingRegistry = project.GetDiscoveryService().BindingRegistryCache;

The GetDiscoveryService() method initiates the discovery on the background thread, as you've described and returns (Fire&Forget). The line above then simply grabs the BindingRegistry as it exists (without waiting for an update to complete).

If that line were changed to:

            var bindingRegistry = await project.GetDiscoveryService().BindingRegistryCache.GetLatest();

would that be sufficient? Would that not also block the UI thread until completion of the ongoing Discovery? or does the fact that it is 'awaited' allow the UI thread to proceed while we wait for the Discovery?

BTW, this line of code is mine; introduced back in March 2024 when trying to solve the problem of the command failing the first time issued but succeeding subsequent times. Would that problem have been better solved if I had first check the BindingRegistryCache to see if it was invalid and if so (and only then) issue the above GetDiscoveryService() call?

[clrudolphi]

Regarding the complexity of supporting Find when we have partial discovery; I suggest we either:

• Create a mechanism by which Discovery is performed on the entire solution (and all commands run off of that), or
• Do a better job at identifying the edge cases and responding with better error messages to explain the limitations

[gasparnagy]

Regarding the complexity of supporting Find when we have partial discovery; I suggest we either:

• Create a mechanism by which Discovery is performed on the entire solution (and all commands run off of that), or
• Do a better job at identifying the edge cases and responding with better error messages to explain the limitations

I am absolutely agree to the second point, but I can't imagine how could we do the discovery for the entire solution. Do you have some concrete ideas for that?

[gasparnagy]

If that line were changed to:

            var bindingRegistry = await project.GetDiscoveryService().BindingRegistryCache.GetLatest();

would that be sufficient?

Good question, we would need to try.

As far as I remember, for the UI thread the await call works in a way that it gives back the UI thread to the general event processing loop (so won't freeze) and when the result arrives it schedules it back to the UI thread. So it won't block the UI, but maybe @Code-Grump knows this better.

[clrudolphi]

Regarding the complexity of supporting Find when we have partial discovery; I suggest we either:

• Create a mechanism by which Discovery is performed on the entire solution (and all commands run off of that), or
• Do a better job at identifying the edge cases and responding with better error messages to explain the limitations

I am absolutely agree to the second point, but I can't imagine how could we do the discovery for the entire solution. Do you have some concrete ideas for that?

In the FindUsages Command, after obtaining the binding registry, it iterates through all the projects of the solution, reads/parses the feature files and finds matches. In other words, we have the topology of the solution.

It would seem we have all we need to spider our way through the solution and ask each project for its binding registry; and then we would merge them. If we processed them in reverse topological sort order (of dependencies on each other), then might we be able to solve the thorny problem you identified earlier of a given StepDef pattern referencing an argument transformation defined elsewhere?

[clrudolphi]

Good question, we would need to try.

Ugh. Not sure how to do this.
This section of code is in the Command's PreExec method, which is an override of the virtual method defined in DeveroomEditorCommandBase. This code is old enough that it doesn't support async/await.

If I used:

            var bindingRegistry = Task.Run(
                 () => project.GetDiscoveryService().BindingRegistryCache.GetLatest()
             ).Wait();

would that be sufficient? I suspect not as the wait would block the thread, right?

Should we upgrade this Class to support async/await versions of PreExec and PostExec? Or would that not work well the vs ide?

[clrudolphi]

Good question, we would need to try.

Ugh. Not sure how to do this. This section of code is in the Command's PreExec method, which is an override of the virtual method defined in DeveroomEditorCommandBase. This code is old enough that it doesn't support async/await.

If I used:

            var bindingRegistry = Task.Run(
                 () => project.GetDiscoveryService().BindingRegistryCache.GetLatest()
             ).Wait();

would that be sufficient? I suspect not as the wait would block the thread, right?

Should we upgrade this Class to support async/await versions of PreExec and PostExec? Or would that not work well the vs ide?

Here is a different idea. What if the bulk of the PreExec method body were put inside a single Task.Run()? For example, here is the FindStepDef command method as it exists today:

    public override bool PreExec(IWpfTextView textView, DeveroomEditorCommandTargetKey commandKey,
        IntPtr inArgs = default)
    {
        Logger.LogVerbose("Find Step Definition Usages");

        var textBuffer = textView.TextBuffer;
        var fileName = GetEditorDocumentPath(textBuffer);
        var triggerPoint = textView.Caret.Position.BufferPosition;

        var project = IdeScope.GetProject(textBuffer);
        bool bindingsNotYetLoaded = false;
        bool projectNotYetLoaded = project == null;
        if (!projectNotYetLoaded)
        {
            Logger.LogVerbose("Find Step Definition Usages:PreExec: project loaded");
            var bindingRegistry = project.GetDiscoveryService().BindingRegistryCache;
            bindingsNotYetLoaded = (bindingRegistry == null || bindingRegistry.Value == ProjectBindingRegistry.Invalid);
            if (bindingsNotYetLoaded)
                Logger.LogVerbose($"Find Step Definition Usages: PreExec: binding registry not available: {(bindingRegistry == null ? "null" : "invalid")}");
        }

        if (project == null || !project.GetProjectSettings().IsReqnrollProject || bindingsNotYetLoaded )
        {
            IdeScope.Actions.ShowProblem(
                "Unable to find step definition usages: the project is not detected to be a Reqnroll project or it is not initialized yet.");
            return true;
        }

        var reqnrollTestProjects = IdeScope.GetProjectsWithFeatureFiles()
            .Where(p => p.GetProjectSettings().IsReqnrollTestProject)
            .ToArray();

        if (reqnrollTestProjects.Length == 0)
        {
            IdeScope.Actions.ShowProblem(
                "Unable to find step definition usages: could not find any Reqnroll project with feature files.");
            return true;
        }

        var asyncContextMenu = IdeScope.Actions.ShowAsyncContextMenu(PopupHeader);
        Task.Run(
            () => FindUsagesInProjectsAsync(reqnrollTestProjects, fileName, triggerPoint, asyncContextMenu,
                asyncContextMenu.CancellationToken), asyncContextMenu.CancellationToken);
        return true;
    }

Here is a revised version in which we invoke GetLatest() on the BindingRegistryCache() and the GetDiscoveryService() need not launch a new thread since everything is already off the UI thread.

    public override bool PreExec(IWpfTextView textView, DeveroomEditorCommandTargetKey commandKey,
        IntPtr inArgs = default)
    {
        Logger.LogVerbose("Find Step Definition Usages");

        var textBuffer = textView.TextBuffer;
        var fileName = GetEditorDocumentPath(textBuffer);
        var triggerPoint = textView.Caret.Position.BufferPosition;

        var project = IdeScope.GetProject(textBuffer);
        bool bindingsNotYetLoaded = false;
        bool projectNotYetLoaded = project == null;
        var asyncContextMenu = IdeScope.Actions.ShowAsyncContextMenu(PopupHeader);

        Task.Run(() => 
       {
       
        if (!projectNotYetLoaded)
        {
            Logger.LogVerbose("Find Step Definition Usages:PreExec: project loaded");
            var bindingRegistry = project.GetDiscoveryService().BindingRegistryCache.GetLatest();
            bindingsNotYetLoaded = (bindingRegistry == null || bindingRegistry.Value == ProjectBindingRegistry.Invalid);
            if (bindingsNotYetLoaded)
                Logger.LogVerbose($"Find Step Definition Usages: PreExec: binding registry not available: {(bindingRegistry == null ? "null" : "invalid")}");
        }

        if (project == null || !project.GetProjectSettings().IsReqnrollProject || bindingsNotYetLoaded )
        {
            IdeScope.Actions.ShowProblem(
                "Unable to find step definition usages: the project is not detected to be a Reqnroll project or it is not initialized yet.");
            return true;
        }

        var reqnrollTestProjects = IdeScope.GetProjectsWithFeatureFiles()
            .Where(p => p.GetProjectSettings().IsReqnrollTestProject)
            .ToArray();

        if (reqnrollTestProjects.Length == 0)
        {
            IdeScope.Actions.ShowProblem(
                "Unable to find step definition usages: could not find any Reqnroll project with feature files.");
            return true;
        }

           FindUsagesInProjectsAsync(reqnrollTestProjects, fileName, triggerPoint, asyncContextMenu,
                asyncContextMenu.CancellationToken):
    },
asyncContextMenu.CancellationToken);
        return true;
    }

[Code-Grump]

await isn't necessarily going to help us. It should be able to, provided the await is able to capture the synchronization context that is the UI thread and schedule the continuation back to the UI thread when the awaited task has completed. This should happen automatically but Visual Studio is a magical beastie and may have other ideas, so I'd want to test to be absolutely certain.

We absolutely cannot use Wait(): at best you will block the UI thread (making Visual Studio unresponsive until the task completes) at worst, you will deadlock the UI thread (because it gets stuck waiting for itself.)

There are two problems being presented (actual coding semantics aside.) Fundamentally this is a piece of lazy evaluation happening, so we need to:

1. Have a way of building the binding cache which will
   • evaluate when it is first requested
   • block callers until it is available
   • immediately return the value on future requests
2. Make sure the UI remains responsive and indicate that work is happening throughout the operation and prevent other requests racing with it, especially if it takes a noticeable amount of time the first time it happens.

So we probably want something that continues to create a task on the thread-pool using Task.Run, but we modify it so it's a lazy-created task that is assigned to a field, and then the task can be awaited by anything that requires it.

In essence a method in a class structured like this (types indicated are for the concept only and can be renamed):

    // A lazy field to ensure we only do the building once. It holds a task, because the building is asynchronous.
    private readonly Lazy<Task<ProjectBindingRegistry>> _lazyRegistryBuildTask;

    public DiscoveryService()
    {
        // The lazy initialization uses LazyThreadSafetyMode.ExecutionAndPublication to ensure work will be 
        // started only once. This uses locks to ensure thread safety, but because creating and scheduling 
        // a task is cheap, this will happen very quickly and the critical section is miniscule.
        _lazyRegistryBuildTask = new Lazy<Task<ProjectBindingRegistry>>(
            BuildBindingRegistryOnThreadPoolAsync,
            LazyThreadSafetyMode.ExecutionAndPublication);
    }

    // The public method which can be awaited. It unwraps the task from the Lazy field. 
    // If the task has not been started, this causes it to be created.
    public Task<ProjectBindingRegistry> GetBindingRegistryAsync() => _lazyRegistryBuildTask.Value;

    // The method which schedules the work to happen on the thread-pool.
    private Task<ProjectBindingRegistry> BuildBindingRegistryOnThreadPoolAsync() => Task.Run(BuildBindingRegistry);

    // The method that does the real work to build the registry. This runs exactly once on the thread pool.
    private ProjectBindingRegistry BuildBindingRegistry()
    {
        // Actual work done here.
    }

[clrudolphi]

I've placed the bulk of the FindStepDefinitionsCommand.PreExec method body inside of a Task. Before running the task, the ShowAsyncContextMenu() is called to cause the pop-up context menu to appear. The task then causes the results of the Find to populate into the context menu. The cancellation token of the popup context menu is passed to the task.

I modified one unit test to reflect that focuses on Not Supporting discovery on Non Reqnroll projects to allow for discovery on ReqnrollLibProjects.

Two items for further investigation:

1. The Discovery related Spec tests fail on my local machine, but passed during the github build. (weird)
2. Solve the scenario where external dependency assemblies rely upon one another.
3. Given the change to how Discovery works, I'd like to re-examine what we're doing with 'FindUnUsedStepDefinitions" when launched from external assembly projects.

Let me know your thoughts on these changes.

[gasparnagy]

I am happy with this so far.

For manual/exploratory testing:

• you can use https://github.com/reqnroll/Reqnroll.ExploratoryTestProjects/tree/main/CleanReqnrollProject, that has an additional binding project as well. Make sure you unload the Net462 project as that is not supported by out VS extension anyway.
• for UI lock issues, you can use the https://github.com/reqnroll/Reqnroll.ExploratoryTestProjects/tree/main/BigReqnrollProject but I also sometimes put some extra thread.sleep to the discovery processing code to simulate slow discovery processing

For auto tests:

• it is painful, but it is a good opportunity that it fails locally. you would need to look at that. maybe you can trace some of the flakyness