From 5232b8d001625e5ea5deaaa62cbb1141848f6e68 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Apr 2022 14:00:53 +0200 Subject: [PATCH 01/17] add first PoC implementation for Transport injection --- packages/browser/src/client.ts | 13 +++-- packages/browser/src/sdk.ts | 4 +- packages/browser/src/transports/index.ts | 2 + packages/browser/src/transports/setup.ts | 62 ++++++++++++++++++++++++ packages/core/src/baseclient.ts | 8 ++- packages/core/src/sdk.ts | 19 ++++++-- packages/core/src/transports/base.ts | 3 +- packages/core/test/mocks/client.ts | 1 + packages/node/src/client.ts | 14 ++++-- packages/node/src/sdk.ts | 5 +- packages/node/src/transports/index.ts | 1 + packages/node/src/transports/setup.ts | 49 +++++++++++++++++++ 12 files changed, 165 insertions(+), 16 deletions(-) create mode 100644 packages/browser/src/transports/setup.ts create mode 100644 packages/node/src/transports/setup.ts diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 5116df6104d1..1db9800d8102 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -1,4 +1,11 @@ -import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, initAPIDetails, Scope, SDK_VERSION } from '@sentry/core'; +import { + BaseClient, + getEnvelopeEndpointWithUrlEncodedAuth, + initAPIDetails, + NewTransport, + Scope, + SDK_VERSION, +} from '@sentry/core'; import { Event, EventHint, Options, Severity, SeverityLevel, Transport, TransportOptions } from '@sentry/types'; import { getGlobalObject, logger, stackParserFromOptions, supportsFetch } from '@sentry/utils'; @@ -40,7 +47,7 @@ export class BrowserClient extends BaseClient { * * @param options Configuration options for this SDK. */ - public constructor(options: BrowserOptions = {}) { + public constructor(options: BrowserOptions = {}, transport: Transport, newTransport?: NewTransport) { options._metadata = options._metadata || {}; options._metadata.sdk = options._metadata.sdk || { name: 'sentry.javascript.browser', @@ -53,7 +60,7 @@ export class BrowserClient extends BaseClient { version: SDK_VERSION, }; - super(options); + super(options, transport, newTransport); } /** diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 60c53b364e9c..94ec87cb4279 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -7,6 +7,7 @@ import { IS_DEBUG_BUILD } from './flags'; import { ReportDialogOptions, wrap as internalWrap } from './helpers'; import { Breadcrumbs, Dedupe, GlobalHandlers, LinkedErrors, TryCatch, UserAgent } from './integrations'; import { defaultStackParsers } from './stack-parsers'; +import { setupBrowserTransport } from './transports/setup'; export const defaultIntegrations = [ new CoreIntegrations.InboundFilters(), @@ -97,7 +98,8 @@ export function init(options: BrowserOptions = {}): void { options.stackParser = defaultStackParsers; } - initAndBind(BrowserClient, options); + const { transport, newTransport } = setupBrowserTransport(options); + initAndBind(BrowserClient, options, transport, newTransport); if (options.autoSessionTracking) { startSessionTracking(); diff --git a/packages/browser/src/transports/index.ts b/packages/browser/src/transports/index.ts index 287e14e0ac50..31871a76d01c 100644 --- a/packages/browser/src/transports/index.ts +++ b/packages/browser/src/transports/index.ts @@ -4,3 +4,5 @@ export { XHRTransport } from './xhr'; export { makeNewFetchTransport } from './new-fetch'; export { makeNewXHRTransport } from './new-xhr'; + +export { setupBrowserTransport } from './setup'; diff --git a/packages/browser/src/transports/setup.ts b/packages/browser/src/transports/setup.ts new file mode 100644 index 000000000000..a639418613aa --- /dev/null +++ b/packages/browser/src/transports/setup.ts @@ -0,0 +1,62 @@ +import { + BaseTransportOptions, + getEnvelopeEndpointWithUrlEncodedAuth, + initAPIDetails, + NewTransport, + NoopTransport, +} from '@sentry/core'; +import { Transport, TransportOptions } from '@sentry/types'; +import { supportsFetch } from '@sentry/utils'; + +import { BrowserOptions } from '../client'; +import { FetchTransport } from './fetch'; +import { makeNewFetchTransport } from './new-fetch'; +import { makeNewXHRTransport } from './new-xhr'; +import { XHRTransport } from './xhr'; + +export interface BrowserTransportOptions extends BaseTransportOptions { + // options to pass into fetch request + fetchParams: Record; + headers?: Record; + sendClientReports?: boolean; +} + +/** + * TODO: additional doc (since this is not part of Client anymore) + * @inheritDoc + */ +// TODO(v7): refactor to only return newTransport +export function setupBrowserTransport(options: BrowserOptions): { transport: Transport; newTransport?: NewTransport } { + if (!options.dsn) { + // We return the noop transport here in case there is no Dsn. + return { transport: new NoopTransport() }; + } + + const transportOptions: TransportOptions = { + ...options.transportOptions, + dsn: options.dsn, + tunnel: options.tunnel, + sendClientReports: options.sendClientReports, + _metadata: options._metadata, + }; + + const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); + const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); + + if (options.transport) { + return { transport: new options.transport(transportOptions) }; + } + if (supportsFetch()) { + const requestOptions: RequestInit = { ...transportOptions.fetchParameters }; + const newTransport = makeNewFetchTransport({ requestOptions, url }); + const fetchTransport = new FetchTransport(transportOptions); + return { transport: fetchTransport, newTransport }; + } + + const newTransport = makeNewXHRTransport({ + url, + headers: transportOptions.headers, + }); + const transport = new XHRTransport(transportOptions); + return { transport, newTransport }; +} diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 8486d5e03b9c..0d28acffef0c 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -92,8 +92,10 @@ export abstract class BaseClient implements Client { * Initializes this client instance. * * @param options Options for the client. + * @param transport The (old) Transport instance for the client to use (TODO(v7): remove) + * @param newTransport The NewTransport instance for the client to use */ - protected constructor(options: O) { + protected constructor(options: O, transport: Transport, newTransport?: NewTransport) { this._options = options; if (options.dsn) { @@ -102,7 +104,9 @@ export abstract class BaseClient implements Client { IS_DEBUG_BUILD && logger.warn('No DSN provided, client will not do anything.'); } - this._transport = this._setupTransport(); + // TODO(v7): remove old transport + this._transport = transport; + this._newTransport = newTransport; } /** diff --git a/packages/core/src/sdk.ts b/packages/core/src/sdk.ts index 381a54839a0a..97c8b349a235 100644 --- a/packages/core/src/sdk.ts +++ b/packages/core/src/sdk.ts @@ -1,11 +1,16 @@ import { getCurrentHub } from '@sentry/hub'; -import { Client, Options } from '@sentry/types'; +import { Client, Options, Transport } from '@sentry/types'; import { logger } from '@sentry/utils'; import { IS_DEBUG_BUILD } from './flags'; +import { NewTransport } from './transports/base'; /** A class object that can instantiate Client objects. */ -export type ClientClass = new (options: O) => F; +export type ClientClass = new ( + options: O, + transport: Transport, + newTransport?: NewTransport, +) => F; /** * Internal function to create a new SDK client instance. The client is @@ -14,7 +19,12 @@ export type ClientClass = new (options: O) * @param clientClass The client class to instantiate. * @param options Options to pass to the client. */ -export function initAndBind(clientClass: ClientClass, options: O): void { +export function initAndBind( + clientClass: ClientClass, + options: O, + transport: Transport, + newTransport?: NewTransport, +): void { if (options.debug === true) { if (IS_DEBUG_BUILD) { logger.enable(); @@ -29,6 +39,7 @@ export function initAndBind(clientClass: Cl if (scope) { scope.update(options.initialScope); } - const client = new clientClass(options); + + const client = new clientClass(options, transport, newTransport); hub.bindClient(client); } diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 663a4c3c686f..8588765c0748 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -51,7 +51,6 @@ export type TransportResponse = { interface InternalBaseTransportOptions { bufferSize?: number; } - export interface BaseTransportOptions extends InternalBaseTransportOptions { // url to send the event // transport does not care about dsn specific - client should take care of @@ -59,7 +58,7 @@ export interface BaseTransportOptions extends InternalBaseTransportOptions { url: string; } -// TODO: Move into Browser Transport +// TODO(v7): Delete export interface BrowserTransportOptions extends BaseTransportOptions { // options to pass into fetch request fetchParams: Record; diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index 36f053429e7f..1310cfc5e24c 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -78,6 +78,7 @@ export class TestClient extends BaseClient { } } +// TODO(v7): wtf? export function init(options: TestOptions): void { initAndBind(TestClient, options); } diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 6c4812b65980..630503e79a2f 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -1,4 +1,11 @@ -import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, initAPIDetails, Scope, SDK_VERSION } from '@sentry/core'; +import { + BaseClient, + getEnvelopeEndpointWithUrlEncodedAuth, + initAPIDetails, + NewTransport, + Scope, + SDK_VERSION, +} from '@sentry/core'; import { SessionFlusher } from '@sentry/hub'; import { Event, EventHint, Severity, SeverityLevel, Transport, TransportOptions } from '@sentry/types'; import { logger, makeDsn, resolvedSyncPromise, stackParserFromOptions } from '@sentry/utils'; @@ -21,7 +28,7 @@ export class NodeClient extends BaseClient { * Creates a new Node SDK instance. * @param options Configuration options for this SDK. */ - public constructor(options: NodeOptions) { + public constructor(options: NodeOptions, transport: Transport, newTransport?: NewTransport) { options._metadata = options._metadata || {}; options._metadata.sdk = options._metadata.sdk || { name: 'sentry.javascript.node', @@ -34,7 +41,7 @@ export class NodeClient extends BaseClient { version: SDK_VERSION, }; - super(options); + super(options, transport, newTransport); } /** @@ -154,6 +161,7 @@ export class NodeClient extends BaseClient { /** * @inheritDoc + * TODO(v7): delete */ protected _setupTransport(): Transport { if (!this._options.dsn) { diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 095bd3114144..b307a2b67128 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -8,6 +8,7 @@ import { NodeClient } from './client'; import { IS_DEBUG_BUILD } from './flags'; import { Console, ContextLines, Http, LinkedErrors, OnUncaughtException, OnUnhandledRejection } from './integrations'; import { nodeStackParser } from './stack-parser'; +import { setupNodeTransport } from './transports'; import { NodeOptions } from './types'; export const defaultIntegrations = [ @@ -130,7 +131,9 @@ export function init(options: NodeOptions = {}): void { setHubOnCarrier(carrier, getCurrentHub()); } - initAndBind(NodeClient, options); + // TODO(v7): Init transport here and pass it to initAndBind + const { transport, newTransport } = setupNodeTransport(options); + initAndBind(NodeClient, options, transport, newTransport); if (options.autoSessionTracking) { startSessionTracking(); diff --git a/packages/node/src/transports/index.ts b/packages/node/src/transports/index.ts index 958562933321..01877029269e 100644 --- a/packages/node/src/transports/index.ts +++ b/packages/node/src/transports/index.ts @@ -4,3 +4,4 @@ export { BaseTransport } from './base'; export { HTTPTransport } from './http'; export { HTTPSTransport } from './https'; export { makeNodeTransport } from './new'; +export { setupNodeTransport } from './setup'; diff --git a/packages/node/src/transports/setup.ts b/packages/node/src/transports/setup.ts new file mode 100644 index 000000000000..4efb56344256 --- /dev/null +++ b/packages/node/src/transports/setup.ts @@ -0,0 +1,49 @@ +import { getEnvelopeEndpointWithUrlEncodedAuth, initAPIDetails, NewTransport, NoopTransport } from '@sentry/core'; +import { Transport, TransportOptions } from '@sentry/types'; +import { makeDsn } from '@sentry/utils'; + +import { NodeOptions } from '../types'; +import { HTTPSTransport, HTTPTransport, makeNodeTransport } from '.'; + +/** + * TODO(v7): Add documentation + * @inheritDoc + */ +// TODO(v7): Adjust when NewTransport is the default +export function setupNodeTransport(options: NodeOptions): { transport: Transport; newTransport?: NewTransport } { + if (!options.dsn) { + // We return the noop transport here in case there is no Dsn. + return { transport: new NoopTransport() }; + } + + const dsn = makeDsn(options.dsn); + + const transportOptions: TransportOptions = { + ...options.transportOptions, + ...(options.httpProxy && { httpProxy: options.httpProxy }), + ...(options.httpsProxy && { httpsProxy: options.httpsProxy }), + ...(options.caCerts && { caCerts: options.caCerts }), + dsn: options.dsn, + tunnel: options.tunnel, + _metadata: options._metadata, + }; + + if (options.transport) { + return { transport: new options.transport(transportOptions) }; + } + + const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); + const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); + + const newTransport = makeNodeTransport({ + url, + headers: transportOptions.headers, + proxy: transportOptions.httpProxy, + caCerts: transportOptions.caCerts, + }); + + if (dsn.protocol === 'http') { + return { transport: new HTTPTransport(transportOptions), newTransport }; + } + return { transport: new HTTPSTransport(transportOptions), newTransport }; +} From a2a72772a42400bf6de03e046d115b0f46678c3c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Apr 2022 15:56:28 +0200 Subject: [PATCH 02/17] fix browser unit tests --- packages/browser/test/unit/index.test.ts | 103 ++++++++++++++--------- 1 file changed, 61 insertions(+), 42 deletions(-) diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index 707354fe7d1d..b842fe4627e0 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -75,7 +75,7 @@ describe('SentryBrowser', () => { describe('showReportDialog', () => { describe('user', () => { const EX_USER = { email: 'test@example.com' }; - const client = new BrowserClient({ dsn }); + const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn })); const reportDialogSpy = jest.spyOn(client, 'showReportDialog'); beforeEach(() => { @@ -140,30 +140,36 @@ describe('SentryBrowser', () => { it('should capture a message', done => { getCurrentHub().bindClient( - new BrowserClient({ - beforeSend: (event: Event): Event | null => { - expect(event.message).toBe('test'); - expect(event.exception).toBeUndefined(); - done(); - return event; + new BrowserClient( + { + beforeSend: (event: Event): Event | null => { + expect(event.message).toBe('test'); + expect(event.exception).toBeUndefined(); + done(); + return event; + }, + dsn, }, - dsn, - }), + new SimpleTransport({ dsn }), + ), ); captureMessage('test'); }); it('should capture an event', done => { getCurrentHub().bindClient( - new BrowserClient({ - beforeSend: (event: Event): Event | null => { - expect(event.message).toBe('event'); - expect(event.exception).toBeUndefined(); - done(); - return event; + new BrowserClient( + { + beforeSend: (event: Event): Event | null => { + expect(event.message).toBe('event'); + expect(event.exception).toBeUndefined(); + done(); + return event; + }, + dsn, }, - dsn, - }), + new SimpleTransport({ dsn }), + ), ); captureEvent({ message: 'event' }); }); @@ -171,11 +177,14 @@ describe('SentryBrowser', () => { it('should not dedupe an event on bound client', async () => { const localBeforeSend = jest.fn(); getCurrentHub().bindClient( - new BrowserClient({ - beforeSend: localBeforeSend, - dsn, - integrations: [], - }), + new BrowserClient( + { + beforeSend: localBeforeSend, + dsn, + integrations: [], + }, + new SimpleTransport({ dsn }), + ), ); captureMessage('event222'); @@ -189,11 +198,14 @@ describe('SentryBrowser', () => { it('should use inboundfilter rules of bound client', async () => { const localBeforeSend = jest.fn(); getCurrentHub().bindClient( - new BrowserClient({ - beforeSend: localBeforeSend, - dsn, - integrations: [new Integrations.InboundFilters({ ignoreErrors: ['capture'] })], - }), + new BrowserClient( + { + beforeSend: localBeforeSend, + dsn, + integrations: [new Integrations.InboundFilters({ ignoreErrors: ['capture'] })], + }, + new SimpleTransport({ dsn }), + ), ); captureMessage('capture'); @@ -246,18 +258,22 @@ describe('SentryBrowser initialization', () => { it('should set SDK data when Sentry.init() is called', () => { init({ dsn }); - const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + // TODO(v7): Check if this is the correct way to go here + // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + const sdkData: any = getCurrentHub().getClient()?.getOptions()._metadata?.sdk!; - expect(sdkData.name).toBe('sentry.javascript.browser'); - expect(sdkData.packages[0].name).toBe('npm:@sentry/browser'); - expect(sdkData.packages[0].version).toBe(SDK_VERSION); - expect(sdkData.version).toBe(SDK_VERSION); + expect(sdkData?.name).toBe('sentry.javascript.browser'); + expect(sdkData?.packages[0].name).toBe('npm:@sentry/browser'); + expect(sdkData?.packages[0].version).toBe(SDK_VERSION); + expect(sdkData?.version).toBe(SDK_VERSION); }); it('should set SDK data when instantiating a client directly', () => { - const client = new BrowserClient({ dsn }); + const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn })); - const sdkData = (client as any).getTransport()._api.metadata?.sdk; + // TODO(v7): Check if this is the correct way to go here + // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + const sdkData = (client as any).getOptions()._metadata?.sdk; expect(sdkData.name).toBe('sentry.javascript.browser'); expect(sdkData.packages[0].name).toBe('npm:@sentry/browser'); @@ -298,15 +314,18 @@ describe('SentryBrowser initialization', () => { describe('wrap()', () => { it('should wrap and call function while capturing error', done => { getCurrentHub().bindClient( - new BrowserClient({ - beforeSend: (event: Event): Event | null => { - expect(event.exception!.values![0].type).toBe('TypeError'); - expect(event.exception!.values![0].value).toBe('mkey'); - done(); - return null; + new BrowserClient( + { + beforeSend: (event: Event): Event | null => { + expect(event.exception!.values![0].type).toBe('TypeError'); + expect(event.exception!.values![0].value).toBe('mkey'); + done(); + return null; + }, + dsn, }, - dsn, - }), + new SimpleTransport({ dsn }), + ), ); try { From bee1c78d9d9dcff825abfbb44bfa49c47148788c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Apr 2022 17:55:19 +0200 Subject: [PATCH 03/17] fix core unit tests --- packages/core/test/lib/base.test.ts | 245 ++++++++++++++++++---------- packages/core/test/mocks/client.ts | 37 +++-- 2 files changed, 174 insertions(+), 108 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 6bb72ed99f16..09eb3c77b78d 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -4,7 +4,7 @@ import { dsnToString, logger, SentryError, SyncPromise } from '@sentry/utils'; import * as integrationModule from '../../src/integration'; import { NoopTransport } from '../../src/transports/noop'; -import { TestClient } from '../mocks/client'; +import { setupTestTransport, TestClient } from '../mocks/client'; import { TestIntegration } from '../mocks/integration'; import { FakeTransport } from '../mocks/transport'; @@ -67,14 +67,16 @@ describe('BaseClient', () => { test('returns the Dsn', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); expect(dsnToString(client.getDsn())).toBe(PUBLIC_DSN); }); test('allows missing Dsn', () => { expect.assertions(1); - const client = new TestClient({}); + const options = {}; + const client = new TestClient(options, setupTestTransport(options).transport); expect(client.getDsn()).toBeUndefined(); }); @@ -82,7 +84,8 @@ describe('BaseClient', () => { test('throws with invalid Dsn', () => { expect.assertions(1); - expect(() => new TestClient({ dsn: 'abc' })).toThrow(SentryError); + const options = { dsn: 'abc' }; + expect(() => new TestClient(options, setupTestTransport(options).transport)).toThrow(SentryError); }); }); @@ -91,7 +94,7 @@ describe('BaseClient', () => { expect.assertions(1); const options = { dsn: PUBLIC_DSN, test: true }; - const client = new TestClient(options); + const client = new TestClient(options, setupTestTransport(options).transport); expect(client.getOptions()).toEqual(options); }); @@ -102,7 +105,7 @@ describe('BaseClient', () => { expect.assertions(2); const options = { dsn: PUBLIC_DSN, transport: FakeTransport }; - const client = new TestClient(options); + const client = new TestClient(options, new FakeTransport()); expect(client.getTransport()).toBeInstanceOf(FakeTransport); expect(TestClient.instance!.getTransport()).toBe(client.getTransport()); @@ -112,7 +115,7 @@ describe('BaseClient', () => { expect.assertions(2); const options = { dsn: PUBLIC_DSN }; - const client = new TestClient(options); + const client = new TestClient(options, setupTestTransport(options).transport); expect(client.getTransport()).toBeInstanceOf(NoopTransport); expect(TestClient.instance!.getTransport()).toBe(client.getTransport()); @@ -123,7 +126,8 @@ describe('BaseClient', () => { test('adds a breadcrumb', () => { expect.assertions(1); - const client = new TestClient({}); + const options = {}; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -136,7 +140,8 @@ describe('BaseClient', () => { test('adds a timestamp to new breadcrumbs', () => { expect.assertions(1); - const client = new TestClient({}); + const options = {}; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -149,7 +154,8 @@ describe('BaseClient', () => { test('discards breadcrumbs beyond maxBreadcrumbs', () => { expect.assertions(2); - const client = new TestClient({ maxBreadcrumbs: 1 }); + const options = { maxBreadcrumbs: 1 }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -163,7 +169,8 @@ describe('BaseClient', () => { test('allows concurrent updates', () => { expect.assertions(1); - const client = new TestClient({}); + const options = {}; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -177,7 +184,8 @@ describe('BaseClient', () => { expect.assertions(1); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - const client = new TestClient({ beforeBreadcrumb }); + const options = { beforeBreadcrumb }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -190,7 +198,8 @@ describe('BaseClient', () => { expect.assertions(1); const beforeBreadcrumb = jest.fn(() => ({ message: 'changed' })); - const client = new TestClient({ beforeBreadcrumb }); + const options = { beforeBreadcrumb }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -203,7 +212,8 @@ describe('BaseClient', () => { expect.assertions(1); const beforeBreadcrumb = jest.fn(() => null); - const client = new TestClient({ beforeBreadcrumb }); + const options = { beforeBreadcrumb }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -216,7 +226,8 @@ describe('BaseClient', () => { expect.assertions(2); const beforeBreadcrumb = jest.fn((breadcrumb, hint) => ({ ...breadcrumb, data: hint.data })); - const client = new TestClient({ beforeBreadcrumb }); + const options = { beforeBreadcrumb }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); @@ -229,7 +240,8 @@ describe('BaseClient', () => { describe('captureException', () => { test('captures and sends exceptions', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureException(new Error('test exception')); @@ -251,7 +263,8 @@ describe('BaseClient', () => { }); test('allows for providing explicit scope', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.setExtra('foo', 'wat'); @@ -278,7 +291,8 @@ describe('BaseClient', () => { }); test('allows for clearing data from existing scope if explicit one does so in a callback function', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.setExtra('foo', 'wat'); @@ -312,7 +326,8 @@ describe('BaseClient', () => { // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it // is encountered, so this test doesn't apply. ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); expect(thrown.__sentry_captured__).toBeUndefined(); @@ -330,7 +345,8 @@ describe('BaseClient', () => { describe('captureMessage', () => { test('captures and sends messages', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureMessage('test message'); @@ -346,7 +362,8 @@ describe('BaseClient', () => { }); test('should call eventFromException if input to captureMessage is not a primitive', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const spy = jest.spyOn(TestClient.instance!, 'eventFromException'); client.captureMessage('foo'); @@ -364,7 +381,8 @@ describe('BaseClient', () => { }); test('allows for providing explicit scope', () => { - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.setExtra('foo', 'wat'); @@ -397,7 +415,8 @@ describe('BaseClient', () => { test('skips when disabled', () => { expect.assertions(1); - const client = new TestClient({ enabled: false, dsn: PUBLIC_DSN }); + const options = { enabled: false, dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({}, undefined, scope); @@ -408,7 +427,8 @@ describe('BaseClient', () => { test('skips without a Dsn', () => { expect.assertions(1); - const client = new TestClient({}); + const options = {}; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({}, undefined, scope); @@ -425,10 +445,11 @@ describe('BaseClient', () => { // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it // is encountered, so this test doesn't apply. ])("doesn't capture an event wrapping the same exception twice - %s", (_name: string, thrown: any) => { + const options = { dsn: PUBLIC_DSN }; // Note: this is the same test as in `describe(captureException)`, except with the exception already wrapped in a // hint and accompanying an event. Duplicated here because some methods skip `captureException` and go straight to // `captureEvent`. - const client = new TestClient({ dsn: PUBLIC_DSN }); + const client = new TestClient(options, setupTestTransport(options).transport); const event = { exception: { values: [{ type: 'Error', message: 'Will I get caught twice?' }] } }; const hint = { originalException: thrown }; @@ -448,7 +469,8 @@ describe('BaseClient', () => { test('sends an event', () => { expect.assertions(2); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, undefined, scope); @@ -467,7 +489,8 @@ describe('BaseClient', () => { test('does not overwrite existing timestamp', () => { expect.assertions(2); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message', timestamp: 1234 }, undefined, scope); @@ -486,7 +509,8 @@ describe('BaseClient', () => { test('adds event_id from hint if available', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, { event_id: 'wat' }, scope); @@ -504,9 +528,10 @@ describe('BaseClient', () => { test('sets default environment to `production` if none provided', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, undefined, scope); @@ -524,10 +549,11 @@ describe('BaseClient', () => { test('adds the configured environment', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, environment: 'env', - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, undefined, scope); @@ -545,10 +571,11 @@ describe('BaseClient', () => { test('allows for environment to be explicitly set to falsy value', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, environment: undefined, - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, undefined, scope); @@ -566,10 +593,11 @@ describe('BaseClient', () => { test('adds the configured release', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, release: 'v1.0.0', - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); client.captureEvent({ message: 'message' }, undefined, scope); @@ -588,7 +616,8 @@ describe('BaseClient', () => { test('adds breadcrumbs', () => { expect.assertions(4); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.addBreadcrumb({ message: 'breadcrumb' }, 100); @@ -603,7 +632,8 @@ describe('BaseClient', () => { test('limits previously saved breadcrumbs', () => { expect.assertions(2); - const client = new TestClient({ dsn: PUBLIC_DSN, maxBreadcrumbs: 1 }); + const options = { dsn: PUBLIC_DSN, maxBreadcrumbs: 1 }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); const hub = new Hub(client, scope); hub.addBreadcrumb({ message: '1' }); @@ -618,7 +648,8 @@ describe('BaseClient', () => { test('adds context data', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.setExtra('b', 'b'); scope.setTag('a', 'a'); @@ -642,7 +673,8 @@ describe('BaseClient', () => { test('adds fingerprint', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const scope = new Scope(); scope.setFingerprint(['abcd']); @@ -660,7 +692,8 @@ describe('BaseClient', () => { }); test('adds installed integrations to sdk info', () => { - const client = new TestClient({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }); + const options = { dsn: PUBLIC_DSN, integrations: [new TestIntegration()] }; + const client = new TestClient(options, setupTestTransport(options).transport); client.setupIntegrations(); client.captureEvent({ message: 'message' }); @@ -673,7 +706,8 @@ describe('BaseClient', () => { test('normalizes event with default depth of 3', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const fourLevelsObject = { a: { b: { @@ -724,10 +758,11 @@ describe('BaseClient', () => { test('normalization respects `normalizeDepth` option', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, normalizeDepth: 2, - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const fourLevelsObject = { a: { b: { @@ -775,10 +810,11 @@ describe('BaseClient', () => { test('skips normalization when `normalizeDepth: 0`', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, normalizeDepth: 0, - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const fourLevelsObject = { a: { b: { @@ -831,7 +867,8 @@ describe('BaseClient', () => { test('normalization applies to Transaction and Span consistently', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const transaction: Event = { contexts: { trace: { @@ -905,7 +942,8 @@ describe('BaseClient', () => { expect.assertions(1); const beforeSend = jest.fn(event => event); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }); @@ -916,7 +954,8 @@ describe('BaseClient', () => { expect.assertions(1); const beforeSend = jest.fn(() => ({ message: 'changed1' })); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }); @@ -927,7 +966,7 @@ describe('BaseClient', () => { expect.assertions(3); const beforeSend = jest.fn(() => null); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }, setupTestTransport({ dsn: PUBLIC_DSN }).transport); const captureExceptionSpy = jest.spyOn(client, 'captureException'); const loggerErrorSpy = jest.spyOn(logger, 'error'); @@ -969,7 +1008,8 @@ describe('BaseClient', () => { }, 1); }), ); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }); jest.runOnlyPendingTimers(); @@ -997,7 +1037,8 @@ describe('BaseClient', () => { }, 1); }), ); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }); jest.runOnlyPendingTimers(); @@ -1025,7 +1066,8 @@ describe('BaseClient', () => { }); }), ); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }); jest.runAllTimers(); @@ -1037,7 +1079,8 @@ describe('BaseClient', () => { expect.assertions(2); const beforeSend = jest.fn((event, hint) => ({ ...event, data: hint.data })); - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const options = { dsn: PUBLIC_DSN, beforeSend }; + const client = new TestClient(options, setupTestTransport(options).transport); client.captureEvent({ message: 'hello' }, { data: 'someRandomThing' }); @@ -1048,12 +1091,15 @@ describe('BaseClient', () => { test('beforeSend records dropped events', () => { expect.assertions(1); - const client = new TestClient({ - dsn: PUBLIC_DSN, - beforeSend() { - return null; + const client = new TestClient( + { + dsn: PUBLIC_DSN, + beforeSend() { + return null; + }, }, - }); + setupTestTransport({ dsn: PUBLIC_DSN }).transport, + ); const recordLostEventSpy = jest.fn(); jest.spyOn(client, 'getTransport').mockImplementationOnce( () => @@ -1070,7 +1116,7 @@ describe('BaseClient', () => { test('eventProcessor can drop the even when it returns null', () => { expect.assertions(3); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const client = new TestClient({ dsn: PUBLIC_DSN }, setupTestTransport({ dsn: PUBLIC_DSN }).transport); const captureExceptionSpy = jest.spyOn(client, 'captureException'); const loggerErrorSpy = jest.spyOn(logger, 'error'); const scope = new Scope(); @@ -1086,7 +1132,8 @@ describe('BaseClient', () => { test('eventProcessor records dropped events', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const recordLostEventSpy = jest.fn(); jest.spyOn(client, 'getTransport').mockImplementationOnce( @@ -1107,7 +1154,8 @@ describe('BaseClient', () => { test('eventProcessor sends an event and logs when it crashes', () => { expect.assertions(3); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const captureExceptionSpy = jest.spyOn(client, 'captureException'); const loggerErrorSpy = jest.spyOn(logger, 'error'); const scope = new Scope(); @@ -1135,10 +1183,11 @@ describe('BaseClient', () => { test('records events dropped due to sampleRate', () => { expect.assertions(1); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, sampleRate: 0, - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); const recordLostEventSpy = jest.fn(); jest.spyOn(client, 'getTransport').mockImplementationOnce( @@ -1161,10 +1210,11 @@ describe('BaseClient', () => { test('setup each one of them on setupIntegration call', () => { expect.assertions(2); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, integrations: [new TestIntegration()], - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(1); @@ -1174,9 +1224,10 @@ describe('BaseClient', () => { test('skips installation if DSN is not provided', () => { expect.assertions(2); - const client = new TestClient({ + const options = { integrations: [new TestIntegration()], - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(0); @@ -1186,11 +1237,12 @@ describe('BaseClient', () => { test('skips installation if enabled is set to false', () => { expect.assertions(2); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, enabled: false, integrations: [new TestIntegration()], - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(0); @@ -1200,10 +1252,11 @@ describe('BaseClient', () => { test('skips installation if integrations are already installed', () => { expect.assertions(4); - const client = new TestClient({ + const options = { dsn: PUBLIC_DSN, integrations: [new TestIntegration()], - }); + }; + const client = new TestClient(options, setupTestTransport(options).transport); // note: not the `Client` method `setupIntegrations`, but the free-standing function which that method calls const setupIntegrationsHelper = jest.spyOn(integrationModule, 'setupIntegrations'); @@ -1226,11 +1279,14 @@ describe('BaseClient', () => { jest.useRealTimers(); expect.assertions(5); - const client = new TestClient({ - dsn: PUBLIC_DSN, - enableSend: true, - transport: FakeTransport, - }); + const client = new TestClient( + { + dsn: PUBLIC_DSN, + enableSend: true, + transport: FakeTransport, + }, + new FakeTransport(), + ); const delay = 1; const transportInstance = client.getTransport() as FakeTransport; @@ -1252,11 +1308,14 @@ describe('BaseClient', () => { jest.useRealTimers(); expect.assertions(5); - const client = new TestClient({ - dsn: PUBLIC_DSN, - enableSend: true, - transport: FakeTransport, - }); + const client = new TestClient( + { + dsn: PUBLIC_DSN, + enableSend: true, + transport: FakeTransport, + }, + new FakeTransport(), + ); const delay = 300; const spy = jest.spyOn(TestClient.instance!, 'eventFromMessage'); @@ -1288,11 +1347,14 @@ describe('BaseClient', () => { jest.useRealTimers(); expect.assertions(2); - const client = new TestClient({ - dsn: PUBLIC_DSN, - enableSend: true, - transport: FakeTransport, - }); + const client = new TestClient( + { + dsn: PUBLIC_DSN, + enableSend: true, + transport: FakeTransport, + }, + new FakeTransport(), + ); const delay = 1; const transportInstance = client.getTransport() as FakeTransport; @@ -1310,7 +1372,8 @@ describe('BaseClient', () => { jest.useRealTimers(); expect.assertions(3); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); return Promise.all([ client.flush(1).then(() => { @@ -1330,7 +1393,8 @@ describe('BaseClient', () => { test('sends sessions to the client', () => { expect.assertions(1); - const client = new TestClient({ dsn: PUBLIC_DSN }); + const options = { dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const session = new Session({ release: 'test' }); client.captureSession(session); @@ -1341,7 +1405,8 @@ describe('BaseClient', () => { test('skips when disabled', () => { expect.assertions(1); - const client = new TestClient({ enabled: false, dsn: PUBLIC_DSN }); + const options = { enabled: false, dsn: PUBLIC_DSN }; + const client = new TestClient(options, setupTestTransport(options).transport); const session = new Session({ release: 'test' }); client.captureSession(session); diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index 1310cfc5e24c..f238887fe459 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -4,6 +4,8 @@ import { resolvedSyncPromise } from '@sentry/utils'; import { BaseClient } from '../../src/baseclient'; import { initAndBind } from '../../src/sdk'; +import { NewTransport } from '../../src/transports/base'; +import { NoopTransport } from '../../src/transports/noop'; export interface TestOptions extends Options { test?: boolean; mockInstallFailure?: boolean; @@ -17,8 +19,8 @@ export class TestClient extends BaseClient { public event?: Event; public session?: Session; - public constructor(options: TestOptions) { - super(options); + public constructor(options: TestOptions, transport: Transport, newTransport?: NewTransport) { + super(options, transport, newTransport); TestClient.instance = this; } @@ -59,26 +61,25 @@ export class TestClient extends BaseClient { public sendSession(session: Session): void { this.session = session; } +} - protected _setupTransport(): Transport { - if (!this._options.dsn) { - // We return the noop transport here in case there is no Dsn. - return super._setupTransport(); - } +export function init(options: TestOptions, transport: Transport, newTransport?: NewTransport): void { + initAndBind(TestClient, options, transport, newTransport); +} - const transportOptions = this._options.transportOptions - ? this._options.transportOptions - : { dsn: this._options.dsn }; +export function setupTestTransport(options: TestOptions): { transport: Transport; newTransport?: NewTransport } { + const noop = { transport: new NoopTransport() }; - if (this._options.transport) { - return new this._options.transport(transportOptions); - } + if (!options.dsn) { + // We return the noop transport here in case there is no Dsn. + return noop; + } + + const transportOptions = options.transportOptions ? options.transportOptions : { dsn: options.dsn }; - return super._setupTransport(); + if (options.transport) { + return { transport: new this._options.transport(transportOptions) }; } -} -// TODO(v7): wtf? -export function init(options: TestOptions): void { - initAndBind(TestClient, options); + return noop; } From 5eb327fcab98172b98b8fdccf6e26e15c024c20e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Apr 2022 18:08:47 +0200 Subject: [PATCH 04/17] fix node unit tests --- packages/node/test/client.test.ts | 39 +++-- packages/node/test/index.test.ts | 252 +++++++++++++++--------------- 2 files changed, 152 insertions(+), 139 deletions(-) diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index f18f66c716af..1cddd0e85e3f 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -1,6 +1,7 @@ import { Scope, SessionFlusher } from '@sentry/hub'; import { NodeClient } from '../src'; +import { setupNodeTransport } from '../src/transports'; const PUBLIC_DSN = 'https://username@domain/123'; @@ -14,7 +15,8 @@ describe('NodeClient', () => { describe('captureException', () => { test('when autoSessionTracking is enabled, and requestHandler is not used -> requestStatus should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const scope = new Scope(); scope.setRequestSession({ status: 'ok' }); @@ -24,7 +26,8 @@ describe('NodeClient', () => { expect(requestSession!.status).toEqual('ok'); }); test('when autoSessionTracking is disabled -> requestStatus should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -38,7 +41,8 @@ describe('NodeClient', () => { expect(requestSession!.status).toEqual('ok'); }); test('when autoSessionTracking is enabled + requestSession status is Crashed -> requestStatus should not be overridden', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -52,7 +56,8 @@ describe('NodeClient', () => { expect(requestSession!.status).toEqual('crashed'); }); test('when autoSessionTracking is enabled + error occurs within request bounds -> requestStatus should be set to Errored', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -66,7 +71,8 @@ describe('NodeClient', () => { expect(requestSession!.status).toEqual('errored'); }); test('when autoSessionTracking is enabled + error occurs outside of request bounds -> requestStatus should not be set to Errored', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -82,7 +88,8 @@ describe('NodeClient', () => { describe('captureEvent()', () => { test('If autoSessionTracking is disabled, requestSession status should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -100,7 +107,8 @@ describe('NodeClient', () => { }); test('When captureEvent is called with an exception, requestSession status should be set to Errored', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -115,7 +123,8 @@ describe('NodeClient', () => { }); test('When captureEvent is called without an exception, requestSession status should not be set to Errored', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -130,7 +139,8 @@ describe('NodeClient', () => { }); test('When captureEvent is called with an exception but outside of a request, then requestStatus should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -147,7 +157,8 @@ describe('NodeClient', () => { }); test('When captureEvent is called with a transaction, then requestSession status should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -161,7 +172,8 @@ describe('NodeClient', () => { }); test('When captureEvent is called with an exception but requestHandler is not used, then requestSession status should not be set', () => { - client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const scope = new Scope(); scope.setRequestSession({ status: 'ok' }); @@ -180,11 +192,12 @@ describe('NodeClient', () => { describe('flush/close', () => { test('client close function disables _sessionFlusher', async () => { jest.useRealTimers(); - const client = new NodeClient({ + const options = { dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.1', - }); + }; + const client = new NodeClient(options, setupNodeTransport(options).transport); client.initSessionFlusher(); // Clearing interval is important here to ensure that the flush function later on is called by the `client.close()` // not due to the interval running every 60s diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts index af12a4b35994..d2cff1e891f3 100644 --- a/packages/node/test/index.test.ts +++ b/packages/node/test/index.test.ts @@ -18,6 +18,7 @@ import { } from '../src'; import { ContextLines, LinkedErrors } from '../src/integrations'; import { nodeStackParser } from '../src/stack-parser'; +import { setupNodeTransport } from '../src/transports'; const stackParser = createStackParser(nodeStackParser); @@ -89,8 +90,7 @@ describe('SentryNode', () => { }); test('record auto breadcrumbs', done => { - const client = new NodeClient({ - stackParser, + const options = { beforeSend: (event: Event) => { // TODO: It should be 3, but we don't capture a breadcrumb // for our own captureMessage/captureException calls yet @@ -99,7 +99,9 @@ describe('SentryNode', () => { return null; }, dsn, - }); + stackParser, + }; + const client = new NodeClient(options, setupNodeTransport(options).transport); getCurrentHub().bindClient(client); addBreadcrumb({ message: 'test1' }); addBreadcrumb({ message: 'test2' }); @@ -120,22 +122,21 @@ describe('SentryNode', () => { test('capture an exception', done => { expect.assertions(6); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect(event.tags).toEqual({ test: '1' }); - expect(event.exception).not.toBeUndefined(); - expect(event.exception!.values![0]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![2]).not.toBeUndefined(); - expect(event.exception!.values![0].value).toEqual('test'); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect(event.tags).toEqual({ test: '1' }); + expect(event.exception).not.toBeUndefined(); + expect(event.exception!.values![0]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![2]).not.toBeUndefined(); + expect(event.exception!.values![0].value).toEqual('test'); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); configureScope((scope: Scope) => { scope.setTag('test', '1'); }); @@ -148,22 +149,21 @@ describe('SentryNode', () => { test('capture a string exception', done => { expect.assertions(6); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect(event.tags).toEqual({ test: '1' }); - expect(event.exception).not.toBeUndefined(); - expect(event.exception!.values![0]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![2]).not.toBeUndefined(); - expect(event.exception!.values![0].value).toEqual('test string exception'); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect(event.tags).toEqual({ test: '1' }); + expect(event.exception).not.toBeUndefined(); + expect(event.exception!.values![0]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![2]).not.toBeUndefined(); + expect(event.exception!.values![0].value).toEqual('test string exception'); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); configureScope((scope: Scope) => { scope.setTag('test', '1'); }); @@ -176,26 +176,25 @@ describe('SentryNode', () => { test('capture an exception with pre/post context', done => { expect.assertions(10); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect(event.tags).toEqual({ test: '1' }); - expect(event.exception).not.toBeUndefined(); - expect(event.exception!.values![0]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1].pre_context).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1].post_context).not.toBeUndefined(); - expect(event.exception!.values![0].type).toBe('Error'); - expect(event.exception!.values![0].value).toBe('test'); - expect(event.exception!.values![0].stacktrace).toBeTruthy(); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect(event.tags).toEqual({ test: '1' }); + expect(event.exception).not.toBeUndefined(); + expect(event.exception!.values![0]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1].pre_context).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1].post_context).not.toBeUndefined(); + expect(event.exception!.values![0].type).toBe('Error'); + expect(event.exception!.values![0].value).toBe('test'); + expect(event.exception!.values![0].stacktrace).toBeTruthy(); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); configureScope((scope: Scope) => { scope.setTag('test', '1'); }); @@ -208,33 +207,32 @@ describe('SentryNode', () => { test('capture a linked exception with pre/post context', done => { expect.assertions(15); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - integrations: [new ContextLines(), new LinkedErrors()], - beforeSend: (event: Event) => { - expect(event.exception).not.toBeUndefined(); - expect(event.exception!.values![1]).not.toBeUndefined(); - expect(event.exception!.values![1].stacktrace!).not.toBeUndefined(); - expect(event.exception!.values![1].stacktrace!.frames![1]).not.toBeUndefined(); - expect(event.exception!.values![1].stacktrace!.frames![1].pre_context).not.toBeUndefined(); - expect(event.exception!.values![1].stacktrace!.frames![1].post_context).not.toBeUndefined(); - expect(event.exception!.values![1].type).toBe('Error'); - expect(event.exception!.values![1].value).toBe('test'); - - expect(event.exception!.values![0]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1]).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1].pre_context).not.toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![1].post_context).not.toBeUndefined(); - expect(event.exception!.values![0].type).toBe('Error'); - expect(event.exception!.values![0].value).toBe('cause'); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + integrations: [new ContextLines(), new LinkedErrors()], + beforeSend: (event: Event) => { + expect(event.exception).not.toBeUndefined(); + expect(event.exception!.values![1]).not.toBeUndefined(); + expect(event.exception!.values![1].stacktrace!).not.toBeUndefined(); + expect(event.exception!.values![1].stacktrace!.frames![1]).not.toBeUndefined(); + expect(event.exception!.values![1].stacktrace!.frames![1].pre_context).not.toBeUndefined(); + expect(event.exception!.values![1].stacktrace!.frames![1].post_context).not.toBeUndefined(); + expect(event.exception!.values![1].type).toBe('Error'); + expect(event.exception!.values![1].value).toBe('test'); + + expect(event.exception!.values![0]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1]).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1].pre_context).not.toBeUndefined(); + expect(event.exception!.values![0].stacktrace!.frames![1].post_context).not.toBeUndefined(); + expect(event.exception!.values![0].type).toBe('Error'); + expect(event.exception!.values![0].value).toBe('cause'); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); try { throw new Error('test'); } catch (e) { @@ -249,42 +247,40 @@ describe('SentryNode', () => { test('capture a message', done => { expect.assertions(2); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect(event.message).toBe('test'); - expect(event.exception).toBeUndefined(); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect(event.message).toBe('test'); + expect(event.exception).toBeUndefined(); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); captureMessage('test'); }); test('capture an event', done => { expect.assertions(2); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect(event.message).toBe('test event'); - expect(event.exception).toBeUndefined(); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect(event.message).toBe('test event'); + expect(event.exception).toBeUndefined(); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); captureEvent({ message: 'test event' }); }); test('capture an event in a domain', done => { const d = domain.create(); - const client = new NodeClient({ + const options = { stackParser, beforeSend: (event: Event) => { expect(event.message).toBe('test domain'); @@ -293,7 +289,8 @@ describe('SentryNode', () => { return null; }, dsn, - }); + }; + const client = new NodeClient(options, setupNodeTransport(options).transport); d.run(() => { getCurrentHub().bindClient(client); @@ -304,21 +301,19 @@ describe('SentryNode', () => { test('stacktrace order', done => { expect.assertions(1); - getCurrentHub().bindClient( - new NodeClient({ - stackParser, - beforeSend: (event: Event) => { - expect( - event.exception!.values![0].stacktrace!.frames![ - event.exception!.values![0].stacktrace!.frames!.length - 1 - ].function, - ).toEqual('testy'); - done(); - return null; - }, - dsn, - }), - ); + const options = { + stackParser, + beforeSend: (event: Event) => { + expect( + event.exception!.values![0].stacktrace!.frames![event.exception!.values![0].stacktrace!.frames!.length - 1] + .function, + ).toEqual('testy'); + done(); + return null; + }, + dsn, + }; + getCurrentHub().bindClient(new NodeClient(options, setupNodeTransport(options).transport)); try { // @ts-ignore allow function declarations in strict mode // eslint-disable-next-line no-inner-declarations @@ -371,8 +366,10 @@ describe('SentryNode initialization', () => { it('should set SDK data when Sentry.init() is called', () => { init({ dsn }); + // TODO(v7): Is this replacement ok? // eslint-disable-next-line @typescript-eslint/no-explicit-any - const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + const sdkData = (getCurrentHub().getClient() as any).getOptions()._metadata?.sdk; expect(sdkData.name).toEqual('sentry.javascript.node'); expect(sdkData.packages[0].name).toEqual('npm:@sentry/node'); @@ -381,10 +378,13 @@ describe('SentryNode initialization', () => { }); it('should set SDK data when instantiating a client directly', () => { - const client = new NodeClient({ dsn }); + const options = { dsn }; + const client = new NodeClient(options, setupNodeTransport(options).transport); // eslint-disable-next-line @typescript-eslint/no-explicit-any - const sdkData = (client as any).getTransport()._api.metadata?.sdk; + // TODO(v7): Is this alright? + // const sdkData = (client as any).getTransport()._api.metadata?.sdk; + const sdkData = (client as any).getOptions()._metadata?.sdk; expect(sdkData.name).toEqual('sentry.javascript.node'); expect(sdkData.packages[0].name).toEqual('npm:@sentry/node'); From a1fec8a8f48c810629751635dc226564949e62b1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Apr 2022 18:25:47 +0200 Subject: [PATCH 05/17] fix tracing tests --- packages/tracing/test/idletransaction.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index b60fec726c54..f2565e1c7e5f 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -14,7 +14,8 @@ export class SimpleTransport extends Transports.BaseTransport {} const dsn = 'https://123@sentry.io/42'; let hub: Hub; beforeEach(() => { - hub = new Hub(new BrowserClient({ dsn, tracesSampleRate: 1, transport: SimpleTransport })); + const options = { dsn, tracesSampleRate: 1, transport: SimpleTransport }; + hub = new Hub(new BrowserClient(options, new SimpleTransport(options))); }); describe('IdleTransaction', () => { From 798f921d3eb6b6dd37cc150ba7574a5e2948ef56 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 10:38:54 +0200 Subject: [PATCH 06/17] fix more node unit tests --- packages/core/src/baseclient.ts | 9 ++++- .../errored-session-aggregate/test.ts | 3 ++ packages/node/test/handlers.test.ts | 31 +++++++++++----- packages/node/test/index.test.ts | 8 +--- packages/node/test/integrations/http.test.ts | 37 +++++++++---------- .../test/integrations/linkederrors.test.ts | 16 +++++--- 6 files changed, 63 insertions(+), 41 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 0d28acffef0c..9b8ddb61c3d2 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -29,7 +29,7 @@ import { uuid4, } from '@sentry/utils'; -import { initAPIDetails } from './api'; +import { APIDetails, initAPIDetails } from './api'; import { IS_DEBUG_BUILD } from './flags'; import { IntegrationIndex, setupIntegrations } from './integration'; import { createEventEnvelope, createSessionEnvelope } from './request'; @@ -107,6 +107,13 @@ export abstract class BaseClient implements Client { // TODO(v7): remove old transport this._transport = transport; this._newTransport = newTransport; + + // TODO(v7): refactor this to keep metadata/api outside of transport. This hack is used to + // satisfy tests until we move to NewTransport where we have to revisit this. + (this._transport as unknown as { _api: Partial })._api = { + ...((this._transport as unknown as { _api: Partial })._api || {}), + metadata: options._metadata || {}, + }; } /** diff --git a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts index e26cafa8a586..5b475af63055 100644 --- a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts +++ b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts @@ -10,8 +10,11 @@ test('should aggregate successful, crashed and erroneous sessions', async () => getEnvelopeRequest(`${url}/error_handled`), getEnvelopeRequest(`${url}/error_unhandled`), ]); + console.log(envelope); expect(envelope).toHaveLength(3); + console.log(envelope[0]); + expect(envelope[0]).toMatchObject({ sent_at: expect.any(String), sdk: { diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index f0959de22132..d7d47fda053d 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -18,6 +18,7 @@ import { tracingHandler, } from '../src/handlers'; import * as SDK from '../src/sdk'; +import { setupNodeTransport } from '../src/transports'; describe('parseRequest', () => { let mockReq: { [key: string]: any }; @@ -223,7 +224,8 @@ describe('requestHandler', () => { }); it('autoSessionTracking is enabled, sets requestSession status to ok, when handling a request', () => { - client = new NodeClient({ autoSessionTracking: true, release: '1.2' }); + const options = { autoSessionTracking: true, release: '1.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const hub = new Hub(client); jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -235,7 +237,8 @@ describe('requestHandler', () => { }); it('autoSessionTracking is disabled, does not set requestSession, when handling a request', () => { - client = new NodeClient({ autoSessionTracking: false, release: '1.2' }); + const options = { autoSessionTracking: false, release: '1.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const hub = new Hub(client); jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -247,7 +250,8 @@ describe('requestHandler', () => { }); it('autoSessionTracking is enabled, calls _captureRequestSession, on response finish', done => { - client = new NodeClient({ autoSessionTracking: true, release: '1.2' }); + const options = { autoSessionTracking: true, release: '1.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const hub = new Hub(client); jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -267,7 +271,8 @@ describe('requestHandler', () => { }); it('autoSessionTracking is disabled, does not call _captureRequestSession, on response finish', done => { - client = new NodeClient({ autoSessionTracking: false, release: '1.2' }); + const options = { autoSessionTracking: false, release: '1.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const hub = new Hub(client); jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -367,7 +372,8 @@ describe('tracingHandler', () => { it('extracts request data for sampling context', () => { const tracesSampler = jest.fn(); - const hub = new Hub(new NodeClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new NodeClient(options, setupNodeTransport(options).transport)); // we need to mock both of these because the tracing handler relies on `@sentry/core` while the sampler relies on // `@sentry/hub`, and mocking breaks the link between the two jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -389,7 +395,8 @@ describe('tracingHandler', () => { }); it('puts its transaction on the scope', () => { - const hub = new Hub(new NodeClient({ tracesSampleRate: 1.0 })); + const options = { tracesSampleRate: 1.0 }; + const hub = new Hub(new NodeClient(options, setupNodeTransport(options).transport)); // we need to mock both of these because the tracing handler relies on `@sentry/core` while the sampler relies on // `@sentry/hub`, and mocking breaks the link between the two jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); @@ -720,7 +727,8 @@ describe('errorHandler()', () => { jest.restoreAllMocks(); }); it('when autoSessionTracking is disabled, does not set requestSession status on Crash', () => { - client = new NodeClient({ autoSessionTracking: false, release: '3.3' }); + const options = { autoSessionTracking: false, release: '3.3' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -739,7 +747,8 @@ describe('errorHandler()', () => { }); it('autoSessionTracking is enabled + requestHandler is not used -> does not set requestSession status on Crash', () => { - client = new NodeClient({ autoSessionTracking: false, release: '3.3' }); + const options = { autoSessionTracking: false, release: '3.3' }; + client = new NodeClient(options, setupNodeTransport(options).transport); const scope = sentryCore.getCurrentHub().getScope(); const hub = new Hub(client); @@ -755,7 +764,8 @@ describe('errorHandler()', () => { }); it('when autoSessionTracking is enabled, should set requestSession status to Crashed when an unhandled error occurs within the bounds of a request', () => { - client = new NodeClient({ autoSessionTracking: true, release: '1.1' }); + const options = { autoSessionTracking: true, release: '1.1' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); @@ -773,7 +783,8 @@ describe('errorHandler()', () => { }); it('when autoSessionTracking is enabled, should not set requestSession status on Crash when it occurs outside the bounds of a request', () => { - client = new NodeClient({ autoSessionTracking: true, release: '2.2' }); + const options = { autoSessionTracking: true, release: '2.2' }; + client = new NodeClient(options, setupNodeTransport(options).transport); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) client.initSessionFlusher(); diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts index d2cff1e891f3..02eaad72b2c0 100644 --- a/packages/node/test/index.test.ts +++ b/packages/node/test/index.test.ts @@ -366,10 +366,8 @@ describe('SentryNode initialization', () => { it('should set SDK data when Sentry.init() is called', () => { init({ dsn }); - // TODO(v7): Is this replacement ok? // eslint-disable-next-line @typescript-eslint/no-explicit-any - // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; - const sdkData = (getCurrentHub().getClient() as any).getOptions()._metadata?.sdk; + const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; expect(sdkData.name).toEqual('sentry.javascript.node'); expect(sdkData.packages[0].name).toEqual('npm:@sentry/node'); @@ -382,9 +380,7 @@ describe('SentryNode initialization', () => { const client = new NodeClient(options, setupNodeTransport(options).transport); // eslint-disable-next-line @typescript-eslint/no-explicit-any - // TODO(v7): Is this alright? - // const sdkData = (client as any).getTransport()._api.metadata?.sdk; - const sdkData = (client as any).getOptions()._metadata?.sdk; + const sdkData = (client as any).getTransport()._api.metadata?.sdk; expect(sdkData.name).toEqual('sentry.javascript.node'); expect(sdkData.packages[0].name).toEqual('npm:@sentry/node'); diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index bc71b994c7a5..5a05d79e3b1b 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -11,18 +11,18 @@ import * as nock from 'nock'; import { Breadcrumb } from '../../src'; import { NodeClient } from '../../src/client'; import { Http as HttpIntegration } from '../../src/integrations/http'; +import { setupNodeTransport } from '../../src/transports'; const NODE_VERSION = parseSemver(process.versions.node); describe('tracing', () => { function createTransactionOnScope() { - const hub = new Hub( - new NodeClient({ - dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', - tracesSampleRate: 1.0, - integrations: [new HttpIntegration({ tracing: true })], - }), - ); + const options = { + dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', + tracesSampleRate: 1.0, + integrations: [new HttpIntegration({ tracing: true })], + }; + const hub = new Hub(new NodeClient(options, setupNodeTransport(options).transport)); addExtensionMethods(); // we need to mock both of these because the tracing handler relies on `@sentry/core` while the sampler relies on @@ -97,18 +97,17 @@ describe('default protocols', () => { const p = new Promise(r => { resolve = r; }); - hub.bindClient( - new NodeClient({ - dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', - integrations: [new HttpIntegration({ breadcrumbs: true })], - beforeBreadcrumb: (b: Breadcrumb) => { - if ((b.data?.url as string).includes(key)) { - resolve(b); - } - return b; - }, - }), - ); + const options = { + dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', + integrations: [new HttpIntegration({ breadcrumbs: true })], + beforeBreadcrumb: (b: Breadcrumb) => { + if ((b.data?.url as string).includes(key)) { + resolve(b); + } + return b; + }, + }; + hub.bindClient(new NodeClient(options, setupNodeTransport(options).transport)); return p; } diff --git a/packages/node/test/integrations/linkederrors.test.ts b/packages/node/test/integrations/linkederrors.test.ts index 296384a5f222..2c2957bf6a69 100644 --- a/packages/node/test/integrations/linkederrors.test.ts +++ b/packages/node/test/integrations/linkederrors.test.ts @@ -4,6 +4,7 @@ import { createStackParser } from '@sentry/utils'; import { Event, NodeClient } from '../../src'; import { LinkedErrors } from '../../src/integrations/linkederrors'; import { nodeStackParser } from '../../src/stack-parser'; +import { setupNodeTransport } from '../../src/transports'; const stackParser = createStackParser(nodeStackParser); @@ -31,7 +32,8 @@ describe('LinkedErrors', () => { expect.assertions(2); const spy = jest.spyOn(linkedErrors, '_walkErrorTree'); const one = new Error('originalException'); - const client = new NodeClient({ stackParser }); + const options = { stackParser }; + const client = new NodeClient(options, setupNodeTransport(options).transport); let event: Event | undefined; return client .eventFromException(one) @@ -54,7 +56,8 @@ describe('LinkedErrors', () => { }), ); const one = new Error('originalException'); - const client = new NodeClient({ stackParser }); + const options = { stackParser }; + const client = new NodeClient(options, setupNodeTransport(options).transport); return client.eventFromException(one).then(event => linkedErrors ._handler(stackParser, event, { @@ -74,7 +77,8 @@ describe('LinkedErrors', () => { one.cause = two; two.cause = three; - const client = new NodeClient({ stackParser }); + const options = { stackParser }; + const client = new NodeClient(options, setupNodeTransport(options).transport); return client.eventFromException(one).then(event => linkedErrors ._handler(stackParser, event, { @@ -107,7 +111,8 @@ describe('LinkedErrors', () => { one.reason = two; two.reason = three; - const client = new NodeClient({ stackParser }); + const options = { stackParser }; + const client = new NodeClient(options, setupNodeTransport(options).transport); return client.eventFromException(one).then(event => linkedErrors ._handler(stackParser, event, { @@ -140,7 +145,8 @@ describe('LinkedErrors', () => { one.cause = two; two.cause = three; - const client = new NodeClient({ stackParser }); + const options = { stackParser }; + const client = new NodeClient(options, setupNodeTransport(options).transport); return client.eventFromException(one).then(event => linkedErrors ._handler(stackParser, event, { From 6d8120d9a9faf29ad07a2d8fb5d694f132fb322f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 10:46:30 +0200 Subject: [PATCH 07/17] fix more browser unit tests --- packages/browser/test/unit/index.test.ts | 8 ++------ .../test/unit/integrations/linkederrors.test.ts | 10 +++++++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index b842fe4627e0..ba458ec91554 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -258,9 +258,7 @@ describe('SentryBrowser initialization', () => { it('should set SDK data when Sentry.init() is called', () => { init({ dsn }); - // TODO(v7): Check if this is the correct way to go here - // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; - const sdkData: any = getCurrentHub().getClient()?.getOptions()._metadata?.sdk!; + const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; expect(sdkData?.name).toBe('sentry.javascript.browser'); expect(sdkData?.packages[0].name).toBe('npm:@sentry/browser'); @@ -271,9 +269,7 @@ describe('SentryBrowser initialization', () => { it('should set SDK data when instantiating a client directly', () => { const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn })); - // TODO(v7): Check if this is the correct way to go here - // const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; - const sdkData = (client as any).getOptions()._metadata?.sdk; + const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; expect(sdkData.name).toBe('sentry.javascript.browser'); expect(sdkData.packages[0].name).toBe('npm:@sentry/browser'); diff --git a/packages/browser/test/unit/integrations/linkederrors.test.ts b/packages/browser/test/unit/integrations/linkederrors.test.ts index 2589487dbcac..6650877ff39b 100644 --- a/packages/browser/test/unit/integrations/linkederrors.test.ts +++ b/packages/browser/test/unit/integrations/linkederrors.test.ts @@ -4,6 +4,7 @@ import { createStackParser } from '@sentry/utils'; import { BrowserClient } from '../../../src/client'; import * as LinkedErrorsModule from '../../../src/integrations/linkederrors'; import { defaultStackParsers } from '../../../src/stack-parsers'; +import { setupBrowserTransport } from '../../../src/transports'; const parser = createStackParser(...defaultStackParsers); @@ -38,7 +39,8 @@ describe('LinkedErrors', () => { one.cause = two; const originalException = one; - const client = new BrowserClient({ stackParser: parser }); + const options = { stackParser: parser }; + const client = new BrowserClient(options, setupBrowserTransport(options).transport); return client.eventFromException(originalException).then(event => { const result = LinkedErrorsModule._handler(parser, 'cause', 5, event, { originalException, @@ -68,7 +70,8 @@ describe('LinkedErrors', () => { one.reason = two; const originalException = one; - const client = new BrowserClient({ stackParser: parser }); + const options = { stackParser: parser }; + const client = new BrowserClient(options, setupBrowserTransport(options).transport); return client.eventFromException(originalException).then(event => { const result = LinkedErrorsModule._handler(parser, 'reason', 5, event, { originalException, @@ -94,7 +97,8 @@ describe('LinkedErrors', () => { one.cause = two; two.cause = three; - const client = new BrowserClient({ stackParser: parser }); + const options = { stackParser: parser }; + const client = new BrowserClient(options, setupBrowserTransport(options).transport); const originalException = one; return client.eventFromException(originalException).then(event => { const result = LinkedErrorsModule._handler(parser, 'cause', 2, event, { From 3a859a40369cf2d0fe0f2a79130cdee2f05694dc Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 11:13:33 +0200 Subject: [PATCH 08/17] fix more tracing unit tests --- .../test/browser/backgroundtab.test.ts | 4 +- .../test/browser/browsertracing.test.ts | 11 +- packages/tracing/test/browser/request.test.ts | 4 +- packages/tracing/test/errors.test.ts | 4 +- packages/tracing/test/hub.test.ts | 132 +++++++++++------- packages/tracing/test/span.test.ts | 24 ++-- 6 files changed, 112 insertions(+), 67 deletions(-) diff --git a/packages/tracing/test/browser/backgroundtab.test.ts b/packages/tracing/test/browser/backgroundtab.test.ts index e46c79695d20..440eb785a609 100644 --- a/packages/tracing/test/browser/backgroundtab.test.ts +++ b/packages/tracing/test/browser/backgroundtab.test.ts @@ -1,4 +1,5 @@ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; import { Hub, makeMain } from '@sentry/hub'; import { JSDOM } from 'jsdom'; @@ -13,7 +14,8 @@ describe('registerBackgroundTabDetection', () => { // @ts-ignore need to override global document global.document = dom.window.document; - hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); // If we do not add extension methods, invoking hub.startTransaction returns undefined diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 8db3cc10e41e..8354bee0b8be 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -1,4 +1,6 @@ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; +import { NoopTransport } from '@sentry/core'; import { Hub, makeMain } from '@sentry/hub'; import { getGlobalObject } from '@sentry/utils'; import { JSDOM } from 'jsdom'; @@ -51,7 +53,8 @@ describe('BrowserTracing', () => { let hub: Hub; beforeEach(() => { jest.useFakeTimers(); - hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); document.head.innerHTML = ''; @@ -472,7 +475,8 @@ describe('BrowserTracing', () => { getGlobalObject().location = dogParkLocation as any; const tracesSampler = jest.fn(); - hub.bindClient(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + hub.bindClient(new BrowserClient(options, setupBrowserTransport(options).transport)); // setting up the BrowserTracing integration automatically starts a pageload transaction createBrowserTracing(true); @@ -488,7 +492,8 @@ describe('BrowserTracing', () => { getGlobalObject().location = dogParkLocation as any; const tracesSampler = jest.fn(); - hub.bindClient(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + hub.bindClient(new BrowserClient(options, setupBrowserTransport(options).transport)); // setting up the BrowserTracing integration normally automatically starts a pageload transaction, but that's not // what we're testing here createBrowserTracing(true, { startTransactionOnPageLoad: false }); diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 2ba1c906818a..a31aa10d5c8a 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -1,4 +1,5 @@ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; import { Hub, makeMain } from '@sentry/hub'; import * as utils from '@sentry/utils'; @@ -72,7 +73,8 @@ describe('callbacks', () => { }; beforeAll(() => { - hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); }); diff --git a/packages/tracing/test/errors.test.ts b/packages/tracing/test/errors.test.ts index 43583b352ef1..ca042672b595 100644 --- a/packages/tracing/test/errors.test.ts +++ b/packages/tracing/test/errors.test.ts @@ -1,4 +1,5 @@ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; import { Hub, makeMain } from '@sentry/hub'; import { registerErrorInstrumentation } from '../src/errors'; @@ -33,7 +34,8 @@ describe('registerErrorHandlers()', () => { let hub: Hub; beforeEach(() => { mockAddInstrumentationHandler.mockClear(); - hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); }); diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 5e4c8a806cac..045d9fa96fb1 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -1,5 +1,6 @@ /* eslint-disable @typescript-eslint/unbound-method */ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; import { Hub, makeMain } from '@sentry/hub'; import * as utilsModule from '@sentry/utils'; // for mocking import { logger } from '@sentry/utils'; @@ -32,7 +33,8 @@ describe('Hub', () => { describe('getTransaction()', () => { it('should find a transaction which has been set on the scope if sampled = true', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); transaction.sampled = true; @@ -45,7 +47,8 @@ describe('Hub', () => { }); it('should find a transaction which has been set on the scope if sampled = false', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark', sampled: false }); @@ -57,7 +60,8 @@ describe('Hub', () => { }); it("should not find an open transaction if it's not on the scope", () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -69,7 +73,8 @@ describe('Hub', () => { describe('default sample context', () => { it('should add transaction context data to default sample context', () => { const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transactionContext = { @@ -85,7 +90,8 @@ describe('Hub', () => { it("should add parent's sampling decision to default sample context", () => { const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const parentSamplingDecsion = false; @@ -103,8 +109,9 @@ describe('Hub', () => { describe('sample()', () => { it('should set sampled = false when tracing is disabled', () => { + const options = {}; // neither tracesSampleRate nor tracesSampler is defined -> tracing disabled - const hub = new Hub(new BrowserClient({})); + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -112,7 +119,8 @@ describe('Hub', () => { }); it('should set sampled = false if tracesSampleRate is 0', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const options = { tracesSampleRate: 0 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -120,7 +128,8 @@ describe('Hub', () => { }); it('should set sampled = true if tracesSampleRate is 1', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -128,7 +137,8 @@ describe('Hub', () => { }); it('should set sampled = true if tracesSampleRate is 1 (without global hub)', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const options = { tracesSampleRate: 1 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); const transaction = hub.startTransaction({ name: 'dogpark' }); expect(transaction.sampled).toBe(true); @@ -136,7 +146,8 @@ describe('Hub', () => { it("should call tracesSampler if it's defined", () => { const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -145,7 +156,8 @@ describe('Hub', () => { it('should set sampled = false if tracesSampler returns 0', () => { const tracesSampler = jest.fn().mockReturnValue(0); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -155,7 +167,8 @@ describe('Hub', () => { it('should set sampled = true if tracesSampler returns 1', () => { const tracesSampler = jest.fn().mockReturnValue(1); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -165,7 +178,8 @@ describe('Hub', () => { it('should set sampled = true if tracesSampler returns 1 (without global hub)', () => { const tracesSampler = jest.fn().mockReturnValue(1); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); const transaction = hub.startTransaction({ name: 'dogpark' }); expect(tracesSampler).toHaveBeenCalled(); @@ -175,7 +189,8 @@ describe('Hub', () => { it('should not try to override explicitly set positive sampling decision', () => { // so that the decision otherwise would be false const tracesSampler = jest.fn().mockReturnValue(0); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark', sampled: true }); @@ -185,7 +200,8 @@ describe('Hub', () => { it('should not try to override explicitly set negative sampling decision', () => { // so that the decision otherwise would be true const tracesSampler = jest.fn().mockReturnValue(1); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark', sampled: false }); @@ -195,7 +211,8 @@ describe('Hub', () => { it('should prefer tracesSampler to tracesSampleRate', () => { // make the two options do opposite things to prove precedence const tracesSampler = jest.fn().mockReturnValue(true); - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0, tracesSampler })); + const options = { tracesSampleRate: 0, tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -205,7 +222,8 @@ describe('Hub', () => { it('should tolerate tracesSampler returning a boolean', () => { const tracesSampler = jest.fn().mockReturnValue(true); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -215,7 +233,8 @@ describe('Hub', () => { it('should record sampling method when sampling decision is explicitly set', () => { const tracesSampler = jest.fn().mockReturnValue(0.1121); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark', sampled: true }); @@ -226,7 +245,8 @@ describe('Hub', () => { it('should record sampling method and rate when sampling decision comes from tracesSampler', () => { const tracesSampler = jest.fn().mockReturnValue(0.1121); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -236,7 +256,8 @@ describe('Hub', () => { }); it('should record sampling method when sampling decision is inherited', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 })); + const options = { tracesSampleRate: 0.1121 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark', parentSampled: true }); @@ -246,7 +267,8 @@ describe('Hub', () => { }); it('should record sampling method and rate when sampling decision comes from traceSampleRate', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 })); + const options = { tracesSampleRate: 0.1121 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -258,7 +280,8 @@ describe('Hub', () => { describe('isValidSampleRate()', () => { it("should reject tracesSampleRates which aren't numbers or booleans", () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); + const options = { tracesSampleRate: 'dogs!' as any }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -266,7 +289,8 @@ describe('Hub', () => { }); it('should reject tracesSampleRates which are NaN', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); + const options = { tracesSampleRate: 'dogs!' as any }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -275,7 +299,8 @@ describe('Hub', () => { // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampleRates less than 0', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); + const options = { tracesSampleRate: -26 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -284,7 +309,8 @@ describe('Hub', () => { // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampleRates greater than 1', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 26 })); + const options = { tracesSampleRate: 26 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -293,7 +319,8 @@ describe('Hub', () => { it("should reject tracesSampler return values which aren't numbers or booleans", () => { const tracesSampler = jest.fn().mockReturnValue('dogs!'); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -302,7 +329,8 @@ describe('Hub', () => { it('should reject tracesSampler return values which are NaN', () => { const tracesSampler = jest.fn().mockReturnValue(NaN); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -312,7 +340,8 @@ describe('Hub', () => { // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampler return values less than 0', () => { const tracesSampler = jest.fn().mockReturnValue(-12); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -322,7 +351,8 @@ describe('Hub', () => { // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampler return values greater than 1', () => { const tracesSampler = jest.fn().mockReturnValue(31); - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); hub.startTransaction({ name: 'dogpark' }); @@ -331,7 +361,8 @@ describe('Hub', () => { }); it('should drop transactions with sampled = false', () => { - const client = new BrowserClient({ tracesSampleRate: 0 }); + const options = { tracesSampleRate: 0 }; + const client = new BrowserClient(options, setupBrowserTransport(options).transport); jest.spyOn(client, 'captureEvent'); const hub = new Hub(client); @@ -348,7 +379,8 @@ describe('Hub', () => { describe('sampling inheritance', () => { it('should propagate sampling decision to child spans', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: Math.random() })); + const options = { tracesSampleRate: Math.random() }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); const child = transaction.startChild({ op: 'ball.chase' }); @@ -360,13 +392,12 @@ describe('Hub', () => { testOnlyIfNodeVersionAtLeast(10)( 'should propagate positive sampling decision to child transactions in XHR header', async () => { - const hub = new Hub( - new BrowserClient({ - dsn: 'https://1231@dogs.are.great/1121', - tracesSampleRate: 1, - integrations: [new BrowserTracing()], - }), - ); + const options = { + dsn: 'https://1231@dogs.are.great/1121', + tracesSampleRate: 1, + integrations: [new BrowserTracing()], + }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -402,13 +433,12 @@ describe('Hub', () => { testOnlyIfNodeVersionAtLeast(10)( 'should propagate negative sampling decision to child transactions in XHR header', async () => { - const hub = new Hub( - new BrowserClient({ - dsn: 'https://1231@dogs.are.great/1121', - tracesSampleRate: 1, - integrations: [new BrowserTracing()], - }), - ); + const options = { + dsn: 'https://1231@dogs.are.great/1121', + tracesSampleRate: 1, + integrations: [new BrowserTracing()], + }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ name: 'dogpark', sampled: false }); @@ -453,7 +483,8 @@ describe('Hub', () => { // sample rate), so make parent's decision the opposite to prove that inheritance takes precedence over // tracesSampleRate mathRandom.mockReturnValueOnce(1); - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.5 })); + const options = { tracesSampleRate: 0.5 }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const parentSamplingDecsion = true; @@ -467,9 +498,10 @@ describe('Hub', () => { }); it("should inherit parent's negative sampling decision if tracesSampler is undefined", () => { + const options = { tracesSampleRate: 1 }; // tracesSampleRate = 1 means every transaction should end up with sampled = true, so make parent's decision the // opposite to prove that inheritance takes precedence over tracesSampleRate - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const parentSamplingDecsion = false; @@ -488,7 +520,8 @@ describe('Hub', () => { const tracesSampler = () => true; const parentSamplingDecsion = false; - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ @@ -506,7 +539,8 @@ describe('Hub', () => { const tracesSampler = () => false; const parentSamplingDecsion = true; - const hub = new Hub(new BrowserClient({ tracesSampler })); + const options = { tracesSampler }; + const hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); makeMain(hub); const transaction = hub.startTransaction({ diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts index 4b0887f2dda7..fe11fbf143ee 100644 --- a/packages/tracing/test/span.test.ts +++ b/packages/tracing/test/span.test.ts @@ -1,4 +1,5 @@ import { BrowserClient } from '@sentry/browser'; +import { setupBrowserTransport } from '@sentry/browser/src/transports'; import { Hub, makeMain, Scope } from '@sentry/hub'; import { Span, Transaction } from '../src'; @@ -9,7 +10,8 @@ describe('Span', () => { beforeEach(() => { const myScope = new Scope(); - hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }), myScope); + const options = { tracesSampleRate: 1 }; + hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport), myScope); makeMain(hub); }); @@ -216,12 +218,11 @@ describe('Span', () => { }); test('maxSpans correctly limits number of spans', () => { - const _hub = new Hub( - new BrowserClient({ - _experiments: { maxSpans: 3 }, - tracesSampleRate: 1, - }), - ); + const options = { + _experiments: { maxSpans: 3 }, + tracesSampleRate: 1, + }; + const _hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); const spy = jest.spyOn(_hub as any, 'captureEvent') as any; const transaction = _hub.startTransaction({ name: 'test' }); for (let i = 0; i < 10; i++) { @@ -233,11 +234,10 @@ describe('Span', () => { }); test('no span recorder created if transaction.sampled is false', () => { - const _hub = new Hub( - new BrowserClient({ - tracesSampleRate: 1, - }), - ); + const options = { + tracesSampleRate: 1, + }; + const _hub = new Hub(new BrowserClient(options, setupBrowserTransport(options).transport)); const spy = jest.spyOn(_hub as any, 'captureEvent') as any; const transaction = _hub.startTransaction({ name: 'test', sampled: false }); for (let i = 0; i < 10; i++) { From 22c0e7c1c1a222dc0557d4d60152b33f0d3a52f5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 11:18:56 +0200 Subject: [PATCH 09/17] fix linter errors --- .../suites/sessions/errored-session-aggregate/test.ts | 2 -- packages/tracing/test/browser/browsertracing.test.ts | 1 - 2 files changed, 3 deletions(-) diff --git a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts index 5b475af63055..c8f2331e39b0 100644 --- a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts +++ b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts @@ -10,10 +10,8 @@ test('should aggregate successful, crashed and erroneous sessions', async () => getEnvelopeRequest(`${url}/error_handled`), getEnvelopeRequest(`${url}/error_unhandled`), ]); - console.log(envelope); expect(envelope).toHaveLength(3); - console.log(envelope[0]); expect(envelope[0]).toMatchObject({ sent_at: expect.any(String), diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 8354bee0b8be..510befc29064 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -1,6 +1,5 @@ import { BrowserClient } from '@sentry/browser'; import { setupBrowserTransport } from '@sentry/browser/src/transports'; -import { NoopTransport } from '@sentry/core'; import { Hub, makeMain } from '@sentry/hub'; import { getGlobalObject } from '@sentry/utils'; import { JSDOM } from 'jsdom'; From 4b951a76bafbb01232da6f4e5c44618ab9326c8c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 11:28:36 +0200 Subject: [PATCH 10/17] fix more core unit tests --- packages/core/test/lib/base.test.ts | 3 ++- packages/core/test/lib/sdk.test.ts | 20 ++++++++++++-------- packages/core/test/mocks/client.ts | 3 ++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 09eb3c77b78d..72c0ef63c211 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -983,8 +983,9 @@ describe('BaseClient', () => { for (const val of invalidValues) { const beforeSend = jest.fn(() => val); + const options = { dsn: PUBLIC_DSN, beforeSend }; // @ts-ignore we need to test regular-js behavior - const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend }); + const client = new TestClient(options, setupTestTransport({ dsn: PUBLIC_DSN }).transport); const loggerErrorSpy = jest.spyOn(logger, 'error'); client.captureEvent({ message: 'hello' }); diff --git a/packages/core/test/lib/sdk.test.ts b/packages/core/test/lib/sdk.test.ts index 04f704d0698e..5bc5f8969f68 100644 --- a/packages/core/test/lib/sdk.test.ts +++ b/packages/core/test/lib/sdk.test.ts @@ -3,7 +3,7 @@ import { Client, Integration } from '@sentry/types'; import { installedIntegrations } from '../../src/integration'; import { initAndBind } from '../../src/sdk'; -import { TestClient } from '../mocks/client'; +import { setupTestTransport, TestClient } from '../mocks/client'; // eslint-disable-next-line no-var declare var global: any; @@ -55,7 +55,8 @@ describe('SDK', () => { new MockIntegration('MockIntegration 1'), new MockIntegration('MockIntegration 2'), ]; - initAndBind(TestClient, { dsn: PUBLIC_DSN, defaultIntegrations: DEFAULT_INTEGRATIONS }); + const options = { dsn: PUBLIC_DSN, defaultIntegrations: DEFAULT_INTEGRATIONS }; + initAndBind(TestClient, options, setupTestTransport(options).transport); expect((DEFAULT_INTEGRATIONS[0].setupOnce as jest.Mock).mock.calls.length).toBe(1); expect((DEFAULT_INTEGRATIONS[1].setupOnce as jest.Mock).mock.calls.length).toBe(1); }); @@ -65,7 +66,8 @@ describe('SDK', () => { new MockIntegration('MockIntegration 1'), new MockIntegration('MockIntegration 2'), ]; - initAndBind(TestClient, { dsn: PUBLIC_DSN, defaultIntegrations: false }); + const options = { dsn: PUBLIC_DSN, defaultIntegrations: false as false }; + initAndBind(TestClient, options, setupTestTransport(options).transport); expect((DEFAULT_INTEGRATIONS[0].setupOnce as jest.Mock).mock.calls.length).toBe(0); expect((DEFAULT_INTEGRATIONS[1].setupOnce as jest.Mock).mock.calls.length).toBe(0); }); @@ -75,7 +77,8 @@ describe('SDK', () => { new MockIntegration('MockIntegration 1'), new MockIntegration('MockIntegration 2'), ]; - initAndBind(TestClient, { dsn: PUBLIC_DSN, integrations }); + const options = { dsn: PUBLIC_DSN, integrations }; + initAndBind(TestClient, options, setupTestTransport(options).transport); expect((integrations[0].setupOnce as jest.Mock).mock.calls.length).toBe(1); expect((integrations[1].setupOnce as jest.Mock).mock.calls.length).toBe(1); }); @@ -89,7 +92,8 @@ describe('SDK', () => { new MockIntegration('MockIntegration 1'), new MockIntegration('MockIntegration 3'), ]; - initAndBind(TestClient, { dsn: PUBLIC_DSN, defaultIntegrations: DEFAULT_INTEGRATIONS, integrations }); + const options = { dsn: PUBLIC_DSN, defaultIntegrations: DEFAULT_INTEGRATIONS, integrations }; + initAndBind(TestClient, options, setupTestTransport(options).transport); // 'MockIntegration 1' should be overridden by the one with the same name provided through options expect((DEFAULT_INTEGRATIONS[0].setupOnce as jest.Mock).mock.calls.length).toBe(0); expect((DEFAULT_INTEGRATIONS[1].setupOnce as jest.Mock).mock.calls.length).toBe(1); @@ -103,12 +107,12 @@ describe('SDK', () => { new MockIntegration('MockIntegration 2'), ]; const newIntegration = new MockIntegration('MockIntegration 3'); - initAndBind(TestClient, { - // Take only the first one and add a new one to it + const options = { defaultIntegrations: DEFAULT_INTEGRATIONS, dsn: PUBLIC_DSN, integrations: (integrations: Integration[]) => integrations.slice(0, 1).concat(newIntegration), - }); + }; + initAndBind(TestClient, options, setupTestTransport(options).transport); expect((DEFAULT_INTEGRATIONS[0].setupOnce as jest.Mock).mock.calls.length).toBe(1); expect((newIntegration.setupOnce as jest.Mock).mock.calls.length).toBe(1); expect((DEFAULT_INTEGRATIONS[1].setupOnce as jest.Mock).mock.calls.length).toBe(0); diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index f238887fe459..2a49f9fea757 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -1,5 +1,5 @@ import { Session } from '@sentry/hub'; -import { Event, Options, Severity, SeverityLevel, Transport } from '@sentry/types'; +import { Event, Integration, Options, Severity, SeverityLevel, Transport } from '@sentry/types'; import { resolvedSyncPromise } from '@sentry/utils'; import { BaseClient } from '../../src/baseclient'; @@ -10,6 +10,7 @@ export interface TestOptions extends Options { test?: boolean; mockInstallFailure?: boolean; enableSend?: boolean; + defaultIntegrations?: Integration[] | false; } export class TestClient extends BaseClient { From f1bce9070c96c1d1288b8be8a81419238a258335 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 12:11:34 +0200 Subject: [PATCH 11/17] fix linter error --- packages/core/test/lib/sdk.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/sdk.test.ts b/packages/core/test/lib/sdk.test.ts index 5bc5f8969f68..578361a97511 100644 --- a/packages/core/test/lib/sdk.test.ts +++ b/packages/core/test/lib/sdk.test.ts @@ -3,7 +3,7 @@ import { Client, Integration } from '@sentry/types'; import { installedIntegrations } from '../../src/integration'; import { initAndBind } from '../../src/sdk'; -import { setupTestTransport, TestClient } from '../mocks/client'; +import { setupTestTransport, TestClient, TestOptions } from '../mocks/client'; // eslint-disable-next-line no-var declare var global: any; @@ -66,7 +66,7 @@ describe('SDK', () => { new MockIntegration('MockIntegration 1'), new MockIntegration('MockIntegration 2'), ]; - const options = { dsn: PUBLIC_DSN, defaultIntegrations: false as false }; + const options: TestOptions = { dsn: PUBLIC_DSN, defaultIntegrations: false }; initAndBind(TestClient, options, setupTestTransport(options).transport); expect((DEFAULT_INTEGRATIONS[0].setupOnce as jest.Mock).mock.calls.length).toBe(0); expect((DEFAULT_INTEGRATIONS[1].setupOnce as jest.Mock).mock.calls.length).toBe(0); From 71c70583481166d5ad4c66593e150e593dbb7a66 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 13:12:09 +0200 Subject: [PATCH 12/17] add setupBrowserTransport tests --- packages/browser/src/transports/setup.ts | 1 + .../test/unit/transports/setup.test.ts | 87 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 packages/browser/test/unit/transports/setup.test.ts diff --git a/packages/browser/src/transports/setup.ts b/packages/browser/src/transports/setup.ts index a639418613aa..4256f53a8673 100644 --- a/packages/browser/src/transports/setup.ts +++ b/packages/browser/src/transports/setup.ts @@ -46,6 +46,7 @@ export function setupBrowserTransport(options: BrowserOptions): { transport: Tra if (options.transport) { return { transport: new options.transport(transportOptions) }; } + if (supportsFetch()) { const requestOptions: RequestInit = { ...transportOptions.fetchParameters }; const newTransport = makeNewFetchTransport({ requestOptions, url }); diff --git a/packages/browser/test/unit/transports/setup.test.ts b/packages/browser/test/unit/transports/setup.test.ts new file mode 100644 index 000000000000..42fea886e91e --- /dev/null +++ b/packages/browser/test/unit/transports/setup.test.ts @@ -0,0 +1,87 @@ +import { NoopTransport } from '@sentry/core'; +import { FetchTransport, setupBrowserTransport, XHRTransport } from '../../../src/transports'; +import { SimpleTransport } from '../mocks/simpletransport'; + +const DSN = 'https://username@domain/123'; + +let fetchSupported = true; +let getNativeFetchImplCalled = false; + +jest.mock('@sentry/utils', () => { + const original = jest.requireActual('@sentry/utils'); + return { + ...original, + supportsFetch(): boolean { + return fetchSupported; + }, + getGlobalObject(): any { + return { + fetch: () => {}, + }; + }, + }; +}); + +jest.mock('@sentry/browser/src/transports/utils', () => { + const original = jest.requireActual('@sentry/browser/src/transports/utils'); + return { + ...original, + getNativeFetchImplementation() { + getNativeFetchImplCalled = true; + return { + fetch: () => {}, + }; + }, + }; +}); + +describe('setupBrowserTransport', () => { + beforeEach(() => { + getNativeFetchImplCalled = false; + }); + + it('returns NoopTransport if no dsn is passed', () => { + const { transport, newTransport } = setupBrowserTransport({}); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(NoopTransport); + expect(newTransport).toBeUndefined(); + }); + + it('returns the instantiated transport passed via the options', () => { + const options = { dsn: DSN, transport: SimpleTransport }; + const { transport, newTransport } = setupBrowserTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(SimpleTransport); + expect(newTransport).toBeUndefined(); + }); + + it('returns fetchTransports if fetch is supported', () => { + const options = { dsn: DSN }; + const { transport, newTransport } = setupBrowserTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(FetchTransport); + expect(newTransport).toBeDefined(); + // This is a weird way of testing that `newTransport` is using fetch but it works. + // Given that the new transports are functions, we cannot test their instance. + // Totally open for suggestions how to test this better here + expect(getNativeFetchImplCalled).toBe(true); + }); + + it('returns xhrTransports if fetch is not supported', () => { + fetchSupported = false; + + const options = { dsn: DSN }; + const { transport, newTransport } = setupBrowserTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(XHRTransport); + expect(newTransport).toBeDefined(); + // This is a weird way of testing that `newTransport` is using fetch but it works. + // Given that the new transports are functions, we cannot test their instance. + // Totally open for suggestions how to test this better here + expect(getNativeFetchImplCalled).toBe(false); + }); +}); From 9c0094a77c490ec8fdad9373e9324b0279b9dd38 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 14:30:39 +0200 Subject: [PATCH 13/17] fix linter errors --- packages/browser/test/unit/index.test.ts | 2 +- packages/browser/test/unit/transports/setup.test.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index ba458ec91554..9371e60073c1 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -269,7 +269,7 @@ describe('SentryBrowser initialization', () => { it('should set SDK data when instantiating a client directly', () => { const client = new BrowserClient({ dsn }, new SimpleTransport({ dsn })); - const sdkData = (getCurrentHub().getClient() as any).getTransport()._api.metadata?.sdk; + const sdkData = (client.getTransport() as any)._api.metadata?.sdk; expect(sdkData.name).toBe('sentry.javascript.browser'); expect(sdkData.packages[0].name).toBe('npm:@sentry/browser'); diff --git a/packages/browser/test/unit/transports/setup.test.ts b/packages/browser/test/unit/transports/setup.test.ts index 42fea886e91e..86decf271762 100644 --- a/packages/browser/test/unit/transports/setup.test.ts +++ b/packages/browser/test/unit/transports/setup.test.ts @@ -1,4 +1,5 @@ import { NoopTransport } from '@sentry/core'; + import { FetchTransport, setupBrowserTransport, XHRTransport } from '../../../src/transports'; import { SimpleTransport } from '../mocks/simpletransport'; From 827b8ca6503f66482f5d750b0e73eef7de729fb4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 14:55:59 +0200 Subject: [PATCH 14/17] add basic setupNodeTransport tests all tests will need refactoring once the new transports become the default --- packages/node/test/transports/setup.test.ts | 44 +++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 packages/node/test/transports/setup.test.ts diff --git a/packages/node/test/transports/setup.test.ts b/packages/node/test/transports/setup.test.ts new file mode 100644 index 000000000000..98d9e9efc413 --- /dev/null +++ b/packages/node/test/transports/setup.test.ts @@ -0,0 +1,44 @@ +import { NoopTransport } from '@sentry/core'; +import { FakeTransport } from '@sentry/core/test/mocks/transport'; +import { HTTPSTransport, HTTPTransport, setupNodeTransport } from '@sentry/node/src/transports'; + +const DSN = 'https://username@domain/123'; + +describe('setupNodeTransport', () => { + it('returns NoopTransport if no dsn is passed', () => { + const { transport, newTransport } = setupNodeTransport({}); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(NoopTransport); + expect(newTransport).toBeUndefined(); + }); + + it('returns the instantiated transport passed via the options', () => { + const options = { dsn: DSN, transport: FakeTransport }; + const { transport, newTransport } = setupNodeTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(FakeTransport); + expect(newTransport).toBeUndefined(); + }); + + it('returns HTTPS transport as a default', () => { + const options = { dsn: DSN }; + const { transport, newTransport } = setupNodeTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(HTTPSTransport); + expect(newTransport).toBeDefined(); + }); + + it('returns HTTP transport if specified in the dsn', () => { + // fetchSupported = false; + + const options = { dsn: 'http://username@domain/123' }; + const { transport, newTransport } = setupNodeTransport(options); + + expect(transport).toBeDefined(); + expect(transport).toBeInstanceOf(HTTPTransport); + expect(newTransport).toBeDefined(); + }); +}); From a555fad6f5c4003a1d463f21bb1461c5eddd2f2d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 15:52:45 +0200 Subject: [PATCH 15/17] improve tests --- .../test/unit/transports/setup.test.ts | 52 +++++++++++-------- packages/node/test/transports/setup.test.ts | 26 +++++++++- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/packages/browser/test/unit/transports/setup.test.ts b/packages/browser/test/unit/transports/setup.test.ts index 86decf271762..2683a0619aea 100644 --- a/packages/browser/test/unit/transports/setup.test.ts +++ b/packages/browser/test/unit/transports/setup.test.ts @@ -1,12 +1,17 @@ import { NoopTransport } from '@sentry/core'; -import { FetchTransport, setupBrowserTransport, XHRTransport } from '../../../src/transports'; +import { + FetchTransport, + makeNewFetchTransport, + makeNewXHRTransport, + setupBrowserTransport, + XHRTransport, +} from '../../../src/transports'; import { SimpleTransport } from '../mocks/simpletransport'; const DSN = 'https://username@domain/123'; let fetchSupported = true; -let getNativeFetchImplCalled = false; jest.mock('@sentry/utils', () => { const original = jest.requireActual('@sentry/utils'); @@ -23,23 +28,32 @@ jest.mock('@sentry/utils', () => { }; }); -jest.mock('@sentry/browser/src/transports/utils', () => { - const original = jest.requireActual('@sentry/browser/src/transports/utils'); +jest.mock('../../../src/transports/new-fetch', () => { + const original = jest.requireActual('../../../src/transports/new-fetch'); return { ...original, - getNativeFetchImplementation() { - getNativeFetchImplCalled = true; - return { - fetch: () => {}, - }; - }, + makeNewFetchTransport: jest.fn(() => ({ + send: () => Promise.resolve({ status: 'success' }), + flush: () => Promise.resolve(true), + })), + }; +}); + +jest.mock('../../../src/transports/new-xhr', () => { + const original = jest.requireActual('../../../src/transports/new-xhr'); + return { + ...original, + makeNewXHRTransport: jest.fn(() => ({ + send: () => Promise.resolve({ status: 'success' }), + flush: () => Promise.resolve(true), + })), }; }); describe('setupBrowserTransport', () => { - beforeEach(() => { - getNativeFetchImplCalled = false; - }); + afterEach(() => jest.clearAllMocks()); + + afterAll(() => jest.resetAllMocks()); it('returns NoopTransport if no dsn is passed', () => { const { transport, newTransport } = setupBrowserTransport({}); @@ -65,10 +79,8 @@ describe('setupBrowserTransport', () => { expect(transport).toBeDefined(); expect(transport).toBeInstanceOf(FetchTransport); expect(newTransport).toBeDefined(); - // This is a weird way of testing that `newTransport` is using fetch but it works. - // Given that the new transports are functions, we cannot test their instance. - // Totally open for suggestions how to test this better here - expect(getNativeFetchImplCalled).toBe(true); + expect(makeNewFetchTransport).toHaveBeenCalledTimes(1); + expect(makeNewXHRTransport).toHaveBeenCalledTimes(0); }); it('returns xhrTransports if fetch is not supported', () => { @@ -80,9 +92,7 @@ describe('setupBrowserTransport', () => { expect(transport).toBeDefined(); expect(transport).toBeInstanceOf(XHRTransport); expect(newTransport).toBeDefined(); - // This is a weird way of testing that `newTransport` is using fetch but it works. - // Given that the new transports are functions, we cannot test their instance. - // Totally open for suggestions how to test this better here - expect(getNativeFetchImplCalled).toBe(false); + expect(makeNewFetchTransport).toHaveBeenCalledTimes(0); + expect(makeNewXHRTransport).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/node/test/transports/setup.test.ts b/packages/node/test/transports/setup.test.ts index 98d9e9efc413..297ff86266f0 100644 --- a/packages/node/test/transports/setup.test.ts +++ b/packages/node/test/transports/setup.test.ts @@ -2,9 +2,26 @@ import { NoopTransport } from '@sentry/core'; import { FakeTransport } from '@sentry/core/test/mocks/transport'; import { HTTPSTransport, HTTPTransport, setupNodeTransport } from '@sentry/node/src/transports'; +import { makeNodeTransport } from '../../src/transports/new'; + +jest.mock('../../src/transports/new', () => { + const original = jest.requireActual('../../src/transports/new'); + return { + ...original, + makeNodeTransport: jest.fn(() => ({ + send: () => Promise.resolve({ status: 'success' }), + flush: () => Promise.resolve(true), + })), + }; +}); + const DSN = 'https://username@domain/123'; describe('setupNodeTransport', () => { + afterEach(() => jest.clearAllMocks()); + + afterAll(() => jest.resetAllMocks()); + it('returns NoopTransport if no dsn is passed', () => { const { transport, newTransport } = setupNodeTransport({}); @@ -23,22 +40,27 @@ describe('setupNodeTransport', () => { }); it('returns HTTPS transport as a default', () => { + // jest.spyOn(nodeTransportCreation, 'makeNodeTransport').mockImplementation(() => ({ + // send: (request: Envelope) => Promise.resolve({ status: 'success' }), + // flush: (timeout: number) => Promise.resolve(true), + // })); + const options = { dsn: DSN }; const { transport, newTransport } = setupNodeTransport(options); expect(transport).toBeDefined(); expect(transport).toBeInstanceOf(HTTPSTransport); expect(newTransport).toBeDefined(); + expect(makeNodeTransport).toHaveBeenCalledTimes(1); }); it('returns HTTP transport if specified in the dsn', () => { - // fetchSupported = false; - const options = { dsn: 'http://username@domain/123' }; const { transport, newTransport } = setupNodeTransport(options); expect(transport).toBeDefined(); expect(transport).toBeInstanceOf(HTTPTransport); expect(newTransport).toBeDefined(); + expect(makeNodeTransport).toHaveBeenCalledTimes(1); }); }); From 86cfd73c3c114c29a65018ca408871bba54baa56 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Apr 2022 17:05:02 +0200 Subject: [PATCH 16/17] delete `setupTransport()`, cleanup TODOs, add doc --- packages/browser/src/client.ts | 50 ++------------------- packages/browser/src/transports/setup.ts | 11 +++-- packages/core/src/baseclient.ts | 8 ---- packages/core/src/transports/base.ts | 8 ---- packages/node/src/client.ts | 56 ++---------------------- packages/node/src/sdk.ts | 1 - packages/node/src/transports/setup.ts | 9 ++-- 7 files changed, 20 insertions(+), 123 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 1db9800d8102..8666eaebe8d5 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -1,19 +1,11 @@ -import { - BaseClient, - getEnvelopeEndpointWithUrlEncodedAuth, - initAPIDetails, - NewTransport, - Scope, - SDK_VERSION, -} from '@sentry/core'; -import { Event, EventHint, Options, Severity, SeverityLevel, Transport, TransportOptions } from '@sentry/types'; -import { getGlobalObject, logger, stackParserFromOptions, supportsFetch } from '@sentry/utils'; +import { BaseClient, NewTransport, Scope, SDK_VERSION } from '@sentry/core'; +import { Event, EventHint, Options, Severity, SeverityLevel, Transport } from '@sentry/types'; +import { getGlobalObject, logger, stackParserFromOptions } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; import { IS_DEBUG_BUILD } from './flags'; import { injectReportDialog, ReportDialogOptions } from './helpers'; import { Breadcrumbs } from './integrations'; -import { FetchTransport, makeNewFetchTransport, makeNewXHRTransport, XHRTransport } from './transports'; /** * Configuration options for the Sentry Browser SDK. @@ -129,40 +121,4 @@ export class BrowserClient extends BaseClient { } super._sendEvent(event); } - - /** - * @inheritDoc - */ - protected _setupTransport(): Transport { - if (!this._options.dsn) { - // We return the noop transport here in case there is no Dsn. - return super._setupTransport(); - } - - const transportOptions: TransportOptions = { - ...this._options.transportOptions, - dsn: this._options.dsn, - tunnel: this._options.tunnel, - sendClientReports: this._options.sendClientReports, - _metadata: this._options._metadata, - }; - - const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); - const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); - - if (this._options.transport) { - return new this._options.transport(transportOptions); - } - if (supportsFetch()) { - const requestOptions: RequestInit = { ...transportOptions.fetchParameters }; - this._newTransport = makeNewFetchTransport({ requestOptions, url }); - return new FetchTransport(transportOptions); - } - - this._newTransport = makeNewXHRTransport({ - url, - headers: transportOptions.headers, - }); - return new XHRTransport(transportOptions); - } } diff --git a/packages/browser/src/transports/setup.ts b/packages/browser/src/transports/setup.ts index 4256f53a8673..0af6aad90676 100644 --- a/packages/browser/src/transports/setup.ts +++ b/packages/browser/src/transports/setup.ts @@ -22,10 +22,15 @@ export interface BrowserTransportOptions extends BaseTransportOptions { } /** - * TODO: additional doc (since this is not part of Client anymore) - * @inheritDoc + * Sets up Browser transports based on the passed `options`. If available, the returned + * transport will use the fetch API. In case fetch is not supported, an XMLHttpRequest + * based transport is created. + * + * @returns an object currently still containing both, the old `Transport` and + * `NewTransport` which will eventually replace `Transport`. Once this is replaced, + * this function will return a ready to use `NewTransport`. */ -// TODO(v7): refactor to only return newTransport +// TODO(v7): Adjust return value when NewTransport is the default export function setupBrowserTransport(options: BrowserOptions): { transport: Transport; newTransport?: NewTransport } { if (!options.dsn) { // We return the noop transport here in case there is no Dsn. diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 9b8ddb61c3d2..2888e9853f5e 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -34,7 +34,6 @@ import { IS_DEBUG_BUILD } from './flags'; import { IntegrationIndex, setupIntegrations } from './integration'; import { createEventEnvelope, createSessionEnvelope } from './request'; import { NewTransport } from './transports/base'; -import { NoopTransport } from './transports/noop'; const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; @@ -687,13 +686,6 @@ export abstract class BaseClient implements Client { ); } - /** - * Sets up the transport so it can be used later to send requests. - */ - protected _setupTransport(): Transport { - return new NoopTransport(); - } - /** * @inheritDoc */ diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 8588765c0748..787e175b9985 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -58,14 +58,6 @@ export interface BaseTransportOptions extends InternalBaseTransportOptions { url: string; } -// TODO(v7): Delete -export interface BrowserTransportOptions extends BaseTransportOptions { - // options to pass into fetch request - fetchParams: Record; - headers?: Record; - sendClientReports?: boolean; -} - export interface NewTransport { send(request: Envelope): PromiseLike; flush(timeout?: number): PromiseLike; diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 630503e79a2f..e660c6c7b421 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -1,18 +1,10 @@ -import { - BaseClient, - getEnvelopeEndpointWithUrlEncodedAuth, - initAPIDetails, - NewTransport, - Scope, - SDK_VERSION, -} from '@sentry/core'; +import { BaseClient, NewTransport, Scope, SDK_VERSION } from '@sentry/core'; import { SessionFlusher } from '@sentry/hub'; -import { Event, EventHint, Severity, SeverityLevel, Transport, TransportOptions } from '@sentry/types'; -import { logger, makeDsn, resolvedSyncPromise, stackParserFromOptions } from '@sentry/utils'; +import { Event, EventHint, Severity, SeverityLevel, Transport } from '@sentry/types'; +import { logger, resolvedSyncPromise, stackParserFromOptions } from '@sentry/utils'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; import { IS_DEBUG_BUILD } from './flags'; -import { HTTPSTransport, HTTPTransport, makeNodeTransport } from './transports'; import { NodeOptions } from './types'; /** @@ -158,46 +150,4 @@ export class NodeClient extends BaseClient { this._sessionFlusher.incrementSessionStatusCount(); } } - - /** - * @inheritDoc - * TODO(v7): delete - */ - protected _setupTransport(): Transport { - if (!this._options.dsn) { - // We return the noop transport here in case there is no Dsn. - return super._setupTransport(); - } - - const dsn = makeDsn(this._options.dsn); - - const transportOptions: TransportOptions = { - ...this._options.transportOptions, - ...(this._options.httpProxy && { httpProxy: this._options.httpProxy }), - ...(this._options.httpsProxy && { httpsProxy: this._options.httpsProxy }), - ...(this._options.caCerts && { caCerts: this._options.caCerts }), - dsn: this._options.dsn, - tunnel: this._options.tunnel, - _metadata: this._options._metadata, - }; - - if (this._options.transport) { - return new this._options.transport(transportOptions); - } - - const api = initAPIDetails(transportOptions.dsn, transportOptions._metadata, transportOptions.tunnel); - const url = getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel); - - this._newTransport = makeNodeTransport({ - url, - headers: transportOptions.headers, - proxy: transportOptions.httpProxy, - caCerts: transportOptions.caCerts, - }); - - if (dsn.protocol === 'http') { - return new HTTPTransport(transportOptions); - } - return new HTTPSTransport(transportOptions); - } } diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index b307a2b67128..0b7fb9bd5671 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -131,7 +131,6 @@ export function init(options: NodeOptions = {}): void { setHubOnCarrier(carrier, getCurrentHub()); } - // TODO(v7): Init transport here and pass it to initAndBind const { transport, newTransport } = setupNodeTransport(options); initAndBind(NodeClient, options, transport, newTransport); diff --git a/packages/node/src/transports/setup.ts b/packages/node/src/transports/setup.ts index 4efb56344256..2cffd2d43ecd 100644 --- a/packages/node/src/transports/setup.ts +++ b/packages/node/src/transports/setup.ts @@ -6,10 +6,13 @@ import { NodeOptions } from '../types'; import { HTTPSTransport, HTTPTransport, makeNodeTransport } from '.'; /** - * TODO(v7): Add documentation - * @inheritDoc + * Sets up Node transport based on the passed `options`. + * + * @returns an object currently still containing both, the old `Transport` and + * `NewTransport` which will eventually replace `Transport`. Once this is replaced, + * this function will return a ready to use `NewTransport`. */ -// TODO(v7): Adjust when NewTransport is the default +// TODO(v7): Adjust return value when NewTransport is the default export function setupNodeTransport(options: NodeOptions): { transport: Transport; newTransport?: NewTransport } { if (!options.dsn) { // We return the noop transport here in case there is no Dsn. From 3afc9ee09cd3901c42e1a4f0094c8120d0501b2c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Apr 2022 11:51:56 +0200 Subject: [PATCH 17/17] apply review suggestions --- .../suites/sessions/errored-session-aggregate/test.ts | 1 - packages/node/test/transports/setup.test.ts | 5 ----- 2 files changed, 6 deletions(-) diff --git a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts index c8f2331e39b0..e26cafa8a586 100644 --- a/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts +++ b/packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts @@ -12,7 +12,6 @@ test('should aggregate successful, crashed and erroneous sessions', async () => ]); expect(envelope).toHaveLength(3); - expect(envelope[0]).toMatchObject({ sent_at: expect.any(String), sdk: { diff --git a/packages/node/test/transports/setup.test.ts b/packages/node/test/transports/setup.test.ts index 297ff86266f0..6864ac89ea1e 100644 --- a/packages/node/test/transports/setup.test.ts +++ b/packages/node/test/transports/setup.test.ts @@ -40,11 +40,6 @@ describe('setupNodeTransport', () => { }); it('returns HTTPS transport as a default', () => { - // jest.spyOn(nodeTransportCreation, 'makeNodeTransport').mockImplementation(() => ({ - // send: (request: Envelope) => Promise.resolve({ status: 'success' }), - // flush: (timeout: number) => Promise.resolve(true), - // })); - const options = { dsn: DSN }; const { transport, newTransport } = setupNodeTransport(options);