Skip to content

fix(await-async-query): false positives for await-async-query #208

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 3 commits into from
Aug 5, 2020

Conversation

thomaslombart
Copy link
Collaborator

@thomaslombart thomaslombart commented Aug 4, 2020

Closes #122.

Changes

  • Move isAwaited and isPromiseResolved to node-utils.ts that was used in three different rules (await-async-utils, await-async-query, await-fire-event)
  • Prevent the await-async-query rule to fire if nothing is imported from one of the testing library modules as declared in LIBRARY_MODULES in utils.ts.
  • Refactor the test cases in await-async-query to get closer to a real test case

Note that this is a partial improvement to the rule. Indeed, if one is using (and importing) a custom render function in his tests, he won't benefit from the rule as this function would not be imported from a testing library module such as @testing-library/react or @testing-library/vue.

A fix to that would be to allow the user to input what custom render function he uses, which is one of the breaking changes coming in V4. I think we will be able to further improve this rule after the V4.

What do you think?

@thomaslombart thomaslombart self-assigned this Aug 4, 2020
return `
import { render } from '@testing-library/react'
test("An example test",${isAsync ? ' async ' : ' '}() => {
${code}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function allows to get closer to a real test. I'm happy to move it to test-utils and refactor some tests (for example in await-async-utils) if needed 🙂

Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. I'd prefer to wait until v4 is released to avoid messing up the code and having to resolve lot of conflicts, but it's definitely a nice to have after it! It would need some improvements tho to be able to customize the import etc.

@thomaslombart thomaslombart requested a review from Belco90 August 4, 2020 16:17
@@ -1,41 +1,21 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { getDocsUrl } from '../utils';
import { getDocsUrl, LIBRARY_MODULES } from '../utils';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there's an improvement to do on the LIBRARY_MODULES naming. Indeed, it doesn't make sense to make the rule fire on the async queries for @testing-library/cypress. Right now it works because it's not included in the LIBRARY_MODULES constants, however it's still a library module.

Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Nice work, it's good to have you back collaborating with this plugin!

As you mentioned, v4 will handle better the custom render functions so we can improve it there.

@Belco90 Belco90 merged commit cbdfd5f into master Aug 5, 2020
@Belco90 Belco90 deleted the fix/await-async-query branch August 5, 2020 17:00
@Belco90
Copy link
Member

Belco90 commented Aug 5, 2020

🎉 This PR is included in version 3.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

False positives on await-async-query and no-await-sync-query
2 participants