Separate AssertionHelper project or assembly

[CharliePoole]

Issue #1211 brings up the question. Maintaining a separate syntax using AssertionHelper is a bit of a burden and we have not kept it up to date. Perhaps it's time to drop it.

I'm creating this issue in case there is a silent fan base for AssertionHelper, so that folks may speak up in its favor. If we keep it, we should update it, but let's decide whether we want to keep it first,

A suboption would be to remove it from NUnit and publish it as a separate addon.

[rprouse]

I didn't even know we had an AssertionHelper 😨

[CharliePoole]

I take that as a vote for removal. :-)

[oznetmaster]

I never knew we had one either :(

[oznetmaster]

What does it do, or what is it supposed to do?

[CharliePoole]

Your test class inherits from it, giving you a syntax for assertions that doesn't require a static class name. See, for example, https://github.com/nunit/nunit-csharp-samples/blob/master/syntax/AssertSyntaxTests.cs and scan for "Expect"

As I commented in #1211, it has not been kept up to date with the latest features of the Assert.That syntax. Ideally, if we keep it, I think it should be spun off into a separate project and maintained by people who actually use it.

[rprouse]

As I commented in #1211, it has not been kept up to date with the latest features of the Assert.That syntax. Ideally, if we keep it, I think it should be spun off into a separate project and maintained by people who actually use it.

I agree. How many ways to we want to support writing asserts? Keeping Assume in sync with Assert is hard enough 😄

[appel1]

In many cases you can use "using static nunit.framework.Assert" instead I think. So I don't mind if it was removed.

[OsirisTerje]

Agree with the above, didn't know it existed either.
Remove or move it out just for safekeeping.

[OsirisTerje]

Could we extract the helper in a nuget package - just create one manually for now since it is a kind of probe, and then see how many would download it ?
If it gets lets say less than 1000 over a 6 month period, then we drop it.

[MSK61]

I use it frequently in my projects, but I'm strongly in favor of phasing it out of NUnit(by either complete removal or moving to subsidiary package). I can easily refactor my projects to do without it; it's just a couple of find and replace for Expect(and the compiler will take care of the rest flagging errors where appropriate).

[CharliePoole]

Looks like a feature. :-)

We could do this in an adhoc way, but I'd like to use it as a pilot for something that was in the original design for NUnit 3. There was this idea that you could layer additional syntaxes on top of the framework assembly. This one is pretty uncomplicated and we might use it to come up with a design for alternate syntaxes added on to NUnit. That would mean it would become a separate project and download, as Terje suggests.

In and of itself, this is a low-priority feature, but we might push it forward in order to break out the design needed to layer on top of the framework. Should be pretty simple to do.

[CharliePoole]

@MSK61 Alternatively, are you interested in helping to maintain it if we move it to a separate package?

[CharliePoole]

PR #1250 has now isolated the use of AssertionHelper within our own tests. It is no longer used at all except in AssertionHelperTests. This enables us to easily separate it or remove it.

I have marked this issue 'status-design' since we need to actually decide what we want to do before proceeding. Here's my take...

I don't think we should eliminate it because at least some people use it. The fact that it's a small number doesn't so much matter to me. We went to 3.0 without deciding on this as a breaking change so I would argue it is too late to do so now. NUnit has a long tradition of continuing to support users way beyond what most commercial projects do. I'd like to keep the tradition.

Further, I think that deciding how to handle AssertionHelper is a useful thing for us to do. The design of NUnit 3.0 originally called for alternative syntactic layers on top of the core of the framework. We set that aside for the initial release but I'd like to get back to it. AssertionHelper is relatively trivial - two files plus one test file - and we can concentrate on the desired architecture for syntactic front ends as a part of doing this.

[In case you have not seen the design docs for a while, syntactic adapters for the framework were intended to serve two purposes: alternative syntaxes for using NUnit and alternative language front ends. Existing 3rd party apps in each category are shouldly and fsunit. We planned to provide a standard way for creating syntax adapters on top of the framework, use it ourselves and offer it to 3rd party developers as a way of encouraging NUnit use. As stated above, this was too big a challenge to get into 3.0, so we have not implemented it.]

Back to AssertionHelper... here are some options...

• Keep it as a part of NUnit
  • Separate namespace (I don't like this because it breaks compatibility)
  • Separate assembly
• Make a new repo
  • Source code only - folks can include it in their own projects
  • Distribute Binaries
  • Do both (similar to what fsUnit does - this is the option I like best)

If our direction includes the distribution of binaries, we have to decide which builds to support. It could be all our existing builds, of course, but I'd be OK with limiting it to .NET 4.5 and Portable. We could add more if their were a demand. Users never tell us if we do too much, but they usually speak up if we do too little. :-)

I would favor moving to use of separate configurations rather than separate projects for each build type. That would limit the number of projects we have to define and we could use conditional compilation as needed. AssertionHelper includes a few features that have to be excluded in the Portable build.

Please add your comments here. When it looks like we have a consensus, I'll move ahead.

[ChrisMaddock]

This seems like a great way to 'deprecate' functionality, by moving it in to a separate repo and add-in packages. It's then clearly a separate maintenance concern, and can be updated just when new features are specifically asked for. (Which based on this topic, may be rarely!) It would also be significantly easier to break out than the other framework builds that are on the list, which would potentially create maintenance problems.

To revitalise the discussion...

I think a separate repo, assembly and nuget package sounds ideal. It would require a new project reference for users ('breaking change') - but seems acceptable, only similar to what's just been required for .NET 2.0 with NUnit.Linq.

Separate namespace (I don't like this because it breaks compatibility)

Agreed the current namespace should be maintained, which is a shame if it's going to be an add-in, but the compatibility issues would outweigh the 'ugliness' in my opinion!

It could be all our existing builds, of course, but I'd be OK with limiting it to .NET 4.5 and Portable.

For 'legacy' functionality, would it perhaps make sense to include/only do the older targets?

I would favor moving to use of separate configurations rather than separate projects for each build type. That would limit the number of projects we have to define and we could use conditional compilation as needed. AssertionHelper includes a few features that have to be excluded in the Portable build.

I assume by this you mean, a single VS project, and instead target it to different runtimes at compile time, i.e. via Cake? If so, I agree.

[rprouse]

Won't this still be tightly coupled to the framework because of its need for the various assertions under the covers? If that is the case, then a separate assembly would link to a specific version of the framework and would have to be released in sync. If that is the case, then isn't it easier to leave it where it is?

[ChrisMaddock]

I presumed there would need to be something equivalent to nunit.engine.api at framework level? I wasn't sure if that was the "syntactic adapter" idea @CharliePoole was referring to.

(I don't have much of an overview of the code here as to how possible that actually is!)

[rprouse]

I'm sure we can figure it out with some sort of indirection like the engine uses, I just wonder if it is worth the effort :-)

[CharliePoole]

Well, it's low priority, so not worth a ton of effort. :-) Something to consider when we decide how to do it.

@ChrisMaddock I've been using priorities slightly differently for issues that are part of Epics. I mark those low that may end up being optional as far as the Epic goes.

@rprouse I think some sort of indirection is possible but that would make it dependent on a much more ambitious refactoring of the framework, similar to what the xunit guys have done. I've thought about that but not created an issue beyond just separating assertions. Maybe it's 4.0 material. :-)

@ChrisMaddock Yes... I was suggesting we have configs like "Portable - Debug", "Desktop - Release", etc. Or maybe we could use the new netstandard definitions instead.

For a low-pri task like this, my first choice would be to let it sit for a while. My second choice, if somebody is dying to give it a shot, would be to make a separate assembly within the existing solution. But really, I'd rather see it wait until after the epic is complete.

@rprouse I was going to post my proposed order of doing the sub-issues on the Epic but was waiting for your comments on it first. Should I just go ahead and post?

[rprouse]

@CharliePoole go ahead and post. We've talked about it enough that we are both on the same page.

[fluffynuts]

I agree -- whilst it doesn't impact me, NUnit has a very wide reach and I'm sure there's someone it would impact.

[fluffynuts]

After discussion with @BobSammers, who has made valuable contribution to NUnit.StaticExpect, I'd just like to remind anyone subscribed to, or stumbling across this thread that NUnit.StaticExpect (nuget: https://nuget.org/packages/NUnit.StaticExpect) provides a fairly effortless drop-in replacement for AssertionHelper. The GitHub page is also a good place to raise issues and discussion, if required.

I didn't want to be a troll on this forum, but @BobSammers pointed out that perhaps people didn't realise the completeness of the solution and how easy it is to switch over, so I'm just putting that out there again. Under the hood, it's still making "proper" NUnit calls (indeed, many of the features are literally implemented as pass-through properties), so this really just provides the syntactic sugar AssertionHelper users have been used to whilst allowing them to prepare for the eventual removal of the class.

[ChrisMaddock]

Great to see NUnit.StaticExpect getting some attention @fluffynuts - this is a great solution to the problem!

---

@nunit/framework-team - I think we're probably close to being able to wrap this issue up. Can I propose the following, based on where we are now?

1. AssertionHelper remains obsolete and unmaintained for the rest of the 3.x series.
2. AssertionHelper is removed in 4.0
3. We update documentation and the ObsoleteAttribute to recommend NUnit.StaticExpect as a drop-in replacement.
   4. NUnit.StaticExpect lives on as a third-party library, which now provides an alternate syntax, as opposed to a 'legacy' syntax. We previously talked about doing this within the NUnit org - I don't see any reason to bring this into the organisation any more. @fluffynuts - if you disagree at all, I'd personally have no objection to moving this into the org - but I think what's key is that your remain maintainer as an independent project. 🙂 (To be discussed in governance)

All thoughts welcome - @rprouse should probably make the final call here, given the implications.

[fluffynuts]

@ChrisMaddock I don't have any problem with bringing this into the NUnit organisation at all -- indeed, I think it might be a good idea because it will look like a more "official" alternative instead of some code cranked out by an arbitrary code-monkey on the interwebs (: Not sure how this is achieved though, so I welcome information (:

I also intend to remain as maintainer -- but it might be a good idea, if there's enough interest in the project, to provide full access to someone else (perhaps @BobSammers, who has contributed already), for those typical "run over by a bus" moments.

The same should be done for package management on nuget.org, for the same reasons.

[ChrisMaddock]

@fluffynuts Ok! 🙂

Let's discuss the future of NUnit.StaticExpect separately, in governance - and keep this issue for deciding what to do about the current NUnit.Framework.AssertionHelper - so we can get this boxed off. I'm out of time for now, but will write up an issue tonight to start the discussion.

[CharliePoole]

Keeping to the topic of this issue, how does the existence of NUnit.StaticExpect impact our decision about AssertionHelper? I had not heard of it before and it looks like an excellent solution. I was ready to drop AssertionHelper cold - even without a replacement - and still am. What about other @nunit/framework-team members? Does NUnit.StaticExpect as an alternative lead you to be ready to drop it?

As a minor matter, I would not call this a "drop-in" replacement since the user has to change the code. It's a small change, but still a change. I suspect it might only be a replacement for C# users as well. In any case, I'm hoping it's enough of a replacement to cause us to accelerate dropping AssertionHelper.

[jnm2]

Does NUnit.StaticExpect as an alternative lead you to be ready to drop it?

If our release notes, and what people will see conspicuously in the NuGet package manager, show a warning at the top of the description like, "Warning: the obsolete AssertionHelper has been removed. To continue using it, you must also reference the NUnit.StaticExpect package." — then personally I'm equally happy doing it next minor release or continuing to wait.

[CharliePoole]

@jnm2 The notice would have to be more like "reference the NUnit.StaticExpect package" and

• stop inheriting from AssertionHelper
• add a using statement
• possibly some code fixes (@fluffynuts is there anything else?)

I'd still drop it myself. IMO StaticExpect's use of static methods instead of inheritance is a significant improvement.

[fluffynuts]

@CharliePoole I can't think of anything to add, offhand, though most of what you mention should be in the README.md already (and if there's anything which isn't, it should be added; indeed, this README should, imo, provide all the information a person needs to switch -- because it would be silly and annoying if it didn't!).

So if you have the time, I'd appreciate a review and commentary on https://github.com/fluffynuts/NUnit.StaticExpect/blob/master/README.md such that it incorporates, in an easy-to-follow manner, the steps required to move from AssertionHelper to NUnit.StaticExpect. If that README.md becomes clear and definitive enough, it might be good enough to make the deprecation notice something like "AssertionHelper has been deprecated in favor of NUnit.StaticExpect. Read https://github.com/fluffynuts/NUnit.StaticExpect/blob/master/README.md to find out more".
Thoughts?

[CharliePoole]

@fluffynuts The only thing I'd want to see added would be a few more examples, e.g.:

• Use of constraints (replacing Is and Has syntax)
• Use with other than Assert/Expect: e.g. Assume, Warn.

I just wanted to be sure it's a complete replaceme (or give notice if it isn't)

