Add support for Satisfy on ReferenceTypeAssertions #2597

siewers · 2024-02-29T19:59:05Z

Added support for Satisfy on ReferenceTypeAssertions allowing to use element inspectors as an alternative to using predicates on Match.

Example:

var myDto = new Dto { Name = "Some Name" };

myDto.Should().Satisfy<Dto>(dto => dto.Name.Should().Be("Some Name"));

IMPORTANT

If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
The code complies with the Coding Guidelines for C#.
The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
- Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

github-actions · 2024-02-29T20:04:09Z

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

Or via our issue tracker: https://jb.gg/qodana-issue
Or share your feedback: https://jb.gg/qodana-discussions

Tests/FluentAssertions.Specs/Primitives/ReferenceTypeAssertionsSpecs.Satisfy.cs

ITaluone · 2024-03-01T06:03:30Z

Missing release notes?

siewers · 2024-03-01T08:33:09Z

I didn't add it to the release notes yet as I'm unsure what version this will be in.

@dennisdoomen I can see that we're at Alpha 3 now, but the release notes only contain Alpha 1. I've added the changes to the current section of the releases.md, but I think something might be wrong. Can you let me know if I need to change something?

ITaluone · 2024-03-01T09:33:38Z

I didn't add it to the release notes yet as I'm unsure what version this will be in.

I think while on develop for the next version it doesn't really matter

siewers · 2024-03-01T09:41:12Z

@ITaluone Alright, I added it the the most recent section. I guess it will be aligned when the final release goes out, however, it might be a bit confusing for people using the Alpha versions to see the release notes not reflecting the latest version.

siewers · 2024-03-01T10:08:01Z

@dennisdoomen @ITaluone I believe I've addressed all comments now.

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

dennisdoomen · 2024-03-02T15:54:25Z

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

+            failuresFromInspector = assertionScope.Discard();
+        }
+
+        if (failuresFromInspector.Length > 0)


🤔 Do we even need to do this? What if you just relied on the assertion scope to throw a combined message? What kind of information would you be missing?

I don't know, I'll test it to see if it's necessary 👍

I tested the changes you suggested, and it looks like we will be losing the outer object context. There is also no indentation on the nested assertions, which I think is useful.

I don't know what is preferred though, however, this is how it's done in Match, and I believe the result should be the same or similar, no?

Using a single assertion scope:

Expected person.Name to be the same string, but they differ at index 1: ↓ (actual) "Buford Howard Tannen" "Biff Tannen" ↑ (expected). Expected person.Age to be 48, but found 87 (difference of 39). Expected address.City to be "Hill Valley, San Diego County, California" with a length of 41, but "Hill Valley" has a length of 11, differs near "y" (index 10).

Using Discard:

Expected complexDto to match inspector, but the inspector was not satisfied: Expected dto.Person to match inspector, but the inspector was not satisfied: Expected person.Name to be the same string, but they differ at index 1: ↓ (actual) "Buford Howard Tannen" "Biff Tannen" ↑ (expected). Expected person.Age to be 48, but found 87 (difference of 39). Expected dto.Address to match inspector, but the inspector was not satisfied: Expected address.City to be "Hill Valley, San Diego County, California" with a length of 41, but "Hill Valley" has a length of 11, differs near "y" (index 10).

When I rely solely on the assertion scope, I get this

Expected dto.Person to be the same string, but they differ at index 1: ↓ (actual) "Buford Howard Tannen" "Biff Tannen" ↑ (expected). Expected dto.Person to be 48, but found 87 (difference of 39). Expected dto.Address to be "Hill Valley, San Diego County, California" with a length of 41, but "Hill Valley" has a length of 11, differs near "y" (index 10).

I personally feel like this is much more readable without all the noisy mentioning of the inspector (which is the wrong term anyway), don't you agree?

The problem with the message is that it doesn't mention the failed nested property names:

complexDto.Person.Name

complexDto.Person.Age

complexDto.Address.City

If those exact property names aren't mentioned in the message, it's not helpful.

As it looks with the new changes, both the Name and Age properties gets merged into a single nonsensical error on the same Person property.
It's wrong to say that Person was expected to be the same string and it was expected to be a certain number :)
Also, Address is another "complex" object, but the error message doesn't present the exact property that failed the assertion.

Ideally, to avoid excessive nesting, the error message could look like this:

Expected instance to match inspector, but the inspector was not satisfied: Expected instance.Person.Name to be the same string, but they differ at index 1: ↓ (actual) "Buford Howard Tannen" "Biff Tannen" ↑ (expected). Expected instance.Person.Age to be 48, but found 87 (difference of 39). Expected instance.Address.City to be "Hill Valley, San Diego County, California" with a length of 41, but "Hill Valley" has a length of 11, differs near "y" (index 10).

I hope that makes sense :)

I need to look into that (again). Later today.

