Add Test Adapter to the NUnit Engine solution

[ChrisMaddock]

Since @OsirisTerje added https://github.com/nunit/nunit3-vs-adapter/pull/759, I think we should be ready to do this now! This will hopefully make for an easier debugging experience.

We'll need to use a nightly build of the adapter, until 3.17 is released

[CharliePoole]

When you do this, can you think about making it easy to ignore for those who don't want to use it?

In fact, isn't it available to whomever wants it already, via the vsix, thus leaving the choice up to the individual developer?

[ChrisMaddock]

The VSIX's are no longer supported.

Can you clarify what you mean by 'easy to ignore', sorry? Is there a particular drawback you're expecting from the adapter being added?

[CharliePoole]

I'm reflecting back to when the NUnit analyzer was added. Since it never worked in my environment (since changed) I had to remove the reference in order to build and then remember to put it back before checkin. Consequently, I stopped contributing for a while.

As my vsix comment shows, I haven't used the adapter for many years, in spite of being the author of it a long time back. Could be I'm living in the past, still thinking of it as a sort of toy app for people with very simple usage. I've found it slows things down a lot and - at least the last time I checked - spontaneously starts discoveries when I don't want them to happen. AFAIK there's no switch to turn it off once installed in a project.

I'm happy (enough) to wait and see, however.

[ChrisMaddock]

I'm reflecting back to when the NUnit analyzer was added.

I'm still disappointed we never managed to figure that problem out. Incidentally, I came across a case today where they'd be really helpful (https://github.com/nunit/nunit.analyzers/issues/247) - I'd love to get them back in the solution, especially now the projects steaming ahead.

Understand your speed concerns. I'm hoping the adapter is a little more capable these days - especially as it's been the primary way of running NUnit .NET Core tests for the last few years!

If I put a PR up at some point, would you be happy to try it out, before we merge? Possibly with both packages at the same time? I'd like to work towards a better 'default' experience for new contributors - and both of these things would contribute towards that, if we can bring them in in a way that aren't detrimental to more experienced committers.

[CharliePoole]

Funny! I didn't realize the analyzer had been removed... thought it was my new environment that changed things for me. I just figured the old enviornment (Windows 7 running on VirtualBox) was one you didn't want to support.

I do think it's important to not make "helpful" additions blockers for contributors working on low-powered machines, etc. IMO as little as possible should be required and unfortunately, when you add a package, it becomes required for everyone who builds.

Of course, I'll try it if you put it up.

[mikkelbu]

@CharliePoole As far as I can remember you were the one that removed them :). Edit: it happened in nunit/nunit-console#690.

[CharliePoole]

No surprise there! See my earlier comment about having to repeatedly remove it to do work and then make sure not to check in that way. :cry: Too bad nobody caught it at the time.

In any case, it illustrates the problem that can arise when somebody can't use a particular tool that is installed. I'm generally in favor of leaving tools up to the individual developer as much as possible. Dotnet global tools make this possible but for .NET Framework and VS, the older vsix approach was the only way to do it, at least as far as I know.

[OsirisTerje]

@CharliePoole Toy app ??? :-( >38 million downloads... used by more than 50000 other github repos, works with VS all versions, with dotnet (.net core), used in Jetbrains Rider and in Resharper. Me think not ;-)

Seriously, it works on all relevant platforms now. So I don't understand why there should be any problem now. Also, it doesn't prevent you from using NUnit.Console.
If it does interfere with anything, we should fix that.

About your comment that it "start discovery" without you doing anything, - no - it doesn't. Real Time Discovery in Visual Studio does that though, but that is pretty recent. The Test Explorer sometimes live a life of its own, but the adapter is not involved. And that can be checked using the Dumps (which I of course have done ;-) ) .

And, @ChrisMaddock , 3.17 was released today :-)

[ChrisMaddock]

Great, thanks Terje! It's on the list. 🙂

[CharliePoole]

@OsirisTerje Sounds like I hit a nerve! Sorry about that. I didn't intend to offend you.

In context, I was writing about a piece of code I wrote myself many many years ago, before you were around, so I felt free to make fun of it and of myself.

I threw together the first adapter in record time as a lark to make some guys at Microsoft happy. I was pleased with myself for getting it done of course, but I didn't think it would amount to anything because, after all, who wants to run tests in Visual Studio? Well, it turns out lots of people did or at least MS was able to convince them that they did.

For the record, I also thought that nobody would ever want to buy books online, rather than going to an actual bookstore!

IOW, my comment was meant to be making fun of myself, not criticizing you of all people!

[CharliePoole]

