Issue3918 white space #4664

manfred-brands · 2024-03-16T10:34:27Z

Adds preparation work for #4662

There are two new constraints:

One to match String.IsNullOrWhiteSpace . This I modeled on 'Empty':

Assert.That(<s>, Is.WhiteSpace)
Assert.That(<s>, Is.Not.WhiteSpace)

The other is a modifier on existing constrains, which I modeled on IgnoreCase:

Assert.That("Hello World"), Is.EqualTo("HelloWorld").IgnoreWhiteSpace)

Adding the constraint was easy, to get sensible error messages in case of failure where strings have different white space was harder. In case of mis-matched white-space, the message now contains two carrots, one for expected and one for actual:

Expected: "abc def", ignoring white-space
-----------------^
But was:  "a b c d e g"
---------------------^

OsirisTerje · 2024-03-16T17:24:39Z

Awesome!

OsirisTerje

This is awesome work! Really like the changes in ConstraintExpression to the Append method, making it generic fix the typecasting that is all over the other constraints. Very good!

manfred-brands · 2024-03-17T08:54:50Z

@OsirisTerje The change to Append is an breaking change as it changes the signature of a public method in a public class.
I managed to keep the original and add the new one. Luckily the compiler is smart enough to distinguish.

Whilst updating the documentation, I noticed an issue with the reporting when texts are too long.
Put PR in draft until I sorted this out.

to: - AnyOfConstraint - CollectionItemsEqualConstaint - ContainsConstraint - EqualConstraint - UniqueItemsConstraint Updates to Clip String Requires individual clipping when white-space is ignored. We then also need to show two different ^ to indicate the mismatched location.

manfred-brands · 2024-03-18T02:59:03Z

All issues fixed, PR ready.

mikkelbu

Looks good to me. I mostly have minor questions and/or comments

mikkelbu · 2024-03-18T20:55:05Z

src/NUnitFramework/framework/Constraints/MessageWriter.cs

        /// <param name="clipping">If true, the strings should be clipped to fit the line</param>
-        public abstract void DisplayStringDifferences(string expected, string actual, int mismatch, bool ignoreCase, bool clipping);
+        public abstract void DisplayStringDifferences(string expected, string actual, int mismatchExpected, int mismatchActual, bool ignoreCase, bool ignoreWhiteSpace, bool clipping);


Isn't this change breaking? A public (abstract) class containing a public (abstract) method

Yes it would be. Thanks for noticing!

The problem then is that if I leave the original signature, I cannot do the message improvements.
Even adding a new abstract method would be breaking.
I think adding a a new virtual method it would be fine as it can be resolved without an implementation having to implement it. The default implementation would call into the original method unless:

if (ignoreWhiteSpace && mismatchExpected != mismatchActual) { throw new NotImplementedException("Please override to show difference with 'ignoreWhiteSpace'"); }

mikkelbu · 2024-03-18T20:57:09Z

src/NUnitFramework/tests/Constraints/EqualConstraintTests.cs

