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

Feat: add linting rule for before/beforeEach just like the async test rule #151

Merged
merged 5 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this issue also occur in after/afterEach lifecycle hooks? Maybe no-async-hooks might be a better rule, if this is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"cypress/no-pause": "error"
}
}
Expand Down
52 changes: 52 additions & 0 deletions docs/rules/no-async-before.md
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure this is always the case? This might be true for certain commands but I don't think generally this applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Arcturus5404 marked this conversation as resolved.
Show resolved Hide resolved

## Rule Details

This rule disallows using `async` before and beforeEach functions.
Arcturus5404 marked this conversation as resolved.
Show resolved Hide resolved

Examples of **incorrect** code for this rule:

```js
describe('my feature', () => {
before('my test case', async () => {
await cy.get('.myClass')
// other operations
})
})
```

```js
describe('my feature', () => {
before('my test case', async () => {
cy
.get('.myClass')
.click()

await someAsyncFunction()
})
})
```

Examples of **correct** code for this rule:

```js
describe('my feature', () => {
before('my test case', () => {
cy.get('.myClass')
// other operations
})
})

```

## 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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).


## Further Reading

- [Commands Are Asynchronous](https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Commands-Are-Asynchronous)
- [Commands Are Promises](https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Commands-Are-Promises)
- [Commands Are Not Promises](https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Commands-Are-Not-Promises)
47 changes: 47 additions & 0 deletions lib/rules/no-async-before.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict'

module.exports = {
meta: {
docs: {
description: 'Prevent using async/await in Cypress before methods',
category: 'Possible Errors',
recommended: true,
},
messages: {
unexpected: 'Avoid using async functions with Cypress before / beforeEach functions',
},
},

create (context) {
function isBeforeBlock (callExpressionNode) {
const { type, name } = callExpressionNode.callee

return type === 'Identifier'
&& name === 'before' || name === 'beforeEach'
}

function isBeforeAsync (node) {
return node.arguments
&& node.arguments.length >= 2
&& node.arguments[1].async === true
}

return {
Identifier (node) {
if (node.name === 'cy' || node.name === 'Cypress') {
const ancestors = context.getAncestors()
const asyncTestBlocks = ancestors
.filter((n) => n.type === 'CallExpression')
.filter(isBeforeBlock)
.filter(isBeforeAsync)

if (asyncTestBlocks.length >= 1) {
asyncTestBlocks.forEach((node) => {
context.report({ node, messageId: 'unexpected' })
})
}
}
},
}
},
}
25 changes: 25 additions & 0 deletions tests/lib/rules/no-async-before.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict'

const rule = require('../../../lib/rules/no-async-before')
const RuleTester = require('eslint').RuleTester

const ruleTester = new RuleTester()

const errors = [{ messageId: 'unexpected' }]
// async functions are an ES2017 feature
const parserOptions = { ecmaVersion: 8 }

ruleTester.run('no-async-before', rule, {
valid: [
{ code: 'before(\'a before case\', () => { cy.get(\'.someClass\'); })', parserOptions },
{ code: 'before(\'a before case\', async () => { await somethingAsync(); })', parserOptions },
{ code: 'async function nonTestFn () { return await somethingAsync(); }', parserOptions },
{ code: 'const nonTestArrowFn = async () => { await somethingAsync(); }', parserOptions },
],
invalid: [
{ code: 'before(\'a test case\', async () => { cy.get(\'.someClass\'); })', parserOptions, errors },
{ code: 'beforeEach(\'a test case\', async () => { cy.get(\'.someClass\'); })', parserOptions, errors },
{ code: 'before(\'a test case\', async function () { cy.get(\'.someClass\'); })', parserOptions, errors },
{ code: 'beforeEach(\'a test case\', async function () { cy.get(\'.someClass\'); })', parserOptions, errors },
],
})