Supporting inheritance of Assert and related classes

[jnm2]

In #2836, the subject came up of allowing Assert and other *Assert classes to be extended through inheritance the way we do for Is, Iz, Does, Has, Throws, and Contains.

A past team, before NUnit 3, had decided that Assert should be inheritable:

nunit/src/NUnitFramework/framework/Assert.cs

Lines 53 to 62 in d411123

	public partial class Assert
	{
	#region Constructor
	/// <summary>
	/// We don't actually want any instances of this object, but some people
	/// like to inherit from it to add other static methods. Hence, the
	/// protected constructor disallows any instances of this object.
	/// </summary>
	protected Assert() { }

@CharliePoole asked for us to reconsider these classes, keeping in mind that, “The current team has never actually decided that we want people to be able to do this.”

➡ What do I mean by “supporting inheritance of Assert,” since it is currently inheritable? From my best understanding of #2836, I mean keeping it in the list of classes we test to ensure inheritance which we started in #2839.

[jnm2]

I think we should explicitly support the same for Assert as for Is. Here is a real-world example of why people might want to inherit from Assert: DotNetAnalyzers/AsyncUsageAnalyzers#52 (comment) (in fact, that's what made me remember to open this issue afterwards).

It seems the same likelihood might exist for the other *Assert classes. If we decide to allow Assert to remain inheritable, there a good reason to split the other classes apart from Assert in this regard? (@CharliePoole had suggested that some good reasons exist.)

[CharliePoole]

I'm not sure which way I would go on this, but it's worth looking at the pros and cons.

The original NUnit 2 team left this "feature" in to allow people to write their own assert methods. Of course, this was back in the day when we only had "classic" asserts like "AreEqual" or "IsNull". Folks would add things to it like "IsEvenNumber". (silly example)

Since then, two things have happened...

1. We now have constraint-based syntax and a test for even numbers can be constructed by creating a custom constraint and extending Is... Assert.That(val, Is.EvenNumber).

2. We now have some heavy-duty boilerplate stuff in the Assert class. We count asserts and record warnings for example. We don't want users to be messing with that stuff. This is not a deal-breaker, however, since we could change visibility of those things if necessary.

I think it comes down to whether we want to encourage or discourage this kind of extension. I lean towards discouraging it. We ourselves are not adding new Classic assertions to Assert any longer so I don't see the point in helping users to do that. A new form of That or Throws is probably more suitable for us to maintain rather than users, if it is needed.

Finally, we should remember that this type of extension is for syntax only. Nothing prevents the user from creating the extension so that it is called as a method of some other class rather than Assert.

In the end, however, I see it as a tossup so long as we don't have to do any work.

As far as other Assert-like classes, there are two groups...

1. Those that function like assert but do something different on failure: Assume, Warn and any similar ones we add. I think we need to do the same thing for all of them.

2. Classes that do special kinds of Asserts:

CollectionAssert

This has a special place in that it was written to map to the features of VS CollectionAssert. I think we should keep it that way and not encourage people to extend it. Note that there's nothing here you can't do with Assert plus the constraint syntax.

DirectoryAssert

We just limited it and may want to phase it out. For that reason, I wouldn't encourage extension.

FileAssert

Same as DirectoryAssert

StringAssert

The sole purpose for it's continued existence has been that it gives a nicer syntax than we had for string tests using constraint syntax. I'd like to see us figure out a better syntax and phase it out as we once did with TypeAssert. So I wouldn't want folks to extend this either.

[jnm2]

For folks working in codebases which aren't using constraint syntax, who don't want to switch styles, the reasons we allow inheriting from Does and Throws also apply to the classes in group 1.
I'm saying this looking at the workaround I gave Johan, but even I don't use the constraint syntax for everything.

I really agree with you about group 2 classes.

We now have some heavy-duty boilerplate stuff in the Assert class. We count asserts and record warnings for example. We don't want users to be messing with that stuff. This is not a deal-breaker, however, since we could change visibility of those things if necessary.

Just to clarify, users already cannot mess with those things any more than they could if Assert was static.

[ChrisMaddock]

I think it comes down to whether we want to encourage or discourage this kind of extension. I lean towards discouraging it. We ourselves are not adding new Classic assertions to Assert any longer so I don't see the point in helping users to do that.

I sorta agree with this point. We have a general intention to no longer update the classic syntax - I'd say that's a good thing, as maintaining a single syntax is enough work!

The classic format is still around for almost 'legacy' purposes. Yes, there's inconsistencies in the current extensibility, but I'd think we'd be better to consider the extension of Assert as a legacy feature rather than offering it in more places.

That said - I have no reason to refuse this feature if anyone particularly desires it - I just get the impression we're adding more to test and support for API consistency, and more functionality to the classic assert format - whereas I'd rather go the other way and keep it intentionally locked down and narrow as much as possible, despite inconsistencies.

[CollectionAssert:] Note that there's nothing here you can't do with Assert plus the constraint syntax.

I don't think this is strictly true anymore - I think we added some variance in #2097 (comment). With hindsight, I'm not sure if that decision really fits our general direction.

[jnm2]

As far as I know, there is a very minimal amount of extra work for us to do either way, since I don't think we should continue in this superposition of support and non-support:

• If we decide not to support inheritance, we should stop effectively supporting inheritance and make Assert, CollectionAssert, etc, static.
  I would ask this though: What's the actual cost of continuing with this support, which we already have, versus becoming more opinionated and restricting a traditional explicit extensibility point?
  A past team has broadcast to all users that this is a low-cost way to extend NUnit syntax, and we have supported it ever since. From any user's perspective, we have already decided that 3.0–3.10 would continue this support. Is there enough reason to depart from this in 3.11?

• If we decide to support inheritance, we should add Assert, CollectionAssert, etc to the list of classes we test to demonstrate that inheritance is possible, as intended.

At some point if we don't make these static I'm going to be coming through and making them abstract since that documents our actual intent, and they shouldn't be instantiable.

[ChrisMaddock]

So that comment suggests we support it for Assert. Based on that, I think it would be good to have tests for Assert specifically, but we shouldn't 'add' this functionality to any other related classes, unless it's already possible to do/we have user requests to do so. 🙂

[jnm2]

@ChrisMaddock So here's what I'm struggling with which maybe I didn't make clear: it's already possible to inherit from Assert, CollectionAssert, and StringAssert. It's not possible to inherit from FileAssert and DirectoryAssert.

I want to leave things static that are already static, but I'm not sure we have a good enough reason (e.g, saving work, being opinionated) to introduce a new change by making Assert, CollectionAssert and StringAssert static.

[ChrisMaddock]

I agree with that! 😄

Let's test what is currently possible, to prevent potential breaking changes for users in future, but not spread this any further.

[jnm2]

I also understood Charlie to be saying here that one of our tenets is to be unopinionated. I may be missing some vital context, but it seems inconsistent with that tenet to remove functionality from Assert by making it static.

So Chris and I are in favor of keeping Assert, CollectionAssert and StringAssert inheritable as they have been and adding them to the existing list of classes to test for continued inheritability, and leaving static classes static.
Charlie, @rprouse, is this okay with you?

[CharliePoole]

@ChrisMaddock I believe that #2097 was implemented in a way that let's you do the same thing with Assert using the AsCollection modifier.

@jnm2 I don't think this falls under the heading of being (un)opinionated, a classic framework issue. It's about what our vision for the product is.

I propose we simply do nothing here, as both the simplest and the easiest solution. As a second preference, you could add tests but it seems unlikely that anyone would arbitrarily change the code for no reason at all.

[jnm2]

As a second preference, you could add tests but it seems unlikely that anyone would arbitrarily change the code for no reason at all.

I'll add tests then, since that is close enough to how static was added to Does.

[CharliePoole]

OK! FWIW, I don't think static was added to Does... it was created that way because it's new and that's the way people do such things nowadays. 😄