Remove AssertionHelper and AssertionHelperTests

[ChrisMaddock]

We obsoleted the AssertionHelper over a year ago in NUnit 3.7. There is also a community NuGet package (NUnit.StaticExpect) which functions as a drop-in replacement.

I think it's been sufficiently long enough that we can now remove the two relevant classes. @nunit/framework-team - anyone disagree?

[mikkelbu]

I think it is fine to remove the class and the tests (perhaps the tests should be "moved" to NUnit.StaticExpect?).

[madelson]

I'm not on the framework team, but as a consumer of NUnit, I would advocate for following SemVer and removing this in the 4.0 release. Is it actively harmful to keep this code around?

[CharliePoole]

IMO it's only harmful if we update it. In the past, we updated it to match every feature that was added to the fluent constraint syntax, but I don't believe that has been done for the past few years. It's deprecated now, right?

[ChrisMaddock]

It was obsoleted a year ago, yes. 👍

@madelson Historically we haven't followed SemVer. But no, it's not actively harmful, just code-cleanliness. 🙂

It was 13 years between NUnit 2.0 and 3.0 - which was a major change to the structure. Historically (for better or for worse - I'm keeping my opinion out of this!), we've not used a major version change when making 'minor' changes. There's been many discussions about this over the years...here's one of the most recent I remember! 😄 #2410

[CharliePoole]

It would be interesting to have a general discussion of SemVer pros and cons. Not sure where to do it.

[ChrisMaddock]

Oops, sorry @CharliePoole, I was editing while you were posting! I reckon #2410 is a good place for that, it's been a well discussed matter! 😅

[CharliePoole]

Wow! Now that you remind me of the time we spent talking about it in #2410, I'm not sure I want to start it up again. 😕 I'll take a shot at a post, however.

[rprouse]

@madelson are you using the assertion helpers? If so, would adding the drop in replacement package be overly difficult?

[madelson]

@rprouse we have hundreds of NUnit test projects in our code base owned by dozens of different teams with widely varying levels of active development. So the short answer is that I have no idea ;-)

Mainly, my objection to this change is that as a consumer of NUnit I think that following basic Semver is really helpful and I wish the project would adopt/stick to it; I was honestly really surprised to learn that this is not the case as pretty much all the other .NET packages we use are Semver at this point.

As discussed in the linked post, to do Semver you need to have a working definition for backwards compatibility so you know when to make breaking changes. In my experience the definition that seems broadest in adoption is to (a) consider any binary breaks as breaking changes and (b) use judgement around behavioral breaks and source breaks (subject to estimated impact / whether the old behavior was a bug or not).

I'm particularly concerned with making binary breaking changes in minor versions, and I'll try to explain why.

In addition to being big users of NUnit in our test projects, at my company we also publish loads of internal NuGet packages as one means of sharing code. A subset of these internal packages are "testing packages" which are designed for consumption in test projects. Testing packages contain things like internal assertion libraries, setup code for common scenarios, integration with other frameworks like Selenium, fakes for core services, etc.

Most of these testing packages take the NUnit package as a dependency. To ensure a compatible version, they will express this dependency as something like [3.5, 4) where 3.5 is the version the package was built against.

Now, let's imagine that my team is consuming NUnit 3.11 as well Company.TestingPackage which was built against 3.6. I don't know it, but Company.TestingPackage references one of these deprecated APIs under the hood. One day, I try to update NUnit to 3.12 to get a new feature and unbeknownst to me that version made a binary break. All the sudden, some percentage of my calls into Company.TestingPackage start failing mysteriously with MissingMethodException! One really annoying thing with binary breaks is that these failures happen at JIT time, so you can get a failure even if your code doesn't hit the exact line where the removed method would be called. Hopefully, I can (a) figure out that this is the problem (non-trivial in particular for engineers less experienced in .NET) and (b) find a new version of Company.TestingPackage built against a more recent NUnit which no longer references the deprecated method in its internals. If I can't, then I need to go talk to the team that owns the package and work with them to publish a new compatible version.

It only gets worse if I didn't actually intend to update NUnit directly but instead I updated or installed Company.SomeOtherTestingPackage which was built against NUnit 3.12 and that is what triggered the NUnit update.

This is the kind of thing that Semver is intended to prevent, and if followed it actually works really well. In the Semver world, the breaking change would have been a 4.0 release, which as a consumer makes me immediately wary that things might break. Indeed, if I try to update to 4.0, NuGet will either tell me that Company.TestingPackage is incompatible or will automatically trigger an update to a compatible version. Furthermore, a new major version coming out is something that gets package publishers' attention, meaning that it is far more likely that someone is already updating Company.TestingPackage to be compatible.

Anyway, sorry for the huge wall of text but hopefully this provides some context into why I'd prefer to see the NUnit project take a stricter stance on versioning and breaking changes.

[CharliePoole]

I wonder what version we would be at now if we had followed SemVer. Maybe instead of 3.11 it would be 14.0. Alternatively, maybe we would not have made certain changes, including fixing some bugs.

But if we had released three or four major updates per year, would the versioning resolve the problem? I have to imagine that there would be the same issue with the changes, whether we used semver or not. And I'm doubtful that a major version change would get the same level of attention if we had done 11 of them since NUnit 3.0.

I post this to suggest that use or non-use of semver isn't really the problem. It's the changes themselves.

One thing I have not been able to find in all the discussions of breaking change is this: how does a project that has a legacy of public methods and types it would like to change dig itself out of that hole in the first place? Just changing the policy of versioning doesn't seem like it would do the job.

[madelson]

@CharliePoole as a result of following Semver I suspect that NUnit would both be on a higher version and would have chosen to make fewer breaking changes (or batch those changes together into larger releases).

As evidence to the first point, Json.NET is on major version 12.0.

Personally, I'm much more in favor of batching together binary breaking changes and disruptive behavioral breaking changes into fewer major releases rather than divvying them up incrementally over time. It's still possible to divvy up the underlying work (e. g. go in and mark things obsolete, remove non-obsolete classes dependencies on obsolete functions, etc). You could even separate out all obsolete APIs into their own partial classes and files and file them away into a separate "Deprecated" folder (with namespaces intact). On the next major release, just delete the folder.

[CharliePoole]

I'm personally pretty much in agreement with the principle. It's tough however when the project probably has many more publicly accessible types and methods than we ever intended to support publicly. This of course goes back to the history of the project, when making things public was the only way to test them. In retrospect, we should have started hiding internals with the start of 3.0!

I appreciate your input and it will definitely have an affect on my next project. For NUnit, I think it would be great to have a major release where a lot of things that were never intended to be exposed got hidden once and for all!

On the particular issue of the AssertionHelper, I think it should stay until that major release. I do like the idea of putting all the stuff that is going to be removed in one easily deleted folder!