+                    "surname": Doe"
+                  }
+                ]
+                """;
+            const string condensedJson = """
+                "persons":[{"name":"John","surname":"Smith"},{"name": "Jane","surname": Doe"}]


Is the missing " before Doe on purpose?

No, it wasn't. I only noticed this myself when I had copied this example into the documentation and the strings were not formatted as expected. Initially I blamed it on the new """ string syntax until I spotted the missing ".

" added in two places.

mikkelbu · 2024-03-18T21:01:57Z

src/NUnitFramework/framework/Internal/Execution/TextMessageWriter.cs

            WriteLine();
+            if (mismatchExpected >= 0 && mismatchExpected != mismatchActual)
+                WriteCaretLine(mismatchExpected);
            WriteActualLine(actual);
            //DisplayDifferences(expected, actual);


Should we remove this line?

I presume you mean the one commented out and not the one I just added?

I suspect this line was commented out when the WriteCaretLine was added, but all of this was already there in the first commit to the repo 15 years ago. I agree it should be removed.

mikkelbu · 2024-03-18T21:08:31Z

src/NUnitFramework/tests/Constraints/MsgUtilTests.cs

+        [TestCase(S52, 26, 0, "abcdefghijklmnopqrstuvwxyz...", TestName = "ClipAtEnd")]
+        [TestCase(S52, 26, 26, "...ABCDEFGHIJKLMNOPQRSTUVWXYZ", TestName = "ClipAtStart")]
+        [TestCase(S52, 22, 26, "...ABCDEFGHIJKLMNOPQRSTUV...", TestName = "ClipAtStartAndEnd")]


Is this related to you comment on the PR (pasted below)? If not then what is this change about?

Whilst updating the documentation, I noticed an issue with the reporting when texts are too long.

No, it isn't. The original code had one places determining if the actual visible string should be clipped and in the called method it actually reduced the maxDisplayLength. I moved all determination code into the first method ClipWhenNeeded and reduced the ClipString method to do only the clipping to the length passed in.
This means that the length passed into the ClipString method is 3 or 6 less than previously and hence the updated numbers in the test.

mikkelbu · 2024-03-18T21:38:04Z

src/NUnitFramework/framework/Constraints/MsgUtils.cs

-                            sb.Append($"\\x{(int)c:X4}");
-                            break;
+            int originalIndex = index;
+            StringBuilder sb = new(s.Length + 32);


Purely to prevent an re-allocation/copy in most cases when there are a few characters to escape.
It could be any number. I can make it the magic '42' (“The Hitchhiker's Guide to the Galaxy”) or 10.

mikkelbu · 2024-03-18T21:42:21Z

src/NUnitFramework/framework/Constraints/MsgUtils.cs

        {
-            int clipLength = maxStringLength;
            StringBuilder sb = new StringBuilder();


Should we set this to be of a certain size - e.g. s.length + 6?

In case anyone ever changes the value of ELLIPSIS, I changed it to s.Length + 2 * ELLIPSIS.Length

src/NUnitFramework/framework/Constraints/MsgUtils.cs

…lder

OsirisTerje · 2024-03-25T17:13:24Z

@mikkelbu @manfred-brands Is this ready now to be merged ?

manfred-brands · 2024-03-25T23:11:15Z

@OsirisTerje I think so, but as I made changes after the review from @mikkelbu, I didn't know if he wants to look at it again.

OsirisTerje · 2024-03-27T20:44:04Z

I'll merge this now. @mikkelbu did approve earlier. If he finds more to change, we can add a new PR to address that. I can't see anything really.
With this merged, we can then continue with #4662

manfred-brands mentioned this pull request Mar 16, 2024

StringAssert.IsNullOrEmpty overloads #4662

Open

manfred-brands force-pushed the Issue3918_WhiteSpace branch from f786669 to cc452f8 Compare March 16, 2024 10:35

OsirisTerje approved these changes Mar 16, 2024

View reviewed changes

manfred-brands marked this pull request as draft March 17, 2024 08:50

manfred-brands added 3 commits March 18, 2024 10:20

Remove need for casts when using ConstraintExpression.Append

e18d099

Fix nullability of StringContraint.Matches

9c2fe6a

Add WhiteSpace Constraint

aa72a73

manfred-brands force-pushed the Issue3918_WhiteSpace branch 2 times, most recently from 7db260e to 21f195b Compare March 18, 2024 02:43

manfred-brands force-pushed the Issue3918_WhiteSpace branch from 21f195b to 8472e69 Compare March 18, 2024 02:51

manfred-brands marked this pull request as ready for review March 18, 2024 02:57

manfred-brands mentioned this pull request Mar 18, 2024

Document Is.WhiteSpace and IgnoreWhiteSpace nunit/docs#922

Merged

mikkelbu approved these changes Mar 18, 2024

View reviewed changes

Code review changes

ddf39d9

manfred-brands requested a review from mikkelbu March 20, 2024 01:21

OsirisTerje reviewed Mar 21, 2024

View reviewed changes

src/NUnitFramework/framework/Constraints/MsgUtils.cs Outdated Show resolved Hide resolved

Make it clear that what the extra size is for when creating StringBui…

c79204b

…lder

OsirisTerje merged commit bb18e96 into nunit:master Mar 27, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue3918 white space #4664

Issue3918 white space #4664

manfred-brands commented Mar 16, 2024

OsirisTerje commented Mar 16, 2024

OsirisTerje left a comment

manfred-brands commented Mar 17, 2024

manfred-brands commented Mar 18, 2024

mikkelbu left a comment

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

mikkelbu Mar 18, 2024

manfred-brands Mar 20, 2024

OsirisTerje commented Mar 25, 2024 •

edited

manfred-brands commented Mar 25, 2024

OsirisTerje commented Mar 27, 2024 •

edited

Issue3918 white space #4664

Issue3918 white space #4664

Conversation

manfred-brands commented Mar 16, 2024

OsirisTerje commented Mar 16, 2024

OsirisTerje left a comment

Choose a reason for hiding this comment

manfred-brands commented Mar 17, 2024

manfred-brands commented Mar 18, 2024

mikkelbu left a 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

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

OsirisTerje commented Mar 25, 2024 • edited

manfred-brands commented Mar 25, 2024

OsirisTerje commented Mar 27, 2024 • edited

OsirisTerje commented Mar 25, 2024 •

edited

OsirisTerje commented Mar 27, 2024 •

edited