fix: set skip in skipIf, ignore undefined options (#11591)

Author: sheremet-va

docs/api/test.md

@@ -75,6 +75,8 @@ test('skipped concurrent test', { skip: true, concurrent: true }, () => {
 
 They both work in exactly the same way. To use either one is purely a stylistic choice.
 
+Options set to `undefined` are ignored. They don't override values from the chain, the parent suite, or [tags](/guide/test-tags).
+
 ### timeout
 
 - **Type:** `number`
@@ -372,11 +374,13 @@ test.skipIf(isDev)('prod only test', () => {
 })
 ```
 
+Unlike `test.skip`, `test.skipIf` always sets the [`skip`](#skip) option: `true` if the condition is truthy, `false` otherwise. A falsy condition overrides `skip` inherited from [tags](/guide/test-tags), the same as `test(name, { skip: false })`.
+
 ## test.runIf
 
 - **Alias:** `it.runIf`
 
-Opposite of [test.skipIf](#test-skipif).
+Opposite of [test.skipIf](#test-skipif). It always sets the [`skip`](#skip) option: `false` if the condition is truthy, `true` otherwise.
 
 ```ts
 import { assert, test } from 'vitest'

docs/guide/test-tags.md

@@ -138,6 +138,13 @@ test('flaky database test', { tags: ['flaky', 'db'], timeout: 120_000 })
 // { timeout: 120_000, retry: 3 }
 ```
 
+Options set to `undefined` are ignored, so they don't override the values from tags:
+
+```ts
+test('flaky database test', { tags: ['flaky', 'db'], timeout: undefined })
+// { timeout: 30_000, retry: 3 }
+```
+
 ## Using Tags in Tests
 
 You can apply tags to individual tests or entire suites using the `tags` option:

packages/vitest/src/runtime/runner/suite.ts

@@ -250,6 +250,15 @@ export function createSuiteHooks(): SuiteHooks {
 
 const POSITIVE_INFINITY = Number.POSITIVE_INFINITY
 
+function assignDefined<T extends object>(target: T, source: T): T {
+  for (const key in source) {
+    if (source[key] !== undefined) {
+      target[key] = source[key]
+    }
+  }
+  return target
+}
+
 function parseArguments<T extends (...args: any[]) => any>(
   optionsOrFn: T | object | undefined,
   timeoutOrTest: T | number | undefined,
@@ -322,7 +331,7 @@ function createSuiteCollector(
       )
       .reduce((acc, tag) => {
         const { name, description, priority, meta, ...options } = tag
-        Object.assign(acc, options)
+        assignDefined(acc, options)
         if (meta) {
           acc.meta = Object.assign(acc.meta ?? Object.create(null), meta)
         }
@@ -331,11 +340,7 @@ function createSuiteCollector(
 
     const testOwnMeta = options.meta
     const parentOptions = collectorContext.currentSuite?.options
-    options = {
-      ...parentOptions,
-      ...tagsOptions,
-      ...options,
-    }
+    options = assignDefined({ ...parentOptions, ...tagsOptions }, options)
     const timeout = options.timeout ?? runner.config.testTimeout
     // TODO: should this be `parentTask.meta`?
     // currently we don't inherit
@@ -449,8 +454,7 @@ function createSuiteCollector(
     }
 
     const test = task(formatName(name), {
-      ...this,
-      ...options,
+      ...assignDefined({ ...this }, options),
       handler,
     }) as unknown as Test
 
@@ -605,10 +609,7 @@ function createSuite() {
 
     const { meta: parentMeta, ...parentOptions } = currentSuite?.options || {}
     // inherit options from current suite
-    options = {
-      ...parentOptions,
-      ...options,
-    }
+    options = assignDefined({ ...parentOptions }, options)
 
     const shuffle =
       this.shuffle ??
@@ -818,10 +819,10 @@ export function createTaskCollector(fn: (...args: any[]) => any): TestAPI {
   }
 
   taskFn.skipIf = function (this: TestAPI, condition: any) {
-    return condition ? this.skip : this
+    return getChainableContext(this).extendContext({ skip: !!condition })
   }
   taskFn.runIf = function (this: TestAPI, condition: any) {
-    return condition ? this : this.skip
+    return getChainableContext(this).extendContext({ skip: !condition })
   }
 
   /**

packages/vitest/src/runtime/runner/types.ts

@@ -526,6 +526,8 @@ export interface InternalChainableContext<API = TestAPI> {
   /** @internal */
   withContext: () => API
   /** @internal */
+  extendContext: (ctx: Partial<InternalTestContext>) => API
+  /** @internal */
   getFixtures: () => TestFixtures
 }
 

packages/vitest/src/runtime/runner/utils/chain.ts

@@ -35,6 +35,7 @@ export function createChainable<T extends string, Args extends any[], R = any>(
     Object.defineProperty(chain, kChainableContext, {
       value: {
         withContext: () => chain.bind(context) as any,
+        extendContext: (ctx) => create({ ...context, ...ctx }) as any,
         getFixtures: () => (context as any).fixtures,
         setContext: (key, value) => {
           context[key as T] = value

test/e2e/test/test-tags.test.ts

@@ -467,6 +467,116 @@ test('custom options override tag options', async () => {
   `)
 })
 
