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

fix(types): allow all elements #834

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Nov 20, 2020

Port of testing-library/react-testing-library#833

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 20, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cdc5980:

Sandbox Source
react-testing-library-examples Configuration

@MatanBobi

This comment has been minimized.

@kentcdodds

This comment has been minimized.

@kentcdodds
Copy link
Member

Here's the CI failure:

[lint] 
[lint] Oops! Something went wrong! :(
[lint] 
[lint] ESLint: 7.14.0
[lint] 
[lint] TypeError: Cannot read property 'name' of undefined
[lint] Occurred while linting /home/runner/work/dom-testing-library/dom-testing-library/src/__tests__/element-queries.js:220
[lint]     at check (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint-plugin-jest-dom/dist/rules/prefer-in-document.js:39:27)
[lint]     at CallExpression[callee.object.callee.name='expect'][callee.property.name=/(toHaveLength|toBeDefined|toBeNull)/] (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint-plugin-jest-dom/dist/rules/prefer-in-document.js:97:7)
[lint]     at /home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/safe-emitter.js:45:58
[lint]     at Array.forEach (<anonymous>)
[lint]     at Object.emit (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
[lint]     at NodeEventGenerator.applySelector (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
[lint]     at NodeEventGenerator.applySelectors (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
[lint]     at NodeEventGenerator.enterNode (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
[lint]     at CodePathAnalyzer.enterNode (/home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
[lint]     at /home/runner/work/dom-testing-library/dom-testing-library/node_modules/eslint/lib/linter/linter.js:952:32

I think it's something wrong with the jest-dom/prefer-in-document eslint plugin rule. We're in violation of that rule a bunch in our repo (many of our tests were in place before that assertion existed). Looks like this is a bug in @testing-library/eslint-plugin-jest-dom and also something maybe we should update?

Unfortunately this is completely unrelated to this PR, but we should probably get it fixed before merging anything else.

@kentcdodds
Copy link
Member

I'm also not opposed to just disabling the rule for now.

@benmonro
Copy link
Member

benmonro commented Nov 30, 2020

Yeah that rule is not in the recommended list yet until it gets a little more vetting. There is a PR in flight that addresses some stability on the rule though.

@nickmccurdy
Copy link
Member Author

nickmccurdy commented Nov 30, 2020

testing-library/eslint-plugin-jest-dom#107 fixes the crash but then we get 76 lint errors, so I'll disable it and we can fix those issues separately.

@benmonro
Copy link
Member

@nickmccurdy the rule is autofixing. fwiw. :)

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #834 (cdc5980) into master (010f8be) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #834   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          934       934           
  Branches       286       286           
=========================================
  Hits           934       934           
Impacted Files Coverage Δ
src/get-queries-for-element.js 100.00% <ø> (ø)

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 010f8be...cdc5980. Read the comment docs.

MatanBobi referenced this pull request in testing-library/react-testing-library Dec 6, 2020
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
@cweekly
Copy link

cweekly commented Jan 6, 2021

Hi folks,
Happy New Year!
Is resolving those last 3 conflicts on anybody's radar?
(The nextjs "with-typescript-eslint-jest" kit features a test util that overrides render, but it breaks on typing the RenderResult; looks like this PR will fix it.)
Thanks for all your work on this awesome library!
Slainte,
Chris

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 this pull request may close these issues.

None yet

6 participants