Homogenize predicate formatting #1619
Conversation
dennisdoomen
left a comment
There was a problem hiding this comment.
Thanks for this contribution. Would you mind updating the release.md as well?
jnyrup
left a comment
There was a problem hiding this comment.
quickly grep'ing for .Body there seems to be a few more cases in GenericCollectionAssertions.cs and a single one in ExceptionAssertion.cs.
Were those intentionally skipped?
Not the one in ExceptionsAssertion.cs. The other ones were not predicates, in that they did not return bool, so they wouldn't match the formatter anyway. I've fixed that now. Note that the result is that parentheses now encloses the formatted output in these cases. |
Extended lambda expression formatting to include non-bool expressions
| // Arrange | ||
| IEnumerable<int> collection = Enumerable.Empty<int>(); | ||
| Expression<Func<int, bool>> expression = item => item == 2; | ||
| var targetValue = 2; |
There was a problem hiding this comment.
Basically, to assert that the lambda output formatting is invoked properly. Previously, having this syntax would have rendered the message "Expected collection to contain a single item matching (item == value(FluentAssertions.Specs.Collections.GenericCollectionAssertionsSpecs+<>c__DisplayClass21_0).targetValue)"
There was a problem hiding this comment.
It seems that When_first_level_properties_are_tested_for_equality_against_constant_expressions_then_output_should_contain_values_of_constant_expressions is responsible for asserting that captured constants in expressions are properly reduced, and not a concern these tests need to exercise.
But I do like that you're replacing the dynamic part of expectedMessage 👍
There was a problem hiding this comment.
When_first_level_properties_are_tested_for_equality_against_constant_expressions_then_output_should_contain_values_of_constant_expressions does indeed test the formatting, but the previous use of .Body resulted in PredicateLambdaExpressionValueFormatter not being used at all. I guess, in a sense, it turns the test into more of an integration test, but that's the case for all tests asserting exception messages.
The dynamic part + the use of an expression with a constant (item => item == 2) is what hid the problem from view. That was my reasoning behind this change. If you which me to reset it to item => item == 2 I will gladly do so.
There was a problem hiding this comment.
If you change it to
Expression<Func<int, bool>> expression = item => item == 2;and
"Expected collection to contain a single item matching (item => item == 2), but the collection is empty."that's perfectly fine with me 👍
You could add another test that asserts on how the message is formatted, but I don't have a strong opinion about that.
Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>
Many predicate assertions uses
predicate.Bodyto format their messages, thereby circumventing the formatting offered byPredicateLambdaExpressionValueFormatter. More specifically, it's the part which resolves constant values which is sorely missing, e.g when usingcollection.Should().Contain(x => x.Name == expectedName && x.Value == expectedValue).This could be fixed by adding a new formatter or modifying the existing, but I think it makes more sense to provide the full lambda to the formatter.
Possible additions would be:
Name == "Expected"instead ofa.Name == "Expected"There's one test,
When_single_item_contains_string_interpolation_it_should_format_brackets_properly, which asserted a specific "Format" output. It can't do that now, but I'm not sure it makes much sense to keep the formatting call as is. This was discussed in #1404. Either way, I think it makes sense to centralize the predicate formatting and deal with any special cases in the formatter.