-
Notifications
You must be signed in to change notification settings - Fork 1
feat(eslint): add a Playwright concern covering specs and their config #38
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| /** | ||
| * Concern: Playwright. Two halves — the specs and the config that runs them. | ||
| * | ||
| * Specs get `eslint-plugin-playwright`'s recommended set (conditional logic in | ||
| * tests, forgotten `await` on assertions, `page.waitForTimeout` sleeps, skipped | ||
| * or focused tests left behind) plus a curated tier the plugin ships but leaves | ||
| * out of recommended. | ||
| * | ||
| * The config half exists because Playwright defaults `actionTimeout` and | ||
| * `navigationTimeout` to 0, meaning "no timeout" rather than a sensible one. | ||
| * `expect()` has its own default, so a config that sets `expect.timeout` looks | ||
| * bounded — but that ceiling only covers assertions. A bare `.click()` or | ||
| * `.goto()` on an element that never becomes actionable waits out the whole test | ||
| * budget, which on a long-running suite is a silent blackout: no output, no | ||
| * failing assertion, and a final error naming the test rather than the call that | ||
| * hung. | ||
| */ | ||
| import playwright from "eslint-plugin-playwright"; | ||
|
|
||
| import { PLAYWRIGHT_CONFIG_FILES, PLAYWRIGHT_SPEC_FILES } from "./globs.js"; | ||
|
|
||
| const REQUIRED_TIMEOUTS = ["actionTimeout", "navigationTimeout"]; | ||
|
|
||
| /** Reads a static property name, ignoring spreads and computed keys. */ | ||
| function propertyName(node) { | ||
| if (node.type !== "Property" || node.computed) return null; | ||
| if (node.key.type === "Identifier") return node.key.name; | ||
| if (node.key.type === "Literal") return String(node.key.value); | ||
| return null; | ||
| } | ||
|
|
||
| /** Finds a direct property of an object expression by name. */ | ||
| function findProperty(objectExpression, name) { | ||
| return objectExpression.properties.find( | ||
| (property) => propertyName(property) === name, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Unwraps the config object from `defineConfig({...})`, `export default {...}`, | ||
| * or a `satisfies`/`as` wrapper, so the rule reads the same shape either way. | ||
| */ | ||
| function configObject(node) { | ||
| let current = node; | ||
| while (current) { | ||
| if ( | ||
| current.type === "TSSatisfiesExpression" || | ||
| current.type === "TSAsExpression" | ||
| ) { | ||
| current = current.expression; | ||
| continue; | ||
| } | ||
| if ( | ||
| current.type === "CallExpression" && | ||
| current.callee.type === "Identifier" && | ||
| current.callee.name === "defineConfig" && | ||
| current.arguments.length > 0 | ||
| ) { | ||
| current = current.arguments[0]; | ||
| continue; | ||
| } | ||
| return current.type === "ObjectExpression" ? current : null; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| const requireActionTimeouts = { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Require explicit actionTimeout and navigationTimeout in a Playwright config", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| missing: | ||
| "Playwright config does not set `{{name}}` in `use`. It defaults to 0 (no timeout), so a locator action on an element that never appears hangs for the entire test timeout instead of failing fast and naming the locator.", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| return { | ||
| ExportDefaultDeclaration(node) { | ||
| const config = configObject(node.declaration); | ||
| if (!config) return; | ||
|
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win The rule ignores CommonJS configs, which the file glob still matches.
Add a visitor for the ♻️ Proposed handler for `module.exports` ExportDefaultDeclaration(node) {
const config = configObject(node.declaration);
if (!config) return;
+ checkConfig(config);
+ },
+ "AssignmentExpression > MemberExpression.left"(node) {
+ if (
+ node.object.type !== "Identifier" ||
+ node.object.name !== "module" ||
+ node.computed ||
+ node.property.name !== "exports"
+ ) {
+ return;
+ }
+ const config = configObject(node.parent.right);
+ if (config) checkConfig(config);
+ },Extract the body of the existing visitor into a local 🤖 Prompt for AI Agents |
||
|
|
||
| const useProperty = findProperty(config, "use"); | ||
| // A config with no `use` block at all still inherits both defaults, so | ||
| // report against the config object rather than skipping it. | ||
| const target = | ||
| useProperty && useProperty.value.type === "ObjectExpression" | ||
| ? useProperty.value | ||
| : null; | ||
|
|
||
| for (const name of REQUIRED_TIMEOUTS) { | ||
| if (target && findProperty(target, name)) continue; | ||
| context.report({ | ||
| node: target ?? config, | ||
| messageId: "missing", | ||
| data: { name }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
|
|
||
| const configPlugin = { | ||
| meta: { name: "playwright-config" }, | ||
| rules: { "require-action-timeouts": requireActionTimeouts }, | ||
| }; | ||
|
|
||
| const recommended = playwright.configs["flat/recommended"]; | ||
|
|
||
| /** | ||
| * Rules the plugin ships but leaves out of recommended. | ||
| * | ||
| * These are `error` on purpose. This package is the bar new work is written | ||
| * against, so the standard states the target rather than the current state of | ||
| * any one suite; a repo adopting mid-stream downgrades specific rules in its own | ||
| * config, which keeps the exception visible and local instead of hidden in the | ||
| * shared baseline. | ||
| * | ||
| * Left off deliberately, as pure convention rather than correctness: | ||
| * `prefer-lowercase-title` (style), `require-tags` and `no-restricted-*` (need a | ||
| * project-specific vocabulary), `require-soft-assertions` (changes failure | ||
| * semantics), `max-expects` (an arbitrary budget), and `no-hooks` (directly | ||
| * contradicts `require-hook` below). | ||
| */ | ||
| const CURATED_SPEC_RULES = { | ||
| // Matcher choice is diagnostic quality: `toBe(true)` reports "expected true, | ||
| // received false", while `toBeTruthy()` reports almost nothing useful. | ||
| "playwright/prefer-to-be": "error", | ||
| "playwright/prefer-to-contain": "error", | ||
| "playwright/prefer-comparison-matcher": "error", | ||
| "playwright/prefer-equality-matcher": "error", | ||
| "playwright/prefer-strict-equal": "error", | ||
| "playwright/require-to-throw-message": "error", | ||
| // An unbounded `toPass` is the same silent-blackout shape as an unbounded | ||
| // action: it retries until the test budget runs out. | ||
| "playwright/require-to-pass-timeout": "error", | ||
| "playwright/no-commented-out-tests": "error", | ||
| // Locator quality. A CSS or nth-based selector binds the test to markup | ||
| // structure, so it breaks on a refactor that changed nothing a user can see, | ||
| // and it reports "element not found" rather than naming the control. | ||
| "playwright/prefer-native-locators": "error", | ||
| "playwright/no-nth-methods": "error", | ||
| "playwright/no-raw-locators": "error", | ||
| "playwright/no-get-by-title": "error", | ||
| // Structure: shared setup belongs in a hook, and a spec should declare what it | ||
| // covers. `test.slow()` triples a budget instead of fixing what is slow. | ||
| "playwright/require-hook": "error", | ||
| "playwright/require-top-level-describe": "error", | ||
| "playwright/no-slowed-test": "error", | ||
| }; | ||
|
|
||
| export default [ | ||
| { | ||
| ...recommended, | ||
| files: PLAYWRIGHT_SPEC_FILES, | ||
| rules: { ...recommended.rules, ...CURATED_SPEC_RULES }, | ||
| }, | ||
|
Comment on lines
+156
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate other concerns and check which entries set a TypeScript parser.
fd . src/eslint -e js --exec rg -n 'parser|languageOptions|files:' {} \;
echo "=== README mentions of composing the playwright concern ==="
fd -i readme -e md --exec rg -n -C 3 -i 'playwright' {} \;Repository: unraid/js-standards Length of output: 1044 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== eslint plugin files ==="
git ls-files src/eslint
echo "=== playwright.js outline/sections ==="
fd -e js playwright.js src/eslint --exec sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1"' sh {}
echo "=== eslint concern files parser context ==="
for f in $(git ls-files src/eslint -e js); do
echo "--- $f"
rg -n 'languageOptions|parser|Files|TS_SOURCE|REACTIVE|VUE|PLAYWRIGHT|recommended|rules:' "$f" -C 2 || true
done
echo "=== exports/config consumers ==="
rg -n "playwright|PLAYWRIGHT_SPEC_FILES|PLAYWRIGHT_CONFIG_FILES|import .*playwright|from .*playwright" .Repository: unraid/js-standards Length of output: 13824 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== eslint plugin files ==="
git ls-files src/eslint
echo "=== playwright.js outline/sections ==="
fd -e js playwright.js src/eslint --exec sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1"' sh {}
echo "=== eslint concern files parser context ==="
for f in $(git ls-files src/eslint -e js); do
echo "--- $f"
rg -n 'languageOptions|parser|files|TS_SOURCE|REACT_FILES|VUE|PLAYWRIGHT|recommended|rules:' "$f" -C 2 || true
done
echo "=== exports/config consumers ==="
rg -n "playwright|PLAYWRIGHT_SPEC_FILES|PLAYWRIGHT_CONFIG_FILES|import .*playwright|from .*playwright" .Repository: unraid/js-standards Length of output: 13824 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== test helper ==="
for f in $(git ls-files test -z | xargs -0 -n1 sed -n '1,120p'); do
:
done 2>/dev/null || true
echo "=== README compose sections ==="
sed -n '140,195p' README.md
echo "=== package dependencies relevant ==="
sed -n '1,80p' package.json | rg -n -C 2 'eslint|pnpm|ts|typescript|`@typescript-eslint`|playwright' || trueRepository: unraid/js-standards Length of output: 5966 🌐 Web query:
💡 Result: In ESLint 10 (as in previous versions using flat config), the Citations:
🌐 Web query:
💡 Result: The ESLint default parser, Espree, does not support TypeScript syntax [1][2][3]. When Espree encounters TypeScript-specific constructs—such as type annotations (e.g., Citations:
Document TypeScript composition for the Playwright spec concern.
🤖 Prompt for AI Agents |
||
| { | ||
| files: PLAYWRIGHT_CONFIG_FILES, | ||
| plugins: { "playwright-config": configPlugin }, | ||
| rules: { "playwright-config/require-action-timeouts": "error" }, | ||
| }, | ||
| ]; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { test } from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import { Linter } from "eslint"; | ||
| import playwrightConfigConcern from "../src/eslint/playwright.js"; | ||
|
|
||
| const linter = new Linter(); | ||
| // Select by rule identity, not position — the concern also ships a spec-file | ||
| // entry, and which one comes first is not part of its contract. | ||
| const concern = playwrightConfigConcern.find( | ||
| (entry) => entry.rules?.["playwright-config/require-action-timeouts"], | ||
| ); | ||
|
|
||
| /** Lint a Playwright config body under the shipped concern's plugin and rules. */ | ||
| const lintConfig = (source) => | ||
| linter.verify(source, { | ||
| plugins: concern.plugins, | ||
| rules: concern.rules, | ||
| languageOptions: { ecmaVersion: "latest", sourceType: "module" }, | ||
| }); | ||
|
|
||
| const missingNames = (source) => | ||
| lintConfig(source) | ||
| .map((message) => /`(\w+)`/.exec(message.message)?.[1]) | ||
| .filter(Boolean); | ||
|
|
||
| test("flags a config that sets neither timeout", () => { | ||
| // The exact shape that motivated this rule: expect() carries a ceiling, so the | ||
| // config looks bounded, while every bare .click() and .goto() is not. | ||
| const source = ` | ||
| import { defineConfig } from "@playwright/test"; | ||
| export default defineConfig({ | ||
| timeout: 600000, | ||
| expect: { timeout: 60000 }, | ||
| use: { baseURL: "http://localhost", viewport: { width: 1440, height: 900 } }, | ||
| }); | ||
| `; | ||
| assert.deepEqual( | ||
| missingNames(source).toSorted((left, right) => left.localeCompare(right)), | ||
| ["actionTimeout", "navigationTimeout"], | ||
| ); | ||
| }); | ||
|
|
||
| test("flags only the timeout that is absent", () => { | ||
| const source = ` | ||
| import { defineConfig } from "@playwright/test"; | ||
| export default defineConfig({ | ||
| use: { actionTimeout: 15000 }, | ||
| }); | ||
| `; | ||
| assert.deepEqual(missingNames(source), ["navigationTimeout"]); | ||
| }); | ||
|
|
||
| test("accepts a config that sets both timeouts", () => { | ||
| const source = ` | ||
| import { defineConfig } from "@playwright/test"; | ||
| export default defineConfig({ | ||
| use: { actionTimeout: 15000, navigationTimeout: 30000 }, | ||
| }); | ||
| `; | ||
| assert.deepEqual(lintConfig(source), []); | ||
| }); | ||
|
|
||
| test("flags a config with no use block, which still inherits both defaults", () => { | ||
| const source = ` | ||
| import { defineConfig } from "@playwright/test"; | ||
| export default defineConfig({ timeout: 600000 }); | ||
| `; | ||
| assert.equal(lintConfig(source).length, 2); | ||
| }); | ||
|
|
||
| test("reads a plain default-exported object, not just defineConfig", () => { | ||
| const source = ` | ||
| export default { use: { actionTimeout: 15000, navigationTimeout: 30000 } }; | ||
| `; | ||
| assert.deepEqual(lintConfig(source), []); | ||
| }); | ||
|
|
||
| test("ignores a module that exports something other than a config object", () => { | ||
| assert.deepEqual(lintConfig(`export default function build() {}`), []); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Playwright default testMatch pattern file extensions💡 Result:
By default, Playwright uses the following glob pattern to identify test files: /*.@(spec|test).?(c|m)[jt]s?(x) [1][2][3]. This pattern includes the following file types and naming conventions: - File extensions:.js,.ts,.mjs,.cjs,.jsx, and.tsx [1][2]. - Naming conventions: The files must end with either.spec or.test before the file extension [1][3]. For example, files named login-screen.spec.ts or user-login.test.js would be matched by this default pattern [1][2]. Matching is performed against the absolute file path, and strings provided to the testMatch configuration option are treated as glob patterns [1][3].
Citations:
🏁 Script executed:
Repository: unraid/js-standards
Length of output: 265
🏁 Script executed:
Repository: unraid/js-standards
Length of output: 4316
Extend
PLAYWRIGHT_SPEC_FILESto match Playwright’s defaulttestMatch.The document states this glob mirrors Playwright’s default, but Playwright also matches Playwright spec files with
.mts,.cts, and.jsx. Add those extensions to keep the pattern and comment consistent; otherwise.mts/.cts/.jsxspecs may not get Playwright lint rules.♻️ Proposed glob extension
📝 Committable suggestion
🤖 Prompt for AI Agents