Eliminate In-Process Execution

[CharliePoole]

Allowing the user to select in-process execution gives no specific benefit although it has the side-effect of making debugging easier. We have discussed removing it in version 4.0. This issue is created as a place for the @nunit/engine-team to discuss the two options I see for this change. I've listed some notes on each option.

Option 1 - Just eliminate In-process execution

1. This is a test runner, which has to deal with code that may behave badly. It's really not reasonable to expect the runner to survive and report all bad behavior by the tests if the tests are running in-process.
2. The test itself benefits by being isolated in a separate process.
3. The change would simplify the engine because it no longer requires verifying that a particular assembly is capable of running in the same process as the runner, based on its target runtime.
4. The main use of in-process is to allow easier debugging, so loosing the option will make debugging something we need to focus on. There is already a way to debug the tests inside the agent process, which we should work to improve.

Option 2 - Eliminate the ProcessModel setting entirely

1. If we drop in-process execution, then all tests run in separate assemblies. The only remaining issue is whether multiple assemblies run in the same process or in separate processes. It's arguable that there is no reason for running in one process. That setting is only available for historical reasons: it was developed before we developed multiple process execution.
2. As with AppDomains, the user has no need to control how the runner allocates assemblies to processes.
3. Multiple Process execution (one process per assembly) is now the default. It's how the engine actually works best.
4. We could simply present Agents (rather than processes) to the user as the main conceptual entity used in running tests. The runner can decide in many cases what those agents are - processes, separate machines, etc.

[ChrisMaddock]

I'm rather attached to --inprocess, but like you say - almost entirely for debugging reasons, which isn't a good reason to keep a feature! I so almost wonder if it's worth considering how much complexity is added by retaining the ability to run tests in process, purely for debugging reasons.

I'm not sure I fully understand the history enough here to make a sensible decision. Why did the project originally give users such a range of choices over how to isolate their test assemblies? Is there a sensible requirement for being able to run inprocess and/or multiple test assemblies in a single agent process? Would be interested in other people's opinions here.

[ChrisMaddock]

There's a good point raised on TestCentric/testcentric-gui#444. How do runners like the VS adapter, which are already launched in a test-assembly-specific-process, cope with the removal of inprocess? Do then end up launching a process to launch a process to launch a third process? Sounds like unnecessary overhead, especially to click and run a single test. Maybe inprocess has a specific performance-based usecase for fast-feedback GUI runners, but we should consider limiting use through e.g. the console runner.

[CharliePoole]

@ChrisMaddock Good question about the adapter. I think we need @OsirisTerje involved in this discussion.

Do we really know that the process launched by VS is specific to the test assembly? I know it was not that way originally. Frankly, if I were writing an adapter today, I'm not sure I'd use the engine at all. If I did, I'd probably link it directly and come into it at a low level that bypasses the top levels of the engine.

That said, Terje is working on his own V4.0, so maybe we should coordinate. 😄

[CharliePoole]

I think the project (at the time I) just gave all those options because it was possible, without too much consideration of maintenance or sustainability. 😕 In fairness, running in a separate process was initially experimental and later multiple processes was an experimental feature and even later parallel multiple processes. At each step, we just kept the old options as well.

Here's how I'm now thinking. You want to run a test. You need to run it somewhere. That somewhere is an agent, which is most likely a process but as a user you don't care. You just want an agent with some capabilities, like being able to run .Net Core 3.1 tests.

As now implemented, btw, we don't start a new process each time you select a single test. The entire assembly is loaded in a process with which the runner communicates. Currently, you only need to reload if the number of agents is less than the number of assemblies but even that is something I think we could fix.

[OsirisTerje]

It is the vstest host that controls this, the adapter grabs some information and forwards that to the engine/framework. Whether removing inprocess may harm that, I am unsure - I do see that we have datacollectors that might need inprocess - if I get that right. I think someone from MS should answer on this part - e.g. @kendrahavens.

About V4 and if the adapter should call lower levels: Yes, it would be cool - the V4 now have a separate internal engine adapter, so it can handle changes in how the adapter interacts with the engine without affecting the rest of adapter code - to a certain degree - there are at least some more separation there now.

[ChrisMaddock]

As now implemented, btw, we don't start a new process each time you select a single test. The entire assembly is loaded in a process with which the runner communicates.

This is accurate for how the engine was intended to be used, and how runners like TestCentric use it, but I don't believe it is for runners like the VS adapter (in GUI form) and ReSharper, which load the assembly for each individual user-click, AFAIK?

Of course we can argue that both pieces of software should be using the engine as intended - but I think there's a balance to be struck in terms of considering the constraints of two of the most popular runners in the ecosystem. 🙂

[CharliePoole]

@OsirisTerje IIRC you load the engine assembly itself, which is part of the adapter package, so you have access to classes within the engine beyond the published API. That would seem to mean that you can run tests any way you choose, so long as we keep the Types and Methods you need public.

That said, and as I mentioned in another note, it seems to me that the entire engine is more than you really need. Maybe you should just be using the engine core or an adapter you create over it. The main purpose of the full engine is to decide where tests will be run and I think that VS really makes that decision for you anyway.

[ChrisMaddock]

I'm not sure I agree with that approach sorry, unless we create another supported interface at a lower level of the engine. Terje and I have both lost time over the years to strange issues caused by the adapter integrating with the engine at a lower level than the main API. For v4, I was hoping we would make all other methods internal, and only allow integrating through the main API.

[CharliePoole]

@ChrisMaddock

Not 100% sure what "that approach" refers to, but if it relates to using the core directly, I think there are things there that need to be exposed in an interface that we support. I'm not sure how you would write a pluggable agent, for example, without that.

Regarding "strange issues" I guess that's after my time on the adapter, so I defer to the two of you. Some of that stuff I know came from doing special things for Microsoft's benefit.

But what's the outcome of this comment? Do we mark the issue of eliminating in-process execution as blocked? That would be too bad, IMO.

[ChrisMaddock]

Not 100% sure what "that approach" refers to, but if it relates to using the core directly,

Yes, it did sorry. Exposing additional things through the interface seems like just the write approach to me however, and I'd expect something like pluggable agents to involve that. 🙂 I'm just keen we have a single supported interface, which we can maintain

But what's the outcome of this comment? Do we mark the issue of eliminating in-process execution as blocked? That would be too bad, IMO.

I think we need to ensure a few things committing to this:

1. The Adapter can function while running out of process.
2. Other major runners (e.g. ReShaper) won't be impacted.
3. We have a solution to continue easily debugging the engine.

On the other hand, would pluggable agents potentially give us a clean solution to running inprocess, so we could maintain the functionality, but remove the complexity? We mentioned that briefly on the TestCentric PR.

[CharliePoole]

I'm satisfied personally WRT 1 and 3. What does it take to resolve them from your point of view?

WRT 2, we have no idea how ReSharper works. They are not a company that shares information readily. And the adapter is called the Visual Studio adapter. So again, how will we resolve this without actually doing it?

[ChrisMaddock]

My thinking:

VS Adapter: We test it. Should hopefully be as simple as removing the InProcess package setting
ReSharper: We try it, and see if it currently launches nunit-agent or not. From memory I think it might do, but I think we should at least check, before making a major breaking change to two of the most widely used runners. 🙂

[ChrisMaddock]

Would you mind documenting the debugging solution? I've always found trying to debug things running in the agent process really awkward...but maybe I'm doing it wrong. 🙁

[CharliePoole]

@ChrisMaddock

The nunit3 framework supports debugging based on a package setting. You can see the code here.

The console runner accepts a --debug option. When it is used, a setting of DebugTests is added to the package. In addition, --workers is set to 0 if it was previously defaulted. This allows easy debugging of normal cases while permitting the user to also set --workers to a specific value in order to debug parallel execution.

The framework reacts to the setting by initiating a break when the test assembly is about to begin execution.

