Allows to override load-time/execution-time interfaces in built-in tests attributes

[xoofx]

Hey,

This PR allows to override the load-time/execution-time interfaces implemented by the various attributes in NUnit (e.g ISimpleTestBuilder, ITestBuilder, IApplyToTest...). It is a relatively very small change, only adding virtual qualifiers to the current implemented methods...

I hit an issue where I wanted to use a standard attribute but slightly change the behavior of the builder method but couldn't do it without rebuilding the whole logic, which is a bit frustrating, considering that these extension points in NUnit are so convenient!

Let me know if there are any issues!
Thanks

[rprouse]

@xoofx for future refererence, please create an issue before submitting a PR. It helps us track what goes into a release and allows us to discuss changes before you put too much work in. We can work with this though.

Our original intention was that users implement our interfaces to add custom attributes, but as you have found, it is often easier to re-use and override our attributes in some cases. A common usage that I see is a dynamic ignore attribute that ignores tests until an external condition is met like a bug is fixed. That sort of usage can be dangerous, but they are your tests 😄

At first pass, this looks good to me. I have a few concerns, but I am not sure they are serious enough not to include these changes. We require two team members to review PRs, so I will leave it to the second reviewer to comment on my concerns.

• Have you ensured that you made all interface methods virtual? Hard to tell from reading the diff, but easier for you to confirm in visual studio.
• I worry that some attributes have complex logic or functionality that isn't apparent so they shouldn't be overriden. I could also argue that it is your foot, go ahead and point the gun at it.
• Some of our attributes have private helper methods that might need to be made protected. We can always deal with these one by one.

@nunit/framework-team please give this a second review. I am okay with the idea in principle, but would like to approach this cautiously because it is the sort of change that cannot be backed out without breaking people's code.

[CharliePoole]

Thanks for this contribution. For future reference, it's a good idea to first create an issue for a new feature or enhancement. That way, we can review the desirability of a proposal before any time is put into its implementation.

I'm not in favor of this change, for reasons which I hope will make some sense to you...

1. Our attributes are not generally designed to be used as base classes, even though it's possible in some cases. Use of virtual methods is an invitation to users to do that and in my view constitutes a new feature of the attribute, which we have to maintain in future releases.

2. Even if we wanted to do it, a wholesale change without tests is not the way we usually operate. Somewhere in all of these small changes, there may be one where simply making a method virtual does not actually make a possible to usefully derive from the class. Each one would need at least one test scenario in order to do that.

3. Our intention was to enable extensibility by use of interfaces alone. Generally, helper methods are used internally for common code, rather than inheritance. A few attributes are designed and used internally as base classes and users can make use of them for extensions.

4. It's not necessary to use virtual methods for most of these attributes anyway, since interface reimplementation is possible instead.

[jnm2]

To Charlie's point 2 (and the foot gun to which Rob referred): I'm in agreement; I don't think wholesale changes are a great benefit. Let's review use cases as they come in. That way we can write intelligent unit tests making sure subclassing is serving its understood goals. With that in mind, @xoofx, would you please share the specific circumstance you'd like to improve?

To point 1: In general, emphasis on ‘general,’ I have always been in favor of this direction, big responsibility though it is.

To point 3, it's terribly irritating when I can't extend something without duplicating gobs of base code simply because the base was so designed that I can't delegate back to well-defined parts of the original implementation– yet another scenario where undirected wholesale virtualization doesn't add as much value as reviewing case-by-case scenarios and potentially breaking up large base methods into logical functions.

To point 4: Sure, I didn't need it here, but having to shadow with interface reimplementation is a code smell IMO no matter what the context is. It's not safe in the face of any potential code which specially understands the base attribute class and calls the method. The brittleness could manifest in seemingly unrelated changes to NUnit, or to third party tools. Attributes are after all public API and calculating how they build tests is just the sort of thing a tool might do.

[jnm2]

Inheritdoc does nothing without a custom XML post-processing step which we currently don't have.
I need to add this to https://github.com/nunit/docs/wiki/Best-practices-for-XML-documentation; this isn't the first time people have tried to use it. ReSharper suggests it but Roslyn doesn't recognize the element. Vote at dotnet/csharplang#313 btw...

[xoofx]

