Syntax for checking if asynchronous exceptions were thrown, appears problematic - am I doing something wrong?

[IanKemp]

My team has chosen to standardise on FluentAssertions as our assertion library over Shouldly, which we were previously using, as we feel that this project is better maintained. However we have hit something of a stumbling block in our migration, to wit the syntax for testing if exceptions were thrown.

Previously in Shouldly, we were writing code like this:

// Act
var expectedException = await Sut.HandleAsync(someParameter)
    .ShouldThrowAsync<ArgumentException>();

// Assert
// expectedException is the thrown ArgumentException
expectedException.Message.ShouldBe("something");

In FluentAssertions we have to create a Func, or use the Awaiting wrapper which does the same:

// Act
var expectation = await Sut.Awaiting(s => s.HandleAsync(someParameter))
    .Should().ThrowAsync<ArgumentException>();

// Assert
expectation
    .WithMessage("something");

However technically we should be separating the Act and Assert portions (as suggested in the documentation), so we should really be writing:

// Act
var expectation = Sut.Awaiting(s => s.HandleAsync(someParameter));

// Assert
await expectation
    .Should()
    .ThrowAsync<ArgumentException>()
    .WithMessage("something");

But the problem with doing it this way is that the method under test is only executed once awaited, i.e. in the Assert section, which means that said method will be executed for every assertion as opposed to just once. This is completely, utterly wrong - if the method under test is stateful, then executing it multiple times can change its state and thus cause subsequent assertions to fail.

In short, I find this an extremely sub-optimal API because it allows you to extremely easily shoot yourself in the foot. Am I correct, or am I just missing something very obvious here?

[jnyrup]

I'm not sure I get your part about "said method will be executed for every assertion"?
Do you have an example with more than one assertion acting on the same SUT?

Since Sut is Pascal-cased, is it an object shared between multiple tests?

[IanKemp]

I'm not sure I get your part about "said method will be executed for every assertion"? Do you have an example with more than one assertion acting on the same SUT?

Sure:

// Act
var expectation = Sut.Awaiting(s => s.HandleAsync(someParameter));

// Assert
await expectation
    .Should()
    .ThrowAsync<ArgumentException>()
    .WithMessage("something");
await expectation
    .Should()
    .NotThrowAsync<NullReferenceException>(); // HandleAsync is executed a second time here (verified by breakpoint in the method and debugging this test)

Since Sut is Pascal-cased, is it an object shared between multiple tests?

Yes and no.

The test class has a base class that uses AutoFixture to automagically Moq up all the dependencies required by the class under test, the instance of which is pointed to by Sut and referenced by each individual test. We then override those dependencies as necessary (typically, in the derived test class's constructor and/or each test) so that we can set them up to perform mocked actions and/or to verify them.

However, since we're using XUnit, the entire test class is torn down and recreated for every test run. Thus there cannot ever be a scenario in which Sut is reused or mutated between multiple tests.

[jnyrup]

If HandleAsync is idempotent/pure(-ish)/etc and throws an ArgumentException the second call will give the same exception making NotThrowAsync<NullReferenceException>() superfluous?

If HandleAsync is not idempotent the code snippet is the idiomatic way to assert that Sut throws an ArgumentException on first invocation and a NullReferenceException on second invocation.

[dennisdoomen]

This is completely, utterly wrong

I don't think that's the nicest way of saying that you may disagree with our design philosophy.

Previously in Shouldly, we were writing code like this:

It seems you're problem may apply to Shouldly as well, right? Because your example doesn't show that second assertion.

In short, I find this an extremely sub-optimal API because it allows you to extremely easily shoot yourself in the foot. Am I correct, or am I just missing something very obvious here?

I don't understand why you want to have that second assertion in the first place. If it throws ArgumentException, then by definition, it can't throw NullReferenceException. Unless you're expecting an AggregateException that contains both. In that case you could use Should().ThrowExactlyAsync<AggregateException>().Where(predicate) to further inspect the properties of that aggregate exception.