fluentassertions: Improve failure message for `StringAssertions.Be`

Discussed in https://github.com/fluentassertions/fluentassertions/discussions/2048 and https://github.com/fluentassertions/fluentassertions/discussions/1941

^{Originally posted by thomOrbelius November 30, 2022} Hi.

In my department we have been starting to use FluentAssertions. One of the selling point I had was the improved failure messages. A colleague of mine pointed out that the Xunit failure messages for strings is much more helpful (see example below) than the FluentAssertion equivalent. The example shows a case where we use a string with a newline separator.

Do any one else think that a message more similar to the one Xunit has is more helpful than the current? If so, should we update the message used?

using FluentAssertions;
using Xunit;

namespace Sectra.Courier.RisAdapter.Tests.Integrations.SectraRis;

public class FluentComparison {
    private const string string1 = "String with \rnewline";
    private const string string2 = "String with \ra newline";

    [Fact]
    public void FluentAssert() {
        // Fails with:
        // newline" has a length of 20, differs near "new" (index 13).
        string1.Should().Be(string2);
    }

    [Fact]
    public void XUnitAssert() {
        // Fails with:
        // Assert.Equal() Failure
        //                         ↓ (pos 13)
        // Expected: String with \rnewline
        // Actual:   String with \ra newline
        //                         ↑ (pos 13)
        Assert.Equal(string1, string2);
    }
}

</div>

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 1
Comments: 47 (46 by maintainers)

Commits related to this issue

Add first rough attempt of improving string assertion failure messages Closes #2050 — committed to IT-VBFK/fluentassertions by IT-VBFK 2 years ago
Add first rough attempt of improving string assertion failure messages Closes #2050 — committed to IT-VBFK/fluentassertions by IT-VBFK 2 years ago

Most upvoted comments

I think that the approach with identifying just the first mismatch is good enough.

I agree that we should probably pursue showing the first mismatch with pointers plus the N leading characters and M trailing characters.

jnyrup on Dec 28, 2022

Mostly a shower thought, I’m not sure it applies well here.

Sequence Alignment is an area in computer science to find an aligment between two sequences, it could be applied to subject and expected.

E.g. one such alignment between "This is a test" and "This is another test" could be:
"This is a       test"
"This is another test"
Hirschberg’s algorithm can be used to compute a maximal global alignment between two sequences.

While I do think that the alignment approach is quite neat. But, if I just look at the result as you showed above, the risk it that I think that there is actually a lot if whitespaces in the first string but the lengths are correct, which could be confusing. The whitespace could of course be replaced by a placeholder symbol (· or *) or indicated on a separate “diff” line, but then the solution is approaching a diff tool again.

I think that the approach with identifying just the first mismatch is good enough. Or rather, I can’t think of a simple way to intuitively display both indices of mismatches combined with the alignment without it getting cluttered or confusing.

thomOrbelius on Dec 28, 2022

That settles the discussion. We’re going to keep the 1-based indexes.

dennisdoomen on Oct 1, 2023

I slightly adapted the implementation in PR #2307, so that when the string consists of multiple lines, the information contains not only the index of the first mismatch, but also the line and column information, e.g. but they differ on line 5 and column 1 (index 93).

vbreuss on Sep 27, 2023

@dennisdoomen @IT-VBFK : I created a draft for improving the failure message in #2307.

The general idea is, that for short single-line strings (up to 8 characters) the behaviour remains unchanged. For longer strings I would add the message with the arrows, e.g.:

var subject = "this is a long text that differs in between";
var expected = "this was too short";
subject.Should().Be(expected, "because we use arrows now");

throws with message

Expected subject to be the same string because we use arrows now, but they differ at index 5:
        ↓ (actual)
  "this is a long text…"
  "this was too short"
        ↑ (expected)."

Whenever the string was truncated at the beginning or end, I would also add ellipsis … to the string, so that it is clearer that it is not the whole string.

If the general approach is okay with you, I would add the missing unit tests (e.g. for BeEquivalentTo), but I wanted to discuss the overall approach first!

vbreuss on Sep 18, 2023

Let’s just say that I dislike the inheritance tree in the current implementation because it violates the principles of inheritance. For example, the StringValidator has all kinds of methods that don’t have an implementation at all. By definition, this means a violation of the Liskov Substitution Principiple. If I had time to redesign it, I would use composition over inheritance. So what you are suggesting is the opposite of what I would do.

So I would recommend to first define a list of all the use cases we need to support (both the positive as the negative ones), then identify what needs to be done per use case, and only then think how you would convert that to an implementation.

dennisdoomen on Feb 4, 2023

Is this we should aim for?

I don’t know. Haven’t thought of the design yet. FWIW, an interface is not a goal. Just see what needs to be done and design accordingly.

dennisdoomen on Jan 19, 2023