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.

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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AriPerkkio
Copy link
Member

Description

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
Copy link
Member Author

AriPerkkio commented Jul 21, 2024

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 AriPerkkio force-pushed the fix/hoist-mocks-syntax-error branch from 4cbdf67 to 05534d9 Compare July 21, 2024 18:05
@AriPerkkio
Copy link
Member Author

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

const declarationNode = getVariableDeclaration(node)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hoisted does not work with istanbul coverage when overriding coverage excludes
1 participant