nunit / nunit-console

NUnit Console runner and test engine
MIT License
215 stars 151 forks source link

v4: Remove IFrameworkDriver extension point #929

Closed ChrisMaddock closed 2 years ago

ChrisMaddock commented 3 years ago

This came up in conversation recently and I've been thinking about it more. Allowing FrameworkDriver's to be extension adds a significant amount of complexity and, as far as we are aware, has never been made use of. I suggest we remove the extension point from v4, as the cost of maintaining it outweighs the benefit to users.

As far as we're aware the IFrameworkDriver extension point is only used for the NUnit v2 driver, which is within the org's control. It's also not entirely an extension itself, as the DriverFactory for it sits within the main engine. Runners that support multiple test framework's appear to have instead implemented their own ways to interact with the "engines" for each framework, or used the vstest runner/adapters, which is now supported by all major test frameworks.

While we support running other frameworks through extensions, we require agents to be able to locate, analyse and load extensions, which requires both the ExtensionService and FrameworkService to be part of the agent, and thus fully cross platform for every agent runtime we support. This also would become a significant burden on third-party maintainers if we look to support extenendable-agents - as it would require each individual agent to also support the ability to load driver extensions. Our aim is for agents to be as slimline as possible, and there are options to refactor this logic into the main engine - however it would be a significant piece of work for a feature we don't believe to be well-used.

I think this is a feature where the maintenance cost is outweighing the benefit. It's been available now for several years and we're not aware of any users of it - I think we should pull it out to help us move forward. @nunit/engine-team, what do we think? 🙂

CharliePoole commented 3 years ago

I think it's a direction with some validity. It redefines the engine as being for NUnit framework only, which is a change in the vision even though it may not be a de-facto change. Personally, I am disappointed that we didn't get more people to take up the idea of creating drivers for other frameworks.

For me, as a potential user, it makes extensibility even more important. To use the NUnit engine, I would want a path to adding a some way to run other frameworks without creating a driver. It's not an appropriate use of pluggable agents unless we add some kind of framework detection capability to agents.

In the GUI flavor of the Engine, I want to experiment with a much simpler encapsulation of extensibility for use in the agent, since the agent never has to deal with conflicting runtimes.

ChrisMaddock commented 3 years ago

Just stumbled across https://github.com/nunit/nunit3-vs-adapter/issues/745 while looking for something else - funny how these things come up!

@Caiptain1 - we're considering removing the IFrameworkDriver extensions point for the next version of the NUnit Engine. The issue you filed above appears to be the only question ever asked about this extension type. Can I ask what you're using the extension point for - is it to run NUnit v2 tests, or have you written your own IFrameworkDriver extension?

CharliePoole commented 3 years ago

Based on past experience, the easiest way to find out who is using something is to remove it. :smiling_imp:

Caiptain1 commented 3 years ago

@ChrisMaddock Thanks for asking. My use of the driver is something unique due to the way we do builds in our organization. What the extension is currently used for is wrapping the NUnit3 driver. We add an assembly resolver to the test domain before test assembly gets inspected for tests. All methods just forward the function calls to the NUnit3 driver and don't do anything else.

What we need In our organization, we have our own build orchestrator. I won't dive into the details on how and why it works as it does, but it's we have really great uses for it. We don't copy DLLs to the output because we have massive products that consist of hundreds sometimes over a thousand projects (thus the build orchestrator which allows us to manage that nicely). We create product directories in another output location that have all the required DLLs and data files. That's where the products can be launched. So the proper thing to do is:

The problem

The solution The assembly resolver has to be in place before the Test DLL gets loaded. Having it in global setups doesn't work since NUnit analyses type information and crashes beforehand. Thus, we have to add an assembly resolver before that and the only place where that can be done not from inside NUnit itself is via IFrameworkDriver extension.

Here's the example code:

namespace MyNUnit3Extension
    {
    public class MyNUnit3FrameworkDriver : IFrameworkDriver
        {
        private NUnit3FrameworkDriver m_nunit3FrameworkDriver;

        public MyNUnit3FrameworkDriver (AppDomain testDomain, AssemblyName reference)
            {
            var myAssempblyLoaderProxyInstance = (MyAssemblyLoaderProxy) testDomain.CreateInstanceFromAndUnwrap(typeof(MyAssemblyLoaderProxy).Assembly.Location, typeof(MyAssemblyLoaderProxy).FullName);
            myAssempblyLoaderProxyInstance.HookAssemblyLoaderOnDomain();
            m_nunit3FrameworkDriver = new NUnit3FrameworkDriver(testDomain, reference);
            }

        public string ID
            {
            get => m_nunit3FrameworkDriver.ID;
            set => m_nunit3FrameworkDriver.ID = value;
            }

        public int CountTestCases (string filter) => m_nunit3FrameworkDriver.CountTestCases(filter);

        public string Explore (string filter) => m_nunit3FrameworkDriver.Explore(filter);

        public string Load (string testAssemblyPath, IDictionary<string, object> settings) => m_nunit3FrameworkDriver.Load(testAssemblyPath, settings);

        public string Run (ITestEventListener listener, string filter) => m_nunit3FrameworkDriver.Run(listener, filter);

        public void StopRun (bool force) => m_nunit3FrameworkDriver.StopRun(force);
        }
    }
Caiptain1 commented 3 years ago

I think it's a direction with some validity. It redefines the engine as being for NUnit framework only, which is a change in the vision even though it may not be a de-facto change. Personally, I am disappointed that we didn't get more people to take up the idea of creating drivers for other frameworks.

For me, as a potential user, it makes extensibility even more important. To use the NUnit engine, I would want a path to adding a some way to run other frameworks without creating a driver. It's not an appropriate use of pluggable agents unless we add some kind of framework detection capability to agents.

In the GUI flavor of the Engine, I want to experiment with a much simpler encapsulation of extensibility for use in the agent, since the agent never has to deal with conflicting runtimes.

@CharliePoole Personally, I see the NUnit framework extensibility as one the few options available right now to circumvent some of Azure DevOps Test Case association limitations. Microsoft only allows associating Test Cases (backlog items in Azure DevOps) only via a few select Visual Studio Test Adapters (https://docs.microsoft.com/en-us/azure/devops/test/associate-automated-test-with-test-case?view=azure-devops#q-what-types-of-tests-are-supported). In our organization we have some unique ways of writting tests, e.g. some big products are command line driven and a test case is basically a .txt file. We could write our own VS Test Adapter but MS won't let us do the association unless we fake our executor ID and say that we are one of the recognized frameworks. Then the only option we have is to use some testing framework like NUnit to wrap our testing framework and then use that functionality.

P.S. Can concur on the 'remove and see if anyone complains' thing 😆

ChrisMaddock commented 3 years ago

Thanks @Caiptain1 - that's really interesting! I do like it when someone's using NUnit in a way we never expected! 😆

While this works, I hope you don't mine me saying it's not really what the extension point was intended for! 🙂 Throwing ideas around, it feels more like what we really need is a way to hook into/override the DomainManager Service, which isn't something that exists yet. It might work quite nicely with something we're looking at to allow entire TestAgents as extensions however...

@CharliePoole - any thoughts?

Caiptain1 commented 3 years ago

I agree that it's not the idea. I worked with what I had to make it work :)

CharliePoole commented 3 years ago

I agree 100%. IMO, you should be able to add both services and runners and even replace the built-in versions. My first thought about @Caiptain1 's problem was that he needed to replace or modify TestDomainRunner but DomainManager would work as well.

CharliePoole commented 2 years ago

We resolved #1019 saying we wouldcontinue to support existing extension points, so closing I'm this without any action.