The only glitch I have noticed is that we may no longer be setting the correct mono option to do this under mono, but that's minor.

[ChrisMaddock]

I think I'd have to try it again to be able to give a fair judgement. My memory from using it previously was that it was a lot slower and more inconvenient than being able to set breakpoints and run inprocess directly in VS, but maybe there's a better way in later versions. Does it still launch a whole new version of VS for the agent debugger?

Again, interested in other @nunit/engine-team member's thoughts there. I'm of course an amateur developer - maybe the more experienced are more comfortable with this process! 😄 I am generally against the idea of code in production just to assist with debugging and testing - but this one in particular I'm unsure whether it's overall beneficial.

[ChrisMaddock]

Charlie there's one point we've missed:

On the other hand, would pluggable agents potentially give us a clean solution to running inprocess, so we could maintain the functionality, but remove the complexity?

This seems like it could give us the best of both worlds? Has your spike got far enough along to make any judgement on this yet?

[CharliePoole]

It's the standard debugger break dialog, which asks you if you want to use an existing instance of VS or start a new one. At that point, you set any breakpoints you want and continue. I agree it's less convenient than setting the breakpoints before running.

Yes, I do think that pluggable agents can generalize to supply a better way of handling this. Personally though, I'd opt to remove in-process execution and rely on the existing debug facility until we create an improved one.

It may sound perverse, but I think we'd do a better job on coming up with an improved approach if we were handicapped by the loss of inprocess execution. 😈

OTOH I could be a bad model here. I spent about the first 20 years of my career without such a thing as an IDE.

[CharliePoole]

Regarding the spike... I set it aside to focus on breaking changes. Catch 22 maybe?

[ChrisMaddock]

It may sound perverse, but I think we'd do a better job on coming up with an improved approach if we were handicapped by the loss of inprocess execution. 😈

I'd buy into this in a professional environment. In a voluntary environment, I worry more it might just discourage people from contributing entirely!

I spent about the first 20 years of my career without such a thing as an IDE.

Haha, yes - and I've never worked well without one! I would like to hear other people's opinions on this - I'm aware my own experience is limited.

Regarding the spike... I set it aside to focus on breaking changes. Catch 22 maybe?

Here's my thinking - on the overall v4.0 roadmap, that spike is key to deciding what we do with .NET 2.0. I think we agree that pluggable agents be the "nice" solution to removing the .NET 2.0 agent from the main solution, but that's dependent on how big an issue it turns out to be. My gut feeling is that it's more achievable that other things on the radar - e.g. .NET Core agents.

Given that, I think the spike is a pre-requisite for v4.0. Given that I think we could afford to defer the decision on this one, until we have an idea of whether pluggable agents will give us a clean way to refactor this feature - which could mean the best of both worlds?

As I understand it, the main advantage of this ticket would be (vast) code simplification. If we can do that whilst retaining easy-debugging, that seems like the best of both worlds to me? What do you think?

[CharliePoole]

Yes, I think that's a nice summary of the situation. Let me think a bit about how to do that spike without disrupting either the engine or the GUI.

[CharliePoole]

@ChrisMaddock @mikkelbu @rprouse @jnm2

Chris is right that the "pluggable agent" issue is central to a v4.0 release. It's also what I called it: a catch 22. That's because eliminating native in-process execution has become, to some extent, dependent on allowing custom agents to run in-process.Meanwhile, creation of the pluggable agent capability would be easier if we (initially) removed in-process execution.

Hence the spike: we'd like to know that we can do both things before we commit to either of them. I won't be doing the spike as part of the engine project, but I'll report on it here and link to the code.

The code itself will use the GUI because the gui already has pluggable agents - but they are only process agents at this point. My approach will be to attempt to create an in-process pluggable agent.

To do this, some design changes are needed - the same changes actually in both the GUI and the nunit engine. I'll outline those here as a separate issue within the next few days.

[CharliePoole]

This is ready to merge after pre-requisite changes in issues under #1102 are merged.