Docs: `mockClear`, `mockReset`, `mockRestore` #6227

stephencookdev · 2018-05-22T11:30:34Z

Summary

This PR is aiming to relieve some of the difficulties flagged up in #4828 and #5143

Specifically, the exact behaviour of mockClear, mockReset, and mockRestore isn't massively clear. I personally felt this pain when trying to debug an issue that turned out to be mockReset being called on a spy, "resetting" it to an empty mock (rather than my original assumption, of the original implementation at the time the spy was attached).

I've also made a minor change to the actual source code of mockClear etc., but only to make it more explicit in the code that mockClear < mockReset < mockRestore (which was already the case, it's just now more obvious at a glance).

Test plan

The website changes appear as expected by cd website ; yarn start.

yarn test yields no (or at least, no additional, since there are errors on master) failures

stephencookdev · 2018-05-22T11:31:43Z

packages/jest-mock/src/index.js

        this._mockConfigRegistry.delete(f);
        return f;
      };

+      f.mockRestore = () => {
+        f.mockReset();
+        return restore ? restore() : undefined;


Ideally I would like this to be return f, to be consistent with the other methods - but keeping the API exactly the same, this is the current behaviour.

SimenB · 2018-05-22T12:17:44Z

CI is failing, mind taking a look?

since there are errors on master

Travis is failing (we really should remove it), but circle and appveyor is fine:

stephencookdev · 2018-05-22T13:47:17Z

Ah, I didn't check the state of the UTs closely enough, then 😞 Thanks for the info

So it looks like the failure is the following:

'mockName is not reset by mockRestore'

Which I didn't actually realise. So it means I misunderstood (again 😅) the behaviour of mockRestore.

This means that mockClear < mockReset < mockRestore is not true (since mockName is reset by mockReset).

But is there any use-case for wanting mockRestore to preserve the mockName? I can't really think of one.

But I can either fix this by

changing the test, so we accept the API change of mockRestore removing the mockName, or
changing the documentation to emphasise that mockClear < mockReset, and emphasise that mockRestore is just an entirely separate thing

SimenB · 2018-05-22T13:50:22Z

@rickhanlonii ^ ideas?

rickhanlonii · 2018-05-26T10:13:27Z

docs/MockFunctionAPI.md

-Resets all information stored in the mock, including any initial implementation
-and mock name given.
+Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also
+removes any mocked return values.


Any mocked return values or implementations

rickhanlonii · 2018-05-26T10:14:57Z

docs/MockFunctionAPI.md

-state.
+This is useful when you want to completely restore a _mock_ back to its initial
+state. (Note that resetting a _spy_ will result in a function with no return
+value).


I wouldn't use the word "restore" here, what do you think about using "completely reset a mock"

Yes, I agree :) Will change

rickhanlonii · 2018-05-26T10:15:38Z

docs/MockFunctionAPI.md

@@ -116,7 +117,8 @@ don't access stale data.

 ### `mockFn.mockRestore()`

-Removes the mock and restores the initial implementation.
+Does everything that [`mockFn.mockReset()`](#mockfnmockreset) does, and also
+restores the original (non-mocked) implementation.


Should this say:

restores the original (non-mocked) implementation for spies

Yeah, that probably makes more sense 👍

rickhanlonii

I think we probably just forgot about removing the mockName for spies when we added the mock names, so it's good to remove (though it may be a breaking bugfix)

stephencookdev · 2018-05-29T10:00:49Z

Thanks for the review @rickhanlonii - addressed your comments, and made it so that mockClear < mockReset < mockRestore is true :)

codecov-io · 2018-05-29T11:47:55Z

Codecov Report

Merging #6227 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master   #6227      +/-   ##
=========================================
+ Coverage   63.89%   63.9%   +<.01%     
=========================================
  Files         228     228              
  Lines        8705    8707       +2     
  Branches        4       4              
=========================================
+ Hits         5562    5564       +2     
  Misses       3142    3142              
  Partials        1       1

Impacted Files	Coverage Δ
packages/jest-mock/src/index.js	`87.5% <50%> (+0.07%)`	⬆️

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 86c91d5...10f1c6d. Read the comment docs.

rickhanlonii · 2018-05-29T14:33:43Z

CHANGELOG.md

@@ -121,6 +121,7 @@

 ### Chore & Maintenance

+* `[docs]` Improve documentation of `mockClear`, `mockReset`, and `mockRestore` ([#6227](https://github.com/facebook/jest/pull/6227/files))


Needs moved to the master section

I think this got messed up after a merge conflict - cheers 😓 Moved back to master :)

rickhanlonii · 2018-05-29T14:34:53Z

website/versioned_docs/version-22.4/MockFunctionAPI.md

@@ -0,0 +1,310 @@
+---


Can only add this to the master docs? Will ship in the 23.1 version

Ah, I misunderstood the process. Yeah, np - will remove my versioned_docs changes

…re` (especially surrounding `spyOn` behaviour)

rickhanlonii

LGTM

github-actions · 2021-05-11T13:11:51Z

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.

facebook-github-bot added the cla signed label May 22, 2018

stephencookdev commented May 22, 2018

View reviewed changes

stephencookdev force-pushed the feature/mock-documentation branch from 7806b26 to 2dc4291 Compare May 22, 2018 11:34

stephencookdev changed the title ~~Documentation improvement~~ Docs: mockClear, mockReset, mockRestore May 22, 2018

rickhanlonii reviewed May 26, 2018

View reviewed changes

rickhanlonii approved these changes May 26, 2018

View reviewed changes

stephencookdev force-pushed the feature/mock-documentation branch from 2dc4291 to 5cc2a0d Compare May 29, 2018 09:59

stephencookdev force-pushed the feature/mock-documentation branch from 5cc2a0d to e934dce Compare May 29, 2018 11:31

rickhanlonii reviewed May 29, 2018

View reviewed changes

Improve the documentation of mockClear, mockReset, and `mockResto…

10f1c6d

…re` (especially surrounding `spyOn` behaviour)

stephencookdev force-pushed the feature/mock-documentation branch from e934dce to 10f1c6d Compare May 29, 2018 15:06

rickhanlonii approved these changes May 29, 2018

View reviewed changes

Merge branch 'master' into feature/mock-documentation

f3e872f

cpojer merged commit bed7e17 into jestjs:master May 30, 2018

SimenB mentioned this pull request Dec 11, 2018

mockClear vs mockReset vs mockRestore #5143

Closed

stephencookdev deleted the feature/mock-documentation branch November 27, 2019 14:04

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: `mockClear`, `mockReset`, `mockRestore` #6227

Docs: `mockClear`, `mockReset`, `mockRestore` #6227

stephencookdev commented May 22, 2018

stephencookdev May 22, 2018

SimenB commented May 22, 2018 •

edited

Loading

stephencookdev commented May 22, 2018

SimenB commented May 22, 2018

rickhanlonii May 26, 2018

rickhanlonii May 26, 2018

stephencookdev May 29, 2018

rickhanlonii May 26, 2018 •

edited

Loading

stephencookdev May 29, 2018

rickhanlonii left a comment

stephencookdev commented May 29, 2018

codecov-io commented May 29, 2018 •

edited

Loading

rickhanlonii May 29, 2018

stephencookdev May 29, 2018

rickhanlonii May 29, 2018

stephencookdev May 29, 2018

rickhanlonii left a comment

github-actions bot commented May 11, 2021

		@@ -121,6 +121,7 @@

		### Chore & Maintenance

		* `[docs]` Improve documentation of `mockClear`, `mockReset`, and `mockRestore` ([#6227](https://github.com/facebook/jest/pull/6227/files))

Docs: mockClear, mockReset, mockRestore #6227

Docs: mockClear, mockReset, mockRestore #6227

Conversation

stephencookdev commented May 22, 2018

Summary

Test plan

Choose a reason for hiding this comment

SimenB commented May 22, 2018 • edited Loading

stephencookdev commented May 22, 2018

SimenB commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickhanlonii May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

stephencookdev commented May 29, 2018

codecov-io commented May 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

github-actions bot commented May 11, 2021

Docs: `mockClear`, `mockReset`, `mockRestore` #6227

Docs: `mockClear`, `mockReset`, `mockRestore` #6227

SimenB commented May 22, 2018 •

edited

Loading

rickhanlonii May 26, 2018 •

edited

Loading

codecov-io commented May 29, 2018 •

edited

Loading