fixes & tests

Author: mydea

packages/replay/src/util/addEvent.ts

@@ -35,13 +35,13 @@ export function addEventSync(replay: ReplayContainer, event: RecordingEvent, isC
  *
  * `isCheckout` is true if this is either the very first event, or an event triggered by `checkoutEveryNms`.
  */
-export async function addEvent(
+export function addEvent(
   replay: ReplayContainer,
   event: RecordingEvent,
   isCheckout?: boolean,
 ): Promise<AddEventResult | null> {
   if (!shouldAddEvent(replay, event)) {
-    return null;
+    return Promise.resolve(null);
   }
 
   return _addEvent(replay, event, isCheckout);
@@ -90,7 +90,6 @@ async function _addEvent(
 
 function shouldAddEvent(replay: ReplayContainer, event: RecordingEvent): boolean {
   if (!replay.eventBuffer || replay.isPaused() || !replay.isEnabled()) {
-    // This implies that `_isEnabled` is false
     return false;
   }
 

packages/replay/src/util/handleRecordingEmit.ts

@@ -2,8 +2,8 @@ import { EventType } from '@sentry-internal/rrweb';
 import { logger } from '@sentry/utils';
 
 import { saveSession } from '../session/saveSession';
-import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types';
-import { addEvent, addEventSync } from './addEvent';
+import type { RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types';
+import { addEventSync } from './addEvent';
 import { logInfo } from './log';
 
 type RecordingEmitCallback = (event: RecordingEvent, isCheckout?: boolean) => void;
@@ -59,7 +59,7 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
       //
       // `isCheckout` is always true, but want to be explicit that it should
       // only be added for checkouts
-      void addSettingsEvent(replay, isCheckout);
+      addSettingsEvent(replay, isCheckout);
 
       // If there is a previousSessionId after a full snapshot occurs, then
       // the replay session was started due to session expiration. The new session
@@ -133,11 +133,11 @@ export function createOptionsEvent(replay: ReplayContainer): ReplayOptionFrameEv
  * Add a "meta" event that contains a simplified view on current configuration
  * options. This should only be included on the first segment of a recording.
  */
-function addSettingsEvent(replay: ReplayContainer, isCheckout?: boolean): Promise<AddEventResult | null> {
+function addSettingsEvent(replay: ReplayContainer, isCheckout?: boolean): void {
   // Only need to add this event when sending the first segment
   if (!isCheckout || !replay.session || replay.session.segmentId !== 0) {
-    return Promise.resolve(null);
+    return;
   }
 
-  return addEvent(replay, createOptionsEvent(replay), false);
+  addEventSync(replay, createOptionsEvent(replay), false);
 }

packages/replay/test/integration/errorSampleRate.test.ts

@@ -816,73 +816,6 @@ describe('Integration | errorSampleRate', () => {
     expect(replay).not.toHaveLastSentReplay();
   });
 
-  it('does not stop replay based on earliest event in buffer', async () => {
-    jest.setSystemTime(BASE_TIMESTAMP);
-
-    const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP - 60000 });
-    mockRecord._emitter(TEST_EVENT);
-
-    expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled();
-    expect(replay).not.toHaveLastSentReplay();
-
-    jest.runAllTimers();
-    await new Promise(process.nextTick);
-
-    expect(replay).not.toHaveLastSentReplay();
-    captureException(new Error('testing'));
-
-    await waitForBufferFlush();
-
-    expect(replay).toHaveLastSentReplay();
-
-    // Flush from calling `stopRecording`
-    await waitForFlush();
-
-    // Now wait after session expires - should stop recording
-    mockRecord.takeFullSnapshot.mockClear();
-    (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>).mockClear();
-
-    expect(replay).not.toHaveLastSentReplay();
-
-    const TICKS = 80;
-
-    // We advance time so that we are on the border of expiring, taking into
-    // account that TEST_EVENT timestamp is 60000 ms before BASE_TIMESTAMP. The
-    // 3 DEFAULT_FLUSH_MIN_DELAY is to account for the `waitForFlush` that has
-    // happened, and for the next two that will happen. The first following
-    // `waitForFlush` does not expire session, but the following one will.
-    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION - 60000 - 3 * DEFAULT_FLUSH_MIN_DELAY - TICKS);
-    await new Promise(process.nextTick);
-
-    mockRecord._emitter(TEST_EVENT);
-    expect(replay).not.toHaveLastSentReplay();
-    await waitForFlush();
-
-    expect(replay).not.toHaveLastSentReplay();
-    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
-    expect(replay.isEnabled()).toBe(true);
-
-    mockRecord._emitter(TEST_EVENT);
-    expect(replay).not.toHaveLastSentReplay();
-    await waitForFlush();
-
-    expect(replay).not.toHaveLastSentReplay();
-    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
-    expect(replay.isEnabled()).toBe(true);
-
-    // It's hard to test, but if we advance the below time less 1 ms, it should
-    // be enabled, but we can't trigger a session check via flush without
-    // incurring another DEFAULT_FLUSH_MIN_DELAY timeout.
-    jest.advanceTimersByTime(60000 - DEFAULT_FLUSH_MIN_DELAY);
-    mockRecord._emitter(TEST_EVENT);
-    expect(replay).not.toHaveLastSentReplay();
-    await waitForFlush();
-
-    expect(replay).not.toHaveLastSentReplay();
-    expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0);
-    expect(replay.isEnabled()).toBe(false);
-  });
-
   it('handles very long active buffer session', async () => {
     const stepDuration = 10_000;
     const steps = 5_000;

packages/replay/test/integration/flush.test.ts

@@ -403,6 +403,8 @@ describe('Integration | flush', () => {
   it('logs warning if adding event that is after maxReplayDuration', async () => {
     replay.getOptions()._experiments.traceInternals = true;
 
+    const spyLogger = jest.spyOn(SentryUtils.logger, 'info');
+
     sessionStorage.clear();
     clearSession(replay);
     replay['_initializeSessionForSampling']();
@@ -422,32 +424,18 @@ describe('Integration | flush', () => {
     // no checkout!
     await advanceTimers(DEFAULT_FLUSH_MIN_DELAY);
 
-    expect(mockFlush).toHaveBeenCalledTimes(1);
-    expect(mockSendReplay).toHaveBeenCalledTimes(1);
-
-    const replayData = mockSendReplay.mock.calls[0][0];
+    // No flush is scheduled is aborted because event is after maxReplayDuration
+    expect(mockFlush).toHaveBeenCalledTimes(0);
+    expect(mockSendReplay).toHaveBeenCalledTimes(0);
 
-    expect(JSON.parse(replayData.recordingData)).toEqual([
-      {
-        type: 5,
-        timestamp: BASE_TIMESTAMP,
-        data: {
-          tag: 'breadcrumb',
-          payload: {
-            timestamp: BASE_TIMESTAMP / 1000,
-            type: 'default',
-            category: 'console',
-            data: { logger: 'replay' },
-            level: 'info',
-            message: `[Replay] Skipping event with timestamp ${
-              BASE_TIMESTAMP + MAX_REPLAY_DURATION + 100
-            } because it is after maxReplayDuration`,
-          },
-        },
-      },
-    ]);
+    expect(spyLogger).toHaveBeenLastCalledWith(
+      `[Replay] Skipping event with timestamp ${
+        BASE_TIMESTAMP + MAX_REPLAY_DURATION + 100
+      } because it is after maxReplayDuration`,
+    );
 
     replay.getOptions()._experiments.traceInternals = false;
+    spyLogger.mockRestore();
   });
 
   /**

packages/replay/test/unit/util/handleRecordingEmit.test.ts

@@ -16,8 +16,8 @@ describe('Unit | util | handleRecordingEmit', () => {
 
   beforeEach(function () {
     jest.setSystemTime(BASE_TIMESTAMP);
-    addEventMock = jest.spyOn(SentryAddEvent, 'addEvent').mockImplementation(async () => {
-      // Do nothing
+    addEventMock = jest.spyOn(SentryAddEvent, 'addEventSync').mockImplementation(() => {
+      return true;
     });
   });