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 IEqualityComparer in string equality assertion #2339

Closed
bart-vmware opened this issue Sep 28, 2023 · 10 comments · Fixed by #2413
Closed

Allow IEqualityComparer in string equality assertion #2339

bart-vmware opened this issue Sep 28, 2023 · 10 comments · Fixed by #2413
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation

Comments

@bart-vmware
Copy link

bart-vmware commented Sep 28, 2023

Background and motivation

I'm trying to assert two strings are equal using a custom comparer. The code below works, though it would be nice if the cast to object wouldn't be necessary and Be() could be used instead of BeEquivalentTo().

using System.Text.RegularExpressions;
using FluentAssertions;

string logLines = @"one\ntwo\nthree";
string expected = @"one\r\ntwo\r\nthree";

((object)logLines).Should().BeEquivalentTo(expected, options => options.Using(IgnoreLineEndingsComparer.Instance));

public sealed class IgnoreLineEndingsComparer : IEqualityComparer<string>
{
    public static readonly IgnoreLineEndingsComparer Instance = new();

    public bool Equals(string x, string y)
    {
        if (x == y)
        {
            return true;
        }

        if (x == null || y == null)
        {
            return false;
        }

        var xLines = Regex.Split(x, "\r\n|\r|\n");
        var yLines = Regex.Split(x, "\r\n|\r|\n");

        return xLines.SequenceEqual(yLines);
    }

    public int GetHashCode(string obj)
    {
        return obj.GetHashCode();
    }
}

API Proposal

public class StringAssertions<TAssertions> : ReferenceTypeAssertions<string, TAssertions>
    where TAssertions : StringAssertions<TAssertions>
{
    public AndConstraint<TAssertions> Be(string expected, IEqualityComparer<string> comparer,
        string because = "", params object[] becauseArgs);
}

API Usage

using System.Text.RegularExpressions;
using FluentAssertions;

string logLines = @"one\ntwo\nthree";
string expected = @"one\r\ntwo\r\nthree";

logLines.Should().Be(expected, IgnoreLineEndingsComparer.Instance);

Alternative Designs

No response

Risks

No response

Are you willing to help with a proof-of-concept (as PR in that or a separate repo) first and as pull-request later on?

No

@bart-vmware bart-vmware added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 28, 2023
@jnyrup
Copy link
Member

jnyrup commented Sep 30, 2023

For the provided example you should be able to use the existing:

logLines.Should().MatchEquivalentOf(expected);

Note: the BeEquivalentTo assertion method for object and string works differently as StringAssertions.BeEquivalentTo is case insensitive, while ObjectAssertions.BeEquivalentTo boils down to StringAssertions.Be.

With v6 you can e.g. write these extension method.

public static class StringAssertionsExtensions
{
    [CustomAssertion]
    public static AndConstraint<TAssertions> Be<TAssertions>(this StringAssertions<TAssertions> parent,
        string expected, IEqualityComparer<string> comparer, string because = "", params object[] becauseArgs)
        where TAssertions : StringAssertions<TAssertions>
    {
        Execute.Assertion
            .ForCondition(comparer.Equals(parent.Subject, expected))
            .BecauseOf(because, becauseArgs)
            .FailWith("Expected {context:string} to be {0}{reason}, but found {1}.", expected, parent.Subject);

        return new AndConstraint<TAssertions>((TAssertions)parent);
    }

    [CustomAssertion]
    public static AndConstraint<TAssertions> Be2<TAssertions>(this StringAssertions<TAssertions> parent,
        string expected, IEqualityComparer<string> comparer, string because = "", params object[] becauseArgs)
        where TAssertions : StringAssertions<TAssertions>
    {
        ((object)parent.Subject).Should().Be(expected, comparer, because, becauseArgs);

        return new AndConstraint<TAssertions>((TAssertions)parent);
    }

    [CustomAssertion]
    public static AndConstraint<TAssertions> BeEquivalentTo<TAssertions>(this StringAssertions<TAssertions> parent,
        string expected, IEqualityComparer<string> comparer, string because = "", params object[] becauseArgs)
        where TAssertions : StringAssertions<TAssertions>
    {
        ((object)parent.Subject).Should().BeEquivalentTo(expected, opt => opt.Using(comparer), because, becauseArgs);

        return new AndConstraint<TAssertions>((TAssertions)parent);
    }
}

That on failure give these failure messages, respectively.

Expected logLines to be "one
two
three", but found "one
two
three1".
Expected logLines to be "one
two
three", but found "one
two
three1".
Expected logLines to be equal to "one
two
three" according to "FluentAssertions.Specs.IgnoreLineEndingsComparer", but "one
two
three1" was not.

With configuration:
- Use declared types and members
- Compare enums by value
- Compare tuples by their properties
- Compare anonymous types by their properties
- Compare records by their members
- Include non-browsable members
- Match member by name (or throw)
- Use FluentAssertions.Specs.IgnoreLineEndingsComparer for objects of type System.String
- Be strict about the order of items in byte arrays
- Without automatic conversion.

