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

expect: Improve report when mock-spy matcher fails, part 4 #8710

Merged
merged 7 commits into from
Jul 24, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

For toHave*ReturnedWith matchers:

  • Display matcher name in regular black instead of dim color
  • Replace sentences with Expected and Received labels
  • For negative matcher, repeat not following Expected label
  • At end of report, display Received number of returns
  • If any calls didn’t return, display Received number of calls

Display up to 3 call results as context, so testers can diagnose problems: in code, or test, or both

Your feedback is especially welcome about questions:

  1. For special case of exactly one call, is it clearer to display Received value: whatever on one line instead of two? (see pictures in following comments)
  2. Is the following context relevant?

Read the following descriptions with the qualifier if they exist:

matcher context
.not.toHaveReturnedWith first 3 results that are equal to expected value
.toHaveReturnedWith first 3 results, including threw an error
.not.toHaveLastReturnedWith last and next-to-last results
.toHaveLastReturnedWith last result and either nearest preceding value that is equal to expected; otherwise, next-to-last result
.not.toHaveNthReturnedWith nth and adjacent results
.toHaveNthReturnedWith nth and either nearest preceding/following value that is equal to expected; otherwise, adjacent results

Test plan

Updated 102 snapshots

long name short name
30 toHaveReturnedWith toReturnWith
30 toHaveLastReturnedWith lastReturnedWith
42 toHaveNthReturnedWith nthReturnedWith

See also pictures in following comments

Example pictures baseline at left and improved at right

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

negative toHaveReturnedWith assertions

Updated pictures according to #8710 (comment)

Here is the first example of question 1, now updated:

ReturnWith true 1 1

Display up to 3 examples of calls which returned the not-expected value:

ReturnWIth true 1 3 4

Notice that baseline dittos the expected asymmetric matcher instead of a received value:

ReturnWith true 3 4 4

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

positive toHaveReturnedWith assertions

Updated pictures according to #8710 (comment)

ReturnWith false 0 0

Here is another example of question 1, now updated:

ReturnWith false 1 1

Display (up to) the first 3 results, including has not returned yet

ReturnWith false 2 3

ReturnWith false 4 4

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

negative toHaveLastReturnedWith assertions

Updated pictures according to #8710 (comment)

LastReturn true 1 1

last result and either nearest preceding value that is equal to expected; otherwise, in this picture next-to-last result:

LastReturn true 1 3

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

positive toHaveLastReturnedWith assertions

Updated pictures according to #8710 (comment)

LastReturn false 0 0

LastReturn false 0 1a

LastReturn false 0 1b

last and next-to-last results:

LastReturn false 0 3

last result and nearest preceding value that is equal to expected:

LastReturn false 1 3

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

negative toHaveNthReturnedWith assertions

Updated pictures according to #8710 (comment)

Here is the picture that caused me to ask question 1:

NthReturn true 1 1

nth and adjacent results, preceding but there is no following:

NthReturn true 2 2

nth and adjacent results, both preceding and following:

NthReturn true 10 12

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 17, 2019

positive toHaveNthReturnedWith assertions

Updated pictures according to #8710 (comment)

NthReturn false 1 0

NthReturn false 1 1

How well can you pick out n with -> in the lines under Received values?

NthReturn false 1 2

nth and adjacent results:

NthReturn false 9 10 - 1

nth and nearest preceding value that is equal to expected:

NthReturn false 9 10 - 2

nth and nearest following value that is equal to expected:

NthReturn false 9 12

@thymikee
Copy link
Collaborator

Don't you think "Received value: " with empty space is a bit confusing at first?
Maybe:

Expected value: undefined
Received values
             1: "some"
			 2: null

or use different delimiter for numbers:

Expected value: undefined
Received value:
             1) "some"
			 2) null

For "not" matchers, could we dim noisy return values and only show the not-expected value in red? (similar for *NthReturned)

Expected value: not undefined
Received value:
             1: <dim>"some"</dim>
			 2: <red>undefined</red>

I'm also not sold on the "Call n", it's actually harder for me to get the error at first. How about:

Expected value: <green>89</green> for 9th call
Received value:
             8: <dim>"some"</dim>
			 9: <red>"rome"</red>
            10: <dim>89</dim>

@pedrottimark
Copy link
Contributor Author

Thank you for sharing your first impressions. By this time, they are long gone for me.

If there is only one call, and it is the last or n = 1, report can display received value on one line:

NthReturn false 1 1 b

For negative assertion, go one step farther to omit received value if it has same serialization:

NthReturn true 1 1 b

What do you think about this alternative to original Call n?

NthReturn false 9 12 b

And if additional values seem like noise instead of context, then I am happy to get rid of them!

@thymikee
Copy link
Collaborator

Like it 👍

@pedrottimark
Copy link
Contributor Author

Updated pictures in preceding comments according to changes from #8710 (comment) by Michał:

  • if there is only one call for negative assertion, omit received value is it has same serialization as expected value
  • otherwise, if there is only one call, display Received value: on one line
  • otherwise display Received value without colon above one or more lines with values
  • replace Call n: with n: label
  • for last or nth matchers, display -> at beginning of line with value of the expected call

Updated 94 snapshots

long name short name
13 toHaveReturnedWith toReturnWith
13 toHaveLastReturnedWith lastReturnedWith
21 toHaveNthReturnedWith nthReturnedWith

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I had no idea there's so many complicated cases with recursive calls / not returned yet.
Found a few messages I'd consider unclear that we could talk about.

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jul 22, 2019

Updated pictures in preceding comments according to changes after review by Tim:

  • for these matchers, replace Received number of returns with Number of returns
  • replace Expected value and Received value with Expected and Received
  • replace has not returned yet with function call has not returned yet
  • replace threw an error with function call threw an error
  • in future PR, replace called with no arguments with function call with no arguments

The result is shorter labels in all reports and longer strings for special cases

Updated 102 snapshots

long name short name
15 toHaveReturnedWith toReturnWith
15 toHaveLastReturnedWith lastReturnedWith
21 toHaveNthReturnedWith nthReturnedWith

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks even better now! I'm happy with how it is already

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants