Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove services from nunit.engine.core assembly #934

Closed
CharliePoole opened this issue Apr 10, 2021 · 3 comments · Fixed by #1140
Closed

Remove services from nunit.engine.core assembly #934

CharliePoole opened this issue Apr 10, 2021 · 3 comments · Fixed by #1140
Assignees
Labels
Milestone

Comments

@CharliePoole
Copy link
Collaborator

As a part of simplifying agents, I suggest we remove some services from nunit.engine.core. In each case, most of the code would remain but it would no longer function as a service. As a start, I suggest removing the following two services, which will only be used by agents 4.0.

  1. DomainManager is only used by the TestDomainRunner, which can access it directly without need for the ServiceManager to be involved. Once we remove the --inprocess option, TestDomainRunner will only ever be created by an agent, simplifying things further.

  2. InProcessTestRunnerFactory is now extremely simple. When running under .NETFX, we use a TestDomainRunner. Under .NET Core, a LocalTestRunner. This code may be directly incorporated where needed rather than providing a service.

Once these two are removed as services, only two will remain in the core: DriverService and ExtensionService. If The former is removed, as has been proposed separately, then ExtensionService is no longer needed in the core and may be moved to the nunit.engine assembly.

If we decide to keep the ability to provide custom drivers, we would need to create a simplified bit of code to locate and load them, which avoided the need to handle incompatible target runtimes through use of conventions.

In either case, removal of the two listed services is beneficial and could result in eventual removal of the ServiceManager itself from the core.

@CharliePoole CharliePoole added this to the 4.0 milestone Apr 10, 2021
@ChrisMaddock
Copy link
Member

ChrisMaddock commented Apr 10, 2021

Sounds good.

I did some work before to look at moving ExtensionService and DriverService into the main engine while keeping custom driver support. The main engine should have all the info to decide on the appropriate driver (including any in extensions), and then just pass a class name and optional assembly path to the agent, to load the driver from.

Unfortunately, another one I never finished!

@CharliePoole
Copy link
Collaborator Author

Yes, we talked about that. I'd like to try both alternatives. I could remember wrong but it seems as if the original extension service was about a third the size of the current one. It didn't make any attempt to deal with conflicting runtimes beforehand, but just handled exceptions gracefully.

We couldn't keep it that way once we started dealing with .Net Standard and Core. But the added complexity is currently borne both by the "main" engine and the agents. That seems unnecessary. Once you have an agent, you only want to deal with a particular runtime anyway. I have a vague idea about how this can be done, but I need to write code and see how it works out.

In any case, the two services above are low-hanging fruit. As I often do, I'm moving ahead with these two in the GUI for later porting to NUnit itself.

@CharliePoole
Copy link
Collaborator Author

I did this in the GUI and it worked well, but when I tried to do the same in a local branch for the console runner, I ran into problems because the engine still supports all the --process options. I'm marking this as blocked by #861.

@CharliePoole CharliePoole changed the title Remove unnecessary services from nunit.engine.core assembly Remove services from nunit.engine.core assembly Feb 17, 2022
@CharliePoole CharliePoole self-assigned this Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants