Questions regarding fluent syntax design for assertions

Crossbow78 commented 4 years ago

Coming from XUnit + FluentAssertions, I must say I love the TestCase feature of NUnit.

At the same time I feel that the assertion syntax seems all over the place. The fluent attempt mostly applies to reading the assertion, but definitely not to writing them, because Intellisense gives many false suggestions. And for the number of constraints listed in documentation the possibilities still seem rather limited.

There's nothing preventing me from using FluentAssertions, but I'm trying to understand the philosophy in the current assertion/constraint design in NUnit, especially since a lot of work seems to have gone into it.

First example of clunkiness: why does the Has class have a 'Length' and 'Count' which only work on classes with that exact property name? This leads to tests seemingly perfect by reading the code, but still failing at runtime because a collection was of an unexpected type. It's just confusing while there is a syntax that always works...

        [Test]
        public void Test_ItemCount()
        {
            var MyArray = new[] { 1, 2, 3 };
            var MyCollection = new List<int> { 1, 2, 3, };
            var MyEnumerable = Enumerable.Range(1, 3);

            // NUnit assertions

            Assert.That(MyArray, Has.Exactly(3).Items);
            Assert.That(MyArray, Has.Length.EqualTo(3));
            //Assert.That(MyArray, Has.Count.EqualTo(3));        // System.ArgumentException : Property Count was not found on System.Int32[]. (Parameter 'name')

            Assert.That(MyCollection, Has.Exactly(3).Items);
            Assert.That(MyCollection, Has.Count.EqualTo(3));
            //Assert.That(MyCollection, Has.Length.EqualTo(3));  // System.ArgumentException : Property Length was not found on System.Collections.Generic.List`1[System.Int32]. (Parameter 'name')

            Assert.That(MyEnumerable, Has.Exactly(3).Items);
            //Assert.That(MyEnumerable, Has.Count.EqualTo(3));   // System.ArgumentException : Property Count was not found on System.Linq.Enumerable+RangeIterator. (Parameter 'name')
            //Assert.That(MyEnumerable, Has.Length.EqualTo(3));  // System.ArgumentException : Property Length was not found on System.Linq.Enumerable+RangeIterator. (Parameter 'name')

            // FluentAssertions

            MyArray.Should().HaveCount(3);
            MyCollection.Should().HaveCount(3);
            MyEnumerable.Should().HaveCount(3);
        }

Other examples of clunkiness when it comes to Intellisense:

            Assert.That(MyArray, Has.None.Zero);    // Pass
            Assert.That(MyArray, Is.Not.Zero);      // Pass - but probably means something entirely different

            // Why do the following even compile?

            //Assert.That(MyArray, Has.Exactly(3).Items.All.Exist);                      // Expected: IEnumerable But was: Int32 (Parameter 'actual')
            //Assert.That(MyArray, Has.Exactly(3).Items.All.Not.EqualTo(0));             // Expected: IEnumerable But was: Int32 (Parameter 'actual')
            //Assert.That(MyArray, Has.Exactly(3).Items.And.All.All.All.Not.EqualTo(0)); // Expected: IEnumerable But was: Int32 (Parameter 'actual')

Finally, I'm wondering whether it's possible to (1) match using a lambda expression, and (2) drill down into a match to assert its properties. Is there something equivalent in NUnit?

            // FluentAssertions

            MyArray.Should().ContainSingle(x => x > 2).Which.Should().Be(3);
            MyArray.Should().ContainSingle(x => x > 2).Which.Should().Be(3).And.NotBe(4);

            var colors = new[] { Color.Red };
            colors.Should().ContainSingle(x => x.IsKnownColor).Which.R.Should().Be(255);

            // NUnit expected syntax

            //Assert.That(MyArray, Contains.Single(x => x > 2)  (...)    // Syntax error
            //Assert.That(MyArray, Has.One.Match(x => x > 2)    (...)    // Syntax error 
            //Assert.That(colors, Has.Exactly(1).Items.With.R   (...)    // Syntax error

CharliePoole commented 4 years ago

Some of what you point out is by design, some things may be bugs and some things are just the way they are because... "History." Also, since fluent syntax goodness is a bit subjective, we may reasonably disagree about what is good... :smile:

(Non)Use of Should or similar...

It's easy to forget that NUnit started with the pre-release of .NET 1.0, so originally such syntax was not even possible.
Personally, I don't like "should" as a word in this context. I started my career working with requirements docs where "should" meant something like "It would be best if this happened, but it's not mandatory."
But even with another word involved (must?) the syntax with an extension method, while more compact, says lest than the existing two-part syntax. NUnit's wrapper methods (ike Assert.That, Assume.That, Warn.If, Warn.Unless) tell you what to do in case the constraint is not met. If I understand correctly, FluentAssertions only does one thing on fail.
I played around with a perl-like syntax (x.should.dosomething.else.warn) but it didn't get much uptake from those listening.
My current favorite approach for a syntax is to embedd the extension method in the NUnit Assert like Assert.That(x.Is.equalto(y)). I doubt that will make it into NUNit but it may be in the next framework I work on.

Count vs Length and similar

On this one, we will probably just have to agree to disagree. I would find Assert.That(anArray, Has.Count.EqualTo(5)) quite surprising if it worked. Arrays as we know don't have counts. NUnit was created with programmers in mind, of course.

