Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[instrumentation-fetch] refactor fetch() tests for clarity, type safety and realism #5268

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
+62 −73
Open

[instrumentation-fetch] refactor fetch() tests for clarity, type safety and realism #5268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG_NEXT.md
View file
Open in desktop
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
Open in desktop
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes more than the type! Despite the name, this variable previously wasn't storing the Response object of the last fetch() request. It stores something that is kind of the parsed response body (~= await response.json()) but also with some additional bespoke processing/normalization that didn't turn out to be necessary.

This changes things so lastResponse actually stores the Response object from the last fetch() request made in the test. Any test that cares about the response body can call await lastResponse.json() in the test itself and do what it needs to do there.

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));
Copy link
Contributor Author

@chancancode chancancode Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a replacement for the init instanceof Request branch below. We don't have this bug anymore, but this assertion will catch it if it regress, and instantly fail all the tests (synchronously).


return new Promise((resolve, reject) => {
const response: any = {
args: {},
url: fileUrl,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is really the "default response body", but it also serves a double-duty as the ResponseInit (second argument to new Response(), which is very confusing. I separated those into appropriately named separate variables.

To be clear, no tests actually cares that the mock "server response body" has the status in it.

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args: {} in the diff is only used by a single test, and all it does with it is assert.deepEquals(parsedResponseBody.args, {}). Maybe it used to do more, but I inferred that these days the only purpose it serves is "did I get the mock response I expected", and so I replaced that with this field.

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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch was replaced entirely. Note that:

  1. It is testing for the bug in fix(instrumentation-fetch): fetch(string, Request) silently drops request body #2411 which has now be fixed, so this branch is never exercised in practice.
  2. To avoid a regression, I replaced the spirit of this with the assert.ok at the top.
  3. This behavior is misleading and in correct – the real fetch() never reject() the fetch promise with a response. Even when the server returns an HTTP error (response.ok === false), the promise is still resolved.

In any case, this branch is not needed anymore.

} 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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now always resolve, as the real fetch() would. The 404 branch is also never exercised in the tests (as evident by the fact that I changed it from resolve() to reject() and nothing broke). As mentioned above, rejecting with a response is never correct.

If we are rejecting here because it's an internal assertion that indicating some test is set up wrong, I'd rather see this reject with an regular Error, or just throw synchronously at the top.

});
}

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the convoluted normalization that didn't turn out to be needed (other than removing this part, most of the diff is just indentation change from removing the now-unecessary try/catch.

As far as I can tell, (other than the previously unfortunate variable naming) all this would do is to normalize the request headers into lower case. However, nothing broke when I stopped doing this, so evidently the two tests that cares don't actually need this.

} 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],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previously badly named variable made this very confusing, but this is actually asserting that we made the request with the correct headers. Specifically that when the URL is not ignored, the instrumentation code will add these additional headers (which then gets put into the mock server response).

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);
}
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was added for #2411. It is basically testing that nothing breaks when passing a Request object into fetch works. However, that pattern is now ubiquitously used in numerous other tests (e.g. see around L550), so I'm not sure this is still necessary anymore. Also as mentioned about, this test is not very realistically written, as the real fetch() would never reject() with a Response like this.


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
Loading