vitest icon indicating copy to clipboard operation
vitest copied to clipboard

fix(vitest): `vi.hoisted` syntax errors on instrumented code

Open AriPerkkio opened this issue 1 year ago • 2 comments

Description

  • Fixes https://github.com/vitest-dev/vitest/issues/6057

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:.

AriPerkkio avatar Jul 21 '24 15:07 AriPerkkio

Current implementation:

import { expect, it, vi } from 'vitest'

let coverageCounter = 0
const nestedHoist = (coverageCounter++, vi.hoisted(() => {
  return 'Nested hoist'
}))

it('nesting vi.hoisted doesnt cause syntax errors', () => {
  expect(nestedHoist).toBe('Nested hoist')
})

becomes:

const { expect, it, vi } = await __vite_ssr_dynamic_import__("/@fs/x/vitest/packages/vitest/dist/index.js")
vi.hoisted(() => {
  return 'Nested hoist'
})

let coverageCounter = 0;
const nestedHoist = (coverageCounter++, );                  // <-- Syntax error
it("nesting vi.hoisted doesnt cause syntax errors", () => {
  expect(nestedHoist).toBe('Nested hoist');
});

I think correct would be

const { expect, it, vi } = await __vite_ssr_dynamic_import__("/@fs/x/vitest/packages/vitest/dist/index.js")
const nestedHoist = vi.hoisted(() => {
  return 'Nested hoist'
})

let coverageCounter = 0;
coverageCounter++;
it("nesting vi.hoisted doesnt cause syntax errors", () => {
  expect(nestedHoist).toBe('Nested hoist');
});

If implementing this via AST modify becomes too difficult I think we could instead add /* istanbul ignore next */ ignore hints in mocking methods before the instrumentation.

AriPerkkio avatar Jul 21 '24 16:07 AriPerkkio

This is getting quite complex. I think better and more maintainable solution is to:

  • Prevent @vitest/coverage-istanbul from instrumenting vi.mock|hoisted|etc method calls. This is done by adding /* istanbul ignore next */ ignore hints before detected method calls.
  • Throw an error around here if anything else than VariableDeclaration is found, e.g. SequenceExpression

https://github.com/vitest-dev/vitest/blob/073a50c9740f75ba25ec1fabba73b6dae73e66f4/packages/vitest/src/node/hoistMocks.ts#L330

AriPerkkio avatar Jul 26 '24 19:07 AriPerkkio

https://github.com/vitest-dev/vitest/issues/6057#issuecomment-2813403136

AriPerkkio avatar Apr 17 '25 16:04 AriPerkkio