yeah, in the old days of Sandcastle, that was the way to handle this, as I don't like the idea to duplicate all interfaces comments as keeping them in sync after is laborious, but if it is the way it is done in NUnit, I will change it.

[CharliePoole]

@xoofx It's not a matter of a decision NUnit has made but simply that it doesn't work for Intellisense. It's academic, however, unless we make the change.

[jnm2]

Same issue with inheritdoc

[jnm2]

Same issue with inheritdoc

[jnm2]

Same issue with inheritdoc

[jnm2]

Comments on <inheritdoc/>, but let's wait for consensus on the design discussion about what we actually want to make virtual right now.

[CharliePoole]

@jnm2 Reimplementing of an interface is a well-documented feature that has been around since .NET 1.0. There is no doubt that it's possible to misuse it, but when you call it a code smell it seems to me that you are simply expressing a personal preference for a certain style.

The main issue with interface re-implementation is using it in places where it won't work, particularly when multiple levels of inheritance are involved. There's also the fact that you have to reimplement the entire interface rather than overloading a single method, but that hardly applies to an interface that only contains one method (two in a few cases).

The main point I would make is that implementing (or reimplementing) interfaces is the way our framework extensibility is designed to work. Changing it is not a small thing and should not be a casual thing. Let's see one case where it doesn't work first, then lets solve that problem Following the "rule of three", I'd prefer to postpone generalization till after we solve the third problem.

I also want to point out that this change is at what might be called the second level of generalization. The first level is a single case. The second level might be "let's make all ApplyToTest methods vertual. This one takes all the implementations of all the interfaces and changes them. That's way too much in my book.

I might feel different about a particular knotty problem that can't be solved otherwise.

@rprouse Regarding letting him shoot himself in the foot... I realize that was somewhat tongue in cheek, but to me making a method virtual is an announcement to all users "It's OK to derive from this class, we planned for it and it will work." But we haven't planned for it, at least not for most of these attributes. If we think inheritance is the best way to handle certain things, then we should do the work of making it possible. That's what we did with PropertyAttribute for example. The base class implements IApplyToTest so that the derived classes don't have to.

There's a further point to keep in mind. Currently, any extension you create must be re-created for each version of NUnit. There was some hope - in my mind at least - that we would be able to generalize in a direction that allowed extensions to be used across multiple framework releases, y putting the interfaces into a separate assembly. Encouraging attribute inheritance, however, pretty much rules that out for the future, since the extension would need to reference the framework itself in order to inherit the attribute.

Changing my mind

[rprouse]

I have rescinded my approval 😄

I agree with @CharliePoole for the most part. I do think that users should be able to override some of our attributes, but I also think that should be focused and each should be a conscious decision by the team.

@xoofx which attribute were you originally trying to override and why?

[xoofx]

Thanks for the feedback, there is almost too much there! 😅

for future refererence, please create an issue before submitting a PRyour. It helps us track what goes into a release and allows us to discuss changes before you put too much work in. We can work with this though.

Yep sorry for that. I will do this in the future.

Our original intention was that users implement our interfaces to add custom attributes, but as you have found, it is often easier to re-use and override our attributes in some cases.

1. Our attributes are not generally designed to be used as base classes, even though it's possible in some cases. Use of virtual methods is an invitation to users to do that and in my view constitutes a new feature of the attribute, which we have to maintain in future releases.

3. Our intention was to enable extensibility by use of interfaces alone. Generally, helper methods are used internally for common code, rather than inheritance. A few attributes are designed and used internally as base classes and users can make use of them for extensions.

Would NUnit accept a PR to mark these attributes sealed then? 😅

Of course not, it will break all clients that are already deriving from NUnit attributes. Most of the time, when you want to build a new attribute, you derive from the simple one likeTestAttribute.

Though I agree that in general, there are very few projects/tests that are using the interfaces ITestBuilder, ISimpleTestBuilder...etc.

What is annoying from a client side, is seeing that ITestBuilder is an official extensible point, almost all NUnit attributes are not sealed (bearing that they are also extendable), all of them are implementing ITestBuilder or ISimpleTestBuilder at least, but if you want to extend attribute with this interface, you can't - or as noted, you need to use explicit interface implementation (to be honest, I forgot that I could do this... maybe I wouldn't have opened this issue then 😅)

