AllSatisfy() fails on empty collections

[ArneMancofi]

Description

new object[0].Should().AllSatisfy(o => { }); fails with

Expected collection to contain only items satisfying the inspector, but collection is empty.

This does not implement the documented effect of AllSatisfy:

Asserts that a collection contains only items which meet the criteria provided by the inspector

In particular, any empty collection should always pass this test.

The bug is probably due to two code lines in

fluentassertions/Src/FluentAssertions/Collections/GenericCollectionAssertions.cs

Lines 2929 to 2930 in c3b9a70

	.ForCondition(subject => subject.Any())
	.FailWith("but collection is empty.")

Reproduction Steps

// Arrange
var emptyArray = new object[0];
// Act
// Assert
emptyArray.Should().AllSatisfy(o => { });

Expected behavior

The test should pass without throwing an exception.

Actual behavior

The test fails with exception

Expected collection to contain only items satisfying the inspector, but collection is empty.

Regression?

This has probably always been this way.

Known Workarounds

Code like this would be a workaround:

if (collection.Any()) collection.Should().AllSatisfy(...);

However, reading the documentation of .AllSatisfy(), it should not be necessary.

Configuration

FluentAssertions 6.9.0
.NET 6.0

Other information

The WikiPedia article on universal quantification states that

By convention, the formula ∀x∈∅ P(x) is always true, regardless of the formula P(x)

so it is the documentated behaviour of .AllSatisfy() that is conventionally correct, and the implementation that is not.

[jnyrup]

This most likely falls under "design choice".
We do not support all vacuous truths.
See e.g. #1492 for a related discussion on this.

Do you have a more realistic example where this is a problem for you?
I'm inclined to ask "why would you want to test use AllSatisfy on an empty collection"?

[dennisdoomen]

I'm inclined to ask "why would you want to test use AllSatisfy on an empty collection"?

Exactly. If your test is expecting an empty collection, make that explicit. I can't imagine a test that sometimes expects an empty collection and sometimes a collection which items match the predicates.

[ArneMancofi]

This most likely falls under "design choice". We do not support all vacuous truths. See e.g. #1492 for a related discussion on this.

Do you have a more realistic example where this is a problem for you? I'm inclined to ask "why would you want to test use AllSatisfy on an empty collection"?

In fact, I came across this bug because I had made a parameterized test that would check required properties of the result. Initially, I had just given nontrivial test case input (producing among other objects a nonempty list), but the code should also work for trivial input (producing among other objects an empty list), and the same properties should still apply for the result. When I added the trivial test case, the test failed.

I was in fact surprised that .AllSatisfy has this unconventional behaviour, especially as it is not noted in its documentation. Also, I think most mathematicians (of whom quite a lot are employed as IT developers) would find vacuous truths most natural. The fact that someone else raised a similar issue previously seems to be an indication that this is what many developers expect.

If you have a need for ensuring a nonempty list as well as a property on each element, I would expect you could do something like

.Should().NotBeEmpty().And.AllSatisfy(...)

making your intention clear and explicit.

Note that I am not able to fluently assert "empty or all satisfy" using .AllSatisfy(), as the .Or operator (corresponding to .And) is not part of FluentAssertions.

If you really want to compound the two tests into a single method, I suggest adding a new method .NotBeEmptyAndAllSatisfy(...); this would be more helpful and intention revealing, and prevent developers from inadvertendly making errors in their assertions.

[jnyrup]

When I added the trivial test case, the test failed.

In trying to understand you case, I have to ask: why would you want to test a vacuous truth?

From #1492
The goal is not to make the API of FA consistent to the underlying .NET framework methods. The goal is to make things explicit and intention revealing.

I think most mathematicians (of whom quite a lot are employed as IT developers) would find vacuous truths most natural.

Conversely I don't think most developers are mathematicians.

From the wiki-page
Outside of mathematics, statements which can be characterized informally as vacuously true can be misleading

The fact that someone else raised a similar issue previously seems to be an indication that this is what many developers expect.

That's selection bias, people don't raise issues with things that works fine 🙃

[ArneMancofi]

OK, this is frustrating, and seems to lead to nowhere. I don't find it ureasonable to want to test "no matter how many elements there might be in a result, xs, they should all satisfy P(x)." In parameterized DRY test methods you can easily end up testing results that for some test cases (i.e., parameter sets) are trivial (i.e., zero, empty, null, etc.). If, for instance, you were to test the List.Filter() method, you would naturally give test cases somewhat along these lines:

[Theory]
[MemberData("FilterTestCases")]
public void FilterTest(List<int> xs, Func<int, bool> p) {
  var rs = xs.Filter(p).ToList();
  rs.AllSatisfy(r => p(r));
}
public static object[][] FilterTestCases { get; } = new object[][] {
  new object[] { new List<int>(), x => x > 3 },
  new object[] { new List<int>{ 1, 2, 3, 4, 5 }, x => x > 3 },
  ...
}

Constructively, I suggest either

1. Carefully re-visiting the design decision of bundling two properties (non-emptyness and element satisfactions) into a single assertion method, and make it more orthogonal and intention-revealing by not requiring non-emptyness (as this can easily be fluently asserted using .NotBeEmpty().And), or
2. (As an adequate alternative) add an assertion method .BeEmptyOrAllSatisfy() that does not require the list to be non-empty.

The attractiveness of FluentAssertions is that it is fluent and orthogonal, so that we can naturally express and combine all our assertions within the framework without using C# control statements like if (xs.Any()) xs.AllSatisfy(...).

The current semantics of .AllSatisfy() makes FluentAssertions less attractive.

[dennisdoomen]

Although you're not wrong, we often have to deal with trade-offs where opinions can differ wildly. In those cases, we sometimes ask the community to provide us with feedback. Now, when I see a statement like:

array.Should().AllSatisfy(some predicate);

Then there would not a doubt in my mind that array is expected to be a non-empty collection. Why else would you use that method and pass in predicate. If that array can be empty, I would most definitely see that as completely different functional behavior and would capture that in a separate test case. In fact, I'm personally quite wary about using [Theory]. It often leads to obfuscation of the test scenarios.

I agree that the documentation should be clearer, but even if you would convince us that your concerns are valid, it would still be a breaking change. And that would probably require revisiting all similar APIs and create a lot of confusion for people upgrading..

[ArneMancofi]

Right.

Should I create a feature request for a .BeEmptyOrAllSatisfy() assertion method?

[dennisdoomen]

Honestly, it doesn't make sense to me. And for consistency, we would need to create overloads like those for every API that deals with collections.

[cmeeren]

I too find the "fail on empty" behavior on .AllSatisfy very confusing, and contrary to accepted standards.

Firstly, the current behavior goes against existing abstractions. For example, LINQ's All(...) method returns true for empty collections:

Returns

true if every element of the source sequence passes the test in the specified predicate, or if the sequence is empty; otherwise, false.

(Emphasis mine.) The same point applies to F#'s forAll functions (e.g. List.forAll), which correspond to LINQ's All. Going against this universally adopted behavior is certainly a cause for confusion.

Secondly, checking for non-emptiness and checking whether all elements satisfy a predicate are two orthogonal concepts. .AllSatisfy currently checks both, and thus mixes two concerns.

Thirdly, if users additionally wish to check that a collection is non-empty, that can be done using collection.Should().NotBeEmpty().And.AllSatisfy(...). Two separate checks for two orthogonal concerns, and very concise. This is a much more composable and clear API where you express exactly what you want to check (where it is both concise, explicit, and possible in the first place). With the current behavior of AllSatisfy, there is, as previously pointed out, no concise way to express empty being OK.

With all of the above in mind, I strongly suggest that the design decision be reevaluated, at least for the next major version.

[cmeeren]

I think the arguments above speak for themselves and don't require further justification in terms of example use-cases. Furthermore, examples may seem solid to one person but weak to another, and there is a danger that one may judge them based on the style of testing and not the domain concepts they represent. Still, with that in mind, here are a few examples, in case it helps with the motivation. For generality (since, I assume, FluentAssertions should be agnostic to the testing methodology), consider them from the perspective of property-based testing or other testing with auto-generated data, where it is often sufficient to test a few assertions on multiple instances of properly generated random data.

• A form can have 0..n responses. If the form does not require approval, all form responses (if there are any) must not have status PendingApproval. This invariant holds for any number of responses, including 0. A single assertion can suffice to check this.

• An order can have 0..n order lines. Each order line must not exceed the order customer's maximum weight per parcel limit. This invariant holds for any number of order lines, including 0. A single assertion can suffice to check this.

[dennisdoomen]

Thank you for your insights. I'll discuss it with @jnyrup,

[cmeeren]

Hi again! Just following up to see if this has been discussed. 🙂

[dennisdoomen]

No, but we'll discuss it as part of the v7 roadmap.