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.

Sign up for GitHub

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

Jump to bottom

feat(rule): prefer expect queryBy #22

Merged
merged 8 commits into from
Oct 16, 2019
Merged

feat(rule): prefer expect queryBy #22

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
62ed216
feat(rule): prefer expect queryBy
emmenko Oct 16, 2019
353cc77
chore: regenerate lockfile
emmenko Oct 16, 2019
093b44b
refactor: improve docs and tests, include also findBy
emmenko Oct 16, 2019
e3e4231
refactor: remove occurrences of findBy
emmenko Oct 16, 2019
53a950c
refactor: generate test cases for all query methods
emmenko Oct 16, 2019
0fd4a6c
refactor: disable autofix implementation
emmenko Oct 16, 2019
6a3f264
refactor: remove fixable badge
emmenko Oct 16, 2019
67d305c
refactor: fixable option to null
emmenko Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: disable autofix implementation
  • Loading branch information
@emmenko
emmenko committed Oct 16, 2019
commit 0fd4a6c3e6c968893028c4ba8eee938f45008ffb
17 changes: 10 additions & 7 deletions lib/rules/prefer-expect-query-by.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,19 @@ module.exports = {
create: context => ({
CallExpression(node) {
if (hasExpectWithWrongGetByQuery(node)) {
const nodesGetBy = mapNodesForWrongGetByQuery(node);
// const nodesGetBy = mapNodesForWrongGetByQuery(node);
context.report({
node: node.callee,
messageId: 'expectQueryBy',
fix(fixer) {
return fixer.replaceText(
nodesGetBy[0],
nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3')
);
},
// TODO: we keep the autofixing disabled for now, until we figure out
// a better way to amend for the edge cases.
// See also the related discussion: https://github.com/Belco90/eslint-plugin-testing-library/pull/22#discussion_r335394402
// fix(fixer) {
// return fixer.replaceText(
// nodesGetBy[0],
// nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3')
// );
// },
});
}
},
Expand Down
14 changes: 5 additions & 9 deletions tests/lib/rules/prefer-expect-query-by.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,26 @@ ruleTester.run('prefer-expect-query-by', rule, {
],
[]
),
invalid: getByVariants.reduce((invalidRules, queryName) => {
const fixedQueryName = queryName.replace('get', 'query');
return [
invalid: getByVariants.reduce(
(invalidRules, queryName) => [
...invalidRules,
{
code: `expect(${queryName}('Hello')).toBeInTheDocument()`,
errors: [{ messageId: 'expectQueryBy' }],
output: `expect(${fixedQueryName}('Hello')).toBeInTheDocument()`,
},
{
code: `expect(rendered.${queryName}('Hello')).toBeInTheDocument()`,
errors: [{ messageId: 'expectQueryBy' }],
output: `expect(rendered.${fixedQueryName}('Hello')).toBeInTheDocument()`,
},
{
code: `expect(${queryName}('Hello')).not.toBeInTheDocument()`,
errors: [{ messageId: 'expectQueryBy' }],
output: `expect(${fixedQueryName}('Hello')).not.toBeInTheDocument()`,
},
{
code: `expect(rendered.${queryName}('Hello')).not.toBeInTheDocument()`,
errors: [{ messageId: 'expectQueryBy' }],
output: `expect(rendered.${fixedQueryName}('Hello')).not.toBeInTheDocument()`,
},
];
}, []),
],
[]
),
});
Belco90 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

You also need to add some tests for autofixing feature, you can find some examples in tests/lib/rules/no-dom-import.js and again @thomlom can help you with it if you need something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah didn't know that. I added it now. Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some tests are going to be needed for sure, something like that:

    {
      code: `
        const message = getByText('Hello')
        expect(message).toBeInTheDocument() 
      `,
      errors: [{ messageId: 'expectQueryBy' }],
      output: `
        const message = queryByText('Hello')
        expect(message).toBeInTheDocument()
      `,
    },

If auto-fixing is complex technically, I think we can leave it as an improvement.

Copy link
Member

Choose a reason for hiding this comment

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

@thomlom those specific tests for matched elements saved in a var are not necessary, but I found an issue with the current autofix so probably we need to leave it as an improvement.

@emmenko you are fixing the query used inside expect statement, but you also need to take care about if the query is imported or not.

Let's say I have this test:

const { getByText } = render(<Foo />);
// ...
expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText } = render(<Foo />);
// ...
expect(queryByText('bar')).not.toBeInTheDocument();

but it's not 100% fixed as queryByText is not defined so you would have to destructure/get it from render (which could be harder than it seems as it should be get from destructured object or var declared by the user).

And another case:

const { getByText, queryByText } = render(<Foo />);
// ...
expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText, queryByText } = render(<Foo />);
// ...
expect(queryByText('bar')).not.toBeInTheDocument();

so here queryByText is already defined and rule doesn't have to change anything related to this.

So here we have to 1) improve the fix function to take care of the new query being defined or not or 2) ignore fixing the rule for now. I'm happy with option 2 and leave the autofix for future improvement as implement the proper autofix here could take longer than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing rules do not need to account for other linting errors. As stated in the eslint docs

Since all rules are run again after the initial round of fixes is applied, it’s not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule

image

So from my understanding, this is totally fine how the rule autofix is implemented regarding its concerns.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I understand what you mean, but even if it's explained in the rule info I don't want to leave this autofix with the possibility of breaking the code as it would harm the developer experience.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I totally get what you mean and I agree on the fact that fixing rules are only useful the first time.

But this plugin is new and the testing library's ecosystem is still growing. There are some best practices emerging but I think it's still not clear for newcomers to get how to make the most of Testing Library. My concern is that people will rely on this plugin to implement best practices for them automatically, possibly break their tests and have a bad experience with it.
Even if we state it in the docs, this rule will be in the recommended ones. And I'm pretty sure there will be people who will:

  1. Install the plugin (and don't care of the docs since they just want the recommended ones)
  2. Run eslint --fix
  3. See their tests break
  4. Remove the plugin
  5. Checkout the code

I prefer to have a limited experience that leaves you the manual work instead of a full experience that might cause your code to break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want to remove/disable the autofix for now? It's fine by me, I just wanted to share my perspective on this topic before we decide on how to proceed.

Copy link
Member

Choose a reason for hiding this comment

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

Of course! This is not about what I want or something like that :) really happy to have your point of view here. Better to remove the fixing implementation (including the badge and the output in the tests, sorry about the last one as I asked that), and think about it in a future issue. For first rules we added to the plugin we also skipped the autofix for most of them as there are some cases we didn't know what to do or it took longer than expected to be implemented, so it's better to go bit by bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's disabled here: 0fd4a6c

I kept it commented out, so that we have a starting point in case we want to pick it up in the future.