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

[hi-ogawa]

Description

Closes #4516

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• Ideally, include a test that fails without this PR but passes with it.
• Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

• Run the tests with pnpm test:ci.

Documentation

• If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

• Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[netlify]

✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name	Link
🔨 Latest commit	8a3dad6
🔍 Latest deploy log	https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65865837d35cb20008288da5

[sheremet-va]

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

[hi-ogawa]

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

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.

[sheremet-va]

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.

[sheremet-va]

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

[hi-ogawa]

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!

[hi-ogawa]

I'll closed this PR in favor of #4819.

---

Within the discussion, you mentioned #4799 (comment)

What I would expect is for tests to have skip state if beforeAll failed (because we don't run any test)

and I agree that makes sense. I'll raise a dedicated issue for this, so it'll be discussed and handled separately.