Crap, you're right. The call to new AssertionScope(CallerIdentifier.DetermineCallerIdentity()) doesn't actually help at all. Sorry about that. I thought I could fix that by changing some of the behavior AssertionScope uses. But it turns out, it doesn't. So maybe we can rewrite Expected dto.Person to match inspector, but the inspector was not satisfied: to be less verbose.

Please also note that CallerIdentifier.DetermineCallerIdentity() is very expensive to call and is only needed when the test fails.
See #1660 and #1657

Please also note that CallerIdentifier.DetermineCallerIdentity() is very expensive to call and is only needed when the test fails.

True, but it's no longer relevant. My suggestion doesn't work, so the original implementation is still the best.

docs/_pages/basicassertions.md

docs/_pages/releases.md

coveralls · 2024-03-11T07:50:34Z

Pull Request Test Coverage Report for Build 8318031359

Details

17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 97.537%

Totals
Change from base Build 8236406738:	0.004%
Covered Lines:	11961
Relevant Lines:	12145

💛 - Coveralls

dennisdoomen · 2024-03-11T08:50:06Z

I believe I've addressed all comments now.

Apparently not.

ITaluone · 2024-03-12T09:42:49Z

@siewers Could you rebase to current develop and force push again?

IT-VBFK · 2024-03-12T16:49:42Z

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

-    /// <exception cref="ArgumentNullException"><paramref name="expected"/> is <see langword="null"/>.</exception>
-    public AndConstraint<TAssertions> Satisfy<T>(Action<T> expected)
+    /// <exception cref="ArgumentNullException"><paramref name="assertion"/> is <see langword="null"/>.</exception>
+    public AndConstraint<TAssertions> Satisfy<T>(Action<T> assertion)


I think assertion is a bit misleading here 🤔
(But feel free to disagree :) )

You're supposed to use that action to build a composite assertion, so I think it's a suitable term.

dennisdoomen · 2024-03-17T20:14:51Z

Did you close it on purpose?

siewers · 2024-03-17T20:17:11Z

Wow, no, I didn't... I don't know what happened.

jnyrup · 2024-03-24T10:11:29Z

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

+        Execute.Assertion
+            .ForCondition(Subject is T)
+            .WithDefaultIdentifier(Identifier)
+            .FailWith("Expected {context:object} to be assignable to {0}{reason}, but {1} is not.", typeof(T), Subject.GetType());


If Subject is null, Subject.GetType() throws an NRE.

I've added a check for null, although the message is almost duplicated.
I also used .Then to chain the calls, but I'm not sure that does the same thing as you've suggested using the result of the first assertion to execute the next.

jnyrup · 2024-03-24T12:00:49Z

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

