From 74ac9173b9c579f53cddd25f8469e757b420d952 Mon Sep 17 00:00:00 2001 From: Hiroshi Ogawa Date: Fri, 22 Dec 2023 16:57:38 +0900 Subject: [PATCH 1/4] fix(vitest): fix skipped suite with snapshots considered obsolete --- packages/vitest/src/runtime/runners/test.ts | 20 +++++++++++++------ .../test/__snapshots__/repro.test.ts.snap | 3 +++ test/core/test/repro.test.ts | 16 +++++++++++++++ 3 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 test/core/test/__snapshots__/repro.test.ts.snap create mode 100644 test/core/test/repro.test.ts diff --git a/packages/vitest/src/runtime/runners/test.ts b/packages/vitest/src/runtime/runners/test.ts index 72e1d17e0d08..526842537a26 100644 --- a/packages/vitest/src/runtime/runners/test.ts +++ b/packages/vitest/src/runtime/runners/test.ts @@ -1,9 +1,9 @@ -import type { CancelReason, Custom, ExtendedContext, Suite, TaskContext, Test, VitestRunner, VitestRunnerImportSource } from '@vitest/runner' +import type { CancelReason, Custom, ExtendedContext, File, Suite, TaskContext, Test, VitestRunner, VitestRunnerImportSource } from '@vitest/runner' import type { ExpectStatic } from '@vitest/expect' import { GLOBAL_EXPECT, getState, setState } from '@vitest/expect' import { getSnapshotClient } from '../../integrations/snapshot/chai' import { vi } from '../../integrations/vi' -import { getFullName, getNames, getWorkerState } from '../../utils' +import { getFullName, getNames, getTests, getWorkerState } from '../../utils' import { createExpect } from '../../integrations/chai/index' import type { ResolvedConfig } from '../../types/config' import type { VitestExecutor } from '../execute' @@ -27,7 +27,17 @@ export class VitestTestRunner implements VitestRunner { this.snapshotClient.clear() } - async onAfterRunFiles() { + async onAfterRunFiles(files?: File[]) { + // mark snapshots in skipped tests as non-obsolete + // TODO: this probably doesn't work when `VitestTestRunner` are handling multiple files concurrently, + // but `snapshotClient.startCurrentRun/finishCurrentRun` might not be working already in that case. + for (const test of getTests(files ?? [])) { + if (test.mode === 'skip') { + const name = getNames(test).slice(1).join(' > ') + this.snapshotClient.skipTestSnapshots(name) + } + } + const result = await this.snapshotClient.finishCurrentRun() if (result) await rpc().snapshotSaved(result) @@ -57,10 +67,8 @@ export class VitestTestRunner implements VitestRunner { if (this.cancelRun) test.mode = 'skip' - if (test.mode !== 'run') { - this.snapshotClient.skipTestSnapshots(name) + if (test.mode !== 'run') return - } clearModuleMocks(this.config) await this.snapshotClient.startCurrentRun(test.file!.filepath, name, this.workerState.config.snapshotOptions) diff --git a/test/core/test/__snapshots__/repro.test.ts.snap b/test/core/test/__snapshots__/repro.test.ts.snap new file mode 100644 index 000000000000..9384baf7a50a --- /dev/null +++ b/test/core/test/__snapshots__/repro.test.ts.snap @@ -0,0 +1,3 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`repro-suite > repro-case 1`] = `"hello"`; diff --git a/test/core/test/repro.test.ts b/test/core/test/repro.test.ts new file mode 100644 index 000000000000..b85f2b9eec78 --- /dev/null +++ b/test/core/test/repro.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from 'vitest' + +// need at least one snapshot test to trigger `SnapshotClient.startCurrentRun` on current file +it('normal case (pre)', () => { + expect(0).toBe(0) +}) + +describe.skipIf(process.env.REPRO_SKIP)('repro-suite', () => { + it('repro-case', () => { + expect('hello').toMatchSnapshot() + }) +}) + +it('normal case (post)', () => { + expect(0).toBe(0) +}) From 2ed44f9f4137f517648feda3654584c2f65ad98a Mon Sep 17 00:00:00 2001 From: Hiroshi Ogawa Date: Fri, 22 Dec 2023 17:51:03 +0900 Subject: [PATCH 2/4] test: add test --- packages/vitest/src/runtime/runners/test.ts | 2 +- .../test/__snapshots__/repro.test.ts.snap | 3 -- test/core/test/repro.test.ts | 16 ------- .../__snapshots__/repro.test.ts.snap | 5 ++ .../test/fixtures/skip-test/repro.test.ts | 19 ++++++++ .../test/fixtures/skip-test/vitest.config.ts | 3 ++ test/snapshots/test/skip-test.test.ts | 48 +++++++++++++++++++ test/snapshots/vitest.config.ts | 2 + 8 files changed, 78 insertions(+), 20 deletions(-) delete mode 100644 test/core/test/__snapshots__/repro.test.ts.snap delete mode 100644 test/core/test/repro.test.ts create mode 100644 test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap create mode 100644 test/snapshots/test/fixtures/skip-test/repro.test.ts create mode 100644 test/snapshots/test/fixtures/skip-test/vitest.config.ts create mode 100644 test/snapshots/test/skip-test.test.ts diff --git a/packages/vitest/src/runtime/runners/test.ts b/packages/vitest/src/runtime/runners/test.ts index 526842537a26..be2d875f4a17 100644 --- a/packages/vitest/src/runtime/runners/test.ts +++ b/packages/vitest/src/runtime/runners/test.ts @@ -28,7 +28,7 @@ export class VitestTestRunner implements VitestRunner { } async onAfterRunFiles(files?: File[]) { - // mark snapshots in skipped tests as non-obsolete + // mark snapshots in skipped tests as not obsolete // TODO: this probably doesn't work when `VitestTestRunner` are handling multiple files concurrently, // but `snapshotClient.startCurrentRun/finishCurrentRun` might not be working already in that case. for (const test of getTests(files ?? [])) { diff --git a/test/core/test/__snapshots__/repro.test.ts.snap b/test/core/test/__snapshots__/repro.test.ts.snap deleted file mode 100644 index 9384baf7a50a..000000000000 --- a/test/core/test/__snapshots__/repro.test.ts.snap +++ /dev/null @@ -1,3 +0,0 @@ -// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html - -exports[`repro-suite > repro-case 1`] = `"hello"`; diff --git a/test/core/test/repro.test.ts b/test/core/test/repro.test.ts deleted file mode 100644 index b85f2b9eec78..000000000000 --- a/test/core/test/repro.test.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { describe, expect, it } from 'vitest' - -// need at least one snapshot test to trigger `SnapshotClient.startCurrentRun` on current file -it('normal case (pre)', () => { - expect(0).toBe(0) -}) - -describe.skipIf(process.env.REPRO_SKIP)('repro-suite', () => { - it('repro-case', () => { - expect('hello').toMatchSnapshot() - }) -}) - -it('normal case (post)', () => { - expect(0).toBe(0) -}) diff --git a/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap b/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap new file mode 100644 index 000000000000..6a18452da4c4 --- /dev/null +++ b/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap @@ -0,0 +1,5 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`repro suite > inner case 1`] = `"hi"`; + +exports[`top-level case 1`] = `"hi"`; diff --git a/test/snapshots/test/fixtures/skip-test/repro.test.ts b/test/snapshots/test/fixtures/skip-test/repro.test.ts new file mode 100644 index 000000000000..1cd0bf3cfccf --- /dev/null +++ b/test/snapshots/test/fixtures/skip-test/repro.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest' + +const ENABLE_SKIP = process.env.ENABLE_SKIP; + +describe.skipIf(ENABLE_SKIP)('repro suite', () => { + it('inner case', () => { + expect('hi').toMatchSnapshot() + }) +}) + +it.skipIf(ENABLE_SKIP)('top-level case', () => { + expect('hi').toMatchSnapshot() +}) + +// requires at least one non-skipped test to trigger +// `SnapshotClient.startCurrentRun` on current file +it('normal case', () => { + expect(0).toBe(0) +}) diff --git a/test/snapshots/test/fixtures/skip-test/vitest.config.ts b/test/snapshots/test/fixtures/skip-test/vitest.config.ts new file mode 100644 index 000000000000..abed6b2116e1 --- /dev/null +++ b/test/snapshots/test/fixtures/skip-test/vitest.config.ts @@ -0,0 +1,3 @@ +import { defineConfig } from 'vitest/config' + +export default defineConfig({}) diff --git a/test/snapshots/test/skip-test.test.ts b/test/snapshots/test/skip-test.test.ts new file mode 100644 index 000000000000..24ba4dc8156d --- /dev/null +++ b/test/snapshots/test/skip-test.test.ts @@ -0,0 +1,48 @@ +import fs from 'node:fs' +import { expect, test } from 'vitest' +import { runVitest } from '../../test-utils' + +test('snapshots in skipped test/suite is not obsolete', async () => { + // create snapshot on first run + fs.rmSync('test/fixtures/skip-test/__snapshots__', { recursive: true, force: true }) + let vitest = await runVitest({ + root: 'test/fixtures/skip-test', + update: true, + }) + expect(vitest.stdout).toContain('Snapshots 2 written') + expect(fs.readFileSync('test/fixtures/skip-test/__snapshots__/repro.test.ts.snap', 'utf-8')).toMatchInlineSnapshot(` + "// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + + exports[\`repro suite > inner case 1\`] = \`"hi"\`; + + exports[\`top-level case 1\`] = \`"hi"\`; + " + `) + + // running with `skipIf` enabled should not show "obsolete" + vitest = await runVitest({ + root: 'test/fixtures/skip-test', + env: { + ENABLE_SKIP: '1', + }, + }) + expect(vitest.stdout).toContain('2 skipped') + expect(vitest.stdout).not.toContain('obsolete') + + // running with `skipIf` and `update` should keep snapshots + vitest = await runVitest({ + root: 'test/fixtures/skip-test', + update: true, + env: { + ENABLE_SKIP: '1', + }, + }) + expect(fs.readFileSync('test/fixtures/skip-test/__snapshots__/repro.test.ts.snap', 'utf-8')).toMatchInlineSnapshot(` + "// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + + exports[\`repro suite > inner case 1\`] = \`"hi"\`; + + exports[\`top-level case 1\`] = \`"hi"\`; + " + `) +}) diff --git a/test/snapshots/vitest.config.ts b/test/snapshots/vitest.config.ts index 872e76b483a4..5f07a09383fa 100644 --- a/test/snapshots/vitest.config.ts +++ b/test/snapshots/vitest.config.ts @@ -1,8 +1,10 @@ import { defineConfig } from 'vite' +import { defaultExclude } from 'vitest/config' export default defineConfig({ test: { globals: true, + exclude: [...defaultExclude, '**/fixtures'], snapshotFormat: { printBasicPrototype: true, }, From ee72d9d7084a23712afccec123ffd459c8b7a391 Mon Sep 17 00:00:00 2001 From: Hiroshi Ogawa Date: Fri, 22 Dec 2023 17:55:27 +0900 Subject: [PATCH 3/4] chore: minor cleanup --- packages/vitest/src/runtime/runners/test.ts | 4 ++-- .../fixtures/skip-test/__snapshots__/repro.test.ts.snap | 4 ++-- test/snapshots/test/fixtures/skip-test/repro.test.ts | 4 ++-- test/snapshots/test/skip-test.test.ts | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/vitest/src/runtime/runners/test.ts b/packages/vitest/src/runtime/runners/test.ts index be2d875f4a17..a0411a87ae8d 100644 --- a/packages/vitest/src/runtime/runners/test.ts +++ b/packages/vitest/src/runtime/runners/test.ts @@ -29,8 +29,8 @@ export class VitestTestRunner implements VitestRunner { async onAfterRunFiles(files?: File[]) { // mark snapshots in skipped tests as not obsolete - // TODO: this probably doesn't work when `VitestTestRunner` are handling multiple files concurrently, - // but `snapshotClient.startCurrentRun/finishCurrentRun` might not be working already in that case. + // TODO: this probably doesn't work when `VitestTestRunner` is handling multiple files concurrently, + // but `snapshotClient.startCurrentRun/finishCurrentRun` doesn't work in that case already? for (const test of getTests(files ?? [])) { if (test.mode === 'skip') { const name = getNames(test).slice(1).join(' > ') diff --git a/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap b/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap index 6a18452da4c4..b27c3b06ed34 100644 --- a/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap +++ b/test/snapshots/test/fixtures/skip-test/__snapshots__/repro.test.ts.snap @@ -1,5 +1,5 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html -exports[`repro suite > inner case 1`] = `"hi"`; +exports[`repro suite > inner case 1`] = `"hi-1"`; -exports[`top-level case 1`] = `"hi"`; +exports[`top-level case 1`] = `"hi-2"`; diff --git a/test/snapshots/test/fixtures/skip-test/repro.test.ts b/test/snapshots/test/fixtures/skip-test/repro.test.ts index 1cd0bf3cfccf..0dc602fc2277 100644 --- a/test/snapshots/test/fixtures/skip-test/repro.test.ts +++ b/test/snapshots/test/fixtures/skip-test/repro.test.ts @@ -4,12 +4,12 @@ const ENABLE_SKIP = process.env.ENABLE_SKIP; describe.skipIf(ENABLE_SKIP)('repro suite', () => { it('inner case', () => { - expect('hi').toMatchSnapshot() + expect('hi-1').toMatchSnapshot() }) }) it.skipIf(ENABLE_SKIP)('top-level case', () => { - expect('hi').toMatchSnapshot() + expect('hi-2').toMatchSnapshot() }) // requires at least one non-skipped test to trigger diff --git a/test/snapshots/test/skip-test.test.ts b/test/snapshots/test/skip-test.test.ts index 24ba4dc8156d..4db3c2771b63 100644 --- a/test/snapshots/test/skip-test.test.ts +++ b/test/snapshots/test/skip-test.test.ts @@ -13,9 +13,9 @@ test('snapshots in skipped test/suite is not obsolete', async () => { expect(fs.readFileSync('test/fixtures/skip-test/__snapshots__/repro.test.ts.snap', 'utf-8')).toMatchInlineSnapshot(` "// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html - exports[\`repro suite > inner case 1\`] = \`"hi"\`; + exports[\`repro suite > inner case 1\`] = \`"hi-1"\`; - exports[\`top-level case 1\`] = \`"hi"\`; + exports[\`top-level case 1\`] = \`"hi-2"\`; " `) @@ -40,9 +40,9 @@ test('snapshots in skipped test/suite is not obsolete', async () => { expect(fs.readFileSync('test/fixtures/skip-test/__snapshots__/repro.test.ts.snap', 'utf-8')).toMatchInlineSnapshot(` "// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html - exports[\`repro suite > inner case 1\`] = \`"hi"\`; + exports[\`repro suite > inner case 1\`] = \`"hi-1"\`; - exports[\`top-level case 1\`] = \`"hi"\`; + exports[\`top-level case 1\`] = \`"hi-2"\`; " `) }) From 4b480ef0f0c73b257dcc3c442e3579a94dcb4d23 Mon Sep 17 00:00:00 2001 From: Hiroshi Ogawa Date: Thu, 28 Dec 2023 23:37:14 +0900 Subject: [PATCH 4/4] chore: comment --- test/snapshots/test/fixtures/skip-test/repro.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/snapshots/test/fixtures/skip-test/repro.test.ts b/test/snapshots/test/fixtures/skip-test/repro.test.ts index 0dc602fc2277..3d049da5697b 100644 --- a/test/snapshots/test/fixtures/skip-test/repro.test.ts +++ b/test/snapshots/test/fixtures/skip-test/repro.test.ts @@ -12,8 +12,9 @@ it.skipIf(ENABLE_SKIP)('top-level case', () => { expect('hi-2').toMatchSnapshot() }) -// requires at least one non-skipped test to trigger -// `SnapshotClient.startCurrentRun` on current file +// at least one non-skipped test is needed to reproduce a bug. +// without this, there will be no SnapshotClient.startCurrentRun, +// so the code to check skip/obsolete snapshot is not exercised. it('normal case', () => { expect(0).toBe(0) })