CONTRIBUTING.md

@@ -1,5 +1,38 @@
 # Contributing
 
+## Installing Dependencies
+
+We use [pnpm](https://pnpm.io) for managing dependencies. You can install the
+necessary dependencies using the following command:
+
+```bash
+pnpm install
+```
+
+## Running Tests
+
+When making changes to lint rules, you can re-run the tests with the following
+command:
+
+```bash
+pnpm test
+```
+
+Or run it in watch mode like so:
+
+```bash
+pnpm test -- --watch
+```
+
+## Adding new rules
+
+When adding new rules, make sure to follow these steps:
+
+1. Add the rule source code in `src/rules`
+1. Add tests for the rule in `test/spec`
+1. Add docs for the rule to `docs/rules`
+1. Add a short description of the rule in `README.md`
+
 ## Releasing
 
 To release a new version with semantic-release, run the following command.

README.md

@@ -101,7 +101,7 @@ under the `playwright` key. It supports the following settings:
   the presence of at least one assertion per test case. This allows such rules
   to recognise custom assertion functions as valid assertions. The global
   setting applies to all modules. The
-  [`expect-expect` rule accepts an option by the same name](./rules/expect-expect.md#additionalassertfunctionnames)
+  [`expect-expect` rule accepts an option by the same name](https://github.com/playwright-community/eslint-plugin-playwright/tree/main/docs/rules/expect-expect.md#additionalassertfunctionnames)
   to enable per-module configuration (.e.g, for module-specific custom assert
   functions).
 

docs/rules/no-skipped-test.md

@@ -31,3 +31,31 @@ test.describe('two tests', () => {
   test('two', async ({ page }) => {});
 });
 ```
+
+## Options
+
+```json
+{
+  "playwright/no-skipped-test": [
+    "error",
+    {
+      "allowConditional": false
+    }
+  ]
+}
+```
+
+### `allowConditional`
+
+Setting this option to `true` will allow using `test.skip()` to
+[conditionally skip a test](https://playwright.dev/docs/test-annotations#conditionally-skip-a-test).
+This can be helpful if you want to prevent usage of `test.skip` being added by
+mistake but still allow conditional tests based on browser/environment setup.
+
+Example of **correct** code for the `{ "allowConditional": true }` option:
+
+```javascript
+test('foo', ({ browserName }) => {
+  test.skip(browserName === 'firefox', 'Still working on it');
+});
+```

src/rules/expect-expect.ts

@@ -1,6 +1,6 @@
 import { Rule } from 'eslint';
 import * as ESTree from 'estree';
-import { isExpectCall, isIdentifier, isTest } from '../utils/ast';
+import { dig, isExpectCall, isTest } from '../utils/ast';
 import { getAdditionalAssertFunctionNames } from '../utils/misc';
 
 function isAssertionCall(
@@ -9,9 +9,7 @@ function isAssertionCall(
 ) {
   return (
     isExpectCall(node) ||
-    additionalAssertFunctionNames.find((name) =>
-      isIdentifier(node.callee, name),
-    )
+    additionalAssertFunctionNames.find((name) => dig(node.callee, name))
   );
 }
 
@@ -38,7 +36,7 @@ export default {
         if (isTest(node, ['fixme', 'only', 'skip'])) {
           unchecked.push(node);
         } else if (isAssertionCall(node, additionalAssertFunctionNames)) {
-          checkExpressions(context.getAncestors());
+          checkExpressions(context.sourceCode.getAncestors(node));
         }
       },
       'Program:exit'() {

src/rules/no-skipped-test.ts

@@ -10,6 +10,8 @@ export default {
   create(context) {
     return {
       CallExpression(node) {
+        const options = context.options[0] || {};
+        const allowConditional = !!options.allowConditional;
         const { callee } = node;
 
         if (
@@ -19,6 +21,12 @@ export default {
         ) {
           const isHook = isTest(node) || isDescribeCall(node);
 
+          // If allowConditional is enabled and it's not a test/describe hook,
+          // we ignore any `test.skip` calls that have no arguments.
+          if (!isHook && allowConditional && node.arguments.length) {
+            return;
+          }
+
           context.report({
             messageId: 'noSkippedTest',
             node: isHook ? callee.property : node,
@@ -52,6 +60,18 @@ export default {
       noSkippedTest: 'Unexpected use of the `.skip()` annotation.',
       removeSkippedTestAnnotation: 'Remove the `.skip()` annotation.',
     },
+    schema: [
+      {
+        additionalProperties: false,
+        properties: {
+          allowConditional: {
+            default: false,
+            type: 'boolean',
+          },
+        },
+        type: 'object',
+      },
+    ],
     type: 'suggestion',
   },
 } as Rule.RuleModule;

src/utils/ast.ts

@@ -158,7 +158,7 @@ export function getMatchers(
  * Digs through a series of MemberExpressions and CallExpressions to find an
  * Identifier with the given name.
  */
-function dig(node: ESTree.Node, identifier: string | RegExp): boolean {
+export function dig(node: ESTree.Node, identifier: string | RegExp): boolean {
   return node.type === 'MemberExpression'
     ? dig(node.property, identifier)
     : node.type === 'CallExpression'

test/spec/expect-expect.spec.ts

@@ -93,7 +93,20 @@ runRuleTester('expect-expect', rule, {
           await assertCustomCondition(page)
         })
       `,
-      name: 'Global settings only',
+      name: 'Custom assert function',
+      settings: {
+        playwright: {
+          additionalAssertFunctionNames: ['assertCustomCondition'],
+        },
+      },
+    },
+    {
+      code: dedent`
+        test('should fail', async ({ myPage, page }) => {
+          await myPage.assertCustomCondition(page)
+        })
+      `,
+      name: 'Custom assert class method',
       settings: {
         playwright: {
           additionalAssertFunctionNames: ['assertCustomCondition'],

test/spec/no-skipped-test.spec.ts

@@ -201,5 +201,9 @@ runRuleTester('no-skipped-test', rule, {
     'this.skip();',
     'this["skip"]();',
     'this[`skip`]();',
+    {
+      code: 'test.skip(browserName === "firefox", "Still working on it");',
+      options: [{ allowConditional: true }],
+    },
   ],
 });