Support Version 3 NUnit Extensions

[CharliePoole]

It was originally decided, without much discussion, that old V3 extensions would not be supported in V4, but would need to be rewritten to use the new interfaces. See #1009. However, this will be quite inconvenient for users, while waiting for the old extensions to be re-issued.

On re-examination, it seems feasible to support old extensions by use of reflection. This will introduce performance issues for extensions which are called frequently, but is still an improvement over not supporting them at all.

A experimental spike already exists, which creates a wrapper for each old extension using the new interfaces. That implementation injects the wrapper at the point where each type of extension is used. It includes a method cache for each extension point to avoid the work of acquiring the MethodInfo each time a call is made.

I'll use the same wrappers and caching mechanism for this issue but will try to wrap and inject the old interfaces centrally, at the point where ExtensionManager creates the ExtensionNode representing an extension.

@nunit/core-team @nunit/engine-team @nunit/team-city-team

[manfred-brands]

It includes a method cache for each extension point to avoid the work of acquiring the MethodInfo each time a call is made.

Even if you have MethodInfo the Invoke call is still expensive.

| Method                   | Runtime              | Mean          | Ratio | Allocated | Alloc Ratio |
|------------------------- |--------------------- |--------------:|------:|----------:|------------:|
| DirectMethodCall         | .NET 8.0             |     0.2048 ns | 0.002 |         - |        0.00 |
| MethodInfoInvoke         | .NET 8.0             |    14.5727 ns | 0.136 |      80 B |        0.71 |
| PrecreatedDelegateInvoke | .NET 8.0             |     1.7281 ns | 0.016 |         - |        0.00 |
| DirectMethodCall         | .NET Framework 4.6.2 |     0.7869 ns | 0.007 |         - |        0.00 |
| MethodInfoInvoke         | .NET Framework 4.6.2 |   107.6194 ns | 1.001 |     112 B |        1.00 |
| PrecreatedDelegateInvoke | .NET Framework 4.6.2 |     1.5585 ns | 0.015 |         - |        0.00 |

However you need at least 10 calls to justify creating a delegate:

| CreateDelegateAndInvoke  | .NET 8.0             |   183.3078 ns | 1.706 |      64 B |        0.57 |
| CreateDelegateAndInvoke  | .NET Framework 4.6.2 | 1,051.6620 ns | 9.785 |      64 B |        0.57 |

[CharliePoole]

Actually, with those numbers the delegate sounds like a good idea. At low usage rates, nobody is going to notice the performance effect. It's really only at high usage rates that it matters.

It also seems as if I could get rid of the cache if we used delegates.

That said, I'll keep the existing wrapper for now and work on the problem of how to inject the wrapper at a central point.

[CharliePoole]

I've done as much as I care to do on this issue. Somebody else could take it further, but read below if you're considering.

Current Status

Two wrappers are up and running, one for ITestEventListener and one for IResultWriter. Those were the easy ones. Of course, that means it would also be easy to just rewrite the extensions that install on those extension points. The first contains a single method returning void, the second has two such methods.

Moving on to IProjectLoader it got harder. That work is only partly done, so the test that uses it is commented out. IProject loader includes properties with getters and setters and methods returning values. Some of the returned values require wrappers themselves and one of them, TestPackage, is a recursive structure.

Looking Ahead

After spending some time on this, my conclusion is that these wrappers will create a future maintenance headache if we release with support for them. Even though they are handy right now in testing out the V4 engine, I think the time I've spent on this could have been better spent creating new versions of the extensions themselves.

So what to do with the work so far?

My thought is to leave the existing wrappers in the code until new extensions are released. We can even keep them through one or two betas if we have to. But I'd like to get rid of them before going into production.

On the Other Hand

If one of you wants to take this over and take it further, let me know. It is an intriguing issue, which is why I ended up going down a rabbit hole with it. Just be forewarned. :-)

---

@nunit/engine-team I'm open to alternative suggestions if you have them.

[CharliePoole]

@manfred-brands Thanks for finishing this up!

Based on what I've seen in discussions and questions, the two wrappers that are working are the ones most used by users who create their own extensions, so perhaps it's not a bad idea to keep them around for a while, even if our own extensions are updated for V4.

What do you think? Shall we close this?

BTW, this also takes care of porting of support for V1.0.9 of the TeamCity event listener.

[manfred-brands]

What do you think? Shall we close this?

@CharliePoole I don't know who is using what extensions, but if existing ones stay working is a good thing.
This ticket needs to clarify that not all V3 extensions are supported, but only the ones using (parts of) these interfaces.

[CharliePoole]

@manfred-brands Let's clarify a bit what we are talking about.

1. There are five extension points, one of which, IService, is so far unused as far as we know. We will continue to support those extension points and add some others in V4.
2. Without any V3 wrappers, we know that know no V3 extensions will work, including ours, without being modified and re-compiled.
3. We support four extensions directly and one in cooperation with JetBrains. All of those will be rewritten for V4.
4. We don't actually know what other extensions exist, although various users have written about extensions they have written over the years. My memory is that most of these used either ITestEventListener or IResultWriter and will most likely work using the wrappers. I say "most likely" because we don't really know what that user code contains. They might be using other parts of the engine.

At the moment, of course, we are in beta and users are not advised to use the pre-releases at all. When we eventually do the final V4 release, our own extensions will be working but we will need to say something about user-created extensions...

"All user-created extensions will need to be rewritten to work under V4." (NO Wrappers)

OR

"For best performance, any user-created extensions should be rewritten under V4." (ALL Wrappers)

OR

"Some user-created extensions will need to be rewritten to work under V4. Extensions using ITestEventListener and IResultWriter are expected to continue to work without rewriting, but with reduced performance." (SOME Wrappers)

---

We have to make both a long-term and a short-term decision.

The long-term one is what we want to have for the final release. The short-term choice, which depends on the long-term, is whether to keep this issue alive and create more wrappers before the next beta.

@nunit/engine-team Thoughts?

[CharliePoole]

Closing this issue so it will show up as completed for the next beta. What is implemented so far...

• The overall infrastructure for using reflection to support Version 3 extensions
• Specific implementations...
  • ITestEventListener works and is used in our tests for the TeamCity event listener.
  • IResultWriter works and is used in our tests for the NUnit V2 result writer.

Both of the above extensions will be converted to the new API before final release.

What is not implemented...

• IProjectLoader, which supports NUnit and Visual Studio projects
• IDriverFactory which supports running tests using other frameworks, including our NUnit V2 driver

The two types of extensions which are not yet supported will not be included in the next beta. If we decide to support their old versions, we'll create separate issues. Below are some thoughts about the viability of the reflection-based approach for these extension points.

Both IProject Loader and IDriverFactory are interfaces which include use of other interfaces or classes for which we would also need to create wrappers. This makes them somewhat more complicated and less efficient to emulate.

All three of our own extensions using these interfaces will be upgraded to the new API before final release. We don't know of any 3rd party extensions that might be affected.

In the case of VSProjectLoader, a complete rewrite is actually needed for other reasons because the existing extension does not support newer project formats very well.