feat(replay): Use vitest instead of jest
#11899
Conversation
WIP, use vitest instead of jest. Some notes: * Using jsdom 24 to deal with `TextEncoder` issues * Use vi/jest `*Async` fake timer functions instead of `process.nextTick` * Our `useFakeTimers` module was setting an interval which was causing all sorts of flakes in tests with the updated fake timers library
| @@ -73,7 +73,10 @@ | |||
| "@sentry-internal/rrweb": "2.15.0", | |||
| "@sentry-internal/rrweb-snapshot": "2.15.0", | |||
| "fflate": "^0.8.1", | |||
| "jsdom-worker": "^0.2.1" | |||
| "jest-matcher-utils": "^29.0.0", | |||
There was a problem hiding this comment.
Our custom matchers use a util fn from this lib to pretty print diffs.
There was a problem hiding this comment.
I guess we can possibly refactor this later to avoid this dependency, but all good for now!
| @@ -1,17 +1,17 @@ | |||
| import { TextEncoder } from 'util'; | |||
There was a problem hiding this comment.
Now included in jsdom Not sure what dependency resolved it in between jsdom/vitest.
| import { vi } from 'vitest'; | ||
| import type { Assertion, AsymmetricMatchersContaining, Mocked, MockedFunction } from 'vitest'; |
There was a problem hiding this comment.
Wasn't sure what people would prefer, globals vs explicit imports. For this PR I'm just importing vi and types, but can follow-up to make it consistent.
| const saveSessionSpy = jest.fn(); | ||
|
|
||
| jest.mock('../../src/session/saveSession', () => { | ||
| return { | ||
| saveSession: saveSessionSpy, | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
This gets hoisted and yells at you for using vars outside of scope
| @@ -48,8 +50,7 @@ describe('Integration | eventProcessors', () => { | |||
| const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); | |||
|
|
|||
| mockRecord._emitter(TEST_EVENT); | |||
| jest.runAllTimers(); | |||
| jest.advanceTimersByTime(1); | |||
| vi.runAllTimers(); | |||
There was a problem hiding this comment.
I think I'll do a separate PR to update how we use fake timers to remove nextTick calls and change runAllTimers to advancedToNextTimerAsync or runAllTimersAsync
| { | ||
| type: 5, | ||
| timestamp: BASE_TIMESTAMP, | ||
| data: { | ||
| tag: 'breadcrumb', | ||
| payload: { | ||
| timestamp: BASE_TIMESTAMP / 1000, | ||
| type: 'default', | ||
| category: 'console', | ||
| data: { logger: 'replay' }, | ||
| level: 'info', | ||
| message: '[Replay] Creating new session', | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Not sure why old test didn't have this
| beforeAll(function () { | ||
| interval = _setInterval(() => jest.advanceTimersByTime(20), 20); | ||
| }); | ||
| import { vi } from 'vitest'; | ||
|
|
||
| afterAll(function () { | ||
| _clearInterval(interval); | ||
| }); |
There was a problem hiding this comment.
Not sure what this was used for but this was causing all sorts of trouble in the tests.
billyvg
changed the title
vitest instead of jest [WIP]vitest instead of jest
| "fix": "run-s fix:biome fix:eslint", | ||
| "fix:eslint": "eslint . --format stylish --fix", | ||
| "fix:biome": "biome check --apply .", |
There was a problem hiding this comment.
Added this, kept running into lint errors because I only want to yarn fix this package.
Use vitest instead of jest. Some notes: * Use vi/jest `*Async` fake timer functions instead of `process.nextTick` * Our `useFakeTimers` module was setting an interval which was causing all sorts of flakes in tests with the updated fake timers library * removed the interval in `use-fake-timers` module which seemed to be unnecessary and was causing lots of flakes in our tests (also was able to remove all of the random tick values in timestamps due to this)
… detection (#11924) This PR makes sure we use the native, unwrapped `setTimeout` implementation of the browser. Some environments, e.g. Angular, monkey patch this for their change detection, leading to performance issues (possibly). We have already changed this in rrweb, but we also have some usage of this in replay itself. This PR _should_ work fine, however all test fail today because we heavily use `jest.useFakeTimers()`, which basically monkey patches fetch too. So with this change, we do not use the patched timers, leading to everything blowing up 🤯 Based on #11864 Depends on #11899 --------- Co-authored-by: Francesco Novy <francesco.novy@sentry.io>
Use vitest instead of jest. Some notes:
*Asyncfake timer functions instead ofprocess.nextTickuseFakeTimersmodule was setting an interval which was causing all sorts of flakes in tests with the updated fake timers libraryuse-fake-timersmodule which seemed to be unnecessary and was causing lots of flakes in our tests (also was able to remove all of the random tick values in timestamps due to this)From the CI runs I've looked at, doesn't seem like much of a runtime difference.