Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(runner): mark tests as failed when beforeAll/afterAll failed #4799

Closed
wants to merge 4 commits into from
Closed

fix(runner): mark tests as failed when beforeAll/afterAll failed #4799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04f49c7
fix(runner): mark tests as failed when beforeAll/afterAll failed
hi-ogawa Dec 23, 2023
5c0cde8
fix: copy errors from suite to child tests
hi-ogawa Dec 23, 2023
31dbb54
test: add test
hi-ogawa Dec 23, 2023
8a3dad6
test: update snapshot
hi-ogawa Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions packages/runner/src/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import limit from 'p-limit'
import { getSafeTimers, shuffle } from '@vitest/utils'
import type { ErrorWithDiff } from '@vitest/utils'
import { processError } from '@vitest/utils/error'
import type { DiffOptions } from '@vitest/utils/diff'
import type { VitestRunner } from './types/runner'
Expand All @@ -8,7 +9,7 @@ import { partitionSuiteChildren } from './utils/suite'
import { getFn, getHooks } from './map'
import { collectTests } from './collect'
import { setCurrentTest } from './test-state'
import { hasFailed, hasTests } from './utils/tasks'
import { getTests, hasFailed, hasTests } from './utils/tasks'
import { PendingError } from './errors'
import { callFixtureCleanup } from './fixture'

Expand Down Expand Up @@ -234,20 +235,31 @@ export async function runTest(test: Test | Custom, runner: VitestRunner) {
updateTask(test, runner)
}