What's the deal with non-C# code? Can you use it in VB for example?

[CharliePoole]

@fluffynuts To be clear, I'd dump AssertionHelper without a replacement but I've been in the minority. However, if we recommend a replacement, I'd like to make it completely clear what it replaces and what (if anything) it doesn't. I like the idea that your readme should explain everything.

[fluffynuts]

@CharliePoole I'm totally on-board with the idea that a readme that the user is directed towards should be explanatory and clear. I'm also aware that I may miss something which you (or anyone else on the NUnit team) might deem important -- hence the call for review. To that end, and to answer some of the issues you raise:

• I'll certainly add some more examples
• Assume / Warn appear to not be part of the AssertionHelper syntax, so they are currently not implemented in NUnit.StaticExpect. Currently, we have two testing methods for proving parity: reflection-based testing which shows that (in all cases but 2) that the methods & properties that would be found on AssertionHelper are mimicked via static methods & properties on Expectations (with the two mentioned having special-case behavioral tests since they have diverged from the exact signature offered by Assert.That) as well as the full AssertionHelper test fixture running against Expecations instead of inheriting from AssertionHelper. The primary aim of the project is to provide a minimal-pain replacement for AssertionHelper, so that's pretty-much where the current state is. Of course, Assume and Warn are still available to the user via regular calls.
• non-C# code: this is a good question. It's all .net, so, sure, you can use it from, say, VB; but I think your question is more around static imports in VB.NET -- a question I can't answer right now (and which my google-fu has failed to answer so far), but which I will find a definitive answer for by simply creating a project and trying. I should be able to report back here later today on this.