2. Even if we wanted to do it, a wholesale change without tests is not the way we usually operate. Somewhere in all of these small changes, there may be one where simply making a method virtual does not actually make a possible to usefully derive from the class. Each one would need at least one test scenario in order to do that.

In my experience, the usefulness in the eye of a library provider can be often beaten by the versatility of usages that he couldn't think of in the first place... Sure as a library provider, you have the right to fear of having to support more extension points in your library... (in my experience, just providing an OSS library and you are already in dark trouble... 😄 )

Even in NUnit code base, it is already overriding this behavior... which indicates at least that it is a relatively common usage...

One reason i didn't mention also in the PR comment is support in ReSharper. In their code, they actually rely on attribute inheritance, not interface attached to them. There is an issue opened for it here. I know that this is a ReSharper problem and mostly none of NUnit business. Looking at there internal code, changes there might require a bit more work there, not sure it is equipped to handle interface inheritance... Just to let you know. But that specific little annoyance is forcing me to reuse one of NUnit existing attribute if I want to provide a smooth experience.

I worry that some attributes have complex logic or functionality that isn't apparent so they shouldn't be overriden. I could also argue that it is your foot, go ahead and point the gun at it.

@xoofx which attribute were you originally trying to override and why?

Let's take the example ITestBuilder or ISimpleTestBuilder: they are providing TestMethod. I don't necessarely need to know exactly what the base class is doing in the details, but I know for sure the contract ITestBuilder and what is is supposed to be. By extending these interfaces, I can get whatever is generated by the base class and transform them, or add new TestMethod derived from them.

Typically, for TestCaseSource, I want to add an enum passed as a property that would allow to generate numeric variations on the original test case values provided by TestCaseSource (mainly in my tests, they are integers...etc.).

Then overriding ITestBuilder to recreate new TestMethod based on the original ones returned by the TestCaseSource.

I also want to point out that this change is at what might be called the second level of generalization. The first level is a single case. The second level might be "let's make all ApplyToTest methods vertual. This one takes all the implementations of all the interfaces and changes them. That's way too much in my book.

What I don't get is that NUnit are using a public extensible interface with a public extensible attribute (not sealed), but decide that users will not need to extend it with information provided by the derived attribute. The library maintainer decide only on user cases when it is allowed... If it was really so a strict rule in NUnit, it should have really been thought about using extensively sealed on all NUnit attributes in the first place...

And frankly, for this specific case IApplyToTest, why would you completely disallow overriding but yet allowing derived attributes? Its purpose is to copy data from TestAttribute members to the Test.Properties. Any derived attributes could need this as well while still allowing base class to do whatever its duty is.

I haven't looked at the codebase in details, but I would be very surprised that letting someone overriding one of these methods would let the user shoot into his foot. If you are overriding these methods, you already know what there contract is assuming. Sure you can mess up the contract, but that's the life of any contract implementations...

Concerning the "change without tests is not the way we usually operate. ", sure but a test for checking that overrides is working sounds a tough price to enter these changes to the codebase. Could you point in NUnit codebase where an override calling only the base method would bring an unsustainable bug hole?

Some of our attributes have private helper methods that might need to be made protected. We can always deal with these one by one.

Yes, it may.

So my main point is that if NUnit is providing public extensibility interfaces - which are advanced usages - and public non sealed attributes, it is not a dramatic change to allow people (with advanced use cases) to virtual/overrides these public interfaces as they have a clear API contract, no matter what NUnit is already doing with these methods.

But the workaround of using explicit implementation interface is fine for me - while not super friendly, so I don't mind closing this PR if you really think that adding virtual generally is a critical problem.

[xoofx]

Also just a note about explicit interface implementation and its usage in NUnit (the code smell mentioned by @jnm2): usually, they are used to hide the methods from the public API, but I have never seen them to provide pseudo virtuality only through interface usage. It becomes dangerous when you inherit from a class that use explicit interface implementation because you can't call the actual base method. Virtuals are also completely broken after that, if you start to use the base class method directly without going through the interface, you will not get the expected behavior...

[rprouse]

