[ISSUE 2629] Improve readability of containsExactly #2638

Giovds · 2022-06-01T23:24:41Z

Removes elementsDifferAtIndex() from assertContainsExactly() and re-use
shouldContainExactlyWithDiffAssertionError() in order to show the actual
and expected sets entirely. Making the differencesFound() check unnecessary.

It also introduces a new shouldContainExactly() and checks for both
the missing and unexpected diffs to be empty. Otherwise cases where the
actual and expected only differ in order, result into logging of missing
elements - which there are none.

Check List:

Fixes Display Improve readability of containsExactly when the order of elements does not match #2629
Unit tests : YES
Javadoc with a code example (on API only) : NA
PR meets the contributing guidelines

Following the contributing guidelines will make it easier for us to review and accept your PR.

vlsi · 2022-06-02T05:33:50Z

src/main/java/org/assertj/core/internal/Iterables.java

-          throw shouldContainExactlyWithDiffAssertionError(diff, actual, values, info);
-        }
-        throw failures.failure(info, elementsDifferAtIndex(actualAsList.get(i), values[i], i, comparisonStrategy));
+        throw shouldContainExactlyWithDiffAssertionError(diff, actual, values, info);


Does it make sense to include the first index of the diff elements into the message as well?

joel-costigliola · 2022-06-02T10:02:53Z

src/test/java/org/assertj/core/error/ShouldContainExactly_create_Test.java

+                                   + "Expecting actual:%n"
+                                   + "  [\"Yoda\", \"Han\", \"Luke\", \"Anakin\"]%n"
+                                   + "to contain exactly (and in same order):%n"
+                                   + "  [\"Yoda\", \"Luke\", \"Han\", \"Anakin\"]%n"));


I think what would be more helpful here in addition to this description is to have the list of mismatches, each mismatch showing the index, the actual element value and the expected element value.

On the example above we could add

but there were differences at these indexes: - index 1: actual element "Han" but expected was "Luke" - index 2: actual element "Luke" but expected was "Han"

or

but there were differences at these indexes: - element at index 1: expected "Han" but was "Luke" - element at index 2: expected "Luke" but was "Han"

I prefer option 2, WDYT?

I'm afraid the list of mismatches could be huge (e.g. both lists like 1000 elements and they are in the reverse order), so there should be a hard limit on the number of printed mismatches

Option 3:

@@ -1,4 +1,4 @@ Yoda -Han Luke +Han Anakin

I do like the idea of showing (some) indexes, since it may not be as easy to see the differences at a first glance, but I didn't want to differ to much from the original suggestion in the issue.

A couple of questions arise:
In the case actual and expected are equal but not in order, you would show 2 indexes per mismatch which might be too verbose? Does it only count for equal but unorder comparisons, or do we and how would we want to handle other cases like indexes of unexpected and/or missing elements? Should we consider indexes on almost equal cases like [1, 2, 3] vs [1, 2, 4, 3] or could it be too complex?

Wouldn't almost all options be big and perhaps unreadable if the lists contain lots of elements? Option 3 would still add 2 lines per mismatch, resulting in a long list where you would have to scroll a while and without color differences to support the - and + signs it might be difficult to spot the differences if they are 10 elements apart? I'd think the creator of the unit test should consider smaller data sets to prove the functionality is working. Ordering a set of 10 elements would be as good of a test as ordering 1000 elements, unless you are testing performance in which case you wouldn't look at the sets itself, but the time it took?

Do note
The current implementation of Iterables#assertContainsExactly() throws a failure at the first difference it has found, after which it will only remember the missing and unexpected elements without any other information. You might currently not even know anything if the actual and expected are of not the same length. So I'm not sure what the impact of such a change will be, I'd have to look into that.

I was playing around with the amount of indexes to display yesterday and the MAX_ELEMENTS_FOR_PRINTING is a lot indeed. The first 10 to 50 seems reasonable to me. It could be useful feedback to display something along the lines:

but there were differences at these indexes: element at index 0: expected "Luke" but was "Han" ... element at index 9: expected "Anakin" but was "Leia" Only showing the first 10 mismatches

WDYT?

What output would you suggest for the following call assertThat(Arrays.asList(10, 1, 2, 3, 4, 5, 6, 7, 8, 9)).containsExactly(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); ?
I guess displaying all the indices as mismatching won't be that helpful.

@Giovds LGTM, maybe we should move Only showing the first 10 mismatches before showing the indexes to be sure users will see it (I'm fine with it at the end too, it's just a suggestion)

