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

[VisualStudio] Detect existence of SpecFlow extension by checking the loaded assemblies and show a warning #16

Closed gasparnagy closed 1 month ago

clrudolphi commented 1 month ago

@gasparnagy I'll take a look into this. When should the detection take place- during the installation of the Reqnroll extension or during the startup of the extension? (or both?) Is there a likelihood of a false negative by looking at loaded assemblies? What if the Reqnroll extension checks this during startup of the extension but the SpecFlow plugin hasn't been loaded yet? How would you like the warning to be worded?

gasparnagy commented 1 month ago

@clrudolphi Thx! Some hints:

The Reqnroll VS extension was forked from the SpecFlow.VS extension that is basically the takeover of the [Deveroom]() extension that I did many years ago to replace the original SpecFlow extension. 😎

When the original VS extension -> Deveroom change was introduced, we had the same problem, that I have solved with the two classes you can find here: https://github.com/specsolutions/deveroom-visualstudio/tree/master/Deveroom.VisualStudio/SpecFlowVsCompatibility

I don't remember the code too much, but as far as I can see, this implements the check (here) by checking the assemblies loaded to the VS process.

This is invoked in two ways:

  1. There is a "fake" classifier registered for the "gherkin" files (in our extension this is now [ContentType(VsContentTypes.FeatureFile)]), so it is checked every time a feature file is opened.
  2. It is also invoked by the DevoomTaggerProvider here, which is when the extension asks our extension to process the feature file. So essentially also when a feature file is opened.

I have no idea why we needed both. Maybe the problem with option 1 was that we could not control whether the DeveroomTaggerProvider or the fake classifier is invoked first. But in our case maybe it does not matter anyway and we can just keep one of the two.

I'm not sure if this is the best option to do the check, but at least hopefully this can be implemented quickly, so let's see if it can be used.

clrudolphi commented 1 month ago
I always stumble across some tidbit of intrigue and history when working on this... If I understand correctly, the Reqnroll.VisualStudio extension is the 4th generation of such extensions. Do I also understand correctly that you previously only checked for overlapping extensions when replacing the first generation? Why not the other overlaps? Should this be implemented as a family of compatibility checkers and we run them all? Or perhaps it's sufficient to run only the new one we want added? (and remove the other) If we want to run them all, should they all be registered under a parent Interface and then have the collection of them injected into the TaggerProvider and Gherkin classifier?
gasparnagy commented 1 month ago

I always stumble across some tidbit of intrigue and history when working on this...

Yeah. Sorry for that. But at least I am here as a historian... :)

the Reqnroll.VisualStudio extension is the 4th generation of such extensions

😀 Correct. If we ever get to a pub somewhere, I will tell you the full story while having a 🍺... or perhaps two... 🍻

Why not the other overlaps?

The 3rd gen worked only on VS2022, while the 2nd on VS2015 & VS2017, so there was no chance of collision. The 3rd to 4th is what we do now.

Should this be implemented as a family of compatibility checkers and we run them all? Or perhaps it's sufficient to run only the new one we want added?

I hope this is going to be the single last one, so this should be a one-time thing.

In principle I can imaging some incompatibilities with other non-bdd plugins (e.g. it does not work with ReSharper), but so far it never happened so I would not prepare too much for it. Once it happens, we will find a way.

clrudolphi commented 1 month ago

@gasparnagy I'm going to need more help from you. I suspect that my install is corrupt, but I'll walk you through what I'm seeing.

I introduced changes similar to what you had done with the earlier SpecFlowVsCompatibilityService. My branch and commit of these changes are here. I've named it slightly differently: as the SpecFlowExtensionDetectionService. I have added the fake ClassifierProvider and also modified the constructor of the DevroomTaggerProvider to call it. This is all in alignment with what you did last time. The major exception being that the detectionService is 'newed up' within the constructor of the DevroomTaggerProvider rather than having it injected. I did this as a short-term step to temporarily avoid having to modify all the test infrastructure that would break if the signature of that constructor changes. (Read below about the problems and let me know if perhaps that short-cut caused these problems?)

My experimental instance has both SpecFlow and Reqnroll extensions installed. When the experimental instance is first launched, VS reports an error (in the ribbon up top). See two screenshots attached, the first shows an error dialog related to SpecFlow VisualStudio, and one for Reqnroll. Screenshot 2024-06-12 145908 Screenshot 2024-06-12 145950

There was a link in that ribbon which I clicked and the VS error log is attached Activity Monitor Log.pdf

From what I can gather, both SF.VS and RnR.VS both register MEF components using the same IDs.

After clicking OK to get out of those error dialogs and opening the sample SpecFlow solution, my detection code runs and properly detects the presence of the SpecFlow.VisualStudio extension. When the ShowProblem method is called, there is another exception thrown (InvalidOperationException) as it seems the VSIdeScopeLoader is getting called recursively to instantiate the Lazy. See attached System.InvalidOperationException.txt stack trace.

What would you like me to change or try?

smritig29 commented 1 month ago

I tried setting this up for a quick demo and I get the same warning. I had previously used SpecFlow and have that extension as well. But I couldn't get past this warning even after updating Visual Studio 2022 to the latest version.

gasparnagy commented 1 month ago

Side note: If you feel that your VS experimental instance is totally wrong, you can "reset" is. I have updated the CONTRIBUTION.md with the details how this can be done.

clrudolphi commented 1 month ago

Tried the reset of the Experimental instance. after doing so, went back in to the Exp instance and re-enabled SpecFlow.VS. Then quit and restarted it to confirm that SpecFlow.VS was working properly. Then attempted to debug the Reqnroll.VS extension. Got errors about one of the Connector.Models not being built in Debug, so I quit, went out to the command line and used the build.ps1 script in the Connectors folder to rebuild the connectors in Debug. Attempted debug again, and got the same set of errors as before about conflicting MEF components (see screenshots above).

At this point, I'm stuck. Do we have anyone successfully loading both VS extensions in one session? Otherwise, this may have to be handled via documentation/instructions to uninstall SF.VS before installing Rnr.VS.

gasparnagy commented 1 month ago

@clrudolphi The documentation already states that the SpecFlow extension has to disabled (https://docs.reqnroll.net/latest/installation/setup-ide.html#setup-vs), but people will probably not read this, that's why it would be good to handle the situation in a nicer way.

I don't think I tried myself enabling both, but I have seen someone who had both enabled and they haven't got such errors. But maybe there were some other special context there.

I can check it tomorrow and see. Maybe I can get some more hints on how to move on.

gasparnagy commented 1 month ago

@clrudolphi I have figured out the root causes of these issues and fixed them on #25 (this does not contain the warning you started to work on, but just fixes these core errors).

I have also figured out how to diagnose these problems and documented here: https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/CONTRIBUTION.md#diagnosing-mef-composition-errors

At the end the problem was that the extension exposed implementation for common interfaces, like IFileSystem and that conflicted with the other plugin which did the same.

Please give a review for #25 and approve it if you are OK. (You can also "Squash and Merge" is.)

gasparnagy commented 1 month ago

@clrudolphi PR #25 merged, so you can retry now with that.

gasparnagy commented 1 month ago

Fixed by #26