From d6d4424cc443deeba3615e0c05377d5b6ec66e2c Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 13 Dec 2024 18:06:49 -0800 Subject: [PATCH] [instrumentation-fetch] refactor fetch tests for clearity/safety Refactor `fetch()` tests for improved clearity, type safety and realism (relative to the real-world `fetch` behavior). See the inline comments on PR #5268 for a detailed explaination of the changes. --- .../test/fetch.test.ts | 134 ++++++++---------- 1 file changed, 61 insertions(+), 73 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts index 7f81d7cd42..e05b593b87 100644 --- a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts @@ -176,7 +176,7 @@ function testForCorrectEvents( describe('fetch', () => { let contextManager: ZoneContextManager; - let lastResponse: any | undefined; + let lastResponse: Response | undefined; let requestBody: any | undefined; let webTracerWithZone: api.Tracer; let webTracerProviderWithZone: WebTracerProvider; @@ -210,12 +210,21 @@ describe('fetch', () => { sinon.stub(core.otperformance, 'now').callsFake(() => fakeNow); function fakeFetch(input: RequestInfo | Request, init: RequestInit = {}) { + // Once upon a time, there was a bug (#2411), causing a `Request` object + // to be incorrectly passed to `fetch()` as the second argument + assert.ok(!(init instanceof Request)); + return new Promise((resolve, reject) => { - const response: any = { - args: {}, - url: fileUrl, - }; - response.headers = Object.assign({}, init.headers); + const responseInit: ResponseInit = {}; + + // This is the default response body expected by the few tests that cares + let responseBody = JSON.stringify({ + isServerResponse: true, + request: { + url: fileUrl, + headers: { ...init.headers }, + }, + }); // get the request body if (typeof input === 'string') { @@ -235,28 +244,22 @@ describe('fetch', () => { } else { input.text().then(r => (requestBody = r)); } - - if (init instanceof Request) { - // Passing request as 2nd argument causes missing body bug (#2411) - response.status = 400; - response.statusText = 'Bad Request (Request object as 2nd argument)'; - reject(new window.Response(JSON.stringify(response), response)); - } else if (init.method === 'DELETE') { - response.status = 405; - response.statusText = 'OK'; - resolve(new window.Response('foo', response)); + if (init.method === 'DELETE') { + responseInit.status = 405; + responseInit.statusText = 'OK'; + responseBody = 'foo'; } else if ( (input instanceof Request && input.url === url) || input === url ) { - response.status = 200; - response.statusText = 'OK'; - resolve(new window.Response(JSON.stringify(response), response)); + responseInit.status = 200; + responseInit.statusText = 'OK'; } else { - response.status = 404; - response.statusText = 'Bad request'; - reject(new window.Response(JSON.stringify(response), response)); + responseInit.status = 404; + responseInit.statusText = 'Not found'; } + + resolve(new window.Response(responseBody, responseInit)); }); } @@ -321,26 +324,16 @@ describe('fetch', () => { api.trace.setSpan(api.context.active(), rootSpan), async () => { fakeNow = 0; - try { - const responsePromise = apiCall(); - fakeNow = 300; - const response = await responsePromise; - - // if the url is not ignored, body.read should be called by now - // awaiting for the span to end - if (readSpy.callCount > 0) await spanEnded; - - // this is a bit tricky as the only way to get all request headers from - // fetch is to use json() - lastResponse = await response.json(); - const headers: { [key: string]: string } = {}; - Object.keys(lastResponse.headers).forEach(key => { - headers[key.toLowerCase()] = lastResponse.headers[key]; - }); - lastResponse.headers = headers; - } catch (e) { - lastResponse = undefined; - } + lastResponse = undefined; + + const responsePromise = apiCall(); + fakeNow = 300; + lastResponse = await responsePromise; + + // if the url is not ignored, body.read should be called by now + // awaiting for the span to end + if (readSpy.callCount > 0) await spanEnded; + await sinon.clock.runAllAsync(); } ); @@ -531,20 +524,24 @@ describe('fetch', () => { ]); }); - it('should set trace headers', () => { + it('should set trace headers', async () => { const span: api.Span = exportSpy.args[1][0][0]; + assert.ok(lastResponse instanceof Response); + + const { request } = await lastResponse.json(); + assert.strictEqual( - lastResponse.headers[X_B3_TRACE_ID], + request.headers[X_B3_TRACE_ID], span.spanContext().traceId, `trace header '${X_B3_TRACE_ID}' not set` ); assert.strictEqual( - lastResponse.headers[X_B3_SPAN_ID], + request.headers[X_B3_SPAN_ID], span.spanContext().spanId, `trace header '${X_B3_SPAN_ID}' not set` ); assert.strictEqual( - lastResponse.headers[X_B3_SAMPLED], + request.headers[X_B3_SAMPLED], String(span.spanContext().traceFlags), `trace header '${X_B3_SAMPLED}' not set` ); @@ -593,18 +590,6 @@ describe('fetch', () => { assert.ok(init.headers.get('foo') === 'bar'); }); - it('should pass request object as first parameter to the original function (#2411)', () => { - const r = new Request(url); - return window.fetch(r).then( - () => { - assert.ok(true); - }, - (response: Response) => { - assert.fail(response.statusText); - } - ); - }); - it('should NOT clear the resources', () => { assert.strictEqual( clearResourceTimingsSpy.args.length, @@ -627,19 +612,23 @@ describe('fetch', () => { sinon.restore(); }); - it('should NOT set trace headers', () => { + it('should NOT set trace headers', async () => { + assert.ok(lastResponse instanceof Response); + + const { request } = await lastResponse.json(); + assert.strictEqual( - lastResponse.headers[X_B3_TRACE_ID], + request.headers[X_B3_TRACE_ID], undefined, `trace header '${X_B3_TRACE_ID}' should not be set` ); assert.strictEqual( - lastResponse.headers[X_B3_SPAN_ID], + request.headers[X_B3_SPAN_ID], undefined, `trace header '${X_B3_SPAN_ID}' should not be set` ); assert.strictEqual( - lastResponse.headers[X_B3_SAMPLED], + request.headers[X_B3_SAMPLED], undefined, `trace header '${X_B3_SAMPLED}' should not be set` ); @@ -959,7 +948,7 @@ describe('fetch', () => { await prepare(url, applyCustomAttributes); const rsp = await response.json(); - assert.deepStrictEqual(rsp.args, {}); + assert.strictEqual(rsp.isServerResponse, true); }); }); @@ -971,22 +960,21 @@ describe('fetch', () => { ignoreUrls: [propagateTraceHeaderCorsUrls], }); }); + afterEach(() => { clearData(); }); + it('should NOT create any span', () => { assert.strictEqual(exportSpy.args.length, 0, "span shouldn't b exported"); }); - it('should pass request object as the first parameter to the original function (#2411)', () => { - const r = new Request(url); - return window.fetch(r).then( - () => { - assert.ok(true); - }, - (response: Response) => { - assert.fail(response.statusText); - } - ); + + it('should accept Request objects as argument (#2411)', async () => { + const response = await window.fetch(new Request(url)); + assert.ok(response instanceof Response); + + const { isServerResponse } = await response.json(); + assert.strictEqual(isServerResponse, true); }); });