Skip to content

Migrate from jest to vitest and stop using jsdom based tests #11084

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

Closed
AbhiPrasad opened this issue Mar 13, 2024 · 10 comments · Fixed by #15549
Closed

Migrate from jest to vitest and stop using jsdom based tests #11084

AbhiPrasad opened this issue Mar 13, 2024 · 10 comments · Fixed by #15549
Assignees

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 13, 2024

After v8 gets merged in, let's look at migrating from jest to vitest. vitest is way faster, and doesn't fall into the same traps jest does in terms of esm compatibility. Example attempt with Vue SDK: #11071

In addition, we should stop using jsdom based tests, and instead move tests that rely on the browser to use playwright instead. Simulating jsdom in an node environment always has it's faults, easier to not attempt to do that.

First we should align on a common vitest testing standard. Right now we use set globals via vitest/globals, but this is not recommended by vitest themselves. We should instead use direct imports like import { describe, test, expect } from 'vitest';.

"types": ["node", "jest", "vitest/globals"]

There's probably some other discussion that needs to be done here too. We can validate our decisions by changing our vitest usage in the Astro and SvelteKit SDKs.

### Update existing vitest usage
- [ ] https://github.com/getsentry/sentry-javascript/pull/13093
- [ ] https://github.com/getsentry/sentry-javascript/pull/13214
- [ ] Nuxt SDK
- [ ] https://github.com/getsentry/sentry-javascript/pull/13028
- [ ] SolidStart SDK
- [ ] https://github.com/getsentry/sentry-javascript/pull/13026
- [ ] https://github.com/getsentry/sentry-javascript/pull/12960
- [ ] https://github.com/getsentry/sentry-javascript/pull/13027

We can then tackle the following list in whatever order we want!

### Convert to using vitest
- [x] Angular https://github.com/getsentry/sentry-javascript/pull/11091
- [ ] https://github.com/getsentry/sentry-javascript/pull/13092
- [ ] https://github.com/getsentry/sentry-javascript/issues/13434
- [x] Browser Utils
- [ ] https://github.com/getsentry/sentry-javascript/pull/15497
- [ ] https://github.com/getsentry/sentry-javascript/pull/15494
- [ ] https://github.com/getsentry/sentry-javascript/issues/13605
- [ ] https://github.com/getsentry/sentry-javascript/pull/15492
- [ ] https://github.com/getsentry/sentry-javascript/pull/12964
- [ ] https://github.com/getsentry/sentry-javascript/pull/11899
- [ ] https://github.com/getsentry/sentry-javascript/pull/12961
- [ ] https://github.com/getsentry/sentry-javascript/pull/12958
- [ ] https://github.com/getsentry/sentry-javascript/pull/12955
- [x] Wasm
- [ ] https://github.com/getsentry/sentry-javascript/pull/15499
- [ ] https://github.com/getsentry/sentry-javascript/pull/15546

We also have some packages that use mocha. We should try to drop that dependency and make them use vitest

### Remove mocha
- [ ] https://github.com/getsentry/sentry-javascript/pull/11296
- [ ] https://github.com/getsentry/sentry-javascript/pull/11412
- [ ] https://github.com/getsentry/sentry-javascript/pull/11436
- [ ] https://github.com/getsentry/sentry-javascript/pull/11449
- [ ] https://github.com/getsentry/sentry-javascript/pull/11666
- [ ] https://github.com/getsentry/sentry-javascript/pull/11688
- [ ] https://github.com/getsentry/sentry-javascript/pull/11733
- [ ] https://github.com/getsentry/sentry-javascript/pull/11758
@AbhiPrasad
Copy link
Member Author

ref #6040

@s1gr1d
Copy link
Member

s1gr1d commented Mar 22, 2024

vistest was already added to Angular here: #11091
I will check it on the list 👍🏻

AbhiPrasad added a commit that referenced this issue Mar 27, 2024
ref #11084

Removes usage of `mocha` from eslint plugin
AbhiPrasad added a commit that referenced this issue Apr 4, 2024
#11412)

I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 1:
`packages/browser/test/integration/suites/api.js`

We have decent coverage around singular calls to these methods, so I
just added the tests that validate calling `captureException` and
`captureMessage` multiple times.

I also fixed a spelling mistake with
`dev-packages/browser-integration-tests/suites/public-api/captureException/linkedErrors/subject.js`,
`linkedErrrors` -> `linkedErrors`

ref #11084
AbhiPrasad added a commit that referenced this issue Apr 5, 2024
I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 2:
`packages/browser/test/integration/suites/breadcrumbs.js`

This adds console and history breadcrumb tests (which didn't exist
before), and expands upon the dom and xhr/fetch tests, and cleans up
some code here and there as well.

ref #11084
day 1: #11412
AbhiPrasad added a commit that referenced this issue Apr 8, 2024
I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 3:
`packages/browser/test/integration/suites/config.js`

I was surprised we never had `ignoreErrors` or `denyUrls` tests in
playwright, so it's good to get the confidence that everything works
here.

ref #11084
day 2: #11436
AbhiPrasad added a commit that referenced this issue Apr 19, 2024
This ports `packages/browser/test/integration/suites/loader.js` and
`packages/browser/test/integration/suites/loader-specific.js` to
playwright. Specifically it just adds tests for SDK source and
breadcrumbs, which were missing previously.

ref #11084
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
ref getsentry#11084

Removes usage of `mocha` from eslint plugin
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
getsentry#11412)

I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 1:
`packages/browser/test/integration/suites/api.js`

We have decent coverage around singular calls to these methods, so I
just added the tests that validate calling `captureException` and
`captureMessage` multiple times.

I also fixed a spelling mistake with
`dev-packages/browser-integration-tests/suites/public-api/captureException/linkedErrors/subject.js`,
`linkedErrrors` -> `linkedErrors`

ref getsentry#11084
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 2:
`packages/browser/test/integration/suites/breadcrumbs.js`

This adds console and history breadcrumb tests (which didn't exist
before), and expands upon the dom and xhr/fetch tests, and cleans up
some code here and there as well.

ref getsentry#11084
day 1: getsentry#11412
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
…ry#11449)

I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 3:
`packages/browser/test/integration/suites/config.js`

I was surprised we never had `ignoreErrors` or `denyUrls` tests in
playwright, so it's good to get the confidence that everything works
here.

ref getsentry#11084
day 2: getsentry#11436
AbhiPrasad added a commit that referenced this issue Apr 24, 2024
ref #11084

This test ports
`packages/browser/test/integration/suites/onunhandledrejection.js`
playwright. Because of the same limitations as outlined with the on
error tests #11666, I
had to use calls to `window.onunhandledrejection` to simulate these
tests instead of just using `Promise.reject` to test the handler.

#11678 tracks being
able to fix this so we can avoid directly calling
`window.onunhandledrejection` to test.

As `onunhandledrejection.js` was the last suite to use the old
integration tests, I fully removed that code and the corresponding GH
action workflow. I also removed the monorepo deps on `karma`, `chai` and
`sinon`. Extremely satisfying.
AbhiPrasad added a commit that referenced this issue Jul 17, 2024
Before: `Time: 2.621 s`

After: `Duration 638ms (transform 343ms, setup 0ms, collect 1.61s, tests
22ms, environment 0ms, prepare 278ms)`

ref #11084

Also removes `edge-runtime/jest-environment` which we weren't using
anyway, removes a lot of unnecessary stuff in our `yarn.lock` (-151
lines!)
@billyvg
Copy link
Member

billyvg commented Jul 17, 2024

@AbhiPrasad Replay was migrated here: #11899

edit though we are using globals and not importing everything from vitest

AbhiPrasad added a commit that referenced this issue Aug 6, 2024
As per https://vitest.dev/config/#globals

> By default, vitest does not provide global APIs for explicitness

I think we should follow vitest defaults here and explicitly import in
the APIs that we need. This refactors our Nestjs SDK tests to do so.

ref #11084
@timfish
Copy link
Collaborator

timfish commented Sep 6, 2024

Looks like the task list needs updating because Node v14 doesn't support vitest?

@AbhiPrasad
Copy link
Member Author

I updated it - looks like we are stuck with jest longer than I thought.

@timfish
Copy link
Collaborator

timfish commented Feb 28, 2025

Almost everything is now migrated to Vitest.

There are a couple of packages where I've created branches to migrate them but got stuck on some blockers:

@sentry/remix

Branch - https://github.com/getsentry/sentry-javascript/tree/timfish/test/vitest-remix

  • upload-sourcemaps.test.ts tests the the files in ./scripts/** which are all CJS and Vitest cannot not mock require so they can't be tested directly with Vitest

Maybe these files should be in TypeScript and transpiled to the output directory anyway?

@sentry/google-cloud-serverless

Branch - https://github.com/getsentry/sentry-javascript/tree/timfish/test/vitest-google

  • google-cloud-grpc.test.ts test fails with UNAUTHENTICATED: Request had invalid authentication credentials
  • Looks like dns isn't getting mocked because it hits Google and gets an auth error?

@sentry/nextjs

Branch - https://github.com/getsentry/sentry-javascript/tree/timfish/test/vitest-nextjs

  • constructWebpackConfig.test.ts failures because mocks don't appear to be working and other strange behaviour

@AbhiPrasad
Copy link
Member Author

upload-sourcemaps.test.ts tests the the files in ./scripts/** which are all CJS and Vitest cannot not mock require so they can't be tested directly with Vitest

maybe you can try the hack I added for gatsby that patches Module.load to do CJS patching:

vi.hoisted(
() =>
void mock('@sentry/webpack-plugin', {
sentryWebpackPlugin: vi.fn().mockReturnValue({}),
}),
);
// Need to override mock because `gatsby-node.js` loads `@sentry/webpack-plugin` as a CJS file.
async function mock(mockedUri: string, stub: any) {
const { Module } = await import('module');
// @ts-expect-error test
Module._load_original = Module._load;
// @ts-expect-error test
Module._load = (uri, parent) => {
if (uri === mockedUri) return stub;
// @ts-expect-error test
return Module._load_original(uri, parent);
};
}
import { onCreateWebpackConfig } from '../gatsby-node';
describe('onCreateWebpackConfig', () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { sentryWebpackPlugin } = require('@sentry/webpack-plugin');

Looks like dns isn't getting mocked because it hits Google and gets an auth error?

These tests are so hard to debug 😬

I'd be open to try to just completely rewrite them.

constructWebpackConfig.test.ts failures because mocks don't appear to be working and other strange behaviour

let's open a draft PR for this one, we can ask Luca and Charly to help take a look given they have more experience with these tests.

@AbhiPrasad
Copy link
Member Author

I took another look at @sentry/google-cloud-serverless and I think we have two options

  1. test using the gcp simulators, and switch to using node integration tests + docker images
  2. mock out the PubSub object entirely:

I asked Claude to give me a mock and here's what it provided:

// Mock the entire PubSub module
vi.mock('@google-cloud/pubsub', () => {
  return {
    PubSub: vi.fn().mockImplementation(() => ({
      topic: vi.fn().mockImplementation(topicName => ({
        publish: vi.fn().mockImplementation(() => {
          return Promise.resolve('1637084156623860');
        }),
        publishMessage: vi.fn().mockImplementation(() => {
          return Promise.resolve('1637084156623860');
        }),
        name: `projects/project-id/topics/${topicName}`,
      })),
      close: vi.fn().mockResolvedValue(undefined),
    })),
  };
});

@timfish
Copy link
Collaborator

timfish commented Mar 4, 2025

Our instrumentation hooks the google-gax module. If you mock out the PubSub object, it'll never hit any of the google-gax code and the instrumentation does not get tested.

I'm OOO in an hour so I've opened a PR with everything migrated apart from this test which is still failing:

AbhiPrasad pushed a commit that referenced this issue Mar 4, 2025
- Ref #11084

`constructWebpackConfig.test.ts` still has test failures. The mocks
don't appear to be working or other strange behaviour.

---------

Co-authored-by: Charly Gomez <charly.gomez@sentry.io>
@AbhiPrasad
Copy link
Member Author

We should write a engineering blog post about this https://sentry.engineering/

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

Successfully merging a pull request may close this issue.

5 participants