Main point I want to make is that by adding any tool to the build through a nuget package, you may be helping some present and future contributors while, at the same time, actually forcing other contributors to use that tool, whether they want to or not. That doesn't seem like a good thing to me.

I'm not saying you can't ever do that but I am saying you should not do it without at least acknowledging that some people may not like it, explicitly weighing the perceived benefits against the potential harm and giving contributors an out if they don't want to install it. IMO we should not be telling people what tools to use.

Of course, in this case, we already have a de facto lock-in to Visual Studio for building the console and engine. Clearly, requiring the adapter will harden that lock-in, even as we continue to sport a vision statement in our governance repository that claims we don't favor any particular IDE.

The other fear I have is that once the adapter is added there will be little motivation to improve the engine and runner to allow .NET Core tests to be run without it. Obviously, that's a question of project direction, which is more in Chris' wheelhouse, but I think it ought to be considered.

[ChrisMaddock]

Main point I want to make is that by adding any tool to the build through a nuget package, you may be helping some present and future contributors while, at the same time, actually forcing other contributors to use that tool, whether they want to or not. That doesn't seem like a good thing to me.

I disagree that anybody is being forced into anything here. Existing test running methods are remaining unchanged. If an existing contributor doesn't want to run tests in VS, then they have the test explorer window shut, and this change will be invisible to them.

That said, for a new contributor who does regularly use the test explorer, they get an much improved experience by having the tests automatically show up in the conventional place, all ready to run. No more trawling through markdown files and build scripts to figure out how to run the tests, it's just all there and ready to go.

Of course, in this case, we already have a de facto lock-in to Visual Studio for building the console and engine.

I didn't think this was the case. As far as I'm aware, the solution builds fine in all modern IDEs, e.g. VS, Rider, VS Code.

The other fear I have is that once the adapter is added there will be little motivation to improve the engine and runner to allow .NET Core tests to be run without it. Obviously, that's a question of project direction, which is more in Chris' wheelhouse, but I think it ought to be considered.

Interesting point! I do think the .NET Core console runner will never be that popular, mainly due to the fact that the adapter provided a command line interface via dotnet test several years earlier than we've managed to get a .NET Core Console off the ground. "Time to market" is a tricky thing in open source, eh?

In my eyes, the console and the adapter serve different purposes. Both are great tools, in have strengths in different areas.

[CharliePoole]

OK... thanks for listening.

[CharliePoole]

@nunit/engine-team Reviewing this, I can see that I may not have understood the issue. Is this talking about making the adapter part of the distributed console package? Or is it only talking about making it easier for those building the console runner to use the engine for testing?

If it's the former, my strenuous objections remain. If it's the latter, I object less, although I'd still encourage anyone on the team to continue to dogfood the console runner.

[mikkelbu]

I'm quite sure that Chris meant the latter - or at least that it is how I read it. New contributors - both in this project and in the frameworks - often find it odd that they cannot easily run the tests and debug them in VS.

[CharliePoole]

That's why I asked. If you read the comments, my objections were very strong because I was assuming the former. I'm OK with having it available but I want the package tests to keep using the console runner. Not sure if package tests are in master yet - I'll have to check that eventually.

[CharliePoole]

@nunit/engine-team @OsirisTerje

So I have added this to the 3.16 milestone and just started working on it. I was quickly stopped by the realization that, by definition, no released version of the adapter use the version of the engine I'm currently building. I'd appreciate input from those who were in favor of this change. Since this is intended to help team members and contributors, what did you have in mind as a way to prevent conflicts between the version of the engine referenced by the adapter and the version being built?

Labeling this as blocked until I have a better understanding of the intent.

[CharliePoole]

@nunit/engine-team @OsirisTerje I haven't heard from anything and don't know what people are expecting from this issue. It seems to me that installing the adapter would also bring in conflicting versions of the engine. Until somebody is able to clarify this, I'm taking it out of the milestone.

[OsirisTerje]

@CharliePoole Sorry saw this just now. The point about how to handle possible conflicts between the adapter's embedded engine and engine-under-work is a good point, and not sure how that should be handled. It may or may not give issues, I believe it all depends. The engine-under-work would be overwriting the embedded engine, so the test would both be working with the engine-under-work and using the engine-under-work. That is a bit like what happens for the testing of the adapter itself, and it has worked out - not 100% but in most cases. When it doesnt, it is a kind of circularity that doesnt resolve itself until you get the correct version in all the places. When that happens, the testing is not doing anything good at all, but as said, it has happened pretty rarely.

[CharliePoole]

Treating this as a future idea