Better failure messages for Subset and Superset constraints #3505

Dreamescaper · 2020-03-15T09:54:05Z

Provide extra/missing items to CollectionSubsetConstraint and CollectionSupersetConstraint failure messages.

E.g.

  Expected: subset of < 1, 2, 3, 4, 5 >
  But was:  < 1, 2, 2, 2, 5 >
  Extra items: < 2, 2 >        <------ ADDED

Dreamescaper · 2020-03-15T22:31:07Z

src/NUnitFramework/framework/Constraints/MsgUtils.cs

        /// <param name="start">The starting point of the elements to write</param>
        /// <param name="max">The maximum number of elements to write</param>
-        public static string FormatCollection(IEnumerable collection, long start, int max)
+        public static string FormatCollection(IEnumerable collection, long start = 0, int max = DefaultMaxItems)


Decided to add default values, since it is used in most cases with those arguments.
Let me know if there are any objections.

Personally, I really like this. It makes it easy to ensure future calls to this will always respect the defaults unless the author explicitly decides not to.
Is there any way you could expose just this change in a separate commit on your branch? I'd like to graft it onto my own branch for #3502 to help simplify things there

Updated PR to have separate commit for MsgUtils change. Still, not sure it's worth reusing it until this change is approved.

I really like this change! Can I suggest updating all the places this method is currently used to make use of it too? The vast majority are currently passing in zero and ten.

ChrisMaddock

What a great little quality-of-life improvement! Nice job adding this, @Dreamescaper!

Left a couple of minor suggestions - this will also need a review from another team member before merging.

ChrisMaddock · 2020-04-05T15:22:35Z

src/NUnitFramework/framework/Constraints/MsgUtils.cs

        /// <param name="start">The starting point of the elements to write</param>
        /// <param name="max">The maximum number of elements to write</param>
-        public static string FormatCollection(IEnumerable collection, long start, int max)
+        public static string FormatCollection(IEnumerable collection, long start = 0, int max = DefaultMaxItems)


I really like this change! Can I suggest updating all the places this method is currently used to make use of it too? The vast majority are currently passing in zero and ten.

ChrisMaddock · 2020-04-05T15:30:00Z

src/NUnitFramework/framework/Constraints/CollectionSupersetConstraint.cs

            tally.TryRemove(_expected);

-            return tally.Result.ExtraItems.Count == 0;
+            _missingItems = tally.Result.ExtraItems;


Is this correct? It's a little confusing to read...maybe worth a comment? 🙂

The reason is that we have

CollectionTally tally = Tally(actual); tally.TryRemove(_expected);

in this file, whereas we have

CollectionTally tally = Tally(_expected); tally.TryRemove(actual);

in CollectionSubsetConstraint.cs. I'm a bit tired, so I cannot determine if we can use MissingItems here and then just swap arund _expected and actual (I'm mostly worried about duplicates).

Added comment.

@mikkelbu
Seems like no issues with duplicates, but it affect order of parameters custom comparer lambda - .Using<int, string>((i, s) => ..) VS .Using<string, int>((s, i) => ..).
Currently this order differs between CollectionSupersetConstraint and CollectionSubsetConstraint.
While this looks like a bug, and would make sense to make it consistent, that would be a breaking change, so I'm not sure what would be better here.

If it will be a breaking change, then I think the current solution - with the comment - is the right one 👍

mikkelbu

Also looks good from me with one comment. My main worry has nothing to do with this change, but that we are using the word set to describe something that can contain duplicates which a set cannot have by its definition, so rather we are talking about multisets or bags here 😄.

mikkelbu · 2020-04-05T20:51:01Z

src/NUnitFramework/framework/Constraints/CollectionSupersetConstraint.cs

            tally.TryRemove(_expected);

-            return tally.Result.ExtraItems.Count == 0;
+            _missingItems = tally.Result.ExtraItems;


The reason is that we have

CollectionTally tally = Tally(actual); tally.TryRemove(_expected);

in this file, whereas we have

CollectionTally tally = Tally(_expected); tally.TryRemove(actual);

in CollectionSubsetConstraint.cs. I'm a bit tired, so I cannot determine if we can use MissingItems here and then just swap arund _expected and actual (I'm mostly worried about duplicates).

CharliePoole · 2020-04-05T22:11:56Z

@mikkelbu Regarding your concern about use of the word "set"...

Much of this comes from our early implementation of CollectionAssert, which was intended to mimic the MsTest class of the same name. That class still contains the method IsSubsetOf, which they define as

Tests whether one collection is a subset of another collection and throws an exception if any element in the subset is not also in the superset

So it never had any reference to "sets" in the mathematical sense. Using "subset" and "superset" in this way is pretty common in normal language as well.

mikkelbu

LGTM

ChrisMaddock

Nice - thanks for doing this @Dreamescaper!

Dreamescaper force-pushed the subset_superset_better_messages branch 2 times, most recently from 6087bad to 951c0f5 Compare March 15, 2020 12:04

Dreamescaper commented Mar 15, 2020

View reviewed changes

Dreamescaper added 2 commits March 22, 2020 22:53

Add default values to FormatCollection method parameters

dda80d6

Better failure messages for Subset and Superset constraints

1f70da3

Dreamescaper force-pushed the subset_superset_better_messages branch from 951c0f5 to 1f70da3 Compare March 22, 2020 20:54

ChrisMaddock previously approved these changes Apr 5, 2020

View reviewed changes

ChrisMaddock added awaiting:contributor awaiting:review labels Apr 5, 2020

stevenaw mentioned this pull request Apr 5, 2020

Unique Constraint failure message only outputs failed items #3502

Closed

Add ExtraItems comment

af46ff1

mikkelbu previously approved these changes Apr 5, 2020

View reviewed changes

Replace FormatCollection usages to use default values.

bbf6a0b

Dreamescaper dismissed stale reviews from mikkelbu and ChrisMaddock via bbf6a0b April 5, 2020 21:15

mikkelbu approved these changes Apr 6, 2020

View reviewed changes

ChrisMaddock approved these changes Apr 7, 2020

View reviewed changes

ChrisMaddock merged commit 8c3fcaa into nunit:master Apr 7, 2020

ChrisMaddock assigned Dreamescaper Apr 7, 2020

ChrisMaddock added this to the 3.13 milestone Apr 7, 2020

ChrisMaddock added closed:done and removed awaiting:contributor awaiting:review labels Apr 7, 2020

Better failure messages for Subset and Superset constraints #3505

Better failure messages for Subset and Superset constraints #3505

Uh oh!

Conversation

Dreamescaper commented Mar 15, 2020

Uh oh!

Dreamescaper Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisMaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikkelbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharliePoole commented Apr 5, 2020

Uh oh!

mikkelbu left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisMaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dreamescaper Mar 15, 2020 •

edited

Loading