+test('skipIf and runIf override skip from tags like the skip option', async () => {
+  const { stderr, testTree } = await runInlineTests({
+    'basic.test.js': `
+      describe('skipped by tag', { tags: ['skipped'] }, () => {
+        test('no options', () => {})
+        test('skip: false', { skip: false }, () => {})
+        test('skip: undefined', { skip: undefined }, () => {})
+        test.skipIf(false)('skipIf(false)', () => {})
+        test.skipIf(undefined)('skipIf(undefined)', () => {})
+        test.skipIf(true)('skipIf(true)', () => {})
+        test.runIf(true)('runIf(true)', () => {})
+        test.runIf(false)('runIf(false)', () => {})
+      })
+    `,
+    'vitest.config.js': {
+      test: {
+        globals: true,
+        tags: [{ name: 'skipped', skip: true }],
+      },
+    },
+  })
+  expect(stderr).toBe('')
+  expect(testTree()).toMatchInlineSnapshot(`
+    {
+      "basic.test.js": {
+        "skipped by tag": {
+          "no options": "skipped",
+          "runIf(false)": "skipped",
+          "runIf(true)": "passed",
+          "skip: false": "passed",
+          "skip: undefined": "skipped",
+          "skipIf(false)": "passed",
+          "skipIf(true)": "skipped",
+          "skipIf(undefined)": "passed",
+        },
+      },
+    }
+  `)
+})
+
+test('undefined options do not override inherited options', async () => {
+  const { stderr, buildTree } = await runInlineTests({
+    'basic.test.js': `
+      test.skip('test.skip', { skip: undefined }, () => {})
+      test('tag timeout', { tags: ['slow'], timeout: undefined }, () => {})
+      test('tag with undefined timeout', { tags: ['slow', 'ci'] }, () => {})
+      describe('suite', { timeout: 30_000, concurrent: true }, () => {
+        test('test', { timeout: undefined, concurrent: undefined }, () => {})
+        describe('nested suite', { timeout: undefined }, () => {
+          test('test', () => {})
+        })
+      })
+    `,
+    'vitest.config.js': /* js */ `
+      export default {
+        test: {
+          globals: true,
+          tags: [
+            { name: 'slow', timeout: 60_000 },
+            { name: 'ci', timeout: undefined, priority: 1 },
+          ],
+        },
+      }
+    `,
+  })
+  expect(stderr).toBe('')
+  expect(buildOptionsTree(buildTree)).toMatchInlineSnapshot(`
+    {
+      "basic.test.js": {
+        "suite": {
+          "nested suite": {
+            "test": {
+              "concurrent": true,
+              "mode": "run",
+              "tags": [],
+              "timeout": 30000,
+            },
+          },
+          "test": {
+            "concurrent": true,
+            "mode": "run",
+            "tags": [],
+            "timeout": 30000,
+          },
+        },
+        "tag timeout": {
+          "mode": "run",
+          "tags": [
+            "slow",
+          ],
+          "timeout": 60000,
+        },
+        "tag with undefined timeout": {
+          "mode": "run",
+          "tags": [
+            "slow",
+            "ci",
+          ],
+          "timeout": 60000,
+        },
+        "test.skip": {
+          "mode": "skip",
+          "tags": [],
+          "timeout": 5000,
+        },
+      },
+    }
+  `)
+})
+
 test('strictFlag: false does not throw an error if test has an undefined tag', async () => {
   const { stderr } = await runInlineTests({
     'basic.test.js': `