So my main point is that if NUnit is providing public extensibility interfaces and public non sealed attributes, it is not a dramatic change to allow people to virtual/overrides these public interfaces as they have a clear API contract, no matter what NUnit is already doing with these methods.

To be honest, we have not done a good job of using sealed where we don't intend classes to be overridden. We also have too many classes marked public that are only intended for internal use.

IApplyToTest is one set of attributes that I would agree should be derivable. The various ITestBuilder attributes are a bit more complex though.

I am going to close this PR as it stands. I would prefer to take a more focused approach on this and consider each attribute on its own so that we can evaluate the consequences of each. If you have attributes that you feel would be good candidates, please open an issue for each so we can review and discuss.

[xoofx]

I am going to close this PR as it stands. I would prefer to take a more focused approach on this and consider each attribute on its own so that we can evaluate the consequences of each. If you have attributes that you feel would be good candidates, please open an issue for each so we can review and discuss.

Fair enough. Just keep in mind that the way NUnit is using explicit interface implementation for pseudo virtual dispatch through interface usage only is a more dangerous design choice than using plain virtual. If I can suggest one thing, remove them and use proper virtual there, at least.

As I can workaround it, I don't feel the strict need to try another PR on the code. On the other hand, I can't make a PR on the Wiki Custom Attributes, but adding something like this could help/warn future users:

• Disclaimer: NUnit does not recommend to extend existing NUnit Attribute but create your own attribute and derive from the following interfaces.
• If you are forced to derive from a NUnit attribute and want to override an interface, use explicit interface implementation. Keep in mind that they will work only if your methods are accessed through the interface they are implementing, which is the case how NUnit is accessing them internally.

[CharliePoole]

@xoofx A few comments, even though Rob has closed the code... Working backwards from your last comment...

Docs... I have had a task for a long time to write the page, chapter, book or whatever about how to extend the framework. I haven't yet done it for the usual reason... little time and spending what time there is on coding. I had not considered giving negative advice (not to extend attributes) but it might be a good idea. I think I would tend to say "NUnit attributes are not designed to be extended" and not much more.

"Ugliness" What code is ugly is a matter of style. I'm of the generation of coders who thought that sealed was ugly, for example. When it comes to style and taste, there's really no argument. Specifically, I learned about interface reimplementation from the Microsoft designers shortly after .NET 1.0 was released. They didn't think it was a hack or ugly, but a useful and important feature. I haven't found any reason to change my thinking.

However, if you say "fragile" my ears perk up and I'm glad to hear an explanation. Fragility of code is something we can reason about, whereas ugliness seems not to be. As the designer of NUnit's framework extensibility, my aim was to make it possible to develop extensions that could be packaged separately from the framework and would work across multiple framework releases. Depending on interfaces rather than the implementation of attributes was part of that strategy. As I move away from working on the framework and others take over, that may not be a goal any longer. If it isn't, then much of what you say makes sense. If it remains the goal, it would probably make sense to move more actively in that direction, faster, so that people are not left confused.

However, none of the points you are making WRT the difficulty of using the underlying attribute implementations make sense if the goal is to allow separate extensions that don't depend on the attributes. The upcoming team will have to make that decision.

Sealed... when you use a framework that's 17 years old, you have to allow for the fact that there are multiple - often contradictory - layers of history to it. Even NUnit 3 was initiated ten years ago! In the case of sealed many of us once considered its use an antipattern. That's why I don't feel so strongly about it's absence inviting users to extend attributes as I do about the presence of virtual on a method. That's just me. Once again, the newer team members have different backgrounds and ideas and may want to seal things that should not be extended.

BTW - this is similar although not related - the old team members (me) and the newer ones also have a completely different take on use of public versus internal. We're trying to figure out a clean, non-breaking way to gradually move to the newer notion that making a method public means that it's OK for people to use it. Lots of younger devs have grown up with that idea, but it wasn't always the case. The designers of NUNit didn't come from languages that supported a range of visibilities and tended to think you used anything that wasn't documented at your own risk.

I mention this because - as with sealed - it isn't something you can or should just change globally, because you can't know what people are accustomed to. It's very slow and tedious to look at each item and evaluate it individually but short of a major version change I think that's what you have to do in an application that wants to keep supporting its users. In fact, some users already think we have changed too much too quickly.

