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

Update promise docs #3201

Merged
merged 8 commits into from
May 2, 2017
Merged

Update promise docs #3201

merged 8 commits into from
May 2, 2017

Conversation

robinpokorny
Copy link
Contributor

@robinpokorny robinpokorny commented Mar 24, 2017

Summary

This pull request changes the documentation at several places regarding the recent async testing handling using .resolves and .rejects.

  • Change wording of .resolves and .rejects API docs.
  • Use ‘lemon’/‘octopus’ in Expect API docs.
  • Remove old (non-resolves) promise assertions everywhere in docs
  • Add info about .rejects matcher in ‘Testing Asynchronous Code’.
  • Update comments in Async tutorial and updated example files.

Related PR: #3172, #3068

Test plan

Changes in the docs.

@robinpokorny
Copy link
Contributor Author

@excitement-engineer and @cpojer, could you please look at this and give me some feedback?

@codecov-io
Copy link

codecov-io commented Apr 10, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3201   +/-   ##
=======================================
  Coverage   63.99%   63.99%           
=======================================
  Files         177      177           
  Lines        6574     6574           
  Branches        4        4           
=======================================
  Hits         4207     4207           
  Misses       2365     2365           
  Partials        2        2

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 46214fb...9d428e0. Read the comment docs.

});
```

Even though the call to `test` will return right away, the test doesn't complete until the promise resolves as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that these examples are really valuable for people using older versions of jest (i.e. Jest 19 and before) because these versions do not include the .resolves yet. So perhaps we should consider keeping this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's keep this please.

await user.getUserName(2);
} catch (object) {
expect(object.error).toEqual('User with 2 not found.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep these code examples. They are really useful people using older versions of jest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can add the .rejects and .resolves examples next to the old examples and mention that these keywords may only be used in jest 20+

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's keep both examples around. I think that is best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I agree, I haven't taken into account the users of the previous versions.

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 went trough it again, and this change only reflects changes already in the docs in the async tutorial https://facebook.github.io/jest/docs/tutorial-async.html

Now the tutorial and sample code differ.

@excitement-engineer
Copy link
Contributor

This code fixes some of the issues I described in issue #3273:) Following issue #3250 , I think we need to make sure that that we keep the old examples and docs around for people using jest 19 and below.

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

@robinpokorny thanks for helping improve the documentation. I agree with @excitement-engineer, let's keep the old examples around. We aren't deprecating the old way of testing for promises, we are only introducing a second way to test promises. We should also make sure to use expect.assertions for any async test – it should become part of the mindset when people write tests :)

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

See comments.

@cpojer
Copy link
Member

cpojer commented Apr 19, 2017

@robinpokorny are you planning on addressing the feedback from my review?

@robinpokorny
Copy link
Contributor Author

@cpojer, Yes, sorry. Will do it today or tomorrow.

@robinpokorny
Copy link
Contributor Author

I will update the ‘Testing Asynchronous Code’ article to include both options and recommends use of expect.assertions. Do you think that every async test should have it?

@cpojer
Copy link
Member

cpojer commented Apr 25, 2017

Yeah, let's add it to every async test to educate people about it :) Whether they'll use it in the end is not our problem then :D We did our best!

@cpojer cpojer added this to the Jest 20 milestone Apr 28, 2017
@cpojer
Copy link
Member

cpojer commented Apr 28, 2017

Hey @robinpokorny, do you think you can give this another final pass?

@robinpokorny
Copy link
Contributor Author

Uff, @cpojer, finally had the time for the rewrite.

I added expect.assertions everywhere, I hope.

I also made sure that in both the async tutorial and async testing article, we show all four combinations:

  • promise with .then/.catch and return
  • promise with async/await
  • .resolves/.rejects with return
  • .resolves/.rejects with async/await

@cpojer cpojer merged commit 5749d38 into jestjs:master May 2, 2017
@cpojer
Copy link
Member

cpojer commented May 2, 2017

Thanks @robinpokorny, this is great :)

@robinpokorny robinpokorny deleted the update-promise-docs branch May 2, 2017 12:58
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Update docs on promises

* Remove promises from `expect.assertions` and `test`

* Keep docs for promise return

* Update async docs

* Update the async tutorial

* Nicer headlines in Testing Async code

* Update TestingAsyncCode.md

* Update TutorialAsync.md
@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 13, 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.

5 participants