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

simplify reporting of Iterable.contains values assertions #56

Closed
robstoll opened this issue Jan 12, 2020 · 5 comments
Closed

simplify reporting of Iterable.contains values assertions #56

robstoll opened this issue Jan 12, 2020 · 5 comments
Labels
do-it it was decided that this issue (or part of it) shall be done needs discussion

Comments

@robstoll
Copy link
Owner

robstoll commented Jan 12, 2020

Taken from #55 (comment) written by @igorakkerman:

The author doesn't care about the cardinality of the item. All that matters is: We wanted an item, we didn't get it. Hence:

◆ contains, in any order: 
  ⚬ an item which:
      »  is less than: 0        (kotlin.Int <1234789>)
        » but no such item was found.

Following the current output:

◆ contains, in any order: 
  ⚬ an entry which: 
      » is less than: 0        (kotlin.Int <1234789>)
    ⚬ ▶ number of occurrences: 0
        ◾ is at least: 1

I agree that we could improve the reporting for contains values assertions. Personally I think we can remove the but no such item was found. as we report only failures but we can also leave it as additional hint, totally fine with me.

One note on the implementation. contains(...) is currently a shortcut for contains.inAnyOrder.atLeast(1).values(...) the verbose output is due to the generic implementation which uses the same for contains.inAnyOrder.atLeast(2).butAtMost(5).values(...) where the number of occurrences is important.

Thus the question, shall we report the number of occurrences if one uses contains.inAnyOrder.atLeast(1).values(...) and not the shortcut -- which means it wouldn't be a shortcut any more or rather we would have to add a configuration option to the reporting of atLeast.

Thoughts?

@igorakkerman
Copy link
Collaborator

Interesting question. Can atLeast(1) be made the default so that it can be left out? Or can atLeast(1) have a synonym (no good name ideas right now) which would not produce any reporting?
The idea for but no such item was found comes from AssertJ and I kind of liked it when I used it.

@robstoll
Copy link
Owner Author

robstoll commented Jan 12, 2020

We could have:

contains.inAnyOrder.values(...)             // does not report occurrences
contains.inAnyOrder.atLeast(1).values(...)  // reports occurrences

my only concern, the difference is very subtle.

A synonym for atLeast(1) could be once but having once not reporting occurrences and atLeast(1) reporting occurrences is really not ideal IMO. The perfect case would be something making it clear that we are not interested in occurrences. We could have a flag like (a better name is more than welcome)

contains.inAnyOrder.atLeast(1, notReportingOccurrences=true).values(...)

Edit as the default case is report only failure, it makes more sense to use reportOccurrences=true with false as default

@robstoll
Copy link
Owner Author

robstoll commented Jan 12, 2020

Another thing we should keep in mind: one can also report holding assertions where IMO it can make sense to use/show the occurrences. E.g. for reviews:

✔ contains, in any order: 
  ⚬ an entry which: 
      » is less than: 0        (kotlin.Int <1234789>)
    ⚬ ▶ number of occurrences: 10
        ◾ is at least: 1

might give a clue to the reviewer that the test-code is written wrong and it should actually be exactly(1) or atLeast(1).butAtMost(2) etc.

@jGleitz
Copy link
Collaborator

jGleitz commented Jan 13, 2020

my only concern, the difference is very subtle.

I second that concern. I fear that it will be irritating to users when to use what.

My proposal: Handle “atLeast(1)” as a special case in reporting because it is so common.

So

  ⚬ an entry which: 
      » is less than: 0        (kotlin.Int <1234789>)

without any further mentioning of a number of occurrences means “at least one”. This is consistent with the normal use in the English language.

So we can then do

◆ contains, in any order: 
  ⚬ an item which:
      »  is less than: 0        (kotlin.Int <1234789>)
        » but no such item was found.

without having to introduce new assertion functions.

@robstoll
Copy link
Owner Author

I would be in favour of changing contains.inAnyOrder.atLeast(1).values(...) to the suggestion as well and keep contains(value) to be a shortcut. Or in other words, contains behaves like contains.inAnyOrder.atLeast(1).values(...) also after the improvement.

We can add a flag as shown once we have HTML reporting where it might make sense in certain scenarios to show the occurrence.

And I would not introduce a shortcut contains.InAnyOrder.values which is the same as atLeast(1) as one can already use contains(...) as shortcut.

@robstoll robstoll added the do-it it was decided that this issue (or part of it) shall be done label Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-it it was decided that this issue (or part of it) shall be done needs discussion
Projects
None yet
Development

No branches or pull requests

3 participants