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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check node argument is defined before use #285

Merged
merged 4 commits into from Jun 22, 2019
Merged

Check node argument is defined before use #285

merged 4 commits into from Jun 22, 2019

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Jun 22, 2019

This fixes errors like this one in no-empty-title:

TypeError: Cannot read property 'type' of undefined
Occurred while linting C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\test\src\utils\isRequestFromSlack.spec.ts:27

TypeError: Cannot read property 'type' of undefined
Occurred while linting C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\test\src\utils\isRequestFromSlack.spec.ts:27
    at isString (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint-plugin-jest\lib\rules\util.js:86:31)
    at CallExpression (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint-plugin-jest\lib\rules\no-empty-title.js:46:14)
    at listeners.(anonymous function).forEach.listener (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\util\safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\util\safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\util\node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\util\node-event-generator.js:280:22)
    at NodeEventGenerator.enterNode (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\util\node-event-generator.js:294:14)
    at CodePathAnalyzer.enterNode (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\code-path-analysis\code-path-analyzer.js:632:23)
    at nodeQueue.forEach.traversalInfo (C:\Users\G-Rath\workspace\projects-personal\repo-event-notifier-lambda\node_modules\eslint\lib\linter.js:752:32)
Process finished with exit code -1

This was caused by a lack of definition guard:

const [firstArgument] = node.arguments;
if (!isString(firstArgument)) {
return;
}

I figured there is no harm in adding node && to all the util methods.
Ironically this (hopefully) would be caught when converting to typescript 馃槈

src/rules/util.js Outdated Show resolved Hide resolved
@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 22, 2019

Random question: for some reason there are a couple of seemingly stray strings in the output on travis:

...
 PASS   lint  src/rules/__tests__/require-tothrow-message.test.js
 PASS   lint  src/rules/prefer-to-have-length.js
 PASS   lint  src/rules/__tests__/no-hooks.test.js
 PASS 27m  lint  src/rules/__tests__/prefer-to-be-null.test.js
 PASS   lint  src/rules/prefer-inline-snapshots.js
 PASS   lint  src/rules/no-hooks.js
 PASS   lint  src/rules/prefer-to-be-undefined.js
 PASS   lint  src/rules/__tests__/no-jest-import.test.js
 PASS   lint  src/rules/prefer-to-be-null.js
 PASS   lint  src/processors/__tests__/snapshot-processor.test.js
 PASS   lint  src/rules/__tests__/prefer-called-with.js
 PASS   lint  src/rules/no-test-prefixes.js
 PASS   lint  src/rules/no-empty-title.js
 PASS   lint  src/rules/no-mocks-import.js
 PASS   lint  src/rules/no-truthy-falsy.js
 PASS   lint  src/rules/no-test-return-statement.js
 PASS   lint  src/rules/__tests__/no-test-return-statement.test.js
 PASS   lint [27m src/__tests__/rules.test.js
 PASS   lint  src/rules/__tests__/prefer-to-have-length.test.js
 PASS   lint  src/rules/require-tothrow-message.js
 PASS   lint  src/rules/prefer-called-with.js
... and so on

Two 27ms; one on the line for prefer-to-be-null.test.js & another on the line for rules.test.js

File                          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------------|----------|----------|----------|----------|-------------------|
All files                     |      100 |      100 |      100 |      100 |                   |
 src                          |      100 |      100 |      100 |      100 |                   |
  index.js                    |      100 |      100 |      100 |      100 |                   |
 src/processors               |      100 |      100 |      100 |      100 |                   |
  snapshot-processor.js       |      100 |      100 |      100 |      100 |                   |
 src/rules                    |      100 |      100 |      100 |      100 |                   |
  consistent-test-it.js       |      100 |      100m |      100 |      100 |                   |
  expect-expect.js            |      100 |      100 |      100 |      100 |                   |
  lowercase-name.js           |      100 |      100 |      100 |      100 |                   |
  no-alias-methods.js         |      100 |      100 |      100 |      100 |                   |
... and so on

A stray m for consistent-test-it.js.

Anyone know what's up w/ that, or is it just random out-of-sync console output?

@SimenB
Copy link
Member

SimenB commented Jun 22, 2019

Could you add a test that fails without this change?

Dunno about the terminal output, but travis's output have been weird to ansi eacapes many times

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 22, 2019

I was a bit worried you'd ask me that, as there are currently no tests for utils.

Would it be enough to add a test for no-empty-title? That'd only cover isString, and not sure if there are other rules that use the other methods in a manner that I can make fail.

Otherwise, I can just write a test where I pass undefined to each method, if that's acceptable 馃槀

@SimenB
Copy link
Member

SimenB commented Jun 22, 2019

A test for no-empty-title is perfect 馃檪

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 22, 2019

Done 馃憤

@SimenB SimenB merged commit 9cba626 into jest-community:master Jun 22, 2019
@SimenB
Copy link
Member

SimenB commented Jun 22, 2019

馃帀 This PR is included in version 22.7.1 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@G-Rath G-Rath deleted the check-node-argument-is-defined-before-use branch June 22, 2019 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants