Skip to content

Commit

Permalink
[instrumentation-fetch] refactor fetch tests for clearity/safety
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@chancancode
chancancode committed Dec 14, 2024
1 parent 6d7a73a commit e188496
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 73 deletions.
1 change: 1 addition & 0 deletions CHANGELOG_NEXT.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

### :house: (Internal)

* refactor(instrumentation-fetch): refactor `fetch()` tests for clarity, type safety and realism [#5268](https://github.com/open-telemetry/opentelemetry-js/pull/5268)
* refactor(sdk-metrics): remove `Gauge` and `MetricAdvice` workaround types in favor of the upstream `@opentelemetry/api` types [#5254](https://github.com/open-telemetry/opentelemetry-js/pull/5254) @chancancode
* chore: remove checks for unsupported node versions [#4341](https://github.com/open-telemetry/opentelemetry-js/pull/4341) @dyladan
* refactor(sdk-trace-base): remove `BasicTracerProvider._registeredSpanProcessors` private property. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna
Expand Down
134 changes: 61 additions & 73 deletions experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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') {
Expand All @@ -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));
});
}

Expand Down Expand Up @@ -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();
}
);
Expand Down Expand Up @@ -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`
);
Expand Down Expand Up @@ -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,
Expand All @@ -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`
);
Expand Down Expand Up @@ -959,7 +948,7 @@ describe('fetch', () => {

await prepare(url, applyCustomAttributes);
const rsp = await response.json();
assert.deepStrictEqual(rsp.args, {});
assert.strictEqual(rsp.isServerResponse, true);
});
});

Expand All @@ -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);
});
});

Expand Down

0 comments on commit e188496

Please sign in to comment.