Skip to content

feat(prefer-in-document): add support for assigments #107

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

Merged
merged 14 commits into from
Nov 30, 2020

Conversation

benmonro
Copy link
Member

@benmonro benmonro commented Nov 27, 2020

What: add support for variable assignments with prefer in document

Why: fixes bugs and adds more use cases. fixes: #104

How: checking variable declarations & assignments.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #107 (e55deab) into master (8bca240) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #107   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          218       238   +20     
  Branches        27        32    +5     
=========================================
+ Hits           218       238   +20     
Impacted Files Coverage Δ
src/queries.js 100.00% <ø> (ø)
src/rules/prefer-in-document.js 100.00% <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 8bca240...e55deab. Read the comment docs.

Copy link
Contributor

@AntonNiklasson AntonNiklasson left a comment

Choose a reason for hiding this comment

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

I ran this code on one of our codebases to validate, and it crashed. This case seems to be reproducing it:

invalidCase(
  `const compressingFeedback = await screen.findByText(/Compressing video/);
  expect(compressingFeedback).toBeDefined();`,
  `const compressingFeedback = await screen.findByText(/Compressing video/);
  expect(compressingFeedback).toBeInTheDocument();`
)

I guess the await changes to structure slightly? Here's the full error:

TypeError: Cannot read property 'property' of undefined
Occurred while linting /Users/anton/code/milkywire/enterprise/components/page-specific/impacter-tools/create/CreatePosts.spec.tsx:213
    at getQueryNodeFromAssignment (/Users/anton/code/github.com/testing-library/eslint-plugin-jest-dom/dist/rules/prefer-in-document.js:90:31)
    at MemberExpression[object.callee.name=expect][property.name=/(toHaveLength|toBeDefined|toBeNull)/][object.arguments.0.type=Identifier] (/Users/anton/code/github.com/testing-library/eslint-plugin-jest-dom/dist/rules/prefer-in-document.js:139:25)
    at /Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /Users/anton/code/milkywire/enterprise/node_modules/eslint/lib/linter/linter.js:952:32

Copy link
Contributor

@AntonNiklasson AntonNiklasson left a comment

Choose a reason for hiding this comment

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

Looking good! I played around with the latest changes and managed to find another test case that currently crashes:

invalidCase(
  `let button;
  it("foo", async () => {
    button = await screen.findByText('click me');
    expect(button).toBeDefined();
  });`,
  `let button;
  it("foo", async () => {
    button = await screen.findByText('click me');
    expect(button).toBeInTheDocument();
  });`
)

I do think this "pattern" is fairly common, but it's not really feasible for this rule to support everything of course. I just felt that this case was pretty similar to what you already had there.

expect(foo).toHaveLength(1);

const bar = screen.queryByText("bar");
expect(bar).toHaveLength(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could include some async examples here now that it's properly supported?

expect(foo).toHaveLength(3);

const bar = screen.queryByText("bar");
expect(bar).not.toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps include some async examples here also, just to be clear.

@nickserv
Copy link
Member

nickserv commented Nov 30, 2020

This isn't a code review, but FWIW this fixes the errors in linting testing-library/dom-testing-library#834. Nice work!

@benmonro benmonro merged commit 7b6f524 into master Nov 30, 2020
@benmonro benmonro deleted the feature/in-document-fixes branch November 30, 2020 21:44
@github-actions
Copy link

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickserv
Copy link
Member

We're getting various runtime failures in dom-testing-library after upgrading and linting with --fix. It seems like it replaces some array assertions where it shouldn't, among other things.

@benmonro
Copy link
Member Author

oy. got any examples?

@nickserv
Copy link
Member

Do you want me to just publish a branch so you can see the CI build?

@benmonro
Copy link
Member Author

that would be excellent, thanks @nickmccurdy

@benmonro
Copy link
Member Author

@nickmccurdy looks liek those are coming from usages of getAllBy that should actually be using getBy, no? Would it maybe make more sense to update those?

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.

Bug: Rule prefer-in-document expects the argument to expect to be CallExpression
3 participants