NUnit.Framework.Does cannot be extended

[richard-fine]

The documentation for custom constraints demonstrates incorporating custom constraints into the fluent syntax like this:

Provide a static class patterned after NUnit's Is class, with properties or methods that construct your custom constructor. If you like, you can even call it Is and extend NUnit's Is, provided you place it in your own namespace and avoid any conflicts.

That works fine for NUnit.Framework.Is, but NUnit.Framework.Does is marked as static, and so cannot be extended in the same way.

Given that it serves basically the same role and is only really there to make the assertion read more nicely, I think it should also be extendable, for the exact same reasons that Is is extendable.

[jnm2]

Thanks for reporting the inconsistency! This was not by design. We should use abstract here.

Would you be interested in contributing a fix with a test ensuring that Does is not sealed? It would be cool (though not necessary) to have tests on all such classes, including Assert and CollectionAssert, etc.

[CharliePoole]

@jnm2 At a glance, the other constraint-generating helper classes (Has, Contains, Throws) all follow the pattern of Is. Assert is another matter, since we have never suggested extending it. I'm not saying that it's a bad idea, just that we should not conflate two things simply because the same code change is needed. If you think Assert should be extensible in the same way, I suggest you create an issue where we discuss the pros and cons. Other assertion classes could be discussed in the same issue, but I think that there are strong arguments against doing it for some of them, which I'd be glad to specify in discussing.

Or, if you prefer the short form, for this issue, let's just fix what has been reported. 😈

Regarding tests. We wouldn't compile if Is suddenly became sealed, because Iz derives from it. Another approach, would be to add a protected constructor to each class.

[jnm2]

@CharliePoole This is why I was pretty sure that Assert was in the same genre:

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() { }

Since it already is extensible in the same way, with a comment making the intention explicit, I'm thinking we should just test it at the same time.

The benefit I see in tests, versus just adding a protected constructor, is that we are keeping the cross-cutting spec in one place.
Also during refactoring, it's a two-step process. For example, I might add static and remove the protected constructor, thinking we didn't want one. Then the test starts failing and at that point I'm confident that it was by design and don't have to waste time asking.

[CharliePoole]

@jnm2 This is an area where we have previously disagreed, so I'm not expecting to convert you. 😸

However, for the sake of discussion, I'll point out that the comment is a carry-over from NUnit V2. The current team has never actually decided that we want people to be able to do this. Such a decision could probably be made with minimal discussion, but I still believe it should happen. We have enough "features" that we never decided to have.

I was ready to push a quick PR to fix what @richard-fine asked about when I read your initial comment. I'm not opposing everything you suggested, just saying that a discussion is needed, and we should have it separately so as not to slow things down. I read you as saying that your extensions to the request are so obvious that we shouldn't have to discuss them. That's fine as a first position, but once somebody says "No, I think there's more to discuss here," I believe we should just have the discussion.

I think it's reasonable to fix what @richard-fine has pointed out and to make the other constraint-generating classes work the same way in a single PR. Obviously, I think tests are essential.

However, I'd prefer to discuss Assert and similar classes separately before we do it.

[jnm2]

@CharliePoole

I'm not opposing everything you suggested, just saying that a discussion is needed, and we should have it separately so as not to slow things down. […] That's fine as a first position, but once somebody says "No, I think there's more to discuss here," I believe we should just have the discussion.

You and I are on the same page here! 😃 Did you have a PR close by, or can I put it up for grabs for tests on all the constraint syntax classes?

[CharliePoole]

I'll submit the PR.

[jnm2]

Charlie, I was so busy thinking of what was important to myself that I failed to listen to your request and your comments in this thread. I dislike this and there's no excuse for it. Would you please forgive me?