Split engine into two assemblies

[CharliePoole]

Fixes #684

This is an alternative approach to issue #684, as compared to that of PR #689.

I've brought this up to the same stage of (partial) completion as #689 so they can be compared. There's lots more to do and I'd like to pick one approach before continuing.

As explained elsewhere, I think this PR, which creates only a single new assembly, is to be preferred at the moment. Starting from scratch, I might do it otherwise, but we're pretty far from that. Creating a single new assembly leaves the door open for further splitting if it should be called for.

Both this PR and #689 only deal with splitting the engine, which is still referenced as if it were a single entity. Once the split is complete, then more work is needed to actually use the lower part by itself in the agent assemblies.

I had hoped this could be done as a set of small PRs. But that would require reviewing one bit of it without knowing what comes next. I'm happy to merge either this or #689 as a whole or up to a certain point. Both PRs are probably best reviewed one commit at a time.

FYI @nunit/engine-team - I'll be spending tomorrow flying back to the US and probably won't get back to this until the day after. Let me know which PR you think I should continue.

[ChrisMaddock]

Hi Charlie - I've had a quick look through both, and long-story-short, I agree. The division in the first version is unclear to me, even as someone who's familiar with the codebase. I'm sure that would start to make sense if I started to pull things apart and learnt more about the inter-dependencies, but second version feels much more approachable to me.

I had hoped this could be done as a set of small PRs. But that would require reviewing one bit of it without knowing what comes next

I prefer working like that myself, if it's possible to cover a quick 'what and why' in the PR description. Whatever works best, though.

[CharliePoole]

