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

prefer-to-have-style: Cannot read property 'range' of undefined #97

Closed
AriPerkkio opened this issue Nov 22, 2020 · 6 comments · Fixed by #98
Closed

prefer-to-have-style: Cannot read property 'range' of undefined #97

AriPerkkio opened this issue Nov 22, 2020 · 6 comments · Fixed by #98

Comments

@AriPerkkio
Copy link
Contributor

Hello, I'm testing stability of well known community ESlint plugins with eslint-remote-tester. This ESLint plugin seems to contain a rule which causes linter to crash. ESlint rules should not crash in any condition since this makes all valid linting problems disappear. If this is a false flag please let me know.

  • eslint-plugin-jest-dom version: 3.2.4
  • node version: v14.15.1
  • npm version: 6.14.8

Relevant code or config

expect(a.style).toHaveProperty('transform');

What you did:

$ node ./node_modules/.bin/eslint ./temp.js 

What happened:

TypeError: Cannot read property 'range' of undefined
Occurred while linting <text>:34
    at Object.fix (/<root-path-removed>/eslint-plugin-jest-dom/dist/rules/prefer-to-have-style.js:148:182)

Reproduction repository:

eslint-remote-tester-results/FormidableLabs_spectacle.md

Rule: prefer-to-have-style

  • Message: Cannot read property 'range' of undefined Occurred while linting <text>:34
  • Path: FormidableLabs/spectacle/src/components/deck/deck.test.js
  • Link
          <div>Slide 1</div>
        </Slide>
      </Deck>
    );
    expect(tree.find('AnimatedDeckDiv').props().style).toHaveProperty(
      'transform'
    );
    expect(tree.find('AnimatedDeckDiv').props().style).not.toHaveProperty(
      'opacity'
    );
TypeError: Cannot read property 'range' of undefined
Occurred while linting <text>:34
    at Object.fix (/<root-path-removed>/eslint-plugin-jest-dom/dist/rules/prefer-to-have-style.js:148:182)
    at normalizeFixes (/<root-path-removed>/eslint/lib/linter/report-translator.js:178:28)
    at /<root-path-removed>/eslint/lib/linter/report-translator.js:347:49
    at Object.report (/<root-path-removed>/eslint/lib/linter/linter.js:920:41)
    at MemberExpression[property.name=style][parent.parent.property.name=toHaveProperty][parent.callee.name=expect] (/<root-path-removed>/eslint-plugin-jest-dom/dist/rules/prefer-to-have-style.js:143:13)
    at /<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:283:22)

Problem description:

ESLint rule should not crash in any condition.

Suggested solution:

Add cases mentioned in this issue as unit tests. In ESLint rule check node attributes for falsy before accessing them.

While I was setting up the minimal case above I came across two more crashing cases:

expect(a.style).toHaveProperty();
TypeError: Cannot read property 'value' of undefined
Occurred while linting /<root-path-removed>/temp.js:1
    at /<root-path-removed>/eslint-plugin-jest-dom/dist/createBannedAttributeRule.js:15:82
    at Array.some (<anonymous>)
    at isBannedArg (/<root-path-removed>/eslint-plugin-jest-dom/dist/createBannedAttributeRule.js:15:42)
    at CallExpression[callee.object.callee.name='expect'][callee.property.name=/toHaveProperty|toHaveAttribute/] (/<root-path-removed>/eslint-plugin-jest-dom/dist/createBannedAttributeRule.js:75:12)
    at /<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:297:14)
expect(a.style).toHaveProperty
TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
Occurred while linting /<root-path-removed>/temp.js:1
    at MemberExpression[property.name=style][parent.parent.property.name=toHaveProperty][parent.callee.name=expect] (/<root-path-removed>/eslint-plugin-jest-dom/dist/rules/prefer-to-have-style.js:141:37)
    at /<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/<root-path-removed>/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/<root-path-removed>/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/<root-path-removed>/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /<root-path-removed>/eslint/lib/linter/linter.js:952:32
    at Array.forEach (<anonymous>)
@benmonro
Copy link
Member

@all-contributors please add @AriPerkkio for bugs

@allcontributors
Copy link
Contributor

@benmonro

I've put up a pull request to add @AriPerkkio! 🎉

@benmonro
Copy link
Member

@AriPerkkio eslint-remote-tester looks like a very cool tool, is that something that can be can run as part of CI on this project?

@AriPerkkio
Copy link
Contributor Author

Thanks for the interest @benmonro.

It definitely can be used as part of CI workflow! ESLint plugin projects like this could use it for smoke test step. On every pull request the CI could run the rules against 30-50 repositories and verify no crashes are encountered. Depending on the CI resources this should take about 30 mins. If you want there could also be a scheduled job checking stability against 100's of repositories once a week or month.

I've been running this tool on github CI with plenty of plugins against + 150 repositories and each run takes about 3h30m. https://github.com/AriPerkkio/eslint-remote-tester/actions?query=workflow%3A%22Lint+remote+typescript%22

I can help setting this up later this week. I'll prepare PR with more info.

@benmonro
Copy link
Member

That's fantastic. Yeah once a week sounds great. Can it just automatically create GitHub issues if it finds a problem during the weekly run?

@AriPerkkio
Copy link
Contributor Author

Can it just automatically create GitHub issues if it finds a problem

That's great idea but this is not supported out-of-the-box. Although I've been thinking about exposing the final results via onLintComplete hook in the configuration file. This way users could do anything they want with the results, e.g. create issues to Github.

I would imagine something like below could work:

onLintComplete: async function(results) {
    // Parse results and create issues to Github with some client

Maybe this Github integration functionality could eventually be provided by the package itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants