Fix false positive when using allowConditional (#379)

* Fix false positive when using `allowConditional`

* Sync

Author: mskelton

docs/rules/no-skipped-test.md

@@ -52,10 +52,24 @@ Setting this option to `true` will allow using `test.skip()` to
 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.
 
+Examples of **incorrect** code for the `{ "allowConditional": true }` option:
+
+```javascript
+test.skip('foo', ({}) => {
+  expect(1).toBe(1)
+})
+
+test('foo', ({}) => {
+  test.skip()
+  expect(1).toBe(1)
+})
+```
+
 Example of **correct** code for the `{ "allowConditional": true }` option:
 
 ```javascript
 test('foo', ({ browserName }) => {
   test.skip(browserName === 'firefox', 'Still working on it')
+  expect(1).toBe(1)
 })
 ```

docs/rules/no-slowed-test.md

@@ -47,10 +47,24 @@ mark a test as slow. This can be helpful if you want to prevent usage of
 `test.slow` being added by mistake but still allow slow tests based on
 browser/environment setup.
 
+Examples of **incorrect** code for the `{ "allowConditional": true }` option:
+
+```javascript
+test.slow('foo', ({}) => {
+  expect(1).toBe(1)
+})
+
+test('foo', ({}) => {
+  test.slow()
+  expect(1).toBe(1)
+})
+```
+
 Example of **correct** code for the `{ "allowConditional": true }` option:
 
 ```javascript
 test('foo', ({ browserName }) => {
   test.slow(browserName === 'firefox', 'Still working on it')
+  expect(1).toBe(1)
 })
 ```

package.json

@@ -35,7 +35,7 @@
     "lint": "eslint .",
     "fmt": "prettier --write .",
     "fmt:check": "prettier --check .",
-    "test": "vitest --run",
+    "test": "vitest --run --hideSkippedTests",
     "test:watch": "vitest --reporter=dot --run",
     "ts": "tsc --noEmit"
   },

src/rules/no-skipped-test.test.ts

@@ -303,6 +303,37 @@ runRuleTester('no-skipped-test', rule, {
         },
       },
     },
+    {
+      code: 'test("foo", ({}) => { test.skip(); })',
+      errors: [
+        {
+          column: 23,
+          endColumn: 34,
+          line: 1,
+          messageId: 'noSkippedTest',
+          suggestions: [{ messageId, output: 'test("foo", ({}) => {  })' }],
+        },
+      ],
+      options: [{ allowConditional: true }],
+    },
+    {
+      code: 'test.skip("foo", ({}) => { expect(1).toBe(1) })',
+      errors: [
+        {
+          column: 6,
+          endColumn: 10,
+          line: 1,
+          messageId: 'noSkippedTest',
+          suggestions: [
+            {
+              messageId,
+              output: 'test("foo", ({}) => {  })',
+            },
+          ],
+        },
+      ],
+      options: [{ allowConditional: true }],
+    },
   ],
   valid: [
     'test("a test", () => {});',
@@ -323,7 +354,15 @@ runRuleTester('no-skipped-test', rule, {
     'this["skip"]();',
     'this[`skip`]();',
     {
-      code: 'test.skip(browserName === "firefox", "Still working on it");',
+      code: 'test("foo", ({ browserName }) => { test.skip(browserName === "firefox", "Still working on it") })',
+      options: [{ allowConditional: true }],
+    },
+    {
+      code: 'test("foo", ({ browserName }) => { if (browserName === "firefox") { test.skip("Still working on it") } })',
+      options: [{ allowConditional: true }],
+    },
+    {
+      code: 'test("foo", ({ browserName }) => { if (browserName === "firefox") { test.skip() } })',
       options: [{ allowConditional: true }],
     },
     // Global aliases

src/rules/no-skipped-test.ts

@@ -1,4 +1,4 @@
-import { getStringValue } from '../utils/ast.js'
+import { findParent, getStringValue } from '../utils/ast.js'
 import { createRule } from '../utils/createRule.js'
 import { parseFnCall } from '../utils/parseFnCall.js'
 
@@ -27,7 +27,12 @@ export default createRule({
 
         // If allowConditional is enabled and it's not a test/describe function,
         // we ignore any `test.skip` calls that have no arguments.
-        if (isStandalone && allowConditional) {
+        if (
+          isStandalone &&
+          allowConditional &&
+          (node.arguments.length !== 0 ||
+            findParent(node, 'BlockStatement')?.parent?.type === 'IfStatement')
+        ) {
           return
         }
 

src/rules/no-slowed-test.test.ts

@@ -133,6 +133,23 @@ runRuleTester('no-slowed-test', rule, {
         },
       ],
     },
+    {
+      code: 'test.slow("foo", ({}) => { expect(1).toBe(1) })',
+      errors: [
+        {
+          column: 6,
+          endColumn: 10,
+          line: 1,
+          messageId: 'noSlowedTest',
+          suggestions: [
+            {
+              messageId,
+              output: 'test("foo", ({}) => { expect(1).toBe(1) })',
+            },
+          ],
+        },
+      ],
+    },
     // Global aliases
     {
       code: 'it.slow("slow this test", async ({ page }) => {});',
@@ -156,6 +173,19 @@ runRuleTester('no-slowed-test', rule, {
         },
       },
     },
+    {
+      code: 'test("foo", ({}) => { test.slow(); })',
+      errors: [
+        {
+          column: 23,
+          endColumn: 34,
+          line: 1,
+          messageId: 'noSlowedTest',
+          suggestions: [{ messageId, output: 'test("foo", ({}) => {  })' }],
+        },
+      ],
+      options: [{ allowConditional: true }],
+    },
   ],
   valid: [
     'test("a test", () => {});',
@@ -172,7 +202,15 @@ runRuleTester('no-slowed-test', rule, {
     'this["slow"]();',
     'this[`slow`]();',
     {
-      code: 'test.slow(browserName === "firefox", "Still working on it");',
+      code: 'test("foo", ({ browserName }) => { test.slow(browserName === "firefox", "Still working on it") })',
+      options: [{ allowConditional: true }],
+    },
+    {
+      code: 'test("foo", ({ browserName }) => { if (browserName === "firefox") { test.slow("Still working on it") } })',
+      options: [{ allowConditional: true }],
+    },
+    {
+      code: 'test("foo", ({ browserName }) => { if (browserName === "firefox") { test.slow() } })',
       options: [{ allowConditional: true }],
     },
     // Global aliases

src/rules/no-slowed-test.ts

@@ -1,4 +1,4 @@
-import { getStringValue } from '../utils/ast.js'
+import { findParent, getStringValue } from '../utils/ast.js'
 import { createRule } from '../utils/createRule.js'
 import { parseFnCall } from '../utils/parseFnCall.js'
 
@@ -23,7 +23,12 @@ export default createRule({
 
         // If allowConditional is enabled and it's not a test function,
         // we ignore any `test.slow` calls that have no arguments.
-        if (isStandalone && allowConditional) {
+        if (
+          isStandalone &&
+          allowConditional &&
+          (node.arguments.length !== 0 ||
+            findParent(node, 'BlockStatement')?.parent?.type === 'IfStatement')
+        ) {
           return
         }