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

Add option to display only unexpected or missing elements in asserts on collections #292

Closed
igorakkerman opened this issue Jan 6, 2020 · 10 comments · Fixed by #953
Closed
Assignees
Milestone

Comments

@igorakkerman
Copy link

Hi, when an assert on elements of large collections fails, it would be very helpful to only see "bad" items, not those that were expected. Currently, all elements in the collection are shown.

For instance, with containsExactly / contains.inOrder.only.entries, a section is dedicated to every single element, the encountered element is equal to the expected one or not. A correct element is annotated using a ✔️. A wrong element at the index, it is marked with an ✘.

When dealing with a large number of elements, it can be tedious to find the bad elements. The test author should be able to indicate which elements should be displayed, all elements or only the differences between good and bad elements.

This could be achieved using an optional boolean flag showAll, defaulting to true, or an optional displayFlags object or enumeration, for example. The option could be selected individually for every assertion or through a global property.

See also this discussion on Slack:
https://kotlinlang.slack.com/archives/C887ZKGCQ/p1578313112006400

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2020

taking a look at an example for the discussion:

expect(listOf(1, 2, 2, 4)).contains.inOrder.only.values(1, 2, 2, 3)

and the output:

expect: [1, 2, 2, 4]        (java.util.Arrays.ArrayList <1234789>)
◆ contains only, in order: 
  ✔ ▶ entry 0: 1        (kotlin.Int <1234789>)
      ◾ to be: 1        (kotlin.Int <1234789>)
  ✔ ▶ entry 1: 2        (kotlin.Int <1234789>)
      ◾ to be: 2        (kotlin.Int <1234789>)
  ✔ ▶ entry 2: 2        (kotlin.Int <1234789>)
      ◾ to be: 2        (kotlin.Int <1234789>)
  ✘ ▶ entry 3: 4        (kotlin.Int <1234789>)
      ◾ to be: 3        (kotlin.Int <1234789>)
  ✔ ▶ size: 4        (kotlin.Int <1234789>)
      ◾ to be: 4        (kotlin.Int <1234789>)

Would you expect that the hint about the size shows up nonetheless or also only if it failed?


Would you personally also use it for Map contains functions? (I intend to add features only if people actually use it)

@igorakkerman
Copy link
Author

Thank you for being so open for feedback. Since you're asking about my personal preferences:

  • Show the size only if it is wrong.

  • Show the size above the content, not at the end. After all, it's the first thing that I'd like to notice, not only after scrolling down.

  • Visually separate the size from the values, e.g. through a blank line or a different format. It happened to me a few times to only notice this was the size after wondering what value that was.

  • I have not yet looked into the options for verifying Map content, but I assume that it would make a lot of sense and I'd most probably also use it.

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2020

Well, thanks for sharing your valuable feedback 👍
Regarding the size, please have also a look at the following example:

expect(listOf(1, 2, 3)).contains.inOrder.only.values(1, 3)

Its output (only showing failures, thus I have removed ✔/✘ marks)

expect: [1, 2, 3]        (java.util.Arrays.ArrayList <1234789>)
◆ contains only, in order: 
  ▶ entry 1: 2        (kotlin.Int <1234789>)
    ◾ to be: 3        (kotlin.Int <1234789>)
  ▶ size: 3        (kotlin.Int <1234789>)
    ◾ to be: 2        (kotlin.Int <1234789>)
      ❗❗ additional entries detected: 
         ⚬ entry 2: 3        (kotlin.Int <1234789>)

would you still place the size including the additional entries detected-hint above the entries?

@igorakkerman
Copy link
Author

igorakkerman commented Jan 6, 2020

I'd put the size above, as it feels like a kind of summary to me. In most tests from other people, I've even seen the size to be asserted before the content, as a fail fast test. I'd leave the additional entries below, though.

Using Atrium, I found "to be" as a bit weird in the list (despite understanding where it comes from).

For the size values, the hashes have no value, are rather confusing and can be left out.

I'd personally prefer:

expect: [1, 2, 3]        (java.util.Arrays.ArrayList <1234789>)
◆ has size:
    ◾ expected: 2
    ◾ actual:   3
◆ contains only, in order: 
  ▶ entry 1: 
    ◾ expected: 3        (kotlin.Int <1234789>)
    ◾ actual:   2        (kotlin.Int <1234789>)
      ❗❗ additional unexpected entries found: 
         ⚬ entry 2: 3        (kotlin.Int <1234789>)

The overall rationale: as visual as possible. When reading the test result, I want to immediately know what was expected, what was the actual value and what is the difference, all without even turning my brain on ;)

@robstoll
Copy link
Owner

robstoll commented Jan 7, 2020

I see a point where Atrium could improve due to your feedback: align values in reporting. This would require a different way we produce the reporting as we don't have knowledge about previous or future assertions which will be added to the list in addition. I have created another issue (#295) where we can discuss it.

On the other hand, you could already:

  • provide an own translation for TO_BE, e.g. is equal to.
  • change the reporting the way you want by providing an own AssertionPairFormatter which puts the value not on the same line and prefixed it with expected:

Let me know in case you want to try it out one of the two (or both) and need help.
However, separating the hint additional entries detected from the size would mean you need to provide a different implementation for in order only and this one is more complicated and probably not worth it, needs to be done in Atrium directly. And then there is another restricting thing: I intend to reflect the assertions made by the developer with bullet points in reporting. In the example there is merely the contains.inOrder.only and thus the size check should be inside of it.

Over all I see it realistic that it could look as follows in the end for you (in case we align values as well):

expect: [1, 2, 3]        (java.util.Arrays.ArrayList <1234789>)
◆ contains only, in order: 
  ▶ size:
    ◾ expected:   2        (kotlin.Int <1234789>)
    ◾ is equal to: 3        (kotlin.Int <1234789>)
  ▶ entry 1: 
    ◾ expected:    3        (kotlin.Int <1234789>)
    ◾ is equal to: 2        (kotlin.Int <1234789>)
 ❗❗ additional unexpected entries found: 
    ⚬ entry 2: 3        (kotlin.Int <1234789>)

IMO it might be confusing if size comes first. What do you think?

@igorakkerman
Copy link
Author

I agree that the size check is confusing if it comes first at the level of the entries. If they have to stay at the "triangle" level, it's better to have them elsewhere...

@robstoll
Copy link
Owner

robstoll commented Feb 1, 2020

moving the size check out of the contains assertion is covered in #299

@robstoll
Copy link
Owner

robstoll commented May 29, 2021

@igorakkerman quite a while since you asked for the feature. Sorry about that, was something I had no need for myself (still don't have it) and no one volunteered / sponsored the issue. Anyway...
I will implement it in this or in the next version where I plan to provide the following syntax:

expect(veryLongList).toContainExactly(1, 2, 3, reportingOptions = { showOnlyFailing() })
expect(veryLongList).toContain.inOrder.only.values(1, 2, 3, reportingOptions = { showOnlyFailing() })

What do you think?
Also, I will most likely provide a way to make this the default via a configuration (but at a later point).

@igorakkerman
Copy link
Author

igorakkerman commented May 29, 2021

@robstoll Unfortunately, I have moved away from Atrium. But if I were using it, I would prefer a shorter syntax like report = onlyFailing.

@robstoll
Copy link
Owner

@igorakkerman Thanks for the feedback, looks good to me as well.

@robstoll robstoll self-assigned this Jul 12, 2021
robstoll added a commit that referenced this issue Jul 12, 2021
I decided that I will apply a default heuristic so that (I hope) most
users don't have to change reporting options.
robstoll added a commit that referenced this issue Jul 12, 2021
I decided that I will apply a default heuristic so that (I hope) most
users don't have to change reporting options.
robstoll added a commit that referenced this issue Jul 12, 2021
I decided that I will apply a default heuristic so that (I hope) most
users don't have to change reporting options.
robstoll added a commit that referenced this issue Jul 12, 2021
…-too-many-elements

#292 show only failing for inOrder.only if more than 10 elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants