Feat: add linting rule for before/beforeEach just like the async test rule #151
Conversation
|
|
|
There was a problem hiding this comment.
Hi @Arcturus5404. Thank you for opening a PR! Can you please fill out the PR description to give reviewers better idea of what you are trying to accomplish with this contribution?
docs/rules/no-async-before.md
Outdated
| @@ -0,0 +1,52 @@ | |||
| # Prevent using async/await in Cypress test cases (no-async-tests) | |||
|
|
|||
| Cypress before that return a promise will cause tests to prematurely start, can cause unexpected behavior. An `async` function returns a promise under the hood, so a before using an `async` function might also error. | |||
There was a problem hiding this comment.
are we sure this is always the case? This might be true for certain commands but I don't think generally this applies.
There was a problem hiding this comment.
I do not know if this is always true, I guess the cypress team knows this better than me. I noticed that the test method was already being executed while the before was still in progress
There was a problem hiding this comment.
interesting. I don't think I have run into this but I will take your word for it. Knowing how mocha hooks process and how we integrate with them, I can absolutely see this being true
| @@ -36,6 +36,7 @@ You can add rules: | |||
| "cypress/assertion-before-screenshot": "warn", | |||
| "cypress/no-force": "warn", | |||
| "cypress/no-async-tests": "error", | |||
| "cypress/no-async-before": "error", | |||
There was a problem hiding this comment.
Does this issue also occur in after/afterEach lifecycle hooks? Maybe no-async-hooks might be a better rule, if this is the case.
There was a problem hiding this comment.
I guess so, but I am not sure they will cause a problem. It might has the same potential to break your testcase. Should the new hook rule also include the test rule? one to rule them all?
There was a problem hiding this comment.
I do know that in before and beforeEach the async code might actually break the test, because of the dependency. But there might not be a problem as such in the after because of race conditions
Co-authored-by: Bill Glesias <[email protected]>
Co-authored-by: Bill Glesias <[email protected]>
|
|
||
| ## When Not To Use It | ||
|
|
||
| If there are genuine use-cases for using `async/await` in your before then you may not want to include this rule (or at least demote it to a warning). |
There was a problem hiding this comment.
| If there are genuine use-cases for using `async/await` in your before then you may not want to include this rule (or at least demote it to a warning). | |
| If there are genuine use-cases for using `async/await` in your `before/beforeEach` function callback, then you may not want to include this rule (or at least demote it to a warning). |
This comment was marked as resolved.
This comment was marked as resolved.
|
I think that will be fine to just have the release triggered when the other things merge |
|
🎉 This PR is included in version 2.15.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
PR for adding linting rules to the before and beforeEach. Having an async in the beforeEach and before can introduce unexpected behaviour, where the async process is not finished yet before the test runs. This can cause a test to be flaky. This rule will mark it as an error.