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 eslint rules for ensuring methods on expect are called correctly #3426

Closed
wants to merge 1 commit into from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 1, 2017

Summary
The rule only validated usage of matchers, not all the extra stuff expect can do.

/cc @anilreddykatta

Test plan
See new tests, yarn jest

expectFunctionsWithoutArgs,
);

function checkExpectFunction(calleeName, node, context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@SimenB SimenB force-pushed the eslint-expect-extras branch 2 times, most recently from 9199211 to 3df5c01 Compare May 1, 2017 08:37
@@ -28,6 +28,8 @@ type JasmineMatchersObject = {[id: string]: JasmineMatcher};

module.exports = (config: {expand: boolean}) => {
global.expect = expect;
// setState is not documented
Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be?

@SimenB SimenB force-pushed the eslint-expect-extras branch 4 times, most recently from 95ff89f to ddcd853 Compare May 1, 2017 10:08
@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #3426 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3426      +/-   ##
==========================================
+ Coverage   64.04%   64.33%   +0.28%     
==========================================
  Files         177      177              
  Lines        6570     6626      +56     
  Branches        4        4              
==========================================
+ Hits         4208     4263      +55     
  Misses       2361     2361              
- Partials        1        2       +1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jest-expect.js 0% <ø> (ø) ⬆️
...kages/eslint-plugin-jest/src/rules/valid-expect.js 100% <100%> (ø) ⬆️
...ests/coverage_report/not-required-in-test-suite.js 0% <0%> (-20%) ⬇️

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 0817d64...a5213d9. Read the comment docs.

@anilreddykatta
Copy link
Contributor

@SimenB Awesome! Thanks 😄

@cpojer
Copy link
Member

cpojer commented May 3, 2017

Hey @SimenB. I thought about this PR a bit over the past two days and have concluded that we shouldn't merge it. I think the maintenance cost is higher than the value it provides. For these functions, when they are invoked improperly, Jest will simply throw and crash the test. I think that is ok.

The alternative here is to surface such mistakes earlier but at the cost of us having to maintain this API changes in two places. I'd prefer to not take on this cost right now but I'm happy to consider different viewpoints and may reopen if you feel strongly that we should do this.

@cpojer cpojer closed this May 3, 2017
@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