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

Add documentation related to auto-mocking #8099

Merged
merged 14 commits into from Mar 18, 2019
Merged

Add documentation related to auto-mocking #8099

merged 14 commits into from Mar 18, 2019

Conversation

mackness
Copy link
Contributor

@mackness mackness commented Mar 10, 2019

Summary

The purpose of this PR is to add more documentation related to how Jest auto-mocks modules. It aims to clarify how Jest mocks and transforms values with jest.genMockFromModule. Resolves #6812

Test plan

Add a few tests to verify the assertions made by the documentation in packages/jest-mock/src/__tests__/index.test.ts.

@codecov-io
Copy link

codecov-io commented Mar 10, 2019

Codecov Report

Merging #8099 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8099   +/-   ##
=======================================
  Coverage   62.44%   62.44%           
=======================================
  Files         264      264           
  Lines       10371    10371           
  Branches     2515     2515           
=======================================
  Hits         6476     6476           
  Misses       3318     3318           
  Partials      577      577

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 76782b8...b3c807f. Read the comment docs.

docs/JestObjectAPI.md Outdated Show resolved Hide resolved

#### `Function`

A new function will be created. The new function will have no formal parameters and when called will return `undefined`. This functionality also applies to `async functions`.
Copy link
Member

Choose a reason for hiding this comment

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

should mocked async functions return Promise<undefined> instead?

Copy link
Contributor Author

@mackness mackness Mar 11, 2019

Choose a reason for hiding this comment

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

I just tested this and it seems like mocked async functions don't return Promise<undefined> instead they return undefined just like synchronous functions.

Copy link
Member

Choose a reason for hiding this comment

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

yep, I meant that maybe we should change this. Good that the current behavior gets documented though!


Example:

<!-- prettier-ignore -->
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for some reason prettier is adding these unwanted parens after the closing class bracket

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove js in the opening code block in the markdown file prettier does not insert the weird parenthesis. 🤔

image

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it's just adding the parense. new class Bar{} and new class Bar{}() behaves the same.

Should it be newed?

I opened up prettier/prettier#5964, not sure if it's a bug or not

Copy link
Contributor Author

@mackness mackness Mar 12, 2019

Choose a reason for hiding this comment

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

Nice, I added another comment that may or may not be related to the same issue. For now I just removed the js attribute from the markdown code block and everything is formatted correctly and syntax hi-lighting seems to be working in the site preview

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is awesome! I especially like the added tests to assert behavior for the different types. Thanks for those.

I left some inline q's.

Can you also update the versioned docs and the changelog?

image

Copy link
Member

@rickhanlonii rickhanlonii 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 doing this!


#### `String`

A new copy of the original stirng is mocked.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A new copy of the original stirng is mocked.
A new copy of the original string is mocked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this!


#### `Object`

Objects are deeply cloned. Their interfaces are maintained and their values are mocked.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Objects are deeply cloned. Their interfaces are maintained and their values are mocked.
Objects are deeply cloned. Their keys are maintained and their values are mocked.


#### `Class`

A new class will be created. The interface of the original class is maintained however all of the class member functions will be mocked.
Copy link
Member

Choose a reason for hiding this comment

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

What about class properties, if there's a property that is an array will it be mocked with the array strategy mentioned below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I can add this case to the example code / description.

docs/JestObjectAPI.md Show resolved Hide resolved
docs/JestObjectAPI.md Outdated Show resolved Hide resolved

#### `Function`

A new [mock function](https://jestjs.io/docs/en/mock-functions.html) will be created. The new function will have no formal parameters and when called will return `undefined`. This functionality also applies to `async functions`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it harm the meaning to write in present tense and active voice as much as possible? For example:

Create a new mock function which has no formal parameters and returns undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right about using present tense. I also don't think present tense effects the meaning or makes it unclear. Since the rest of the docs are written in present tense I think I should update this section to match. I'll work on switching everything to present tense.


#### `String`

A new copy of the original string is mocked.
Copy link
Contributor

Choose a reason for hiding this comment

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

This made me stop to think (which is not necessarily bad :)

By a new copy of primitive type like string or number, is the implicit assumption as value of a property?

If yes, then is it possible to summarize the behavior, which I think confirms what testers expect as one heading for all primitive types (with a few additional tests, if needed) including boolean, symbol, null, undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to summarize all primitives under one heading since the mocking functionality is the same. Instead of a string and number header I can create a primitives header and maybe add the following tests to the docs: https://github.com/facebook/jest/blob/445e6cb9f5fddf87174e510a602cf8bc11a840b1/packages/jest-mock/src/__tests__/index.test.ts#L33-L36

docs/JestObjectAPI.md Outdated Show resolved Hide resolved
docs/JestObjectAPI.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to fill this gap and willingness to follow through on review comments.

I will leave for Simen to confirm the changes that he requested.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks!

CHANGELOG.md Outdated
@@ -13,6 +13,7 @@

### Chore & Maintenance

- `[*]` Add documentation and tests related to auto-mocking ([#8086](https://github.com/facebook/jest/pull/8099))
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this up under master?

(Might have to merge in master or rebase first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synced with upstream master and moved the changelog line under the master heading. Thanks for the feedback and help with this @SimenB !

@SimenB SimenB merged commit 90c6002 into jestjs:master Mar 18, 2019
@SimenB
Copy link
Member

SimenB commented Mar 18, 2019

Thank you!

thymikee added a commit to Brantron/jest that referenced this pull request Mar 19, 2019
* upstream/master: (391 commits)
  more precise circus asyncError types (jestjs#8150)
  Add typeahead watch plugin (jestjs#6449)
  fix: getTimerCount not taking immediates and ticks into account (jestjs#8139)
  website: add an additional filter predicate to backers (jestjs#7286)
  [🔥] Revised README (jestjs#8076)
  [jest-each] Fix test function type (jestjs#8145)
  chore: improve bug template labels for easier maintenance (jestjs#8141)
  Add documentation related to auto-mocking (jestjs#8099)
  Add support for bigint to pretty-format (jestjs#8138)
  Revert "Add fuzzing based tests in Jest (jestjs#8012)"
  chore: remove console.log
  chore: Improve description of optional arguments in ExpectAPI.md (jestjs#8126)
  Add fuzzing based tests in Jest (jestjs#8012)
  Move @types/node to the root package.json (jestjs#8129)
  chore: use property initializer syntax (jestjs#8117)
  chore: delete flow types from the repo (jestjs#8061)
  Move changelog entry to the proper version (jestjs#8115)
  Release 24.5.0
  Expose throwOnModuleCollision (jestjs#8113)
  add matchers to expect type (jestjs#8093)
  ...
@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.

Add more documentation on auto-mocking
6 participants