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

Check if page methods are missing await (missing-playwright-await) #310

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Check if page methods are missing await (missing-playwright-await) #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4c32ba
Check if page methods are awaited
camchenry Aug 20, 2024
ef66ae1
Add more methods
camchenry Aug 20, 2024
af9ea25
Update docs
camchenry Aug 20, 2024
4b8b2a7
Remove waitForEvent
camchenry Aug 20, 2024
cf053ac
Remove wait for request/response
camchenry Aug 20, 2024
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
2 changes: 2 additions & 0 deletions docs/rules/missing-playwright-await.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Example of **incorrect** code for this rule:
```javascript
expect(page).toMatchText('text')
expect.poll(() => foo).toBe(true)
page.goto('https://example.com')

test.step('clicks the button', async () => {
await page.click('button')
Expand All @@ -20,6 +21,7 @@ Example of **correct** code for this rule:
```javascript
await expect(page).toMatchText('text')
await expect.poll(() => foo).toBe(true)
await page.goto('https://example.com')

await test.step('clicks the button', async () => {
await page.click('button')
Expand Down
24 changes: 24 additions & 0 deletions src/rules/missing-playwright-await.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,21 @@ runRuleTester('missing-playwright-await', rule, {
},
},
},
// Page methods
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love to hear more ideas on test cases here. It's a bit simpler to check page methods than the test/except cases, since it's not a chained call.

Copy link
Member

Choose a reason for hiding this comment

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

Let's add test cases to assert myPage.goto, frame.goto, myFrame.goto, etc.

{
code: `page.goto('https://example.com')`,
errors: [
{ column: 1, endColumn: 10, endLine: 1, line: 1, messageId: 'page' },
],
output: `await page.goto('https://example.com')`,
},
{
code: test(`page.goto('https://example.com')`),
errors: [
{ column: 28, endColumn: 37, endLine: 1, line: 1, messageId: 'page' },
],
output: test(`await page.goto('https://example.com')`),
},
],
valid: [
// Basic
Expand Down Expand Up @@ -368,5 +383,14 @@ runRuleTester('missing-playwright-await', rule, {
},
},
},
// Page methods
{ code: `await page.goto('https://example.com')` },
{ code: `await page.title()` },
// Other page methods are ignored
{ code: `page.frames()` },
// Other methods with the same name are ignored
{ code: `randomObject.title()` },
// Does not need to be awaited when returned
{ code: `() => { return page.content() }` },
Comment on lines +387 to +394
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a few tests for this, e.g. this.page.goto()

Copy link
Member

Choose a reason for hiding this comment

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

Also, this probably doesn't support stuff like this: page.locator('.foo').click(). Without that, this rule is pretty limited in usefulness.

],
})
85 changes: 84 additions & 1 deletion src/rules/missing-playwright-await.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Rule } from 'eslint'
import ESTree from 'estree'
import { getParent, getStringValue, isIdentifier } from '../utils/ast'
import { getParent, getStringValue, isIdentifier, isPage } from '../utils/ast'
import { createRule } from '../utils/createRule'
import { ParsedFnCall, parseFnCall } from '../utils/parseFnCall'

Expand Down Expand Up @@ -57,6 +57,73 @@ const playwrightTestMatchers = [
'toBeInViewport',
]

const pageMethods = new Set([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please double check these 🙏 I tried to alphabetically sort them and make sure I included all methods that return Promise and not any that don't, but I could have made a mistake here.

'$', // deprecated
'$$', // deprecated
'$eval', // deprecated
'$$eval', // deprecated
'addInitScript',
'addLocatorHandler',
'addScriptTag',
'addStyleTag',
'bringToFront',
'check', // deprecated
'click', // deprecated
'close',
'content',
'dblclick', // deprecated
'dispatchEvent', // deprecated
'dragAndDrop',
'emulateMedia',
'evaluate',
'evaluateHandle',
'exposeBinding',
'exposeFunction',
'fill', // deprecated
'focus', // deprecated
'getAttribute', // deprecated
'goBack',
'goForward',
'goto',
'hover', // deprecated
'innerHTML', // deprecated
'innerText', // deprecated
'inputValue', // deprecated
'isChecked', // deprecated
'isDisabled', // deprecated
'isEditable', // deprecated
'isEnabled', // deprecated
'isHidden', // deprecated
'isVisible', // deprecated
'opener',
'pause',
'pdf',
'press', // deprecated
'reload',
'removeLocatorHandler',
'route',
'routeFromHAR',
'screenshot',
'selectOption', // deprecated
'setChecked', // deprecated
'setContent',
'setExtraHTTPHeaders',
'setInputFiles', // deprecated
'setViewportSize',
'tap', // deprecated
'textContent', // deprecated
'title',
'type', // deprecated
'unroute',
'unrouteAll',
'waitForFunction',
'waitForLoadState',
'waitForNavigation', // deprecated
'waitForSelector', // deprecated
'waitForTimeout', // deprecated
'waitForURL',
])

function getReportNode(node: ESTree.Node) {
const parent = getParent(node)
return parent?.type === 'MemberExpression' ? parent : node
Expand Down Expand Up @@ -139,6 +206,21 @@ export default createRule({

return {
CallExpression(node) {
// Checking validity of calls to methods on the page object
if (isPage(node) && node.callee.type === 'MemberExpression') {
const method = getStringValue(node.callee.property)
const isValid = checkValidity(node)

if (!isValid && pageMethods.has(method)) {
context.report({
data: { method },
fix: (fixer) => fixer.insertTextBefore(node, 'await '),
messageId: 'page',
node: node.callee,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Go ahead and return at the end of this if statement, if it is a page method, no need to run parseFnCall since it won't be a expect or test function

}

const call = parseFnCall(context, node)
if (call?.type !== 'step' && call?.type !== 'expect') return

Expand Down Expand Up @@ -167,6 +249,7 @@ export default createRule({
messages: {
expect: "'{{matcherName}}' must be awaited or returned.",
expectPoll: "'expect.poll' matchers must be awaited or returned.",
page: "'{{method}}' must be awaited or returned.",
testStep: "'test.step' must be awaited or returned.",
},
schema: [
Expand Down
9 changes: 8 additions & 1 deletion src/utils/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,17 @@ export function dig(node: ESTree.Node, identifier: string | RegExp): boolean {
: false
}

export function isPage(node: ESTree.CallExpression) {
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'm kind of dubious on whether this function is a good idea or not, since you have to check if node.callee is a MemberExpression anyway to get the type checking to work properly. Open to thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

How about instead accepting a MemberExpression and then doing isPage(node.callee).

return (
node.callee.type === 'MemberExpression' &&
dig(node.callee.object, /(^(page|frame)|(Page|Frame)$)/)
)
}

export function isPageMethod(node: ESTree.CallExpression, name: string) {
return (
node.callee.type === 'MemberExpression' &&
dig(node.callee.object, /(^(page|frame)|(Page|Frame)$)/) &&
isPage(node) &&
isPropertyAccessor(node.callee, name)
)
}
Expand Down