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 matchers to expect type #8093

Merged
merged 3 commits into from
Mar 11, 2019
Merged

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

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

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

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

5 participants