+
+        using (var assertionScope = new AssertionScope())
+        {
+            assertion((T)Subject);


If Satisfy is used inside an AssertionScope, then the execution won't stop if Subject is T above fails, meaning we would send in a null to action.

object subject = null; Action act = () => { using var _ = new AssertionScope(); subject.Should().Satisfy<object>(x => x.Should().NotBeNull()); };

What we usually do to guard against this.

bool success = Execute.Assertion .ForCondition(...) .FailWith(...); if (success) { Execute.Assertion .ForCondition(...) .FailWith(...); }

I don't think I understand exactly what you mean.
Is it about using an assertion scope within Satisfy like this:

personAndAddressDto.Should().Satisfy<PersonAndAddressDto>(dto => { dto.Person.Name.Should().NotBeNullOrEmpty(); using(new AssertionScope()) { dto.Address.Should().Satisfy<AddressDto>(address => address.City.Should().NotBeNullOrEmpty()); } }

After I added the null-check, it seems to work as I would expect.

I'm not sure why anyone would add a nested assertion scope though, but of course it needs to be handled correctly.

What @jnyrup means is that if you're new Satisfy is wrapped in an AssertionScope (just like you do yourself), the FailWith call with not throw an exception and return false instead.

I'm so sorry for the late reply. I will try to support that.

Does this test verify the expected behavior (it passes with the current implementation)?

[Fact] public void Using_assertion_scope_with_null_subject() { // Arrange object subject = null; // Act Action act = () => { using (new AssertionScope()) { subject.Should().Satisfy<object>(x => x.Should().NotBeNull()); } }; // Assert act.Should().Throw<XunitException>() .WithMessage("Expected subject to be assignable to System.Object, but found <null>."); }

If this is not what you mean, I need some help understanding the scenario you have in mind.

…spectors

Co-authored-by: Dennis Doomen <dennis.doomen@avivasolutions.nl>

jnyrup · 2024-04-10T15:57:16Z

Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs

+    /// <param name="assertion">The element inspector which must be satisfied by the <typeparamref name="TSubject" />.</param>
+    /// <returns>An <see cref="AndConstraint{T}" /> which can be used to chain assertions.</returns>
+    /// <exception cref="ArgumentNullException"><paramref name="assertion"/> is <see langword="null"/>.</exception>
+    public AndConstraint<TAssertions> Satisfy<T>(Action<T> assertion)


Should this also have string because = "", params object[] becauseArgs?

It might, but I don't know how to connect that to the nested assertions or how the resulting assertion message should look.
In practice, the nested assertions can have the because, but the parent would probably always have the message "because the assertions should be satisfied" or something like that.

siewers mentioned this pull request Feb 29, 2024

Allow Should().Satisfy() on objects and not only collections #2516

Open

ITaluone reviewed Mar 1, 2024

View reviewed changes

Tests/FluentAssertions.Specs/Primitives/ReferenceTypeAssertionsSpecs.Satisfy.cs Outdated Show resolved Hide resolved

siewers force-pushed the develop branch from c330735 to 0161483 Compare March 1, 2024 08:34

dennisdoomen reviewed Mar 2, 2024

View reviewed changes

siewers force-pushed the develop branch 2 times, most recently from 84bfdf1 to a7be412 Compare March 12, 2024 13:44

IT-VBFK reviewed Mar 12, 2024

View reviewed changes

siewers closed this Mar 17, 2024

dennisdoomen mentioned this pull request Mar 17, 2024

Allow nested AssertionScopes to chain their context #2607

Merged

7 tasks

siewers reopened this Mar 17, 2024

jnyrup reviewed Mar 24, 2024

View reviewed changes

siewers and others added 8 commits March 24, 2024 20:48

Add support for Satisfy on reference type assertions using element in…

4782fa4

…spectors

Combine Act and Assert in tests that should not throw

be7a019

Reorganize and add additional test for complex object not throwing

b7a72b0

Update release notes

c4147b9

Update tests to follow guidelines

4d56d81

Add tests asserting derived types

b766164

Use date extensions in assertions

1890189

Rephrase release notes

df10e70

Co-authored-by: Dennis Doomen <dennis.doomen@avivasolutions.nl>

siewers added 4 commits March 24, 2024 20:48

Rephrase XML documentation

cdd78c6

Fix documentation layout

809d814

Approve API

b9af249

Check subject for null

528af7d

siewers force-pushed the develop branch from a6b5b8c to 528af7d Compare March 24, 2024 19:48

jnyrup mentioned this pull request Apr 10, 2024

[API Proposal]: Allow to assert RegEx matched groups #2625

Open

jnyrup reviewed Apr 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Satisfy on ReferenceTypeAssertions #2597

Add support for Satisfy on ReferenceTypeAssertions #2597

siewers commented Feb 29, 2024 •

edited

github-actions bot commented Feb 29, 2024 •

edited

ITaluone commented Mar 1, 2024

siewers commented Mar 1, 2024 •

edited

ITaluone commented Mar 1, 2024

siewers commented Mar 1, 2024

siewers commented Mar 1, 2024

dennisdoomen Mar 2, 2024

siewers Mar 8, 2024

dennisdoomen Mar 11, 2024

siewers Mar 11, 2024

dennisdoomen Mar 11, 2024

siewers Mar 23, 2024 •

edited

dennisdoomen Mar 24, 2024

dennisdoomen Mar 24, 2024

jnyrup Mar 24, 2024

dennisdoomen Mar 25, 2024

coveralls commented Mar 11, 2024 •

edited

dennisdoomen commented Mar 11, 2024

ITaluone commented Mar 12, 2024

IT-VBFK Mar 12, 2024 •

edited

dennisdoomen Mar 12, 2024

dennisdoomen commented Mar 17, 2024

siewers commented Mar 17, 2024

jnyrup Mar 24, 2024

siewers Mar 24, 2024

jnyrup Mar 24, 2024

siewers Mar 24, 2024

dennisdoomen Mar 25, 2024

siewers Apr 25, 2024

siewers Apr 27, 2024

jnyrup Apr 10, 2024

siewers Apr 11, 2024

Add support for Satisfy on ReferenceTypeAssertions #2597

Are you sure you want to change the base?

Add support for Satisfy on ReferenceTypeAssertions #2597

Conversation

siewers commented Feb 29, 2024 • edited

IMPORTANT

github-actions bot commented Feb 29, 2024 • edited

Qodana for .NET

ITaluone commented Mar 1, 2024

siewers commented Mar 1, 2024 • edited

ITaluone commented Mar 1, 2024

siewers commented Mar 1, 2024

siewers commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siewers Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 11, 2024 • edited

Pull Request Test Coverage Report for Build 8318031359

Details

💛 - Coveralls

dennisdoomen commented Mar 11, 2024

ITaluone commented Mar 12, 2024

IT-VBFK Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisdoomen commented Mar 17, 2024

siewers commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siewers commented Feb 29, 2024 •

edited

github-actions bot commented Feb 29, 2024 •

edited

siewers commented Mar 1, 2024 •

edited

siewers Mar 23, 2024 •

edited

coveralls commented Mar 11, 2024 •

edited

IT-VBFK Mar 12, 2024 •

edited