[expect] Improve mock "return" matchers to properly handle calls that throw

[UselessPickles]

Summary

New mock return matchers that were recently added did not properly handle calls to the mock that threw errors. A call that throws an error should not be considered to have successfully returned.

More details here: #5879 (comment)

This PR improves the mock return matchers to properly interpret calls that threw errors.

Also includes:

• Some improvements to matcher failure messages.
• Additional edge cases covered by unit tests.
• Made mock return/throw tracking more robust by explicitly tracking a boolean indicator of whether the call threw an error. This allows for proper detection of a call having thrown an error even if the value undefined was thrown.

Test plan

See new/updated unit tests.

Ran the following to test affected code:

yarn jest jest_mock
yarn jest spy_matchers

NOTE: running the full unit test suite results in failures unrelated to my changes (failures happened before I made any changes). Some of the failures look like file system permissions issues on my machine (a windows machine). A couple others have something to do with lodash mocking (completely unrelated to my changes).

[UselessPickles]

@rickhanlonii @SimenB

[UselessPickles]

Added validation that "nth" is within the range of the number of calls

[UselessPickles]

Added more precision to the message depending on exactly why it failed

[UselessPickles]

Any suggestions for a better name?

[SimenB]

I wonder if we should use Symbols in the array, instead of sticking in undefined. Symbol('error thrown'), Symbol('no error') etc instead of having the check in multiple arrays, or using Boolean flags.

These Symbols could be exports from jest-mock for ease of checking

Also, please run yarn lint:md to fix CI :)

[UselessPickles]

I agree that the parallel arrays are getting a bit excessive, but I feel like using symbols to distinguish between "no error because an error wasn't thrown" vs "no error because undefined was explicitly thrown" doesn't do anything to reduce complexity, but just reorganizes the complexity into a less familiar and less convenient format.

I think a more intuitive approach would be to combine everything into a single "results" array, with one entry per call of this type:

export interface MockFunctionResult {
    didThrowError: boolean; // true if error is thrown, false if successfully returned
    returnValue: any; // undefined when didThrow === true, or if undefined was returned
    thrownError: any; // undefined when didThrow === false, or if undefined was thrown
}

Again, this doesn't reduce complexity, but I think it reorganizes it into a more convenient and intuitive form by getting rid of the 3 parallel arrays, and creates a more obvious coupling between the 3 values.

[UselessPickles]

I ran yarn lint:md, but it didn't cause any changes to any files:

D:\DevProjects\github\jest>yarn lint:md
yarn run v1.5.1
$ yarn --silent lint:md:ci --fix
Done in 1.44s.

D:\DevProjects\github\jest>git status
On branch spy_return_matchers_fix
Your branch is up-to-date with 'origin/spy_return_matchers_fix'.
nothing to commit, working tree clean

[SimenB]

I like bthe idea of a single array of calls, and them being objects. Suuuuper breaking, but maybe ok? @cpojer thoughts?

[SimenB]

didThrowError should probably just be didThrow - you can throw whatever you want in JS not just Errors.

The advantage of a Symbol is that then we only need 2 fields (or 2 arrays), and not 3. Might not help of course :)

[UselessPickles]

So something like this?

export interface MockFunctionCallData {
    args: any[]; // list of args values passed to this call
    didThrow: boolean; // true if value is thrown, false if successfully returned
    returnValue: any; // undefined when didThrow === true, or if undefined was returned
    thrownValue: any; // undefined when didThrow === false, or if undefined was thrown
}

I think that's an ideal design, but as you said, "super breaking". At least for the scope of this PR, I think it's best to keep the calls array separate.

More justification for the didThrow boolean as opposed to using Symbol...

The boolean both directly and clearly indicates how the call "completed" (it either threw or returned), and is also used to decide which value (returnValue vs thrownValue) to check for further details about how the call "completed". This is pretty intuitive and resulting code will be very readable:

const result = myFn.mock.results[0];
// IDE code completion with TS type defs will allow "didThrow" to be easily discoverable
if (!result.didThrow) {
   const returnValue = result.returnValue;
}

If we get rid of didThrow and use Symbols instead, which property do we store a Symbol on? Store a symbol on returnValue when an error is thrown? Store a symbol on thrownValue when an error is NOT thrown? Both? What's the justification for choosing one over the other? If the decision is "both", then a single piece of information is duplicated in 2 places. It requires introducing what amounts to "magic values" (the symbols), and the resulting code will be unintuitive:

const result = myFn.mock.results[0];
// Need to check "thrownError" before using "returnValue" (awkward).
// Developer needs to know about magic symbol to import it and use it.
// Not discoverable through IDE auto completion.
if (result.thrownValue === NO_ERROR_THROWN) {
   const returnValue = result.returnValue;
}

[rickhanlonii]

I was initially thinking using a symbol in the existing returnValues and thrownValues as @SimenB said, but after this discussion I think I would recommend:

• keep the calls the way they are (super breaking, we can combine in a later release)
• switch to storing both the returned and thrown values in a single mock.results field
• each item in the results have this value:

export interface MockFunctionResult {
    type: string; // either 'return' or 'throw', could be a symbol
    value: any; // whatever the result was
}

This makes the standard and custom matchers easy to implement, for example:

const nthReturnedWith= string => (received, nth, expected ) => {
  const nthResult = received.mock.results[nth - 1];
  
  // simplified equality for example
  const pass = nthResult && nthResult.type === 'return' && nthResult.value === expected; 

  return {message: 'omitted', pass};
};

const nthThrownWith= string => (received, nth, expected ) => {
  const nthResult = received.mock.results[nth - 1];
  
  // simplified equality for example
  const pass = nthResult && nthResult.type === 'throw' && nthResult.value === expected; 

  return {message: 'omitted', pass};
};

[UselessPickles]

@rickhanlonii Collapsing the thrownValue and returnValue into a single value seems so obvious now that you suggested it :)

How about this?

export interface MockFunctionResult {
    // or isThrow? isThrown?
    didThrow: boolean; // true if thrown, false if returned
    value: any; // whatever value was thrown/returned
}

As an end user of jest, a boolean is much more preferable than a string or symbol. A boolean clearly represents that there are only 2 possible values, and the possible values are already known (intuitive to write/read/understand).

A string or symbol type property requires separate knowledge of the possible values to understand how many possibilities there are, and know exactly what values to compare it to. Once the code is written, it would be fairly intuitive to read/understand, but writing the code in the first place is not intuitive because it would require reading documentation to discover the magic values. There's also a much higher chance of typos that allow code to execute without runtime errors, but produce incorrect behavior.

[SimenB]

I think it should also include arguments to the function so it encapsulates the whole function call, and we can soft-deprecate the .calls array

[UselessPickles]

@SimenB I just realized one subtle difference between maintaining call args separately, vs combined with result data.

The current implementation pushes the call args to calls immediately before executing the underlying mock/spy implementation, which would allow code executed by the mock/spy itself to already have access to the call args of the call that is currently executing.

If we combine call args with results info, then the call args will not be recorded until AFTER the mock/spy implementation executes.

This may be a good reason to maintain call args separately.

If anyone already relies on the current behavior, the change would be extremely breaking without any straightforward migration steps to follow.

[UselessPickles]

@rickhanlonii I restructured how results are maintained in MockFunctionState and resolved a conflict with master.

[SimenB]

@UselessPickles CI is failing because of linting errors in the markdown. Seems like our pattern doesn't work on windows, but you can do yarn prettylint docs/ExpectAPI.md --fix to fix it.

This also needs a rebase 🙂

[codecov-io]

Codecov Report

Merging #6172 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6172      +/-   ##
==========================================
+ Coverage   64.37%   64.38%   +<.01%     
==========================================
  Files         219      219              
  Lines        8453     8454       +1     
  Branches        4        3       -1     
==========================================
+ Hits         5442     5443       +1     
  Misses       3010     3010              
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-mock/src/index.js	87.42% <100%> (+0.03%)	⬆️

---

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 9867e16...006f616. Read the comment docs.

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