The downside of these is that they won't provide as pretty formatted failure messages as StringAssertions.BeEquivalentTo does, e.g. pointing at the index of the first mismatch etc.

Back to the API suggestion.
I think it's an valid proposal to provide an IEqualityComparer when the built-in string assertions are not sufficient.
E.g. GenericCollectionAssertions has HaveCount(Expression<Func<int, bool>> countPredicate) and several types have a (variant of) Match(Expression<Func<T, bool>> predicate) for similar purposes.
v6.12 #2243 introduced overloads in ObjectAssertions for Be/NotBe/BeOneOf that takes an IEqualityComparer, which is used in the Be2 extension method above.

I think we might also want look at the entire StringAssertions class if there are other methods in there where providing an IEqualityComparer<string> would be useful.

With the changes in #2296 it should be easy (famous last words) to built an adapter for IStringComparisonStrategy wrapping an IEqualityComparer<string>.

Related discussion for generic collections: #699

@dennisdoomen dennisdoomen changed the title [API Proposal]: Allow IEqualityComparer in string equality assertion Allow IEqualityComparer in string equality assertion Oct 1, 2023
@dennisdoomen
Copy link
Member

I also think we should rename StringAssertions.BeEquivalentTo to something else so it no longer overrides the structural equality API. Maybe something like BeCaseInsensitiveMatch (which is what ChatGPT suggested).

@bart-vmware
Copy link
Author

@jnyrup Thanks for looking into this.

For the provided example you should be able to use the existing:

logLines.Should().MatchEquivalentOf(expected);

This doesn't actually work, due to the bug here. But more importantly, MatchEquivalentOf ignores casing and line breaks, which is not what I need. For example, "three\n" should not be considered the same as "thr\nee\n". And I don't want * and ? to be interpreted as wildcards. My proposed workaround seems to be the only way to accomplish that.

The downside of these is that they won't provide as pretty formatted failure messages as StringAssertions.BeEquivalentTo does, e.g. pointing at the index of the first mismatch etc.

The only way I can think of to address that is to backtrack on match failure, which would be expensive if not vectorized.

@jnyrup
Copy link
Member

jnyrup commented Oct 2, 2023

I also think we should rename StringAssertions.BeEquivalentTo to something else so it no longer overrides the structural equality API. Maybe something like BeCaseInsensitiveMatch (which is what ChatGPT suggested).

In C# terms StringAssertions.BeEquivalentTo doesn't override any ObjectAssertions.BeEquivalentTo as it derives from ReferenceTypeAssertions.

@jnyrup
Copy link
Member

jnyrup commented Oct 2, 2023

The downside of these is that they won't provide as pretty formatted failure messages as StringAssertions.BeEquivalentTo does, e.g. pointing at the index of the first mismatch etc.

The only way I can think of to address that is to backtrack on match failure, which would be expensive if not vectorized.

#2307 is working on improving the failure message when comparing strings.
I wouldn't want to miss out on those beautiful failure messages.

@jnyrup
Copy link
Member

jnyrup commented Oct 3, 2023

Maybe something like BeCaseInsensitiveMatch

StringAssertions.BeEquivalentTo also ignores leading and trailing whitespace.

I'm dreaming of somethings like this

var subject = """
    Du
    bist
    süß
    """;

subject.Should().BeEquivalentTo("du bist süss", opt => opt
    .IgnoringLeadingWhitespace()
    .IgnoringTrailingWhitespace()
    .IgnoringNewlines()
    .IgnoringCase()
    .Using(new CultureInfo("de-DE")));

@dennisdoomen
Copy link
Member

In C# terms StringAssertions.BeEquivalentTo doesn't override any ObjectAssertions.BeEquivalentTo as it derives from ReferenceTypeAssertions.

I meant figuratively

@dennisdoomen
Copy link
Member

dennisdoomen commented Oct 3, 2023

I'm dreaming of somethings like this

var subject = """
    Du
    bist
    süß
    """;

subject.Should().BeEquivalentTo("du bist süss", opt => opt
    .IgnoringLeadingWhitespace()
    .IgnoringTrailingWhitespace()
    .IgnoringNewlines()
    .IgnoringCase()
    .Using(new CultureInfo("de-DE")));

Make it a separate issue. I'd love that.

@vbreuss
Copy link
Contributor

vbreuss commented Oct 4, 2023

I'm dreaming of somethings like this

var subject = """
    Du
    bist
    süß
    """;

subject.Should().BeEquivalentTo("du bist süss", opt => opt
    .IgnoringLeadingWhitespace()
    .IgnoringTrailingWhitespace()
    .IgnoringNewlines()
    .IgnoringCase()
    .Using(new CultureInfo("de-DE")));

Make it a separate issue. I'd love that.

I would be happy to work on such extensions for BeEquivalentTo after #2307 is merged, as I am now quite deeply involved in the StringEqualityStrategy :-)

Should I create a separate issue to discuss the possible options and API structure first?

@dennisdoomen
Copy link
Member

Should I create a separate issue to discuss the possible options and API structure first?

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
4 participants