function failTask(result: TaskResult, err: unknown, diffOptions?: DiffOptions) {
function failTask(result: TaskResult, err: unknown, diffOptions?: DiffOptions): ErrorWithDiff[] {
if (err instanceof PendingError) {
result.state = 'skip'
return
return []
}

result.state = 'fail'
const errors = Array.isArray(err)
? err
: [err]
for (const e of errors) {
const error = processError(e, diffOptions)
result.errors ??= []
result.errors.push(error)
const processedErrors = errors.map(e => processError(e, diffOptions))
result.errors = [...result.errors ?? [], ...processedErrors]
return processedErrors
}

function markTasksAsFailed(suite: Suite, errors: ErrorWithDiff[], runner: VitestRunner) {
for (const t of getTests(suite)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like doing this for every test because the terminal seems bloated. We have a suite.result and it's enough. I think reporters should handle this case if they don't distinguish between suite fails and test fails

Screenshot 2023-12-28 at 11 20 33

Copy link
Contributor Author

@hi-ogawa hi-ogawa Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like doing this for every test because the terminal seems bloated

That sounds fair. To explain my perspective, the reason I went with this is that I thought beforeAll/afterAll failures should be more aligned with beforeEach/afterEach failures.

I experimented with current behavior on stackblitz:

https://stackblitz.com/edit/vitest-dev-vitest-m7bw4w?file=test%2Frepro.test.ts

Vitest UI screenshot

image

For example, currently afterAll failure would keep tests as success and I thought that's not an intended behavior.

To avoid cluttering terminal with a single error appearing in all tests, I think I could do something like this while still flagging tests as "fail":

      t.result = {
        ...t.result,
        // flag as "fail"
        state: 'fail',
        // but don't have to copy errors
        // errors: [...t.result?.errors ?? [], ...errors],
      }

What do you think?

EDIT: actually I did this first, then I realized currently junit reporter checks task.result.errors to generate <failure> tags. So, if we went with this, we still require modifying around this code:

vitest/packages/vitest/src/node/reporters/junit.ts

Lines 185 to 188 in 039814b

if (task.result?.state === 'fail') {
const errors = task.result.errors || []
for (const error of errors) {
await this.writeElement('failure', {

Okay, I'll also experiment with approach to fix only on junit reporter side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain my perspective, the reason I went with this is that I thought beforeAll/afterAll failures should be more aligned with beforeEach/afterEach failures.

afterAll/beforeAll are not bound to any test, they are suite hooks. beforeEach/afterEach are test hooks. What I would expect is for tests to have skip state if beforeAll failed (because we don't run any test), and any state if afterAll failed (only the suite is marked as failed)

For example, currently afterAll failure would keep tests as success and I thought that's not an intended behavior.

afterAll fails only a suite which seems correct to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afterAll fails only a suite which seems correct to me.

This is consistent with mocha for example:

import { test, after } from 'mocha'

after(() => {
  throw new Error('after')
})

test('test', () => {})

Although in mocha afterEach doesn't fail the test while in Vitest it does.

If we are making it consistent, then 1) this is definitely a breaking change, 2) we need to decide what is consistent

Copy link
Contributor Author

@hi-ogawa hi-ogawa Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afterAll/beforeAll are not bound to any test, they are suite hooks. beforeEach/afterEach are test hooks.

Totally good point. I didn't have that understanding.

Okay, I'll see what I can do with junit reporter only first. Probably I'll close this PR and create a new one.

Thanks again for the review!

if (t.mode === 'run') {
t.result = {
...t.result,
state: 'fail',
errors: [...t.result?.errors ?? [], ...errors],
}
updateTask(t, runner)
}
}
}

Expand Down Expand Up @@ -316,15 +328,17 @@ export async function runSuite(suite: Suite, runner: VitestRunner) {
}
}
catch (e) {
failTask(suite.result, e, runner.config.diffOptions)
const errors = failTask(suite.result, e, runner.config.diffOptions)
markTasksAsFailed(suite, errors, runner)
}

try {
await callSuiteHook(suite, suite, 'afterAll', runner, [suite])
await callCleanupHooks(beforeAllCleanups)
}
catch (e) {
failTask(suite.result, e, runner.config.diffOptions)
const errors = failTask(suite.result, e, runner.config.diffOptions)
markTasksAsFailed(suite, errors, runner)
}

if (suite.mode === 'run') {
Expand Down
2 changes: 2 additions & 0 deletions test/fails/test/__snapshots__/runner.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ exports[`should fail hook-timeout.test.ts > hook-timeout.test.ts 1`] = `"Error:

exports[`should fail hooks-called.test.ts > hooks-called.test.ts 1`] = `
"Error: after all
Error: before all
Error: after all
Error: before all"
`;

Expand Down
39 changes: 39 additions & 0 deletions test/reporters/fixtures/suite-hook-failure/basic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { afterAll, afterEach, beforeAll, beforeEach, describe, it } from 'vitest'

describe("fail beforeEach", () => {
beforeEach(() => {
throw new Error("fail");
});

it("run", () => {});
it.skip("skip", () => {});
})

describe("fail beforeAll", () => {
beforeAll(() => {
throw new Error("fail");
});

it("run", () => {});
it.skip("skip", () => {});
})

describe("fail afterEach", () => {
afterEach(() => {
throw new Error("fail");
});

it("run", () => {});
it.skip("skip", () => {});
})

describe("fail afterAll", () => {
afterAll(() => {
throw new Error("fail");
});

it("run", () => {});
it.skip("skip", () => {});
})

it("ok", () => {});
14 changes: 13 additions & 1 deletion test/reporters/tests/default.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'pathe'
import { describe, expect, test } from 'vitest'
import { runVitestCli } from '../../test-utils'
import { runVitest, runVitestCli } from '../../test-utils'

const resolve = (id = '') => path.resolve(__dirname, '../fixtures/default', id)
async function run(fileFilter: string[], watch = false, ...args: string[]) {
Expand Down Expand Up @@ -54,4 +54,16 @@ describe('default reporter', async () => {
expect(vitest.stdout).not.toContain('✓ b1 test')
expect(vitest.stdout).not.toContain('✓ b2 test')
})

test('suite hook failure', async () => {
const vitest = await runVitest({
root: 'fixtures/suite-hook-failure',
})
expect(vitest.stdout).toContain('× basic.test.ts > fail beforeEach > run')
expect(vitest.stdout).toContain('× basic.test.ts > fail beforeAll > run')
expect(vitest.stdout).toContain('× basic.test.ts > fail afterEach > run')
expect(vitest.stdout).toContain('× basic.test.ts > fail afterAll > run')
expect(vitest.stdout).toContain('✓ basic.test.ts > ok')
expect(vitest.stdout).toContain('4 failed | 1 passed | 4 skipped')
})
}, 120000)
57 changes: 57 additions & 0 deletions test/reporters/tests/junit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,60 @@ test('emits <failure> if a test has a syntax error', async () => {
expect(xml).toContain('<testsuite name="with-syntax-error.test.js" timestamp="TIMESTAMP" hostname="HOSTNAME" tests="1" failures="1" errors="0" skipped="0" time="0">')
expect(xml).toContain('<failure')
})

test('suite hook failure', async () => {
const vitest = await runVitest({
root: 'fixtures/suite-hook-failure',
reporters: 'junit',
})
let xml = vitest.stdout
xml = xml.replaceAll(/time=".*?"/g, 'time="..."')
xml = xml.replaceAll(/timestamp=".*?"/g, 'timestamp="..."')
xml = xml.replaceAll(/hostname=".*?"/g, 'hostname="..."')
expect(xml).toMatchInlineSnapshot(`
"<?xml version="1.0" encoding="UTF-8" ?>
<testsuites name="vitest tests" tests="9" failures="4" errors="0" time="...">
<testsuite name="basic.test.ts" timestamp="..." hostname="..." tests="9" failures="4" errors="0" skipped="4" time="...">
<testcase classname="basic.test.ts" name="fail beforeEach &gt; run" time="...">
<failure message="fail" type="Error">
Error: fail
❯ basic.test.ts:5:11
</failure>
</testcase>
<testcase classname="basic.test.ts" name="fail beforeEach &gt; skip" time="...">
<skipped/>
</testcase>
<testcase classname="basic.test.ts" name="fail beforeAll &gt; run" time="...">
<failure message="fail" type="Error">
Error: fail
❯ basic.test.ts:14:11
</failure>
</testcase>
<testcase classname="basic.test.ts" name="fail beforeAll &gt; skip" time="...">
<skipped/>
</testcase>
<testcase classname="basic.test.ts" name="fail afterEach &gt; run" time="...">
<failure message="fail" type="Error">
Error: fail
❯ basic.test.ts:23:11
</failure>
</testcase>
<testcase classname="basic.test.ts" name="fail afterEach &gt; skip" time="...">
<skipped/>
</testcase>
<testcase classname="basic.test.ts" name="fail afterAll &gt; run" time="...">
<failure message="fail" type="Error">
Error: fail
❯ basic.test.ts:32:11
</failure>
</testcase>
<testcase classname="basic.test.ts" name="fail afterAll &gt; skip" time="...">
<skipped/>
</testcase>
<testcase classname="basic.test.ts" name="ok" time="...">
</testcase>
</testsuite>
</testsuites>
"
`)
})