That gets me to my final point... it's best not to give us global changes. It's not a value judgement on your work, but simply advice on the best way to get things accepted and merged. From many years of hard experience, we have learned we work best when we focus in on very small things and do them one at a time. For example, with your current request, if I were making a unilateral decision you would have the best chance of having it accepted if it talked about making the change to virtual for either a single attribute or for all the implementations of a single interface. In the latter case, it had better be one of the simpler interfaces as well - for example, IApplyToTest.

Obviously, making methods virtual won't break any of our existing tests, but I'd still want to see some tests that showed it working for the purpose for which you were making the change. Those would serve as regression tests for future changes. You've said that you probably won't redo the PR, but this is my suggestion just in case.

---

PS: If you are going to go the interface reimplementation route, keep in mind that it's not exactly the same thing as explicit implementation, although the two are often used together. If your derived class states that it implements an interface, then you can implement the methods explicitly or not as you like and you must implement the entire interface.

---

PPS: (Really the last) This would take a lot of thought, but it occurs to me that we could support two kinds of extension:

1. Adhoc extensions within the test assembly itself, which might very well use inheritance
2. Packaged external extensions that relied only on the interfaces, assuming we packaged the interfaces separately of course.

Hmmm... If not NUnit, maybe my next framework. 😄

[jnm2]

However, if you say "fragile" my ears perk up and I'm glad to hear an explanation.

I said brittle, @xoofx said fragile. Here's a live example.

I also want to point out that the granularity of interface reimplementation is per interface method, not per interface. (example)

[jnm2]

C# should have classes sealed by default instead of just having methods sealed by default. It's a nice change from Java which should have been applied at the class level too. I don't disagree that writing sealed is ugly, but I've come to identify any of my own classes that aren't either sealed or abstract with healthy suspicion as to whether the abstraction I'm using actually matches the problem domain.

[xoofx]

Sealed... when you use a framework that's 17 years old, you have to allow for the fact that there are multiple - often contradictory - layers of history to it.

I completely understand this.

I used the argument sealed not to ask you that the attribute classes really should have it, but to put more dramatic emphasis that I really don't want them but instead that the usage of the custom attribute interfaces in the NUnit attributes should be honored - most of the time - by using virtual for the sake of the extensibility opportunities they already provide publicly (and also the fact that again, interface dispatch for a pseudo virtual inheritance is really really bad...)

Let's have a look at the attributes and interfaces usage:

	IApplyContext	ITestBuilder	ISimpleTestBuilder	IFixtureBuilder	IWrapSetUpTearDown	`IParameterDataSource
Category	x
CombiningStrategy	x	x
Culture	x
DefaultFloatingPointTolerance	x
Explicit	x
Ignore	x
MaxTime					x
Order	x
Parallelizable	x
Platform	x
Random						x
Repeat					x
RequiresThread	x
Retry					x
SetCulture	x
SetUICulture	x
SetUpFixture		x
SingleThreaded	x
Test	x		x
TestCase		x
TestCaseSource		x
TestFixture				x
TestFixtureSource				x
Timeout	x
Values						x
ValueSource						x

• IApplyContext: obviously, this one, you want virtual everywhere.
• For the builder attributesITestBuilder, ISimpleTestBuilder, IFixtureBuilder: I have already showed my use-case above for TestCaseAttribute where I wanted to build variations on arguments. I'm pretty sure I could come up with a use-case for each of the 7 attributes using them in the table above.
• For the IWrapSetUpTearDown, this one is also typically convenient to extend. I could write a RepeatWithMaxTime, inherit from Repeat, expose a MaxTime property, internally reuse the MaxTimeAttribute by composition, to recompose the Repeat command with the MaxTime command...etc.

I don't know enough well all these attributes, but I would be curious if you know one that a client with advanced scenarios should really not extend...

Sure, I could submit a patch only for the TestCaseAttribute and TestAttribute, as I have never used the others... and only care about my little use case... but I like to think about other people in the same situation than me on other attributes, and I'm likely too nitpick about these extensible interfaces and the virtual design consistency to make a PR only for a few of them... 😅

But hey, despite all my annoying insistence here, I still love NUnit (I have a workaround for this issue) as it has way more practical extensibility than any other .NET Unit Testing frameworks out there! 😉