Skip to content

Commit f1af9e1

Browse files
authored
test(browser): Fix browserTracingIntegration unit test (#20604)
After seeing this flake, I looked into this and found this to be fundamentally flawed. The reason being, that while we used fake timers, it turns out that the pageload span logic does not use `Date.now()` as start date, but instead it uses the browser origin. So depending on how/when the test ran, the timing was actually off and we did not really test what we wanted to. Also, we lacked a unit test for the redirect case here. I added that as well and ensures that this properly tests what we want. Fixes #20391
1 parent ae8ff88 commit f1af9e1

1 file changed

Lines changed: 79 additions & 49 deletions

File tree

‎packages/browser/test/tracing/browserTracingIntegration.test.ts‎

Lines changed: 79 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ import {
1717
spanToJSON,
1818
startInactiveSpan,
1919
TRACING_DEFAULTS,
20+
browserPerformanceTimeOrigin,
21+
getSpanDescendants,
2022
} from '@sentry/core';
2123
import { JSDOM } from 'jsdom';
2224
import { TextDecoder, TextEncoder } from 'util';
@@ -58,6 +60,8 @@ const originalGlobalHistory = WINDOW.history;
5860
describe('browserTracingIntegration', () => {
5961
beforeEach(() => {
6062
vi.useFakeTimers();
63+
// Ensure start time aligns with cached origin time, which is used as pageload start time
64+
vi.setSystemTime(browserPerformanceTimeOrigin()!);
6165
getCurrentScope().clear();
6266
getIsolationScope().clear();
6367
getCurrentScope().setClient(undefined);
@@ -228,11 +232,11 @@ describe('browserTracingIntegration', () => {
228232
setCurrentClient(client);
229233
client.init();
230234

231-
const span = getActiveSpan();
235+
const span = getActiveSpan()!;
232236
expect(span).toBeDefined();
233-
expect(spanIsSampled(span!)).toBe(true);
234-
expect(span!.isRecording()).toBe(true);
235-
expect(spanToJSON(span!)).toEqual({
237+
expect(spanIsSampled(span)).toBe(true);
238+
expect(span.isRecording()).toBe(true);
239+
expect(spanToJSON(span)).toEqual({
236240
description: '/',
237241
op: 'pageload',
238242
origin: 'auto.pageload.browser',
@@ -254,13 +258,13 @@ describe('browserTracingIntegration', () => {
254258
vi.advanceTimersByTime(1600);
255259
WINDOW.history.pushState({}, '', '/test');
256260

257-
expect(span!.isRecording()).toBe(false);
261+
expect(span.isRecording()).toBe(false);
258262

259-
const span2 = getActiveSpan();
263+
const span2 = getActiveSpan()!;
260264
expect(span2).toBeDefined();
261-
expect(spanIsSampled(span2!)).toBe(true);
262-
expect(span2!.isRecording()).toBe(true);
263-
expect(spanToJSON(span2!)).toEqual({
265+
expect(spanIsSampled(span2)).toBe(true);
266+
expect(span2.isRecording()).toBe(true);
267+
expect(spanToJSON(span2)).toEqual({
264268
description: '/test',
265269
op: 'navigation',
266270
origin: 'auto.navigation.browser',
@@ -290,15 +294,16 @@ describe('browserTracingIntegration', () => {
290294
const dom2 = new JSDOM(undefined, { url: 'https://example.com/test2' });
291295
Object.defineProperty(global, 'location', { value: dom2.window.document.location, writable: true });
292296

297+
vi.advanceTimersByTime(1600);
293298
WINDOW.history.pushState({}, '', '/test2');
294299

295-
expect(span2!.isRecording()).toBe(false);
300+
expect(span2.isRecording()).toBe(false);
296301

297-
const span3 = getActiveSpan();
302+
const span3 = getActiveSpan()!;
298303
expect(span3).toBeDefined();
299-
expect(spanIsSampled(span3!)).toBe(true);
300-
expect(span3!.isRecording()).toBe(true);
301-
expect(spanToJSON(span3!)).toEqual({
304+
expect(spanIsSampled(span3)).toBe(true);
305+
expect(span3.isRecording()).toBe(true);
306+
expect(spanToJSON(span3)).toEqual({
302307
description: '/test2',
303308
op: 'navigation',
304309
origin: 'auto.navigation.browser',
@@ -325,6 +330,66 @@ describe('browserTracingIntegration', () => {
325330
});
326331
});
327332

333+
it('starts redirect when URL changes after < 1.5s', () => {
334+
const client = new BrowserClient(
335+
getDefaultBrowserClientOptions({
336+
tracesSampleRate: 1,
337+
integrations: [browserTracingIntegration()],
338+
}),
339+
);
340+
setCurrentClient(client);
341+
client.init();
342+
343+
const span = getActiveSpan()!;
344+
expect(span).toBeDefined();
345+
expect(spanIsSampled(span)).toBe(true);
346+
expect(span.isRecording()).toBe(true);
347+
expect(spanToJSON(span)).toEqual({
348+
description: '/',
349+
op: 'pageload',
350+
origin: 'auto.pageload.browser',
351+
data: {
352+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
353+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
354+
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
355+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
356+
},
357+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
358+
start_timestamp: expect.any(Number),
359+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
360+
});
361+
362+
// this is what is used to get the span name - JSDOM does not update this on it's own!
363+
const dom = new JSDOM(undefined, { url: 'https://example.com/test' });
364+
Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true });
365+
366+
vi.advanceTimersByTime(100);
367+
WINDOW.history.pushState({}, '', '/test');
368+
369+
expect(span.isRecording()).toBe(true);
370+
371+
const span2 = getActiveSpan()!;
372+
expect(span2).toBeDefined();
373+
374+
// span is still active now
375+
expect(getActiveSpan()).toBe(span);
376+
377+
// span has connected redirect span
378+
expect(getSpanDescendants(span).map(span => spanToJSON(span))).toContainEqual(
379+
expect.objectContaining({
380+
data: {
381+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation.redirect',
382+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser',
383+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
384+
},
385+
description: '/test',
386+
op: 'navigation.redirect',
387+
origin: 'auto.navigation.browser',
388+
parent_span_id: span.spanContext().spanId,
389+
}),
390+
);
391+
});
392+
328393
describe('startBrowserTracingPageLoadSpan', () => {
329394
it('works without integration setup', () => {
330395
const client = new BrowserClient(
@@ -1301,7 +1366,6 @@ describe('browserTracingIntegration', () => {
13011366

13021367
describe('idleTimeout', () => {
13031368
it('is created by default', () => {
1304-
vi.useFakeTimers();
13051369
const client = new BrowserClient(
13061370
getDefaultBrowserClientOptions({
13071371
tracesSampleRate: 1,
@@ -1336,8 +1400,6 @@ describe('browserTracingIntegration', () => {
13361400
});
13371401

13381402
it('can be a custom value', () => {
1339-
vi.useFakeTimers();
1340-
13411403
const client = new BrowserClient(
13421404
getDefaultBrowserClientOptions({
13431405
tracesSampleRate: 1,
@@ -1440,36 +1502,4 @@ describe('browserTracingIntegration', () => {
14401502
expect(spanJson2.links).toBeUndefined();
14411503
});
14421504
});
1443-
1444-
// TODO(lforst): I cannot manage to get this test to pass.
1445-
/*
1446-
it('heartbeatInterval can be a custom value', () => {
1447-
vi.useFakeTimers();
1448-
1449-
const interval = 200;
1450-
1451-
const client = new BrowserClient(
1452-
getDefaultBrowserClientOptions({
1453-
tracesSampleRate: 1,
1454-
integrations: [browserTracingIntegration({ heartbeatInterval: interval })],
1455-
}),
1456-
);
1457-
1458-
setCurrentClient(client);
1459-
client.init();
1460-
1461-
const mockFinish = vi.fn();
1462-
// eslint-disable-next-line deprecation/deprecation
1463-
const transaction = getActiveTransaction() as IdleTransaction;
1464-
transaction.sendAutoFinishSignal();
1465-
transaction.end = mockFinish;
1466-
1467-
const span = startInactiveSpan({ name: 'child-span' }); // activities = 1
1468-
span!.end(); // activities = 0
1469-
1470-
expect(mockFinish).toHaveBeenCalledTimes(0);
1471-
vi.advanceTimersByTime(interval * 3);
1472-
expect(mockFinish).toHaveBeenCalledTimes(1);
1473-
});
1474-
*/
14751505
});

0 commit comments

Comments
 (0)