And to re-iterate, for clarity: I appreciate review, commentary and suggestions on the README because it really should be a friendly, informative landing-point for anyone considering the switch to NUnit.StaticExpect. All suggestions welcome.

[fluffynuts]

@CharliePoole to my glee:
Imports NUnit.StaticExpect.Expectations
provides the same functionality in VB.NET as
using static NUnit.StaticExpectations.Expectations
does in C#. Just tested this now in a small VB.NET test library.

[fluffynuts]

(and, as a side-note, I now understand why AssertionHelper copied off the methods on (for example), Is - because I can't, in VB.Net, do
Expect(1, Is.EqualTo(1)),
but I can (through the static import) do:
Expect(1, EqualTo(1))

So I just need to flesh out the README more. I can give that a look tonight (:

[CharliePoole]

I was only thinking you might want to let people know they can use Assume or Warn along with the static constraint-building properties.

WRT Is in VB - that's why we have Iz. 😄

[BobSammers]

As @fluffynuts mentioned, StaticExpect tests against the AH functionality via reflection to ensure complete coverage of AH. Currently, 2 of these tests fail (actually are special-cased out) because the functionality of AH has diverged from the Is and Does members that it shadows (specifically InRange and Exactly, respectively).

I'd be interested in how long AH is likely to remain in NUnit. If it's likely to be for a couple more releases (which makes sense if sign-posts to StaticExpect can be put in to its obsoletion message), would you be interested in taking a pull request for changes to AssertionHelper.InRange() and AssertionHelper.Exactly() to close the divergence (I'm assuming AH was left behind accidentally)? This would give AH and SE exactly equivalent in functionality before AH is deprecated (and help with the SE test suite!).

[CharliePoole]

@BobSammers We haven't been adding features to AssertionHelper for a while, so it doesn't surprise me that it has fallen behind some new features. Feel free to create an issue for any divergence you want to fix... I put it that way, because it probably wouldn't get much priority otherwise.

@nunit/framework-team There hasn't been much recent comment here, so it's hard to respond to @fluffynuts and @BobSammers in a meaningful way. It seems like we "sort of" decided to get rid of AssertionHelper in the past and I think we were planning to direct users to the separate assembly I created. But we suffer a bit from the lack of a clear decision, it seems.

I believe we have had two fundamentally different directions under discusssion up to now:

1. Stop support for AssertionHelper entirely.
2. Move it to a separate package, which we may later deprecate and remove.

All the other options we discussed seem to me to be just details in one case or the other. Most of them have to do with how fast we implement one strategy or the other.

The availablility of StaticExpect seems to make alternative (2) a bit silly. There is no reason for us to compete with StaticExpect, especially since it looks like a better solution than inheritance. I suppose that this could open up a third option, if we want to continue supporting the API

3. Incorporate StaticExpect into NUnit, replacing AssertionHelper. In fact, NUnit had a static class named ConstraintFactory until quite recently, which could have been used in much the same way as StaticExpect if any of us had thought of it. 😞

@rprouse This is looking to me like one of those strategic decisions that needs to be made one way or the other by the project lead. After that, we can debate the details.

[fluffynuts]

Whichever way things go, I've updated the README.md tonight, largely borrowing from the AssertionHelper documentation.
As a side-note I've also update the solution to use a single, multi-target project, so it's a little neater. And included generated XML documentation in the package.

[ChrisMaddock]

@nunit/framework-team There hasn't been much recent comment here, so it's hard to respond to @fluffynuts and @BobSammers in a meaningful way.

Sorry Charlie - but it's only been a little over 24 hours! Afraid I can't always respond to things that fast. 😞

---

Previously, I was keen on just removing AssertionHelper in a minor release. What changed my mind was the issues we saw with removing TestFixtureSetUpAttribute etc in 3.8. To summarise - in 3.8 we removed some attributes which had been deprecated since 3.0 - assuming this would be a trivial find/replace fix for any users still relying on these. What we found out is that certain third party libraries were still relying on these attributes - meaning users of these 3rd party libraries were unable to update NUnit, until the library was patched. Of course - that's not impossible, but it meant that what we assumed would be a trivial fix, was not at all for a number of users. The same situation may well apply here.

AssertionHelper is now entirely segregated from the rest of the codebase thanks to @CharliePoole's work - hence my preference now for just leaving it tucked away in the corner to rot until 4.0. We can recommend users migrate to NUnit.StaticExpect anyway.

---

Regarding the points you laid out Charlie, I agree (2) would now be silly. On (3) - I'm personally keen on thinning out the core API we're providing, and think NUnit.StaticExpect would be a great third-party alternative syntax, rather than something that should be part of the core library. My overall preference would still be as I proposed above - but I think we'll need to agree to disagree there!

I'll also kick off this project contribution issue in governance tonight - sorry, ran out of time there yesterday.

[ChrisMaddock]

Separate issue for discussing bringing NUnit.StaticExpect into the NUnit organisation: nunit/governance#33

[CharliePoole]

@ChrisMaddock Actually, you're the one who responded already! That said, we seem to have left this undecided for something like nine months and we've been silent since July, until @fluffynuts just pushed on it again.

I like your proposal with one reservation. I think we should modify the Obsolete message to recommend use of StaticExpect now, rather than waiting.

FWIW, my desire to remove this sooner was predicated on the notion that I had to be around as part of the team so as to add my replacement project. I just wanted to get a pending project off my plate. If we can drop a separate AssertionHelper package as an option, I'm happy to have the feature hang around till 4.0.

[ChrisMaddock]

I like your proposal with one reservation. I think we should modify the Obsolete message to recommend use of StaticExpect now, rather than waiting.

I agree - these were meant as three separate points - not an ordered list. 😄

[mikkelbu]

I think that #1212 (comment) very precisely captures my thoughts (and is expressed much better than I can do), especially regarding the "thinning out the core API".