[instrumentation-fetch] refactor fetch() tests for clarity, type safety and realism #5268
Conversation
Refactor `fetch()` tests for improved clearity, type safety and realism (relative to the real-world `fetch` behavior). See the inline comments on PR open-telemetry#5268 for a detailed explaination of the changes.
chancancode
force-pushed
the
refactor-fetch-test
branch
from
1b5ad66 to
e188496
Compare
| @@ -176,7 +176,7 @@ function testForCorrectEvents( | |||
|
|
|||
| describe('fetch', () => { | |||
| let contextManager: ZoneContextManager; | |||
| let lastResponse: any | undefined; | |||
| let lastResponse: Response | undefined; | |||
There was a problem hiding this comment.
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.
| @@ -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)); | |||
There was a problem hiding this comment.
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).
| const response: any = { | ||
| args: {}, | ||
| url: fileUrl, | ||
| }; |
There was a problem hiding this comment.
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.
|
|
||
| // This is the default response body expected by the few tests that cares | ||
| let responseBody = JSON.stringify({ | ||
| isServerResponse: true, |
There was a problem hiding this comment.
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.
| // 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)); |
There was a problem hiding this comment.
This branch was replaced entirely. Note that:
- 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. - To avoid a regression, I replaced the spirit of this with the
assert.okat the top. - This behavior is misleading and in correct – the real
fetch()neverreject()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.
| } | ||
|
|
||
| resolve(new window.Response(responseBody, responseInit)); |
There was a problem hiding this comment.
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.
| Object.keys(lastResponse.headers).forEach(key => { | ||
| headers[key.toLowerCase()] = lastResponse.headers[key]; | ||
| }); | ||
| lastResponse.headers = headers; |
There was a problem hiding this comment.
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.
| assert.strictEqual( | ||
| lastResponse.headers[X_B3_TRACE_ID], | ||
| request.headers[X_B3_TRACE_ID], |
There was a problem hiding this comment.
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).
| assert.fail(response.statusText); | ||
| } | ||
| ); | ||
| }); |
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #5268 +/- ##
=======================================
Coverage 94.56% 94.56%
=======================================
Files 316 316
Lines 8037 8037
Branches 1628 1628
=======================================
Hits 7600 7600
Misses 437 437 |
Which problem is this PR solving?
Refactor
fetch()tests for improved clearity, type safety and realism (relative to the real-worldfetchbehavior).I would like to propose something for #4888 to start a discussion, but in doing so I find that the current tests are quite confusing to work with, so I split out these refactors to the tests as a separate PR/proposal.
I think this represents a significant enough improvements to the tests that it makes sense as a PR, but I may something else that I want to try/propose next week.
Short description of the changes
See the inline commits on the PR.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
npm run lintnpm run test:browserChecklist:
addedupdated