Split engine into upper and lower parts

[CharliePoole]

I did PR #580 "Split engine spike" as a spike, i.e. code to prove a concept, intended to be discarded and re-developed more carefully under test. Based on the spike, I'm writing this issue as a proposal to go ahead and move to the actual implementation. Comments on the spike already indicated a general consensus that this would be a desirable evolution of the engine. Before starting to code for real I'd like to have some review of what I propose here.

PROPOSAL

Split the current engine assembly nto two parts, a lower and an upper part, with the upper part making use of the lower part. The upper and lower parts would be built for potentially different targets - more for the lower part than the upper.

I will continue to call the upper part the engine. It needs to be built for platforms on which developers actually edit, compile and test their programs. Initially, I'd retain .NET 2.0 and .NET Core 2.0 for this purpose.

I'm calling the lower part the "agent core" for the moment. It needs to be built for any targets on which we want the tests to run, i.e. all current targets. If, however, we are about to remove any existing targets, it would be convenient to do it before starting this work.

APPROACH

My preference is to do this as a series of PRs, including tests, so that the change takes place in smaller, easier to review steps. If preferred, I can use a "big bang" approach, however.

REVIEW

I'm looking for a review of the general direction before starting work. Obviously, the details will be looked at again in code review but I'd like to know that this is wanted before spending time on it.

[ChrisMaddock]

I agree the general direction covers where we want to go, and I'm glad to hear you're willing to contribute this work to this project, rather than just have this change in a fork.

I think we need to consider the name for the "engine lower part" from the point of view of new codebase contributors - "agent core" suggests to me something which is specifically tied to the engine. Something like "Engine services" or "common services" was suggested before, which I personally prefer.

On the approach - I'd be very much in favour of smaller PRs.

One last consideration you mentioned on email Charlie, this change has the potential to cause merge conflicts with @jnm2's current work on replacing the engine/agent communication protocol. Given Joseph's picked that up again just last week - I'd prefer we try and avoid merge conflicts with that work as far as possible - I think it's currently blocked awaiting review of #683, which I'm afraid I've had limited time to look at recently. I expect some of the early incremental PR's can still be completed with limited conflict?

[jnm2]

@ChrisMaddock My plan was to start from scratch and copy pieces from my second spike into one or more small PRs because there was too much noise in the branch's current history around standard IO streams. Does that seem like a good way to handle it? I'm not sure, should I close the open PR since I intend to replace all the commits anyway?

[CharliePoole]

Here are the file counts (*.cs only) for the key assemblies in my spike.

Assembly	Files
nunit.engine.api	43
nunit.engine	33
nunit.engine.core	65

While not definitive, there's some indication here that it might be worth splitting the lower part (nunit.engine.core) into two components.

Here is an approximate breakdown of the 65 files in in nunit.engine.core.

Function	Files
Agents, Runners and Drivers	20
Services and extensibility	20
Internal and Miscellaneous	25

The first group is essential to the purpose of the engine core or agent part of the split. Most of the services, internal and miscellaneous stuff would probably fit better in a third assembly, referenced by both the upper and lower parts of the engine. This could be adjusted in the course of implementation, with the general goal being to keep services and other classes used by both parts of the engine in that third assembly.

Thoughts?

[CharliePoole]

Taking advantage of this issue to install a new Windows 10 VM to work on. Hopefully, that will allow me to work on the console without local patches as I have to do under Windows 7.

[CharliePoole]

@mikkelbu @ChrisMaddock Well, that didn't work so well. I'm still unable to use the analyzer under either Windows 7 (which I suspected) or a new install of Windows 10. The only thing I can think of is that running on a machine under VirtualBox is somehow causing the problem. Have either of you done that?

Anyway, my Windows 10 machine is taking twice as long to build as Windows 7, so for now I'm going back to 7, since both require the same mod for me to build successfully.

[CharliePoole]

I called this a "feature" rather than a "build" change. It seems much more significant than just how we build the engine.

[CharliePoole]

@nunit/engine-team In order to get started, I need to pick some names. They can be changed in code review, of course, as well as at any point before a release is done. Here's what I'm using for now:

Assembly	Function
nunit.engine	The "upper" part of the engine.
nunit.engine.agents	The "lower" part of the engine
nunit.engine.core	Shared functionality, used by both parts
nunit.engine.api	The existing (unchanged for now) API assembly

Notes:

1. Both parts of the engine contain runners. The lower part contains runners that run under the control of agents. The upper part contains higher level runners like ProcessRunner and MultipleProcessRunner`.

2. The shared (core) assembly will contain core services used by both parts like ExtensionService and services only used by agents and lower level runners like the DriverService.

3. As we proceed with dividing the assemblies, we will need to review the API assembly to ensure that it only publishes interfaces for accessible services, runners and other features. Alternatively, to avoid breaking changes, we may need to pass through some functions from the lower level to the upper (i.e. client) level.

Hopefully this makes sense. Also hopefully, it will make more sense as the code is produced.

[jnm2]

I'm pretty sure that the Windows 7/10 thing has to be unrelated. There's nothing technologically original or special about Roslyn analyzers that would differ from the rest of the Roslyn compiler itself, and I've long used such analyzers on both Windows 7 and Windows 10. I would guess the same about VirtualBox.

[CharliePoole]

@nunit/engine-team In order to get started, I need to pick some names. They can be changed in code review, of course, as well as at any point before a release is done. Here's what I'm using for now:

Assembly	Function
nunit.engine	The "upper" part of the engine.
nunit.engine.agents	The "lower" part of the engine
nunit.engine.core	Shared functionality, used by both parts
nunit.engine.api	The existing (unchanged for now) API assembly

Notes:

1. Both parts of the engine contain runners. The lower part contains runners that run under the control of agents. The upper part contains higher level runners like ProcessRunner and MultipleProcessRunner`.

2. The shared (core) assembly will contain core services used by both parts like ExtensionService and services only used by agents and lower level runners like the DriverService.

3. As we proceed with dividing the assemblies, we will need to review the API assembly to ensure that it only publishes interfaces for accessible services, runners and other features. Alternatively, to avoid breaking changes, we may need to pass through some functions from the lower level to the upper (i.e. client) level.

Hopefully this makes sense. Also hopefully, it will make more sense as the code is produced.

[ChrisMaddock]

Looks good. I'm wasn't expecting there to be 4 separate assemblies, but you've investigated this a lot more than I have, so I trust your judgement here.

Point three is (unfortunately) important - ideally the API assembly would protect us from this, but unfortunately we're not quite there yet. I think we can also use this refactor as a good step towards bringing more internal-intended things internal, and then having a more active clean up in v4.

---

@jnm2 Small PR's sounds brilliant. My only concern here is that yours and Charlie's work doesn't clash too much. It sounds like both can be broken down into sufficiently incremental steps we might be able to avoid the worst of that. 🙂

[CharliePoole]

As we talked about (I think) elsewhere, it's possible to do it with three, i.e. just adding one more "lower" assembly. But that means the "upper" assembly has to make use of the "lower" for two different purposes:

1. Telling it to go and run tests using a certain framework, etc.

2. Making use of some shared types for it's own purposes.

This feels a bit messy to me, but I could be convinced to go either way.

[ChrisMaddock]

@CharliePoole what you say makes sense. It wasn't what I was expecting the codebase to look like, but I reckon it will become clearer to me once it's in place. 😄

[CharliePoole]

Thought experiment I'm using...

Imagine an engine version that does not allow running in process. Such an engine will reference nunit.engine.core, of course, but it should not need nunit.engine.agents.

When I'm in doubt about where to put something, I'm using that criterion to decide.