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

Simplify or remove loading of engine by reflection via TestEngineActivator #948

Open
ChrisMaddock opened this issue May 9, 2021 · 3 comments

Comments

@ChrisMaddock
Copy link
Member

Having just lost yet another day to weird reflection issues, I'd like to have a look at whether we can remove this structure for v4.0, inline with our "Improved Developer Experience" objective.

If I understand the history correctly, the reason this was implemented in v3 was with the expectation that users would install a single centralised engine, and have runners discover and make use of that. We've found instead that runners have chosen to each distribute their own local copy of the engine, meaning this has become a source of extra complication in the engine which isn't well used.

Having made a start on #885, I'm now looking at the below structure for the API assemblies:

nunit.engine.extensions.api.dll
ExtensionAttributes
ITestPackage

nunit.extension.project-loader.api.dll (And 4 other similar assemblies)
API for individual extensions. Broken into individual assemblies to allow us to make any necessary breaking changes in a less disruptive manner.

nunit.engine.api.dll
API's for runners to interact with the engine.

Once I'd got that far, I'm now thinking that nunit.engine.api.dll doesn't serve much purpose except to expose the public types from nunit.engine.dll.

How about we merge what's left of nunit.engine.api.dll into the nunit.engine assembly, and simply expose only the API types that we want people to use. (As we planned in #886 anyway). This would also mean that TestEngineActivator could simply create a TestEngine via the constructor, and we'd remove a good amount of the reflection complexity, impeded debugging ability and type loading complications we currently face.

What do people think? 🙂

@CharliePoole
Copy link
Member

CharliePoole commented May 10, 2021

Having an API assembly does a few things...

  1. You can modify the implementation under that API.
  2. You can allow other people to replace all or part of the engine, provided the API is respected.
  3. You can decouple API versioning from engine versioning.
  4. You can do drop-in replacement of the engine without rebuilding the runner that is using it.

There's no doubt that all of the above are potentially useful. The question is how useful and at what cost.

@CharliePoole CharliePoole changed the title Remove loading of engine by reflection via TestEngineActivator Simplify or remove loading of engine by reflection via TestEngineActivator Dec 31, 2024
@CharliePoole CharliePoole modified the milestones: 4.0, 4.0.0-beta.1 Dec 31, 2024
@CharliePoole CharliePoole self-assigned this Dec 31, 2024
@CharliePoole
Copy link
Member

This issue has been dormant for more than three years. At this point, it's clear that we either want to simplify TestEngineActivator significantly or eliminate it. I've changed the issue title accordingly.

At one time, we were planning to allow drop-in replacement of the engine without updating any runners. This is not something we have ever implemented and there really doesn't seem to be a need for it. Generally, any runner comes with an engine build that matches it.

This means that searching for the engine anywhere except in the same directory that contains the API is no longer useful. It also means that having a runner require a certain minimum version of the engine is redundant. It will use the engine that is packaged with it.

The above-mentioned simplification is already in place, as a part of other changes we have made. TestEngineActivator simply creates an instance of ITestEngine without any other changes. The question to resolve here is whether to go to the next step and eliminate TestEngineActivator entirely. This would leave the choice up to the runner as to whether to only use the interface or to call the TestEngine class directly.

@CharliePoole
Copy link
Member

@nunit/engine-team

Options as I see them...

  1. Leave code as it is, the current "simplified" implementation of TestEngineActivator.
  2. Modify TestEngineActivator to use new to construct the engine.
  3. Have the user use new to construct the engine and drop TestEngineActivator

Option 2 requires either merging the API into the engine assembly or allowing the api to reference the engine. Merging the API into the engine assembly may also be considered in combination with options 1 and 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants