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 2 #8649

Merged
merged 7 commits into from
Jul 10, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Jul 5, 2019

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
Copy link
Contributor Author

pedrottimark commented Jul 5, 2019

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)

toHaveBeenCalled false

toHaveReturned false

Residue includes adding a test to cover the limit:

toHaveBeenCalled true 3 5 3

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

toHaveReturned true

@pedrottimark
Copy link
Contributor Author

toHaveBeenCalledTimes false

toHaveBeenCalledTimes true

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

@pedrottimark
Copy link
Contributor Author

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

toHaveReturned true 1 3

@pedrottimark
Copy link
Contributor Author

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
Copy link
Member

SimenB commented Jul 5, 2019

Give it a rebase, it should fix ci 🙂

@codecov-io
Copy link

codecov-io commented Jul 5, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

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.

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

packages/expect/src/spyMatchers.ts Show resolved Hide resolved
@pedrottimark
Copy link
Contributor Author

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:

toReturn positive

Negative assertions:

toReturn negative

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)

toBeCalled positive serialize long

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

@jeysal
Copy link
Contributor

jeysal commented Jul 8, 2019

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
Copy link
Contributor Author

pedrottimark commented Jul 8, 2019

@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
Copy link
Contributor Author

pedrottimark commented Jul 8, 2019

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:

toHaveBeenCalled true paren 1

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

toHaveBeenCalled true paren 3

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
Copy link
Contributor

jeysal commented Jul 10, 2019

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
Copy link
Contributor Author

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

toHaveBeenCalled true 2 5 3

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

toHaveBeenCalled true 2 1 0

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
Copy link
Contributor

jeysal commented Jul 10, 2019

@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
Copy link
Collaborator

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
Copy link
Contributor Author

Thank you for constructive critique, my friends.

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

toHaveBeenCalled true 3 5 3

toHaveBeenCalled true 3 1 0

@thymikee
Copy link
Collaborator

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

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

@pedrottimark
Copy link
Contributor Author

@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.

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.

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
Copy link
Contributor Author

@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
Copy link
Contributor

jeysal commented Jul 10, 2019

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
Copy link
Contributor Author

pedrottimark commented Jul 10, 2019

@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
Copy link
Contributor

jeysal commented Jul 10, 2019

+1 for called with no arguments :)

@pedrottimark pedrottimark merged commit b2c9fe3 into jestjs:master Jul 10, 2019
@pedrottimark pedrottimark deleted the improve-mock-spy-2 branch July 10, 2019 22:21
@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.

6 participants