Mark AssertionHelper as Obsolete

[CharliePoole]

This PR is part of the fix for #1212 but is not the complete solution. I feel like enough has gone into it that we should be able to decide how to proceed and that more work could be wasted if it goes in the wrong direction.

At this point, I'm building the AssertionHelper class in a separate assembly and have a separate test assembly for it. In addition, the class is now entirely in one file so users could easily incorporate it in their test projects if they want to use it.

Let's decide if this is the way we want to go. If we are going to separate it into it's own package, then we should proceed but I think we should be relatively sure that further changes will not be forthcoming so we don't disturb the users more than once!

[ChrisMaddock]

My vote is a separate NuGet package. If nothing else, that will give us an idea of usage.

Can we also [Obsolete] it at the same time? We can discourage it, and the revisit eventual removal based on usage levels.

[CharliePoole]

@ChrisMaddock It seems a bit odd to me to release a new package that is already obsolete. OTOH, we do want to somehow indicate that this is an old facility with no plans for further improvement and will eventually be removed or archived.

If we use Obsolete with Error=false, then users will start getting warnings, if they have set warnings as errors, they will have to insert pragmas in their code to stop the warning. That seems a bit heavy-handed if we are not planning to remove it in the following version.

So I think we should decide what our plan is for the future. One possibility is that we will keep supporting this package but not enhance it. Eventually, we would decide that we no longer wanted to keep rebuilding it for each new NUnit release. At that point, we could mark it as obsolete for one final release. @rprouse What do you think?

Here's a crazy thought for a more general solution to the legacy problem. What if we had an attribute NUnit.Framework.LegacyAttribute that added a property to a test indicating that it used some legacy feature. I could also include a field that indicated how strongly we want to discourage users... maybe Allow, Warning, Error. We could then issue some warning ourselves when running the test. If this sounds like a way to go, I can create an issue.

[CharliePoole]

In the latest commit, I have removed AssertionHelper completely. I'm working on a new repo for it.

I'll actually do another commit before we merge, squashing out all the intermediary changes.

[CharliePoole]

I replaced earlier incremental changes with a single commit, removing the AssertionHelper entirely. I'm working on a new repo for AssertionHelper.

We can either merge this now or wait for the new repo packages to be available.

[rprouse]

I disagree with the [Obsolete] attribute, but am good with merging this and releasing a separate repo. Sorry, reviewing on my phone, but does this reference a specific version of the NUnit Framework? Is it like NUnitLite in that we will need to release them in lockstep? If not, then I am okay with a separate repo and NuGet package since we will likely make few changes, but if we have to update and release with every release of the framework, I don't want to add to our release burden.

[CharliePoole]

@rprouse

Well, yes and no...

Yes, it has to reference a separate version of the framework. All framework extensions, custom attributes, etc. work that way, whether by us, by third parties or by users.

But No, we don't have to release in lock step. If a new framework release comes out and we don't release a new AssertionHelper then those users who depend on AssertionHelper will not be able to upgrade. The question for us, I think, is whether AssertionHelper is so important to us that we would feel obligated to release it each time a new framework is released.

Basically, I think we need to make a policy decision and then go in one of several directions as outlined in the issue itself.

[ChrisMaddock]

We still don't really have an idea of the usage of this - we suspect it's low, but we're well aware we normally only hear about such things after a breaking change!

I suggest, for the next release, we release it as a separate NuGet package. From there, we can get download stats/feedback, so we can work out usage level. The following release can potentially Obsolete - if we see usage is low enough/nobody wants to take over.

If a new framework release comes out and we don't release a new AssertionHelper then those users who depend on AssertionHelper will not be able to upgrade.

This sounds to me like dropping it. I personally think there should be at least one 'obsoleted' release before just discontinuing, to give users a chance to gradually convert tests over, or make a decision to fix their NUnit version.

I'd also agree with Rob, that while we're releasing in sync with the framework (at least the next release, hopefully) - that's easier from the same repo.

NUnit.Framework.LegacyAttribute feels to me like reinventing the wheel - I don't see the advantage over System.ObsoleteAttribute?

[rprouse]

I agree with @ChrisMaddock, I would prefer that this is at least initially a separate project or even solution in the main solution so that we can release it at the same time. I think there will be two potential outcomes,

1. We will see that the package is barely used. In which case we can obsolete it and then fully remove it in a couple of releases.
2. We will see that it is used, in which case we can move it to another repository and find a maintainer.

I would also suggest that when we move it to another repository (now or later) that we switch to using only Visual Studio 2017 for it so that we can use the new CSPROJ format so that we can multi-target. This would allow one project file to target .NET 2.0 -> 4.5 and .NET Standard.

[CharliePoole]

I guess I originally misunderstood your suggestion of a new package as implying a new repo. But that's why I did this in stages. I'll roll back to the commit with a separate project and we can go on from there. I'll reply to some of the other comments when I'm on my computer.

[CharliePoole]

Well, I did this as separate commits, in stages, but it looks like I squashed them all! I have the projects for assertionhelper separate locally, so I'll put them back.

[CharliePoole]

@ChrisMaddock Releasing an assertionhelper package on a different cycle from the framework is not the same as dropping it, although it would be an inconvenience for whomever uses it. For example, if we released it on each minor upgrade, but not on hot fixes, users of the package would have to choose between getting the hot fixes and being able to use the assertionhelper. Some of them would stop using it, in that case - not necessarily a bad thing.

In the future, we could stop maintaining the package entirely. That would mean somebody else could take it over. I think we have to remember that this is open source, not a commercial product. Anybody is free to fork our package and maintain it. If somebody wants to use it badly enough, that will happen. This is a powerful thing. Among other outcomes, it makes it possible for us to decide what we want to work on and not be obligated to work on other things. However, separating it out in some way is a prerequisite.

I think we should obsolete features that are part of packages we maintain. But if we are going to stop maintaining a package like AssertionHelper, it isn't obsolete, it's just a package looking for a maintainer, which is an (albeit subtle) distinction. This is exactly the status of several of our old packages: nunitv2, nunitlite (1.0), nunit-cf, nunit-silverlight.

For the above reasons, I still feel that we have to choose between marking AssertionHelper obsolete and breaking it out into a separate package. When we mark it obsolete, we are deciding it will go away. When we put it into a separate package, we don't know what will happen in the future.

[rprouse]

Personally, I am for obsoleting and removing. It hasn't been well maintained by us and is missing much of the functionality of the constraints or even the classic asserts, so anyone using it is likely not using many new features and has few reasons to upgrade. That isn't a hard stance, just an opinion 😄

[CharliePoole]

@rprouse Thought I had replied to your comment yesterday, but I don't see it here.

I"m OK with removing it. You said you prefer obsoleting and removing it. Do you mean obsolete it for 3.7 and then remove in 3.8?

[rprouse]

@CharliePoole yes, Obsolete in 3.7 and remove in 3.8 or 3.9 depending on the reaction.

[CharliePoole]

OK... I'll be back soon with that change.

[CharliePoole]

I obsoleted Assertion helper and also used the version of AssertionHelper I created for the separate package. It combines the code for ConstraintFactory, which was previously used in several places but is now only used by AssertionHelper.

When we get around to removing AssertionHelper, we can post that source file somewhere and it can be included in the test file by anyone who wants to use it or even improve it.

[rprouse]

Not sure if Obsolete is needed on the tests, but it is a good reminder. Looks good.

[CharliePoole]

Obsolete is needed on the test class because it inherits from AssertionHelper.

[CharliePoole]

Re-titled and labelled for inclusion in next release.

[psandhu79]

+1 We are still using it.