Has.Exactly was invented to solve this problem and solves it well. If there were a new major release of NUnit. I would favor removing both Length and Count, which are simply syntactic sugar over Has.Property("Length") etc. BTW, that should explain why we compile and don't fail until runtime... we are testing whether an object has a property with a certain name.

Scalar vs Vector operations

I think you may have found a bug - or several - here. NUnit compiles but errors when you apply a constraint to a scalar value, which only works on vectors. (See below for why this has to be a runtime error.) However, nothing similar happens when an operation requires a scalar value but a vector is supplied. So Assert.That(myArray, Is.Not.Zero) and the equivalent Assert.That(myArray, Is.Not.EqualTo(0) are valid and pass because myArray is most definitely not equal to zero!

Treating Everything as an Object

The heading is actually the answer to many "why does NUnit..." questions. NUnit rarely treats values as typed. At the time it was created, it was not possible to use generics, so all examination of the type of arguments and conversion between types was handled under the covers by NUnit itself. If you look at asserts, you will see that there are some generic methods, but the underlying implementation as objects remains.

IMO this is the biggest weakness of the current implementation. Several attempts have been made to convert the entire Assert class to use generics and to test for compatibility between the actual values and the constraints at compile time. I made two attempts myself. So far, it has not been possible to do this in a backward compatible way. Backward compatibility, of course, is a requirement for any change to NUnit barring a major release (i.e. NUnit 4). So I suggest continuing to press for improvements you want in that context.

Things that compile but should not

Taking All.All.All as an example, I think that should fail. We have a set of tests that attempt to compile things that should not compile and that ought to be added to them. I guess that over the years things got added without adding to the compilation tests.

BTW, it may not be obvious, but the addition of And and Or to the NUnit syntax is the cause of a lot of difficulty in implementation. It's a good order of magnitude easier to implement a fluent syntax that is entirely linear... adding those operators more than tripled the amount of code needed to implement what we have. If I were starting from scratch, I might not include them as operators.

For things in the above that are bugs, I urge you to submit them to the nunit team. For those changes that can't be done until NUnit 4, I recommend showing up to lobby for the stuff you want.

Fair warning: I don't work much on NUnit any longer. I answered here because the NUnit fluent syntax was originally my design but others will need to decide where NUnit is going next.

Crossbow78 commented 4 years ago

Thanks for your elaborate response, and for bringing structure into my questions.

I agree that separating the condition from the action has a beautiful elegance, and I do miss something like the Assume in FluentAssertions. I hadn't even read about Warn, thanks for bringing it to my attention.

About the counts/lengths, I can totally understand this comes from history and backward compatibility. It could be an idea to mark these as obsolete to at least guide the developer to a better alternative. I call it better, since the intent clearly is to validate how many items a 'collection' contains, without caring about details like the type of collection. I found myself refactoring some return type from List<> to IEnumerable<> and everything compiled fine, but tests suddenly failed because the 'Count' property no longer existed.

So are you saying a syntax like the following is impossible, simply due to the fact that internally everything is type-less? Assert.That(myArray, Contains.Single(x => x.IsActive).With.Name.EqualTo("Test")) I can imagine it's not a small task to modify that in existing code, but given the powerful possibilities it would deliver, it could be interesting to explore. Is one of these earlier attempts preserved in a branch somewhere?

I'll submit the syntax bugs as a separate issue.

CharliePoole commented 4 years ago

I wouldn't say that such an expression is impossible, but I do think it requires a fairly complete rewrite of the underlying code that (sort of) parses the syntax as the expression is evaluated. Personally, I think there is a need for two approaches coexisting. One typed approach such as we are discussing and a legacy object approach for dealing with the worst case scenario - collections of mixed types. In fact, such collections are pretty rare these days anyway, so maybe we can make that usage fade away.

My own attempts are gone. @jnm2 didn't you work on this once? Do you have any lessons learned for @Crossbow78 ?

jnm2 commented 4 years ago

Yes, I tried once or twice and ran into backwards compatibility issues. The biggest learning from my own experiences outside NUnit is that fluent APIs are a lot of work, and fluent APIs that are context-dependent are a lot of work squared (so to speak).

We can augment the compiler type checks with our own analyzer errors and warnings. The upside there is that there are no backwards compatibility concerns (the analyzer is very new and isn't at 1.0 yet). The downside is that it's probably even more maintenance work to do this checking in the analyzer.

I've been using Shouldly along with NUnit for a while. It's my current favorite for assertion statements. NUnit is my favorite for test case organization, custom attributes, and the "batteries included" selection of built-in attributes that cover common scenarios.

CharliePoole commented 4 years ago

If anyone is interested, I'm currently rewriting the whole constraint-building thing for my TCLite framework. If I can get it to work, it might be useful here.

jnm2 commented 4 years ago

@CharliePoole I'm generally interested. A month ago I tried again to retrofit true-async assertions and I have some thoughts on that too. Is that something you're including?

CharliePoole commented 4 years ago

I think that's an important issue but it's orthogonal to the question of generics. So far, I've only implemented just enough assertions and constraints to exercise the builder because otherwise each little change I make ramifies into too many files.

nunit / docs