Throw when asserting on AndConstraint

[eNeRGy164]

This is an attempt to help people when they write .Should() directly after .And by throwing an exception.

As classes with assertions are not decorated with something to recognize (e.g. a shared base class or interface) I use reflection to get all the return types for Should methods on the AssertionExtensions class.

Had to rewrite unit tests that were using ObjectAssertions to test cross-assembly type comparison as they would also fail with the new exception.

Should fix #1617

IMPORTANT

• The code complies with the Coding Guidelines for C#.
• The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
• If the contribution adds a feature or fixes a bug, please update the release notes, which are published on the website.

[dennisdoomen]

The release notes are missing, but now I'm in doubt whether we should retain the develop branch. If we add the release notes, it'll become available on the website right away.

[lg2de]

If you write "Unreleased" instead of "6.1.0" the Changelog is correct.

https://keepachangelog.com/

[dennisdoomen]

If you write "Unreleased" instead of "6.1.0" the Changelog is correct.

Oh, I like that.

[jnyrup]

I've thought of an alternative approach where we instead add honeypot overloads of Should.

Pros:

• It's very explicit
• Does not have any runtime impact on Should(object)
• [Obsolete] gives compiler warning/error
• Returns void, so no further chaining is possible

Cons:

• Changes to API

I could find 12 base assertions, that should cover all cases:

• BooleanAssertions
• DateTimeAssertions
• DateTimeOffsetAssertions
• ExecutionTimeAssertions
• GuidAssertions
• MethodInfoSelectorAssertions
• NumericAssertions
• PropertyInfoSelectorAssertions
• ReferenceTypeAssertions
• SimpleTimeSpanAssertions
• TaskCompletionSourceAssertions
• TypeSelectorAssertions

/// <summary>
/// You are asserting the 'AndConstaint' itself. You probably want to remove the 'Should()' method directly following 'And'.
/// </summary>
[Obsolete("You are asserting the 'AndConstaint' itself. You probably want to remove the 'Should()' method directly following 'And'", error: true)]
public void Should(ReferenceTypeAssertions<TSubject, TAssertions>)
    where TAssertions : ReferenceTypeAssertions<TSubject, TAssertions>
{
    InvalidShouldCall();
}

/// <summary>
/// You are asserting the 'AndConstaint' itself. You probably want to remove the 'Should()' method directly following 'And'.
/// </summary>
[Obsolete("You are asserting the 'AndConstaint' itself. You probably want to remove the 'Should()' method directly following 'And'", error: true)]
public void Should(BooleanAssertions<TAssertions>)
    where TAssertions : BooleanAssertions<TAssertions>
{
    InvalidShouldCall();
}

... 10 more ...

private static void InvalidShouldCall() => 
    throw new InvalidOperationException("You are asserting the 'AndConstaint' itself. You probably want to remove the 'Should()' method directly following 'And'.");

[dennisdoomen]

I've thought of an alternative approach where we instead add honeypot overloads of Should.

I like both approaches equally. The advantage of this "honeypot" Should is that you can also mark it with an [Obsolete] attribute that provides immediate feedback.

[jnyrup]

Another benefit!
It also prevents you from trying to chain further, as it returns void instead of ObjectAssertions.

[dennisdoomen]

It also prevents you from trying to chain further, as it returns void instead of ObjectAssertions.

Fair enough. @eNeRGy164 what do you think?

[lg2de]

May be another approach: Instead of pervert the call of Should it could return the AndConstraint.

[eNeRGy164]

It also prevents you from trying to chain further, as it returns void instead of ObjectAssertions.

Fair enough. @eNeRGy164 what do you think?

I didn't go on this route as it changed the API (as mentioned by @jnyrup) and will require attention for authors in the future if they add new assertion classes.
However, I also expected this would require 60 new methods, but as Jonas points out the number is limited due to inheritance.

So, I will modify the code to this.

I think the remark about the return value by @lg2de is also interesting. [Obsolete] will produce a warning or error, but we could chain it anyway and leave it up for the user to correct their mistake. But this will require all 60 methods again as otherwise the chaining won't return the correct type.

It could also be an Analyzer for that matter.

You could argue that making the method void would be a breaking change in the API. Although somebody using this construct is probably not testing what they expect. Any further thoughts on this?

[lg2de]

Technical there are breaking changes. But there are no functional breaking changes, except there are users explicitly asserting on AndConstraint or similar.

Adding [Obsolete] gets breaking change for users with "warning as error" enabled.

[eNeRGy164]

Adding [Obsolete] gets breaking change for users with "warning as error" enabled.

Because of the , true) in the attribute it is an error, not a warning. So, it becomes uncompilable.
Therefore, I also can't create a unit test for this anymore.

[dennisdoomen]

We could debate is that we're just fixing a false positive for them.

[lg2de]

@dennisdoomen How about support chaining instead of preventing?
Using second Should() is still a fluent statement, isn't it?

[dennisdoomen]

How about support chaining instead of preventing?
Using second Should() is still a fluent statement, isn't it?

Not sure what that would look like. Extension methods on all concrete versions of AndConstraint<T>?

[jnyrup]

public static TAssertions Should<TAssertions>(this BooleanAssertions<TAssertions> @this)
    where TAssertions : BooleanAssertions<TAssertions>
{
    return (TAssertions)@this;
}

Would allow one to write

true.Should().BeTrue().And.Should().BeFalse();

As the second Should() is a no-op, you can also write

true.Should().BeTrue().And.Should().Should().Should().BeFalse();

[dennisdoomen]

Nice. Just tried it.

[eNeRGy164]

So, which direction do you want to go? The prevention or the NOOP chaining?

[dennisdoomen]

I'm fine with the direction we took. @jnyrup needs to approve the PR.