diff --git a/README.md b/README.md index 9e38942..be718bc 100644 --- a/README.md +++ b/README.md @@ -24,20 +24,24 @@ Exit codes: `0` no errors (warnings allowed), `1` errors found, `2` bad input or ## Rules + | Rule | Default | Detects | | --- | --- | --- | -| `no-fixed-wait` | error | `I.wait(5)` with a number. Use `I.waitForElement`, `I.waitForText`, `I.see` | -| `no-sleep` | error | `setTimeout`, including `await new Promise(r => setTimeout(r, ms))` and `timers/promises`, outside helpers | -| `no-if-in-test` | error | `if` inside a Scenario or hook. Use `tryTo()` from `codeceptjs/effects` | -| `no-fetch` | error | `fetch()` outside helpers. Use the REST helper: `I.sendGetRequest`, `I.sendPostRequest` | -| `no-console` | warn | `console.*` outside helpers. Use `I.say()` | -| `no-only` | error on CI, warn locally | `Scenario.only`, `Feature.only`, `Data(...).only` | -| `no-pause` | error on CI, warn locally | `pause()` | -| `no-skip` | warn | `Scenario.skip`, `Feature.skip`, `xScenario`, `xFeature` | -| `secret-credentials` | error | `I.fillField` on a password, token, secret or API key field, or such a `process.env.*` passed to `I.*`, without `secret()` | -| `await-grab` | error | `I.grab*()` assigned or used without `await` | -| `no-actor-in-helper` | error | `I` inside a class extending `Helper`. Use `this.helpers[...]` | -| `raw-browser-in-test` | warn | `I.use*To`, `I.executeScript` in a Scenario. Move it into a helper or page object | +| `no-fixed-wait` | error | I.wait(5) sleeps for a fixed time. Wait for a condition with I.waitForElement, I.waitForText or I.see | +| `no-sleep` | error | setTimeout, including window.setTimeout, await new Promise(r => setTimeout(r, ms)) and timers/promises, sleeps for a fixed time. Wait for a condition instead | +| `no-if-in-test` | error | if inside a Scenario or hook makes the test non-deterministic. Use tryTo() from codeceptjs/effects for optional steps | +| `no-fetch` | error | fetch() in tests or page objects, including window.fetch and globalThis.fetch, bypasses CodeceptJS steps and logs. Use the REST helper: I.sendGetRequest, I.sendPostRequest | +| `no-console` | warn | console.log in tests or page objects is not part of the test report. Use I.say() | +| `no-only` | error on CI, warn locally | Scenario.only, Feature.only and Data().only run only focused tests. Error on CI, warning locally | +| `no-pause` | error on CI, warn locally | pause() and this.pause() stop the test for interactive debugging. Error on CI, warning locally | +| `no-skip` | warn | Scenario.skip, Feature.skip, xScenario and xFeature leave tests out of the run | +| `secret-credentials` | error | Passwords, tokens and API keys passed to I.* in plain text are printed in logs. Wrap them with secret() | +| `await-grab` | error | I.grab*() returns a promise. Assigned, destructured or passed anywhere without await — including as an argument or object property, but not in Promise.all or .then() chains — it is a bug | +| `no-actor-in-helper` | error | I is not available inside a helper. Call other helpers via this.helpers[...] | +| `raw-browser-in-test` | warn | I.usePlaywrightTo, I.useWebDriverTo and I.executeScript in a Scenario. Move raw browser code into a helper or page object | + + +Rules live as one file per rule in [`lib/rules/`](lib/rules); run `npm run docs` to regenerate this table after changing a rule. "On CI" means the `CI` environment variable is set, which every CI provider does. Locally `pause()` and `.only` stay warnings, so a debugging stub doesn't fail the lint. @@ -67,6 +71,8 @@ I.wait(1) TypeScript is read with Node's type stripping (Node 22.13+). Files that use `enum` or `namespace` are reported as parse errors. +A TypeScript config that imports `.js` paths of `.ts` files (the tsx/esbuild convention) can't be loaded by plain Node. If the linted project has `tsx` installed, the linter registers its loader automatically; otherwise loading such a config falls back to linting without it — pass paths explicitly in that case. + ## CI ```yaml diff --git a/bin/lint.js b/bin/lint.js index 498544c..0d3600c 100755 --- a/bin/lint.js +++ b/bin/lint.js @@ -3,7 +3,7 @@ import fs from 'fs' import path from 'path' import { parseArgs } from 'util' import Linter from '../lib/Linter.js' -import rules from '../lib/rules.js' +import rules from '../lib/rules/index.js' const { values: options, positionals: paths } = parseArgs({ allowPositionals: true, @@ -69,7 +69,7 @@ for (const file of files) { findings.push(...linter.lintFile(file)) } catch (err) { process.exitCode = 2 - console.error(`${path.relative(process.cwd(), file)} parse error ${err.message}`) + console.error(err.file ? `parse error ${err.message}` : `${path.relative(process.cwd(), file).replaceAll('\\', '/')} parse error ${err.message}`) } } @@ -124,5 +124,5 @@ async function hook(linter) { } function format(finding) { - return `${path.relative(process.cwd(), finding.file)}:${finding.line}:${finding.column} ${finding.level} ${finding.rule} ${finding.message}` + return `${path.relative(process.cwd(), finding.file).replaceAll('\\', '/')}:${finding.line}:${finding.column} ${finding.level} ${finding.rule} ${finding.message}` } diff --git a/lib/Linter.js b/lib/Linter.js index e02bcfa..69a0418 100644 --- a/lib/Linter.js +++ b/lib/Linter.js @@ -5,10 +5,12 @@ import { pathToFileURL } from 'url' import * as acorn from 'acorn' import * as walk from 'acorn-walk' import { globSync } from 'glob' -import rules from './rules.js' +import rules from './rules/index.js' const EXTENSIONS = ['.js', '.ts', '.mjs', '.cjs'] const CONFIG_FILES = ['codecept.conf.js', 'codecept.conf.ts', 'codecept.conf.cjs', 'codecept.conf.mjs', 'codecept.config.js', 'codecept.config.ts'] +const ACORN_OPTIONS = { ecmaVersion: 'latest', locations: true } +const MAX_SOURCE = 120 export default class Linter { constructor(config = {}, root = process.cwd(), only = []) { @@ -24,6 +26,7 @@ export default class Linter { if (!file) file = CONFIG_FILES.find(name => fs.existsSync(name)) if (!file) return new Linter({}, process.cwd(), only) file = path.resolve(file) + if (file.endsWith('.ts')) await registerTsx(file) const mod = await import(pathToFileURL(file)) const config = mod.config || mod.default?.config || mod.default || {} return new Linter(config, path.dirname(file), only) @@ -49,7 +52,7 @@ export default class Linter { } files.push(...this.supportFiles()) } - return [...new Set(files)].filter(file => EXTENSIONS.includes(path.extname(file))) + return [...new Set(files)].filter(file => EXTENSIONS.includes(path.extname(file))).map(file => file.replaceAll('\\', '/')) } includes(file) { @@ -83,21 +86,16 @@ export default class Linter { const comments = [] let ast try { - ast = acorn.parse(code, { - ecmaVersion: 'latest', - sourceType: 'module', - locations: true, - onComment: comments, - }) + ast = acorn.parse(code, { ...ACORN_OPTIONS, sourceType: 'module', onComment: comments }) } catch (err) { comments.length = 0 - ast = acorn.parse(code, { - ecmaVersion: 'latest', - sourceType: 'script', - locations: true, - allowReturnOutsideFunction: true, - onComment: comments, - }) + try { + ast = acorn.parse(code, { ...ACORN_OPTIONS, sourceType: 'script', allowReturnOutsideFunction: true, onComment: comments }) + } catch (scriptErr) { + scriptErr.file = file + scriptErr.message = `${file}: ${scriptErr.message}` + throw scriptErr + } } const disabled = [] @@ -123,10 +121,27 @@ export default class Linter { rule: rule.id, level, message, - source: code.slice(node.start, node.end), + source: truncate(code.slice(node.start, node.end)), }) } }) return findings.sort((a, b) => a.line - b.line || a.column - b.column) } } + +function truncate(source) { + const line = source.split('\n')[0] + return line.length > MAX_SOURCE ? `${line.slice(0, MAX_SOURCE - 3)}...` : line +} + +// TS configs often import './x.js' for an './x.ts' file, which plain Node cannot resolve. +// If the linted project has tsx installed, register its loader hooks so the config imports work. +async function registerTsx(configFile) { + try { + const require = module.createRequire(configFile) + const tsx = await import(pathToFileURL(require.resolve('tsx/esm/api')).href) + await tsx.register() + } catch { + // tsx not installed — configs limited to plain Node resolution + } +} diff --git a/lib/rules.js b/lib/rules.js deleted file mode 100644 index 084601d..0000000 --- a/lib/rules.js +++ /dev/null @@ -1,210 +0,0 @@ -const TEST_BLOCKS = ['Scenario', 'Before', 'After', 'BeforeSuite', 'AfterSuite'] -const CREDENTIALS = /pass(word|wd)|(^|_)pass(_|$)|token|secret|api[\s_-]?key/i - -function actorMethod(node) { - if (node.type !== 'CallExpression') return - if (node.callee.type !== 'MemberExpression') return - if (node.callee.object.name !== 'I') return - return node.callee.property.name -} - -function insideTest(ancestors) { - return ancestors.some(node => { - if (node.type !== 'CallExpression') return false - const callee = node.callee - if (TEST_BLOCKS.includes(callee.name)) return true - if (callee.type !== 'MemberExpression') return false - return TEST_BLOCKS.includes(callee.object.name) || callee.property.name === 'Scenario' - }) -} - -function insideHelper(ancestors) { - return ancestors.some(node => node.superClass && node.superClass.name === 'Helper') -} - -class Rule { - get level() { - return 'error' - } -} - -class CommitRule extends Rule { - get level() { - if (process.env.CI) return 'error' - return 'warn' - } -} - -class NoFixedWait extends Rule { - id = 'no-fixed-wait' - description = 'I.wait(5) sleeps for a fixed time. Wait for a condition with I.waitForElement, I.waitForText or I.see' - - check(node) { - if (actorMethod(node) !== 'wait') return - if (typeof node.arguments[0]?.value !== 'number') return - return 'I.wait() sleeps unconditionally. Wait for a condition: I.waitForElement / I.waitForText / I.see' - } -} - -class NoSleep extends Rule { - id = 'no-sleep' - description = 'setTimeout, including await new Promise(r => setTimeout(r, ms)) and timers/promises, sleeps for a fixed time. Wait for a condition instead' - - check(node, ancestors) { - if (node.type !== 'CallExpression' || node.callee.name !== 'setTimeout') return - if (insideHelper(ancestors)) return - return 'setTimeout pauses for a fixed time. Wait for a condition: I.waitForElement / I.waitForText / I.waitForFunction' - } -} - -class NoIfInTest extends Rule { - id = 'no-if-in-test' - description = 'if inside a Scenario or hook makes the test non-deterministic. Use tryTo() from codeceptjs/effects for optional steps' - - check(node, ancestors) { - if (node.type !== 'IfStatement') return - if (!insideTest(ancestors)) return - return 'if makes the test non-deterministic. Use tryTo() from codeceptjs/effects for optional steps' - } -} - -class NoFetch extends Rule { - id = 'no-fetch' - description = 'fetch() in tests or page objects bypasses CodeceptJS steps and logs. Use the REST helper: I.sendGetRequest, I.sendPostRequest' - - check(node, ancestors) { - if (node.type !== 'CallExpression' || node.callee.name !== 'fetch') return - if (insideHelper(ancestors)) return - return 'fetch() bypasses CodeceptJS steps and logs. Use the REST helper: I.sendGetRequest / I.sendPostRequest' - } -} - -class NoConsole extends Rule { - id = 'no-console' - description = 'console.log in tests or page objects is not part of the test report. Use I.say()' - - get level() { - return 'warn' - } - - check(node, ancestors) { - if (node.type !== 'CallExpression' || node.callee.object?.name !== 'console') return - if (insideHelper(ancestors)) return - return 'console output is not part of the test report. Use I.say()' - } -} - -class NoOnly extends CommitRule { - id = 'no-only' - description = 'Scenario.only, Feature.only and Data().only run only focused tests. Error on CI, warning locally' - - check(node) { - if (node.type !== 'MemberExpression' || node.property.name !== 'only') return - const target = node.object.name || node.object.callee?.name - if (!['Scenario', 'Feature', 'Data'].includes(target)) return - return `${target}.only limits the run to focused tests. Remove it before commit` - } -} - -class NoPause extends CommitRule { - id = 'no-pause' - description = 'pause() stops the test for interactive debugging. Error on CI, warning locally' - - check(node) { - if (node.type !== 'CallExpression' || node.callee.name !== 'pause') return - return 'pause() stops the test for debugging. Remove it before commit' - } -} - -class NoSkip extends Rule { - id = 'no-skip' - description = 'Scenario.skip, Feature.skip, xScenario and xFeature leave tests out of the run' - - get level() { - return 'warn' - } - - check(node) { - if (node.type !== 'CallExpression') return - const callee = node.callee - if (callee.name === 'xScenario' || callee.name === 'xFeature') return `${callee.name} skips the test` - if (callee.property?.name !== 'skip') return - if (!['Scenario', 'Feature'].includes(callee.object.name)) return - return `${callee.object.name}.skip skips the test` - } -} - -class SecretCredentials extends Rule { - id = 'secret-credentials' - description = 'Passwords, tokens and API keys passed to I.* in plain text are printed in logs. Wrap them with secret()' - - check(node) { - const method = actorMethod(node) - if (!method) return - for (const arg of node.arguments) { - const env = arg.type === 'MemberExpression' && arg.object.property?.name === 'env' && arg.property.name - if (env && CREDENTIALS.test(env)) return `process.env.${env} is printed in logs. Wrap it: secret(process.env.${env})` - } - if (method !== 'fillField') return - const [locator, value] = node.arguments - if (typeof locator?.value !== 'string' || !CREDENTIALS.test(locator.value)) return - if (!value || value.callee?.name === 'secret') return - return `I.fillField('${locator.value}', ...) types a credential that is printed in logs. Wrap the value: secret(...)` - } -} - -class AwaitGrab extends Rule { - id = 'await-grab' - description = 'I.grab*() returns a promise. Its result must be awaited before use' - - check(node, ancestors) { - const method = actorMethod(node) - if (!method || !method.startsWith('grab')) return - const parent = ancestors[ancestors.length - 2] - if (!['VariableDeclarator', 'AssignmentExpression', 'BinaryExpression', 'TemplateLiteral', 'MemberExpression'].includes(parent.type)) return - return `I.${method}() returns a promise. Use: await I.${method}()` - } -} - -class NoActorInHelper extends Rule { - id = 'no-actor-in-helper' - description = 'I is not available inside a helper. Call other helpers via this.helpers[...]' - - check(node, ancestors) { - if (node.type !== 'Identifier' || node.name !== 'I') return - if (!insideHelper(ancestors)) return - return 'I is not available inside a helper. Call other helpers via this.helpers[...]' - } -} - -class RawBrowserInTest extends Rule { - id = 'raw-browser-in-test' - description = 'I.usePlaywrightTo, I.useWebDriverTo and I.executeScript in a Scenario. Move raw browser code into a helper or page object' - - get level() { - return 'warn' - } - - check(node, ancestors) { - const method = actorMethod(node) - if (!method) return - if (method !== 'executeScript' && !/^use\w+To$/.test(method)) return - if (!insideTest(ancestors)) return - return `I.${method} runs raw browser code in a test. Move it into a helper or page object` - } -} - -export default [ - new NoFixedWait(), - new NoSleep(), - new NoIfInTest(), - new NoFetch(), - new NoConsole(), - new NoOnly(), - new NoPause(), - new NoSkip(), - new SecretCredentials(), - new AwaitGrab(), - new NoActorInHelper(), - new RawBrowserInTest(), -] diff --git a/lib/rules/await-grab.js b/lib/rules/await-grab.js new file mode 100644 index 0000000..40dfb75 --- /dev/null +++ b/lib/rules/await-grab.js @@ -0,0 +1,18 @@ +import { Rule } from './base.js' +import { actorMethod } from './utils.js' + +const FLAGGED_PARENTS = ['VariableDeclarator', 'AssignmentExpression', 'BinaryExpression', 'TemplateLiteral', 'MemberExpression', 'CallExpression', 'Property'] + +export class AwaitGrab extends Rule { + id = 'await-grab' + description = 'I.grab*() returns a promise. Assigned, destructured or passed anywhere without await — including as an argument or object property, but not in Promise.all or .then() chains — it is a bug' + + check(node, ancestors) { + const method = actorMethod(node) + if (!method || !method.startsWith('grab')) return + const parent = ancestors[ancestors.length - 2] + if (parent.type === 'MemberExpression' && parent.object === node && ['then', 'catch', 'finally'].includes(parent.property?.name)) return + if (!FLAGGED_PARENTS.includes(parent.type)) return + return `I.${method}() returns a promise. Use: await I.${method}()` + } +} diff --git a/lib/rules/base.js b/lib/rules/base.js new file mode 100644 index 0000000..8519dc7 --- /dev/null +++ b/lib/rules/base.js @@ -0,0 +1,12 @@ +export class Rule { + get level() { + return 'error' + } +} + +export class CommitRule extends Rule { + get level() { + if (process.env.CI) return 'error' + return 'warn' + } +} diff --git a/lib/rules/index.js b/lib/rules/index.js new file mode 100644 index 0000000..2034fe1 --- /dev/null +++ b/lib/rules/index.js @@ -0,0 +1,27 @@ +import { NoFixedWait } from './no-fixed-wait.js' +import { NoSleep } from './no-sleep.js' +import { NoIfInTest } from './no-if-in-test.js' +import { NoFetch } from './no-fetch.js' +import { NoConsole } from './no-console.js' +import { NoOnly } from './no-only.js' +import { NoPause } from './no-pause.js' +import { NoSkip } from './no-skip.js' +import { SecretCredentials } from './secret-credentials.js' +import { AwaitGrab } from './await-grab.js' +import { NoActorInHelper } from './no-actor-in-helper.js' +import { RawBrowserInTest } from './raw-browser-in-test.js' + +export default [ + new NoFixedWait(), + new NoSleep(), + new NoIfInTest(), + new NoFetch(), + new NoConsole(), + new NoOnly(), + new NoPause(), + new NoSkip(), + new SecretCredentials(), + new AwaitGrab(), + new NoActorInHelper(), + new RawBrowserInTest(), +] diff --git a/lib/rules/no-actor-in-helper.js b/lib/rules/no-actor-in-helper.js new file mode 100644 index 0000000..69b132b --- /dev/null +++ b/lib/rules/no-actor-in-helper.js @@ -0,0 +1,14 @@ +import { Rule } from './base.js' +import { insideHelper } from './utils.js' + +export class NoActorInHelper extends Rule { + id = 'no-actor-in-helper' + description = 'I is not available inside a helper. Call other helpers via this.helpers[...]' + message = 'I is not available inside a helper. Call other helpers via this.helpers[...]' + + check(node, ancestors) { + if (node.type !== 'Identifier' || node.name !== 'I') return + if (!insideHelper(ancestors)) return + return this.message + } +} diff --git a/lib/rules/no-console.js b/lib/rules/no-console.js new file mode 100644 index 0000000..8d15e4d --- /dev/null +++ b/lib/rules/no-console.js @@ -0,0 +1,18 @@ +import { Rule } from './base.js' +import { insideHelper } from './utils.js' + +export class NoConsole extends Rule { + id = 'no-console' + description = 'console.log in tests or page objects is not part of the test report. Use I.say()' + message = 'console output is not part of the test report. Use I.say()' + + get level() { + return 'warn' + } + + check(node, ancestors) { + if (node.type !== 'CallExpression' || node.callee.object?.name !== 'console') return + if (insideHelper(ancestors)) return + return this.message + } +} diff --git a/lib/rules/no-fetch.js b/lib/rules/no-fetch.js new file mode 100644 index 0000000..f1c172c --- /dev/null +++ b/lib/rules/no-fetch.js @@ -0,0 +1,14 @@ +import { Rule } from './base.js' +import { insideHelper, isCalleeName } from './utils.js' + +export class NoFetch extends Rule { + id = 'no-fetch' + description = 'fetch() in tests or page objects, including window.fetch and globalThis.fetch, bypasses CodeceptJS steps and logs. Use the REST helper: I.sendGetRequest, I.sendPostRequest' + message = 'fetch() bypasses CodeceptJS steps and logs. Use the REST helper: I.sendGetRequest / I.sendPostRequest' + + check(node, ancestors) { + if (node.type !== 'CallExpression' || !isCalleeName(node.callee, 'fetch')) return + if (insideHelper(ancestors)) return + return this.message + } +} diff --git a/lib/rules/no-fixed-wait.js b/lib/rules/no-fixed-wait.js new file mode 100644 index 0000000..30f7625 --- /dev/null +++ b/lib/rules/no-fixed-wait.js @@ -0,0 +1,14 @@ +import { Rule } from './base.js' +import { actorMethod } from './utils.js' + +export class NoFixedWait extends Rule { + id = 'no-fixed-wait' + description = 'I.wait(5) sleeps for a fixed time. Wait for a condition with I.waitForElement, I.waitForText or I.see' + message = 'I.wait() sleeps unconditionally. Wait for a condition: I.waitForElement / I.waitForText / I.see' + + check(node) { + if (actorMethod(node) !== 'wait') return + if (typeof node.arguments[0]?.value !== 'number') return + return this.message + } +} diff --git a/lib/rules/no-if-in-test.js b/lib/rules/no-if-in-test.js new file mode 100644 index 0000000..2449179 --- /dev/null +++ b/lib/rules/no-if-in-test.js @@ -0,0 +1,14 @@ +import { Rule } from './base.js' +import { insideTest } from './utils.js' + +export class NoIfInTest extends Rule { + id = 'no-if-in-test' + description = 'if inside a Scenario or hook makes the test non-deterministic. Use tryTo() from codeceptjs/effects for optional steps' + message = 'if makes the test non-deterministic. Use tryTo() from codeceptjs/effects for optional steps' + + check(node, ancestors) { + if (node.type !== 'IfStatement') return + if (!insideTest(ancestors)) return + return this.message + } +} diff --git a/lib/rules/no-only.js b/lib/rules/no-only.js new file mode 100644 index 0000000..687e244 --- /dev/null +++ b/lib/rules/no-only.js @@ -0,0 +1,13 @@ +import { CommitRule } from './base.js' + +export class NoOnly extends CommitRule { + id = 'no-only' + description = 'Scenario.only, Feature.only and Data().only run only focused tests. Error on CI, warning locally' + + check(node) { + if (node.type !== 'MemberExpression' || node.property.name !== 'only') return + const target = node.object.name || node.object.callee?.name + if (!['Scenario', 'Feature', 'Data'].includes(target)) return + return `${target}.only limits the run to focused tests. Remove it before commit` + } +} diff --git a/lib/rules/no-pause.js b/lib/rules/no-pause.js new file mode 100644 index 0000000..3db26c4 --- /dev/null +++ b/lib/rules/no-pause.js @@ -0,0 +1,16 @@ +import { CommitRule } from './base.js' + +export class NoPause extends CommitRule { + id = 'no-pause' + description = 'pause() and this.pause() stop the test for interactive debugging. Error on CI, warning locally' + message = 'pause() stops the test for debugging. Remove it before commit' + + check(node) { + if (node.type !== 'CallExpression') return + const callee = node.callee + const bare = callee.type === 'Identifier' && callee.name === 'pause' + const thisPause = callee.type === 'MemberExpression' && callee.object.type === 'ThisExpression' && callee.property?.name === 'pause' + if (!bare && !thisPause) return + return this.message + } +} diff --git a/lib/rules/no-skip.js b/lib/rules/no-skip.js new file mode 100644 index 0000000..b9e9444 --- /dev/null +++ b/lib/rules/no-skip.js @@ -0,0 +1,19 @@ +import { Rule } from './base.js' + +export class NoSkip extends Rule { + id = 'no-skip' + description = 'Scenario.skip, Feature.skip, xScenario and xFeature leave tests out of the run' + + get level() { + return 'warn' + } + + check(node) { + if (node.type !== 'CallExpression') return + const callee = node.callee + if (callee.name === 'xScenario' || callee.name === 'xFeature') return `${callee.name} skips the test` + if (callee.property?.name !== 'skip') return + if (!['Scenario', 'Feature'].includes(callee.object.name)) return + return `${callee.object.name}.skip skips the test` + } +} diff --git a/lib/rules/no-sleep.js b/lib/rules/no-sleep.js new file mode 100644 index 0000000..d7619d3 --- /dev/null +++ b/lib/rules/no-sleep.js @@ -0,0 +1,14 @@ +import { Rule } from './base.js' +import { insideHelper, isCalleeName } from './utils.js' + +export class NoSleep extends Rule { + id = 'no-sleep' + description = 'setTimeout, including window.setTimeout, await new Promise(r => setTimeout(r, ms)) and timers/promises, sleeps for a fixed time. Wait for a condition instead' + message = 'setTimeout pauses for a fixed time. Wait for a condition: I.waitForElement / I.waitForText / I.waitForFunction' + + check(node, ancestors) { + if (node.type !== 'CallExpression' || !isCalleeName(node.callee, 'setTimeout')) return + if (insideHelper(ancestors)) return + return this.message + } +} diff --git a/lib/rules/raw-browser-in-test.js b/lib/rules/raw-browser-in-test.js new file mode 100644 index 0000000..0c66e86 --- /dev/null +++ b/lib/rules/raw-browser-in-test.js @@ -0,0 +1,19 @@ +import { Rule } from './base.js' +import { actorMethod, insideTest } from './utils.js' + +export class RawBrowserInTest extends Rule { + id = 'raw-browser-in-test' + description = 'I.usePlaywrightTo, I.useWebDriverTo and I.executeScript in a Scenario. Move raw browser code into a helper or page object' + + get level() { + return 'warn' + } + + check(node, ancestors) { + const method = actorMethod(node) + if (!method) return + if (method !== 'executeScript' && !/^use\w+To$/.test(method)) return + if (!insideTest(ancestors)) return + return `I.${method} runs raw browser code in a test. Move it into a helper or page object` + } +} diff --git a/lib/rules/secret-credentials.js b/lib/rules/secret-credentials.js new file mode 100644 index 0000000..0660f3e --- /dev/null +++ b/lib/rules/secret-credentials.js @@ -0,0 +1,21 @@ +import { Rule } from './base.js' +import { actorMethod, CREDENTIALS, envPropertyName } from './utils.js' + +export class SecretCredentials extends Rule { + id = 'secret-credentials' + description = 'Passwords, tokens and API keys passed to I.* in plain text are printed in logs. Wrap them with secret()' + + check(node) { + const method = actorMethod(node) + if (!method) return + for (const arg of node.arguments) { + const env = envPropertyName(arg) + if (env && CREDENTIALS.test(env)) return `process.env.${env} is printed in logs. Wrap it: secret(process.env.${env})` + } + if (method !== 'fillField') return + const [locator, value] = node.arguments + if (typeof locator?.value !== 'string' || !CREDENTIALS.test(locator.value)) return + if (!value || value.callee?.name === 'secret') return + return `I.fillField('${locator.value}', ...) types a credential that is printed in logs. Wrap the value: secret(...)` + } +} diff --git a/lib/rules/utils.js b/lib/rules/utils.js new file mode 100644 index 0000000..cf3aadc --- /dev/null +++ b/lib/rules/utils.js @@ -0,0 +1,36 @@ +export const TEST_BLOCKS = ['Scenario', 'Before', 'After', 'BeforeSuite', 'AfterSuite'] +export const CREDENTIALS = /pass(word|wd)|(^|_)pass(_|$)|token|secret|api[\s_-]?key/i + +export function actorMethod(node) { + if (node.type !== 'CallExpression') return + if (node.callee.type !== 'MemberExpression') return + if (node.callee.object.name !== 'I') return + return node.callee.property.name +} + +export function insideTest(ancestors) { + return ancestors.some(node => { + if (node.type !== 'CallExpression') return false + const callee = node.callee + if (TEST_BLOCKS.includes(callee.name)) return true + if (callee.type !== 'MemberExpression') return false + return TEST_BLOCKS.includes(callee.object.name) || callee.property.name === 'Scenario' + }) +} + +export function insideHelper(ancestors) { + return ancestors.some(node => node.superClass && node.superClass.name === 'Helper') +} + +export function isCalleeName(callee, name) { + if (callee.type === 'Identifier') return callee.name === name + if (callee.type !== 'MemberExpression' || callee.property?.name !== name) return false + return callee.object.type === 'Identifier' && ['window', 'globalThis', 'global', 'self'].includes(callee.object.name) +} + +export function envPropertyName(arg) { + if (arg?.type !== 'MemberExpression') return + if (arg.object?.type !== 'MemberExpression' || arg.object.property?.name !== 'env' || arg.object.object?.name !== 'process') return + if (!arg.computed && arg.property?.type === 'Identifier') return arg.property.name + if (arg.computed && arg.property?.type === 'Literal' && typeof arg.property.value === 'string') return arg.property.value +} diff --git a/package.json b/package.json index cb44f7c..cf80715 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,8 @@ "node": ">=22.13.0" }, "scripts": { - "test": "mocha test" + "test": "mocha test", + "docs": "node scripts/update-readme.mjs" }, "repository": { "type": "git", diff --git a/scripts/update-readme.mjs b/scripts/update-readme.mjs new file mode 100644 index 0000000..17956b2 --- /dev/null +++ b/scripts/update-readme.mjs @@ -0,0 +1,18 @@ +import fs from 'fs' +import path from 'path' +import { fileURLToPath } from 'url' +import rules from '../lib/rules/index.js' +import { CommitRule } from '../lib/rules/base.js' + +const readme = path.join(path.dirname(fileURLToPath(import.meta.url)), '..', 'README.md') +const rows = rules.map(rule => `| \`${rule.id}\` | ${rule instanceof CommitRule ? 'error on CI, warn locally' : rule.level} | ${rule.description} |`) +const table = ['', '| Rule | Default | Detects |', '| --- | --- | --- |', ...rows, ''].join('\n') + +const content = fs.readFileSync(readme, 'utf8') +const updated = content.replace(/[\s\S]*/, table) +if (updated === content && !content.includes('')) { + console.error('No ... block found in README.md') + process.exit(1) +} +fs.writeFileSync(readme, updated) +console.log(`README.md updated with ${rules.length} rules`) diff --git a/test/lint_test.js b/test/lint_test.js index f7e8bdc..ebf4f30 100644 --- a/test/lint_test.js +++ b/test/lint_test.js @@ -45,6 +45,11 @@ describe('@codeceptjs/lint', () => { expect(rules('class X extends Helper {\n m() { setTimeout(() => {}, 1) }\n}')).to.deep.equal([]) }) + it('no-sleep catches window.setTimeout and globalThis.setTimeout', () => { + const code = 'window.setTimeout(() => {}, 1)\nglobalThis.setTimeout(() => {}, 1)\nself.setTimeout(() => {}, 1)\nfoo.setTimeout(() => {}, 1)\nclass X extends Helper {\n m() { window.setTimeout(() => {}, 1) }\n}' + expect(rules(code)).to.deep.equal(['1:no-sleep', '2:no-sleep', '3:no-sleep']) + }) + it('no-if-in-test', () => { const code = "Scenario('a', ({ I }) => {\n if (x) I.click('a')\n})\nBefore(({ I }) => {\n if (y) I.click('b')\n})\nfunction f() {\n if (z) return\n}" expect(rules(code)).to.deep.equal(['2:no-if-in-test', '5:no-if-in-test']) @@ -55,6 +60,11 @@ describe('@codeceptjs/lint', () => { expect(rules("await fetch('/api')\nclass X extends Helper {\n m() { fetch('/api') }\n}")).to.deep.equal(['1:no-fetch']) }) + it('no-fetch catches window.fetch and globalThis.fetch', () => { + const code = "window.fetch('/api')\nglobalThis.fetch('/api')\nfoo.fetch('/api')\nclass X extends Helper {\n m() { globalThis.fetch('/api') }\n}" + expect(rules(code)).to.deep.equal(['1:no-fetch', '2:no-fetch']) + }) + it('no-console', () => { const findings = lint("console.log('a')\nconsole.error('b')\nclass X extends Helper {\n m() { console.log('c') }\n}") expect(findings.map(f => `${f.line}:${f.rule}:${f.level}`)).to.deep.equal(['1:no-console:warn', '2:no-console:warn']) @@ -67,6 +77,10 @@ describe('@codeceptjs/lint', () => { expect(lint(code).map(f => f.level)).to.deep.equal(['error', 'error', 'error', 'error']) }) + it('no-pause catches this.pause()', () => { + expect(rules('this.pause()\nobj.pause()')).to.deep.equal(['1:no-pause']) + }) + it('no-skip', () => { expect(rules("Scenario.skip('a', () => {})\nxScenario('b', () => {})\nFeature.skip('f')\nScenario('c', () => {})")).to.deep.equal(['1:no-skip', '2:no-skip', '3:no-skip']) }) @@ -82,11 +96,27 @@ describe('@codeceptjs/lint', () => { expect(rules(code)).to.deep.equal(['1:secret-credentials', '4:secret-credentials']) }) + it('secret-credentials catches computed env access and only process.env', () => { + const code = [ + "I.fillField(loc, process.env['PASSWORD'])", + "I.fillField(loc, secret(process.env['PASSWORD']))", + "I.fillField(loc, process.env['api-key'])", + "I.fillField(loc, process.env['USERNAME'])", + 'I.fillField(loc, foo.env.TOKEN)', + ].join('\n') + expect(rules(code)).to.deep.equal(['1:secret-credentials', '3:secret-credentials']) + }) + it('await-grab', () => { const code = ['const a = I.grabTextFrom("h1")', 'const b = await I.grabTextFrom("h1")', 'return I.grabTextFrom("h1")', 'I.grabTextFrom("h1").length', 'await Promise.all([I.grabTitle()])'].join('\n') expect(lint(`async function f() {\n${code}\n}`).map(f => `${f.line}:${f.rule}`)).to.deep.equal(['2:await-grab', '5:await-grab']) }) + it('await-grab allows promise chains, flags destructuring, arguments and properties', () => { + const code = ['const { text } = I.grabTextFrom("h1")', 'I.grabTextFrom("h1").then(t => t)', 'I.grabTextFrom("h1").catch(e => e)', 'I.see(I.grabTextFrom("h1"))', 'const o = { title: I.grabTitle() }'].join('\n') + expect(lint(`async function f() {\n${code}\n}`).map(f => `${f.line}:${f.rule}`)).to.deep.equal(['2:await-grab', '5:await-grab', '6:await-grab']) + }) + it('no-actor-in-helper', () => { expect(rules('class X extends Helper {\n m() {\n I.click("a")\n }\n}\nI.click("b")')).to.deep.equal(['3:no-actor-in-helper']) }) @@ -122,11 +152,41 @@ describe('@codeceptjs/lint', () => { expect(rules("const x = require('x')\nreturn I.wait(1)")).to.deep.equal(['2:no-fixed-wait']) }) + it('truncates finding source to a single line', () => { + const code = "Scenario('a', ({ I }) => {\n if (x) {\n I.click('a')\n I.click('b')\n }\n})" + const [finding] = lint(code) + expect(finding.source).to.equal('if (x) {') + expect(lint('I.wait(5)')[0].source).to.equal('I.wait(5)') + const long = `I.fillField('${'password'.repeat(50)}', 'v')` + expect(lint(long)[0].source.length).to.be.at.most(120) + expect(lint(long)[0].source.endsWith('...')).to.be.true + }) + + it('includes the file in parse errors', () => { + try { + lint('const =', 'bad.js') + throw new Error('should have thrown') + } catch (err) { + expect(err.message).to.match(/^bad\.js: /) + expect(err.file).to.equal('bad.js') + } + }) + + it('keeps one rule per file in lib/rules', async () => { + const rules = (await import('../lib/rules/index.js')).default + const files = fs.readdirSync(path.join(__dirname, '../lib/rules')).filter(f => f.endsWith('.js') && !['base.js', 'utils.js', 'index.js'].includes(f)) + expect(rules.map(rule => rule.id)).to.have.lengthOf(files.length) + for (const rule of rules) { + expect(files).to.include(`${rule.id}.js`) + } + expect(new Set(rules.map(rule => rule.id)).size).to.equal(rules.length) + }) + it('collects tests, include and helper files from config', async () => { const linter = await Linter.load(path.join(project, 'codecept.conf.js')) const files = linter .files() - .map(f => path.relative(project, f)) + .map(f => path.relative(project, f).replaceAll('\\', '/')) .sort() expect(files).to.deep.equal(['checkout_test.js', 'custom_helper.js', 'existing_test.js', 'pages/login.js', 'steps_file.js']) expect(linter.includes(path.join(project, 'new_test.js'))).to.be.true @@ -150,6 +210,26 @@ describe('@codeceptjs/lint', () => { expect(run(['-r', 'nope']).status).to.equal(2) }) + it('reports parse errors with exit code 2', () => { + const result = run(['bad_syntax.js']) + expect(result.status).to.equal(2) + expect(result.stderr).to.include('bad_syntax.js') + expect(result.stderr).to.include('parse error') + }) + + it('prints findings as JSON with --json', () => { + const result = run(['--json']) + const findings = JSON.parse(result.stdout) + expect(findings).to.be.an('array').with.length.greaterThan(0) + for (const key of ['file', 'line', 'column', 'rule', 'level', 'message', 'source']) { + expect(findings[0]).to.have.property(key) + } + for (const finding of findings) { + expect(finding.level).to.be.oneOf(['error', 'warn']) + } + expect(result.status).to.equal(1) + }) + it('lists rules', () => { const result = run(['rules']) expect(result.stdout).to.include('no-if-in-test') diff --git a/test/project/bad_syntax.js b/test/project/bad_syntax.js new file mode 100644 index 0000000..66c7cb3 --- /dev/null +++ b/test/project/bad_syntax.js @@ -0,0 +1 @@ +const const = 1