but there were differences at these indexes (only showing the first 10 mismatches): element at index 0: expected "Luke" but was "Han" ... element at index 9: expected "Anakin" but was "Leia"

If we have less than 10 mismatches, no need to mention only showing the first 10 mismatches

this is a bit of a contrived example @vlsi, I feel the proposed error message is still helpful enough in this case.

I'm also happy to increase to 50 mismatches by default

Why do you mean the example is contrieved? If the use-case is to assert elements in a strict order, then failure cases would likely be "almost matching" like I show here. There was almost an exact match, except the last element is misplaced.

If all the elements are completely shuffled, then there's no way to briefly describe the mismatch. However, if the elements almost match, then AssertJ could help the user to understand the mismatch

Giovds · 2022-06-07T22:03:31Z

I'm not sure about this approach, but I'd like to hear your thoughts on it!

src/test/java/org/assertj/core/error/ShouldContainExactly_create_Test.java

joel-costigliola · 2022-07-03T10:19:37Z

@Giovds is your PR ready for a review?

Giovds · 2022-07-03T12:46:10Z

@Giovds is your PR ready for a review?

@joel-costigliola Yes, that would be great!

Removes elementsDifferAtIndex() from assertContainsExactly() and re-use shouldContainExactlyWithDiffAssertionError() in order to show the actual and expected sets entirely. Making the differencesFound() check unnecessary. It also introduces a new shouldContainExactly() and checks for both the missing and unexpected diffs to be empty. Otherwise cases where the actual and expected only differ in order, result into logging of missing elements - which there are none.

Introduces an IndexedDiff to keep track of the diff at an index. If the lists are of the same length and contain the same elements, but are in not in the same order, print the indices - limited to 50 - where a mismatch occurred.

joel-costigliola · 2022-07-15T10:09:17Z

Integrated in 41179e5 thanks @Giovds!

vlsi reviewed Jun 2, 2022

View reviewed changes

joel-costigliola requested changes Jun 2, 2022

View reviewed changes

Giovds commented Jun 7, 2022

View reviewed changes

src/test/java/org/assertj/core/error/ShouldContainExactly_create_Test.java Outdated Show resolved Hide resolved

scordio force-pushed the main branch from a43399a to addf7d2 Compare June 17, 2022 23:03

Giovds force-pushed the issue-2629-improve-readability-of-contains-exactly branch from 1005feb to d6f1d90 Compare June 29, 2022 19:34

Giovds added 4 commits July 13, 2022 21:16

[ISSUE 2629] Add indices in containsExactly()

e1167da

Introduces an IndexedDiff to keep track of the diff at an index. If the lists are of the same length and contain the same elements, but are in not in the same order, print the indices - limited to 50 - where a mismatch occurred.

No need to import everything

d749eed

[ISSUE 2629] Add license to new file

4ba9631

Giovds force-pushed the issue-2629-improve-readability-of-contains-exactly branch from d6f1d90 to 4ba9631 Compare July 13, 2022 19:16

joel-costigliola closed this Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE 2629] Improve readability of containsExactly #2638

[ISSUE 2629] Improve readability of containsExactly #2638

Giovds commented Jun 1, 2022

vlsi Jun 2, 2022

joel-costigliola Jun 2, 2022 •

edited

joel-costigliola Jun 2, 2022

vlsi Jun 2, 2022

vlsi Jun 2, 2022

Giovds Jun 2, 2022

Giovds Jun 4, 2022

vlsi Jun 4, 2022

joel-costigliola Jun 6, 2022

joel-costigliola Jun 6, 2022

vlsi Jun 7, 2022 •

edited

Giovds commented Jun 7, 2022

joel-costigliola commented Jul 3, 2022

Giovds commented Jul 3, 2022

joel-costigliola commented Jul 15, 2022

[ISSUE 2629] Improve readability of containsExactly #2638

[ISSUE 2629] Improve readability of containsExactly #2638

Conversation

Giovds commented Jun 1, 2022

Check List:

Choose a reason for hiding this comment

joel-costigliola Jun 2, 2022 • edited

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

vlsi Jun 7, 2022 • edited

Choose a reason for hiding this comment

Giovds commented Jun 7, 2022

joel-costigliola commented Jul 3, 2022

Giovds commented Jul 3, 2022

joel-costigliola commented Jul 15, 2022

joel-costigliola Jun 2, 2022 •

edited

vlsi Jun 7, 2022 •

edited