expect: Improve report when mock-spy matcher fails, part 2

[pedrottimark]

Summary

For mock-spy matchers whose criterion is number of calls or returns:

• Display matcher name in regular black instead of dim color
• Replace sentences with Expected and Received labels and number values

For negative matcher:

• with expected times, repeat not following Expected label
• without expected times, display >= 1 following Expected label
• without expected times, display args or value as lines with one-based call number as label

Questions to reviewers

1. What do you think about change to consistent PRINT_LIMIT = 3 instead of:

   const CALL_PRINT_LIMIT = 3;
   const RETURN_PRINT_LIMIT = 5;

2. For return matchers, is the number of calls relevant information [EDIT: if it is different from the number of returns] to include on an additional line in report to trouble shoot situations when a test fails because of incorrect assumption about calls versus returns? (see picture in comment below)

Residue

• Factor out repeated code, especially to avoid map for spy calls unless negative matcher fails
• Edit test names to replace in the error message with as received in matcher hint
• Replace some any types, if possible
• Add tests to increase coverage

Test plan

Updated 46 snapshots

	long name	short name
3	toHaveBeenCalled	toBeCalled
4	toHaveBeenCalledTimes	toBeCalledTimes
8	toHaveReturned	toReturn
8	toHaveReturnedTimes	toReturnTimes

See also pictures in following comments

Example pictures baseline at left and improved at right

[pedrottimark]

To my eyes >= 1 contrasts better than > 0 with 0 and is more similar to explicit expected argument 1 in toHaveBeenCalledTimes or toHaveReturned matchers (see below)

Residue includes adding a test to cover the limit:

Updated the preceding picture s/zuerst/erste/ found by Tim, and then according to #8649 (comment)

[pedrottimark]

I forgot to take baseline pictures of toHaveReturnedTimes which is similar to the above

[pedrottimark]

The call number gives a hint that some invocations did not return (see Question 2)

[pedrottimark]

The following errors in CI also occurred when I ran yarn jest locally on Node 10.16.0:

1. ScriptTransformer transforms a file properly - ScriptTransformer transforms a file properly
2. generateEmptyCoverage generates an empty coverage object for a file without running it - generateEmptyCoverage generates an empty coverage object for a file without running it

I will wait for advice before I push update to CHANGELOG.md to save easy way to restart CI :)

[SimenB]

Give it a rebase, it should fix ci 🙂

[codecov-io]

Codecov Report

Merging #8649 into master will increase coverage by 0.03%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master    #8649      +/-   ##
==========================================
+ Coverage   63.42%   63.45%   +0.03%     
==========================================
  Files         274      274              
  Lines       11385    11382       -3     
  Branches     2772     2769       -3     
==========================================
+ Hits         7221     7223       +2     
+ Misses       3547     3546       -1     
+ Partials      617      613       -4

Impacted Files	Coverage Δ
packages/expect/src/spyMatchers.ts	91.02% <97.05%> (+1.99%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d051b0d...d0f8d05. Read the comment docs.

[jeysal]

Awesome work as always @pedrottimark!
To answer your questions:

1. I would guess it is there because argument lists tend to take up more space than single return values, which is a valid reason but I don't have a strong opinion whether it matters that much.
2. I think that would indeed be useful and we can spare that extra line in those cases where it really differs, since I think it is not immediately obvious what the difference even is.

BTW, as a native speaker, I must inform you that the German sequence in your screenshot needs s/zuerst/erste/ ;D

[pedrottimark]

Here are pictures when received number of calls is not equal to returns

Updated toHaveReturned is at left and toHaveReturnedTimes is at right

Positive assertions:

Negative assertions:

Updated 14 snapshots

	long name	short name
4	toHaveReturned	toReturn
3	toHaveReturnedTimes	toReturnTimes	changed one test from negative to positive

Here is picture of args that serialize to long strings as requested in #8649 (comment)

P.S. Danke, Tim: erste is adjective but zuerst is adverb, nicht wahr?

[jeysal]

The arguments in the screenshot still look reasonably readable. I'm wondering though (for other PRs):

• How can we prevent broken output in extreme cases such as multiline strings (ellipsis abbreviations?)
• Does it make sense to use limited multiline printing if there are many arguments or an argument is an array/object with many properties

Also, should we perhaps print the arguments as 42, {} instead of [42, {}] because users might think that the array means that it's one argument that is an array?

---

And yes you're right, zuerst is an adverb and erster/erste/erstes a numeral/adjective ;)

[pedrottimark]

@jeysal Your review comments are sooo helpful

• For multiline strings, what do you think about ↵ U+21b5 downwards arrow with corner leftwards.
• The stringify function has optional maxDepth = 10 arg. What value do you think in this context?

The reason I printed args as array is possibility of call with no arguments. The toHaveBeen*CalledWith matchers will have ...expected in matcher hint, but these matchers lack that hint, pardon the pun.

• If you think nothing following the label communicates clearly, then I am happy to write helper function to print values separated by commas, not enclosed in brackets.
• Or how about enclosed in parentheses instead of brackets? So () means no arguments.

[pedrottimark]

Here are two variations on idea to enclose call args in parentheses instead of brackets.

Similar to example above one object arg enclosed in parentheses which have black color:

Adapted to 3 args enclosed in parentheses and separated by commas which have dim color:

When we get to toHaveBeenLastCalledWith and toHaveBeenNthCalledWith matchers, we will see whether or not it communicates clearly for arguments themselves to have dim color in report if matcher failed because received args match but not at the expected call.

[jeysal]

The reason I printed args as array is possibility of call with no arguments

For that case, maybe a placeholder No arguments would work and allow us to do it comma-separated :) Although using colors to signify that it's not an actual array is also an interesting idea

[pedrottimark]

In report for negative toHaveBeenCalled matcher, call printReceivedArgs function:

• arguments have red color (as usual)
• enclose in parentheses and separate by comma-space (as usual) which have dim color

Although the example of unexpected call with no args requires me to look twice:

I think we will see in future improvement for toHaveBeen*CalledWith matchers that contrast between red arguments and dim punctuation will help compare received to expected

Updated 4 snapshots and changed 2 of them to illustrate 3 string args

Let’s defer multiline strings and object depth to formatting for data-driven diff

[jeysal]

@SimenB @thymikee wdyt about the parentheses?
I think I'm still in favor of just commas and nothing at the start/end, with a fallback message for no args.

[thymikee]

I think I'm still in favor of just commas and nothing at the start/end, with a fallback message for no args.

I'm with @jeysal on that.

[pedrottimark]

Thank you for constructive critique, my friends.

How do commas and no args look with normal black color?

[thymikee]

Can we provide an information that 2 more calls were skipped?

1: "pierwszy"
2: "drugi"
3: "trzeci"
...skipped 2 more calls.

[pedrottimark]

@thymikee Let’s wait to decide what to do until PR to improve report for toHaveBeen*CalledWith matchers, because they will probably have print-limited call lines with non-consecutive numbers.

[jeysal]

Thanks for your patience @pedrottimark!
Just one more question: Is there perhaps a similar message in spy matchers that we could adapt to "No args" to make it feel consistent?
I can't think of one atm, something like "but it was not called" doesn't fit well here.

[pedrottimark]

@jeysal You’re welcome. Can you express your question in other words. I didn’t quite get it:

Is there perhaps a similar message in spy matchers that we could adapt to "No args" to make it feel consistent?

[jeysal]

Can you express your question in other words. I didn’t quite get it:

"No args" looked somewhat odd to me and I wondered if we had a similar text somewhere that we could refer to for wording. But maybe it's "args" that looks a bit "vulgar" as an abbreviation. Doesn't matter much anyway 😄

[pedrottimark]

@jeysal Aha, I see what you mean. It can be misunderstood as an error instead of neutral information.

Two existing strings will survive the “prose purge” in next PR for toHave*ReturnedWith matchers:

Expected value: 12                     [green color]
Received value:
             1: threw an error         [black color]
             2: has not returned yet   [black color]
             3: 13                     [red color]

Here is a first draft of a phrase that is parallel to their past tense grammar:

1: was called with no arguments        [black color]

Thinking ahead to PR for toHaveBeen*CalledWith matchers, maybe terser but not vulgar?

Expected arguments: called with no arguments
Received arguments:
                 1: false
                 2: true
                 3: null

[jeysal]

+1 for called with no arguments :)

[github-actions]

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.