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.

Sign up for GitHub

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

Jump to bottom

add matchers to expect type #8093

Merged
merged 3 commits into from
Mar 11, 2019
Merged

add matchers to expect type #8093

merged 3 commits into from
Mar 11, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Mar 9, 2019

Summary

We know which matchers we have, no reason to just have it as an index string.

Future improvements:

  • verify that extending the exported Matcher interface actually mixes new matchers into the correct interface
  • move snapshot matchers out. They should be added wherever we declare the global expect variable (or potentially mixed in in jest-circus/jest-jasmine2 if possible)
  • get rid of type casts and actually ensure statically that
    • all matchers adhere to the contract
    • we export all the matchers we say we do
    • we don't export any matchers we don't say we do

Test plan

TS build should still succeed

@SimenB SimenB requested review from pedrottimark and thymikee March 9, 2019 10:06
@SimenB SimenB force-pushed the add-matchers-to-expect-type branch from 631cb59 to b0568ca Compare March 9, 2019 10:08
@codecov-io
Copy link

Codecov Report

Merging #8093 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8093   +/-   ##
=======================================
  Coverage   62.46%   62.46%           
=======================================
  Files         263      263           
  Lines       10371    10371           
  Branches     2514     2513    -1     
=======================================
  Hits         6478     6478           
  Misses       3317     3317           
  Partials      576      576
Impacted Files Coverage Δ
packages/expect/src/index.ts 84.92% <100%> (ø) ⬆️

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 17ffcc9...b0568ca. Read the comment docs.

thymikee
thymikee approved these changes Mar 9, 2019
View reviewed changes
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM. let's see what @pedrottimark has to say about it

@pedrottimark
Copy link
Contributor

I will review the details tomorrow. Can you take a first draft at describing benefit for future blog?

For test files written in TypeScript, will it supersede quality validation attempted by lint rules?

@SimenB
Copy link
Member Author

SimenB commented Mar 9, 2019

Can you take a first draft at describing benefit for future blog?

Sneaky

For test files written in TypeScript, will it supersede quality validation attempted by lint rules?

In a slightly different way, but yes. The main benefit though is doing ctrl+space and getting code completion of the actual matchers - no need to guess which ones exist or search through docs, and both docs and potentially examples are right there in the tooltip.

image

Also some type information (e.g. the above toHaveReturnedTimes takes exactly one argument, a number)

@pedrottimark
Copy link
Contributor

Thank you for seeing how to improve developer experience!

Let’s communicate that TypeScript conversion has outward-facing not just inward-facing benefits.

@SimenB
Copy link
Member Author

SimenB commented Mar 10, 2019 •
edited
Loading

All of your suggestions sounds good to me @pedrottimark! Feel free to push them to this branch (I'm on mobile)

EDIT: Back at a computer this morning, addressed the feedback! 🙂

@SimenB SimenB force-pushed the add-matchers-to-expect-type branch from b0568ca to da3c3e1 Compare March 11, 2019 09:27
pedrottimark
pedrottimark approved these changes Mar 11, 2019
View reviewed changes
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.

Thanks again for taking this step forward.

@SimenB SimenB merged commit aece656 into jestjs:master Mar 11, 2019
@SimenB SimenB deleted the add-matchers-to-expect-type branch March 11, 2019 21:17
@SimenB
Copy link
Member Author

SimenB commented Mar 12, 2019

Future improvement - pull these descriptions into the docs (https://jestjs.io/docs/en/expect) so they're not duplicated

@pedrottimark
Copy link
Contributor

Yes, these short descriptions from Definitely Typed are good push to review ExpectAPI.md file.

@SimenB
Copy link
Member Author

SimenB commented Mar 12, 2019

Exporting both expect's and the jest object's APIs from code into doc would be super nice

thymikee added a commit to Brantron/jest that referenced this pull request Mar 19, 2019
@thymikee
Merge remote-tracking branch 'upstream/master' into pr/7167
9d39e68 
* 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)
  ...
@snyk-bot snyk-bot mentioned this pull request Dec 15, 2019
Open
This was referenced Mar 21, 2020
Closed
Merged
@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.
Reviewers

@pedrottimark pedrottimark pedrottimark approved these changes

@thymikee thymikee thymikee approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SimenB @codecov-io @pedrottimark @thymikee @facebook-github-bot