Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
siewers opened this issue Dec 15, 2023 · 20 comments
Open

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

siewers opened this issue Dec 15, 2023 · 20 comments
Labels
api-approved API was approved, it can be implemented enhancement

Comments

@siewers
Copy link

siewers commented Dec 15, 2023

Background and motivation

When I'm asserting many properties on multiple instances, I often find it hard to keep track of the context of what I'm asserting.
Like with collections, we have a collection.Should().AllSatisfy(item => { /* do FluentAssertions on item here */ }, but I feel like this is missing for single instances.

The closest thing I've found is instance.Should().Match(/* something that returns true or false */), which is not really fluent as I see it.
The alternative is to do object comparison, where I need to construct the expected object before and then do instance.Should().BeEquivalentTo(expected).

What I'm suggesting is to allow scoped assertions on a single instance, like this:

var person = new Person { Name = "Peter", Age = 30 };

person.Should().Satisfy(p => 
{
    p.Name.Should().StartWith("P");
    p.Age.Should().BeGreaterThan(25);
});

Providing this, also allows me to extract generic assertions as methods, which can be reused.

I am aware that the above is equivalent to this:

var person = new Person { Name = "Peter", Age = 30 };

person.Name.Should().StartWith("P");
person.Age.Should().BeGreaterThan(25);

However, if I have many assertions where the variable names are long, it becomes quite a lot of noise in the tests.
For instance:

Person someInstanceOfPersonComingFormACallbackFromNSubstitute;

someInstanceOfPersonComingFormACallbackFromNSubstitute.Name.Should().StartWith("P");
someInstanceOfPersonComingFormACallbackFromNSubstitute.Age.Should().BeGreaterThan(25);

This would be, IMHO, more readable like this:

Person someInstanceOfPersonComingFormACallbackFromNSubstitute;

someInstanceOfPersonComingFormACallbackFromNSubstitute.Should().Satisfy(p => 
{
    p.Name.Should().StartWith("P");
    p.Age.Should().BeGreaterThan(25);
});

I am well aware that there might be a good reason for this to not exist and that it's possible to create a simple extension method like this:

public static void ShouldSatisfy<T>(this  T actual, Action<T> assert) where T : class
{
    assert(actual);
}

I also understand that this approach could lead to confusion since the Satisfy() method would have to exist on all types being asserted.

It might also be the case that my tests are too complex, however, it's an integration test, so I have a lot to assert, which makes the need clear to me.

Alternative Concerns

No response

Are you willing help with a pull-request?

Yes, please assign this issue to me.

@jnyrup
Copy link
Member

jnyrup commented Dec 15, 2023

We had a similar discussion in #2263, where I also added some additional use cases and benefits for having Satisfy available on single objects in addition to collections.

I like this idea 👍

A next step would be to dig into which XyzAssertions (perhaps multiple) this should be available from.

@jnyrup jnyrup added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed triage labels Dec 15, 2023
@siewers
Copy link
Author

siewers commented Dec 15, 2023

@jnyrup I'm sorry, I didn't find that specific issue when I searched, but it does look similar to what I'm proposing here.

@dennisdoomen
Copy link
Member

dennisdoomen commented Dec 15, 2023

A next step would be to dig into which XyzAssertions (perhaps multiple) this should be available from.

Can you pick that up @siewers ? When we have the full list, we can approve the API.

@siewers
Copy link
Author

siewers commented Dec 16, 2023

@dennisdoomen I'm a bit unsure how to proceed. I don't have an overview of the FluentAssertions architecture and design. I might simply start by "brute-forcing" the idea in and take it from there. It's usually the easiest way for me to learn.

@dennisdoomen
Copy link
Member

Sure, that could work.

@siewers
Copy link
Author

siewers commented Dec 25, 2023

I've looked into it and I must admit it doesn't seem possible to do what I'm asking. Because I need the Should()extension to be generic, I think this would be a major breaking change and likely not feasible.
@jnyrup Do you have any suggestions on how to achieve this?

@siewers
Copy link
Author

siewers commented Dec 27, 2023

The closest thing I've been able to get is where I explicitly make the Satisfy method generic, similar to Match, so it would look something like this:

SomeType subject = new SomeType();
subject.Should()
       .Satisfy<SomeType>(s => s.Property1.Should().NotBeEmpty());
/* Etc. */

The generic parameter on Satisfy could be constrained to match TSubject, but it doesn't make any difference since TSubject is lost when using ObjectAssertions that are not already built using one of the know type extensions of Should.

@dennisdoomen
Copy link
Member

The closest thing I've been able to get is where I explicitly make the Satisfy method generic, similar to Match, so it would look something like this:

That's the only way I can think of to make this work. You would have to cast the object back to TSubject, but that could be an acceptable tradeoff.

@siewers
Copy link
Author

siewers commented Feb 15, 2024

In that case, I believe an additional overload to Match that accepts an element inspector would be a better solution.
It would also make sense to reuse the existing terminology of "match" which makes sense to me since I'm asking whether it matches either a predicate or an inspector.

@siewers
Copy link
Author

siewers commented Feb 15, 2024

I've added similar tests as what is on the current Match method:

Using predicates:

someObject.Should().Match(o => o == null, "it is not initialized yet");
someObject.Should().Match((SomeDto d) => d.Name.Length == 0, "it is not initialized yet");

Using element inspectors:

someObject.Should().Match<object>(o => o.Should().BeNull("it is not initialized yet"));
someObject.Should().Match<SomeDto>(d => d.Name.Should().HaveLength(0, "it is not initialized yet"));

It is a bit longer, but it is a lot more expressive this way. I can write assertions on each property with the expectations and "because", which, in the end, produces a lot more meaningful output.

It will also allow you to do more complex assertions:

// Arrange / Act
var someObject = new SomeDto
{
    Name = "Dennis Doomen",
    Age = 36,
    Birthdate = new DateTime(1973, 9, 20)
};

// Assert
someObject.Should().Match<SomeDto>(d =>
{
    d.Name.Should().Be("Someone Else");
    d.Age.Should().BeGreaterThan(40, because: "the person is at least 40");
    d.Birthdate.Should().BeAfter(new DateTime(1982, 9, 22));
});

This will return the following error message:

Expected someObject to match inspector, but the inspector is not satisfied:
    Expected d.Name to be the same string, but they differ at index 0:
       ↓ (actual)
      "Dennis Doomen"
      "Someone Else"
       ↑ (expected).
    Expected d.Age to be greater than 40 because the person is at least 40, but found 36 (difference of -4).
    Expected d.Birthdate to be after <1982-09-22>, but found <1973-09-20>.

I've also tested with nested assertions. It's a bit complex, but I find it a lot more readable (arguably, this is probably too much, but it's just there as an example).

// Arrange / Act
var someObject = new SomeComplexDto
{
    Person = new SomeDto
    {
        Name = "Buford Howard Tannen",
        Age = 48,
        Birthdate = new DateTime(1937, 3, 26)
    },
    Address = new AddressDto
    {
        Street = "Mason Street",
        Number = "1809",
        City = "Hill Valley",
        Country = "United States",
        PostalCode = "CA 91905"
    }
};

// Assert
someObject.Should().Match<SomeComplexDto>(dto =>
{
    dto.Person.Should().Match<SomeDto>(person =>
    {
        person.Name.Should().Be("Biff Tannen");
        person.Age.Should().Be(48);
        person.Birthdate.Should().Be(new DateTime(1937, 3, 26));
    });
    dto.Address.Should().Match<AddressDto>(address =>
    {
        address.Street.Should().Be("Mason Street");
        address.Number.Should().Be("1809");
        address.City.Should().Be("Hill Valley, San Diego County, California");
        address.Country.Should().Be("United States");
        address.PostalCode.Should().Be("CA 91905");
    });
});

This will produce the following output:

Expected someObject to match inspector, but the inspector is not satisfied:
    Expected dto.Person to match inspector, but the inspector is not satisfied:
        Expected person.Name to be the same string, but they differ at index 1:
            ↓ (actual)
          "Buford Howard Tannen"
          "Biff Tannen"
            ↑ (expected).
    Expected dto.Address to match inspector, but the inspector is 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).

@dennisdoomen dennisdoomen changed the title [Feature]: Allow Should().Satisfy() on objects and not only collections Allow Should().Satisfy() on objects and not only collections Feb 15, 2024
@dennisdoomen
Copy link
Member

There's already a Match, so this can't work. But a Satisfy method would also align well with the one on collections.

@siewers
Copy link
Author

siewers commented Feb 16, 2024

I did implement it as an overload for Match so it does work, but if Satisfy is better that's fine with me. I just thought it was more natural to have the option to use either predicates or inspectors.
Satisfy is also overloaded with both predicates and inspectors.

@jnyrup
Copy link
Member

jnyrup commented Feb 24, 2024

A next step would be to dig into which XyzAssertions (perhaps multiple) this should be available from.

IIRC this lists all root Assertion types and from that it seems to me that implementing the API on only ReferenceTypeAssertions is fine. That also aligns with the location of the existing ReferenceTypeAssertions.Match.

I vote for adding Satisfy over overloading Match.

@dennisdoomen dennisdoomen added api-approved API was approved, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 24, 2024
@dennisdoomen
Copy link
Member

Let's go for Satisfy then. @siewers you're good to go.

@siewers
Copy link
Author

siewers commented Feb 29, 2024

Great! I'll introduce the Satisfy method.

On a side-note, I'm getting hit by an annoying bug in Rider, causing the tests to fail due to Rider trimming whitespaces inside raw string literals when saving.
I ended up splitting the Match and Satisfy tests into separate partial classes, which I hope is okay.

@siewers
Copy link
Author

siewers commented Feb 29, 2024

#2597

@dennisdoomen
Copy link
Member

I ended up splitting the Match and Satisfy tests into separate partial classes, which I hope is okay.

Yes, but as a separate commit or PR please. Also, add a nested class to the root of each partial class to provide a kind of grouping and context so you don't have to repeat words like match and satisfy.

@siewers
Copy link
Author

siewers commented Mar 1, 2024

Sounds good.
I didn't see any other example of nested test classes, so I tried to follow the pattern of the tests I was modifying. It would be nice if there was some guidelines on the test class pattern (I didn't find any when I looked through the contribution guidelines).

@dennisdoomen
Copy link
Member

There are some guidelines in #1340 (pinned to the top of the issues page), but I agree it could be a bit better.

@siewers
Copy link
Author

siewers commented Mar 1, 2024

I'm so sorry, I read that, but apparently, I wasn't thorough enough. I'll update the tests to reflect that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved, it can be implemented enhancement
Projects
None yet
Development

No branches or pull requests

3 participants