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

ESlint: Handle modifiers on expect in valid-expect #3306

Merged
merged 1 commit into from
Apr 17, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 16, 2017

Summary
Part 1 of #3082 split up, see that for justification. I'll send a PR for locations soon™️

Test plan
See added tests which failed before this change

@SimenB SimenB changed the title Eslint not eslint-plugin: Handle not being used, and check for typo Apr 16, 2017
@SimenB
Copy link
Member Author

SimenB commented Apr 16, 2017

/cc @jkimbo

@SimenB
Copy link
Member Author

SimenB commented Apr 16, 2017

I'm not good enough at Flow to resolve the type error... Too bad eslint does not have an official flow typing.

Error:

packages/eslint-plugin-jest/src/rules/valid-expect.js:49
 49:               propertyName = node.parent.parent.property.name;
                                                     ^^^^^^^^ property `property`. Property not found in
 49:               propertyName = node.parent.parent.property.name;
                                  ^^^^^^^^^^^^^^^^^^ object type


Found 1 error

@SimenB
Copy link
Member Author

SimenB commented Apr 17, 2017

Seg fault now, I don't think that's on me, though...

@wyze
Copy link
Contributor

wyze commented Apr 17, 2017

From what I can tell, the Flow error is because the CallExpression type doesn't contain a key for property.

We just need to refine the type to be more specific.

           let propertyName = node.parent.property.name;
-          let grandParentType = node.parent.parent.type;
+          let grandParent = node.parent.parent;
  
            // a property is accessed, get the next node
            if (grandParent.type === 'MemberExpression') {
              // a modifier is used, just get the next one
              if (expectProperties.indexOf(propertyName) > -1) {
-               /* $FlowFixMe */
-               propertyName = node.parent.parent.property.name;
-               grandParentType = node.parent.parent.parent.type;
+               propertyName = grandParent.property.name;
+               grandParent = grandParent.parent;
              } else {
                // only a few properties are allowed
                context.report({
                  message: `"${propertyName}" is not a valid property of expect.`,
                  node,
                });
              }
            }
  
            // matcher was not called
-           if (grandParentType === 'ExpressionStatement') {
+           if (grandParent.type === 'ExpressionStatement') {
              context.report({
                message: `"${propertyName}" was not called.`,
                node,
              });
            }

@SimenB
Copy link
Member Author

SimenB commented Apr 17, 2017

@wyze Thank you, I could remove the suppression now!
TBH I don't get why that should matter (I tried doing const grandparent: MemberExpression = node.parent.parent; inside the if, but that made it complain more).
I suppose it's unable to infer based on the .type check what type it really is.

@SimenB SimenB changed the title eslint-plugin: Handle not being used, and check for typo ESlint: Handle modifiers on expect in valid-expect Apr 17, 2017
@codecov-io
Copy link

Codecov Report

Merging #3306 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3306      +/-   ##
==========================================
+ Coverage    63.9%   63.95%   +0.05%     
==========================================
  Files         176      176              
  Lines        6475     6484       +9     
  Branches        4        4              
==========================================
+ Hits         4138     4147       +9     
  Misses       2336     2336              
  Partials        1        1
Impacted Files Coverage Δ
...kages/eslint-plugin-jest/src/rules/valid-expect.js 100% <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 1ab9ea9...965fa93. Read the comment docs.

@cpojer cpojer merged commit 1f6b320 into jestjs:master Apr 17, 2017
@cpojer
Copy link
Member

cpojer commented Apr 17, 2017

Thanks @SimenB

@SimenB SimenB deleted the eslint-not branch April 17, 2017 16:02
skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 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