@ChrisMaddock I'll continue in this way - splitting into two assemblies - and I'll eventually delete the old ones (both #689 and #580).

Regarding small PRs, let's use this one as a roadmap. I've added @jnm2 to the requested reviewers. @jnm2 can you figure out where in the series of commits this starts interfering with what you are doing? It's trivial to break this big PR up if we want.

Meanwhile, I'm continuing in this path, using some of the jet-lag energy I have after flying home yesterday.

[CharliePoole]

This is ready for review now.

All the commits up to the last one simply move code to the new assembly and make adjustments so that the tests keep passing. The final commit is the real meat of it, since it causes the agents to reference the nunit.engine.core assembly and use CoreEngine, rather than nunit.engine and TestEngine.

There are a few more things that could be done, but this PR is as big as I want to go. I'm also willing to split it up into several PRs if anyone sees value in that.

[CharliePoole]

@nunit/engine-team I'd appreciate somebody - or even everybody - chiming in on this. I'm blocked on about four other things while waiting for clarification on which way you want to go. To be clear, even if it isn't quite ready to be merged, I need to know what direction folks want to go before I'll be able to work on those other things. So if you don't have time for a detailed review, that's OK, but your views on (1) whether to break up the engine this way, (2) whether to have two pieces or three and (3) whether I need to break up this PR to accommodate other work in progress would really help.

[ChrisMaddock]

Hi Charlie. Have left my thoughts on direction above. This PR looks fine to review as is to me, it's a lot of changes, but the vast majority are just file moves

[CharliePoole]

Good! Now if somebody could review it please!

@nunit/engine-team Chris has said he doesn't have a lot of time, so it would be great if somebody else took a look.

@jnm2 I'm particularly concerned about not messing up the long-running PR you are working on.

[ChrisMaddock]

Hey Charlie,

This looks great! I've left a bunch of comments on minor bits and pieces throughout - the overall picture looks a lot clearer than what we currently have. Some additional thoughts, as I can't leave a comment attached to a file move...

• I would have suggested moving the files in Core/Agents out of the Engine Core to the agent - but I think @jnm2's work will shortly be removing those classes anyway, so I'm not too worried.
• It would be great to get RuntimeFramework, and all the related classes over to the main engine - it looks like it's only used by the agent to create a single log message! That can be a separate PR however, there's enough in this one!
• Also for future: I wonder if there's any way we can get the ExtensionService over to nunit.engine as well. That feels like a big piece of functionality, which shouldn't be required in the agent. (I can see that it's currently needed by the DriverService, however!)

[ChrisMaddock]

If this is in the core library, I guess it can just be referenced rather than linked in now, right? That would make it consistent with the rest of the structure.

[CharliePoole]

It's tricky. The console runner would then have to directly reference the core engine directly. Otherwise we end up with duplication of the class in console tests. I followed that path for a bit, until it became too hairy and I reverted to shared source code for this particular helper.

[ChrisMaddock]

As above

[ChrisMaddock]

This look unused?

[CharliePoole]

Odd... it was used. Looking just at the code, it appears that extension points in the nunit.engine.core and nunit.engine.api will be found but not those in nunit.engine. But we know they are found because the tests pass!

Obviously, in the course of implementing this three times, I must have changed the approach. I'll verify and possibly remove this.

[CharliePoole]

OK... I realized at some point that all our extension points are currently defined in nunit.engine.api and nunit.engine.core (only one in the latter). I think RootAssemblies was needed in a three-way split, but not in a two-way split.

I think some way to specify roots to be searched for extension points will be needed at some point - for example if we want to allow extension points in nunit3-console. However, it could be removed now or just commented as being for future use. I'm OK either way.

[ChrisMaddock]

As above - can this just reference this class directly, rather than the link, to be consistent with the other Util classes?

[CharliePoole]

See earlier comment.

[CharliePoole]

@ChrisMaddock Thanks for your comments. I'll go through and make fixes this evening - moving house today!

Regarding putting some of the things with the agent itself... I envision a future where we have multiple agents, which is why I was playing around with the idea of a third library to hold stuff that is used in common by agents. I saw this change as the minimum needed to be done all at once to have something that made sense but I didn't want to add more.

I'll definitely be going further with this approach - perhaps in my own build, or simultaneously with NUnit if we end up going the same way. For what it's worth, my notion of an "agent" is that it encapsulates the "where we run" part of things along with the particular type of communication used to send commands and receive events and results. Right now, we only have one true agent, which runs in a separate process on the same machine and uses remoting to communicate. IME the big benefit of splitting out agents and making them as simple as possible is that we don't have to limit how many agents we support.

[ChrisMaddock]

@CharliePoole Oh! We'll need to update all the packaging for this too, right? NuGet, Choco and the msi will all definitely be affected - not sure about how the zip is gathered in Cake, off the top of my head.

[CharliePoole]

Agreed. Are you OK with that as a separate PR?

[ChrisMaddock]

I'd prefer to keep master releasable with this one (i.e. all in one PR) - but happy to go with two separate PR's, if you feel this is getting laborious to work with. 🙂

[CharliePoole]

Amount of work for me is about the same but one PR gives re-reviews of the whole thing. Also, current PR will likely cause merge conflicts for others whereas packaging probably won't.

[CharliePoole]

Easiest packaging will be to retain same packages for this release. Consider adding separate core package later.

[ChrisMaddock]

Absolutely agree on not adding 'core package' - was just thinking the new assembly needs to be included in the current packages, otherwise they won't work! I think it would be better to do that in the same PR if that's ok with you - it's only a few lines, and easy to review separately. 🙂

At this point, I'm planning to rebase other PR's over this one, rather than the other way round - as I think that'll be easier to do.

In terms of reviews on this, I'm happy to merge this just with mine and yours reviews, if you would be - I appreciate others are pretty busy at the moment, and this is a lot to look through! I'm happy that enough seen/commented on #684 and #580 to agree with the approach - and the details are pretty clear-cut here if we're aiming for 'minimum change', and can always be changed in future. 👍

[CharliePoole]

OK... let's do that. Ill-treatment to finish today, Pacific time.

[ChrisMaddock]

No rush on my part! 🙂

[CharliePoole]

@ChrisMaddock Ready for another review. I added nunit.engine.core to all the packages with nunit.engine and verified the contents for everything but the msi, which I'm not set up to build locally. Someone should check that.

[ChrisMaddock]

Ok - let's get this merged! There's a couple of 'tidy-up' bits, but at this point, I'm keen to get this in, and handle those in a separate PR.

Thanks for pulling this together @CharliePoole - it's a real big step along the road to more minimal agents. 😄

[jnm2]

I'm particularly concerned about not messing up the long-running PR you are working on.

Thanks. I was already planning to redo it all though, just referencing my current PR as I go and trying to do it in as small of PRs as I can figure out. I hope you are able to feel free not to worry about the long-running PR. (Maybe I should close it for now?)