Add the fetch-mock-jest library to replace the manual mocking of fetch #4020

julienw · 2022-04-29T07:45:34Z

This mock mocks the fetch command. fetch can be asserted like a jest mock function (it is a mock function) but its behavior can be configured in a more expressive way.
I considered also using nock like we do in the server, but settled on that library for now for this reason mainly.

The library also comes with custom jest assertion matchers, but I didn't add those to the jest library definition yet, because I didn't use them here. Actually I found that jest's usual matchers work fine already.

Most test files are much simpler, some of them are similar.

Be careful as the changes for receive-profile.test.js is hidden by default by github.

src/types/libdef/npm/node-fetch_v2.x.x.js

package.json

codecov · 2022-04-29T07:51:18Z

Codecov Report

Merging #4020 (d4a1262) into main (3aed2a9) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head d4a1262 differs from pull request most recent head acb64c9. Consider uploading reports for the commit acb64c9 to get more accurate results

@@            Coverage Diff             @@
##             main    #4020      +/-   ##
==========================================
- Coverage   87.29%   87.29%   -0.01%     
==========================================
  Files         279      278       -1     
  Lines       24088    24078      -10     
  Branches     6374     6377       +3     
==========================================
- Hits        21027    21018       -9     
+ Misses       2820     2819       -1     
  Partials      241      241

Impacted Files	Coverage Δ
src/components/timeline/TrackThread.js	`89.65% <0.00%> (-2.56%)`	⬇️
src/components/app/TabBar.js	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3aed2a9...acb64c9. Read the comment docs.

yarn.lock

julienw · 2022-04-29T07:56:30Z

src/test/setup.js

+beforeEach(function () {
+  // Install fetch and fetch-related objects globally.
+  // Using the sandbox ensures that parallel tests run properly.
+  global.fetch = fetchMock.sandbox();


This isn't strictly the recommended way to setup the mock, but the examples are either "in node" or "in browsers", and we're in-between because we're "in node in a jsdom environment".
I initially tried using the cross-fetch library that installs node-fetch as a global directly, and try to apply the mock from here, but couldn't make it work properly. Maybe I didn't try it properly.

Anyway this seems to work fine so I think we can revisit later.
Especially when moving to node v18 and fetch will be available globally in node, we might need to revisit the approach.

julienw · 2022-04-29T07:57:18Z

src/test/setup.js

+  // Do the same with fetch mocks
+  fetchMock.mockReset();


I'm not sure this is strictly needed because we add a new version of the sandbox for each test. But better be safe than sorry :)

Also I'm not deleting the fetch property from the global. This seems to work fine for now but we might need to adjust in the future.

julienw · 2022-04-29T08:00:41Z

src/test/components/AppLocalizationProvider.test.js

@@ -210,8 +188,8 @@ describe('AppLocalizationProvider', () => {

    expect(await screen.findByText(translatedText('de'))).toBeInTheDocument();
    expect(document.documentElement).toHaveAttribute('lang', 'de');
-    expect(window.fetch).toBeCalledWith('/locales/de/app.ftl');
-    expect(window.fetch).toBeCalledWith('/locales/en-US/app.ftl');
+    expect(window.fetch).toBeCalledWith('/locales/de/app.ftl', undefined);


It looks like fetch is always called with 2 arguments now, even if we don't specify the second argument in code, because of how mock-fetch-jest works (it installs another proxy on top of the mock). Therefore we need to match it too in the jest matcher.

julienw · 2022-04-29T08:04:02Z

src/test/components/AppLocalizationProvider.test.js

-    });
-    (window: any).fetch = fetch;
+    window.fetch
+      .catch(404) // catchall


The default for catch is to return an empty 200 response. That's why I specify it everywhere,

However whenever a request goes to the catch there's a warning to the console (this can be disabled). So I tried to specify the rules for every tests so that catch isn't even reached.

While changing this I found the warning useful, that's why I decided to keep it. But we could possibly configure a global catch that would throw instead of returning 200.

julienw · 2022-04-29T08:07:26Z

src/test/components/ListOfPublishedProfiles.test.js

-            statusText: 'Method not allowed',
+      window.fetch
+        .catch(404) // Catchall
+        .mock(endpointUrl, (urlString, options) => {


This one is mostly similar to the previous code, due to wheresrhys/fetch-mock#636

julienw · 2022-04-29T08:07:53Z

src/test/components/Root-history.test.js

-  (window: any).fetch = fetch;
+  window.fetch
+    .catch(404) // catchall
+    .get(url, profile);


much smaller 😁

Definitely! ✨

src/test/components/Root-history.test.js

julienw · 2022-04-29T08:09:21Z

src/test/store/__snapshots__/receive-profile.test.js.snap

-      "arrayBuffer": [Function],
-      "headers": Object {
-        "get": [Function],
+    Response {


The Reponse object is different so these snapshots reflect that change

julienw · 2022-04-29T08:11:24Z

src/test/store/receive-profile.test.js

+        .get(expectedUrl, makeProfileSerializable(_getSimpleProfile()), {
+          overwriteRoutes: false,
+        });


See wheresrhys/fetch-mock#637 for the background on using { overwriteRoutes }.

I don't find this very intuitive to be honest. We can also make overwriteRoutes: false the default in the setup. I didn't do it yet but if we find the behavior is better for us this is possible.

Ah yeah, this looks interesting. Thanks for filing an issue.

julienw · 2022-04-29T08:14:10Z

src/test/store/receive-profile.test.js

+      window.fetch.get(
+        expectedUrl,
+        compress(serializeProfile(_getSimpleProfile())),
+        { sendAsJson: false }


sendAsJson: true is the default behavior. This means that if we send something that's not a normal object (like array buffers here), we need to specify sendAsJson: false. We might want to do the opposite: set sendAsJson false by default, but set it to true when needed. This might be more intuitive. We can also do it in a file setup so that it's global to a test file only.

I decided to wait a bit before changing the default, to see which one would be better.

Note: that's also why you see I replaced encode(serializeProfile(...)) with makeProfileSerializable in a lot of places, to let the library to the JSON converting. But maybe it would be better in this file to always do the conversion ourselves. I guess we can wait and see.

Hm yeah, I don't feel strongly about either solution. Waiting and seeing works for me.

julienw · 2022-04-29T08:17:55Z

src/test/store/receive-profile.test.js

-      isJSON?: true,
-      arrayBuffer?: () => Promise<Uint8Array>,
-      json?: () => Promise<mixed>,
+      content: 'generated-zip' | 'generated-json' | Uint8Array,


note: a bigger change, to better account that isZipped / isJSON / arrayBuffer were mutually exclusive. Also json wasn't used.

julienw · 2022-04-29T08:19:48Z

src/test/unit/profile-store.test.js

-          statusText: 'Method not allowed',
-        });
-      }
+    window.fetch


Similarly to the test for ListOfPublishedProfiles.test.js this isn't wildly different than what it was before.

julienw · 2022-04-29T08:20:33Z

src/test/unit/shorten-url.test.js

+      }
+
+      const payload = JSON.parse(body);
+      return responseFromRequestPayload(payload);


We don't use the functionality to have a response depending on the request body, but I decided to keep it still.

canova

Looks good to me! It looks like it the new mock makes things a lot easier and shorter 🎉

canova · 2022-05-02T09:52:06Z

src/test/components/Root-history.test.js

-  (window: any).fetch = fetch;
+  window.fetch
+    .catch(404) // catchall
+    .get(url, profile);


Definitely! ✨

canova · 2022-05-02T09:55:12Z

src/test/fixtures/mocks/response.js

@@ -1,55 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


canova · 2022-05-02T09:59:44Z

src/test/store/receive-profile.test.js

    beforeEach(function () {
      window.TextDecoder = TextDecoder;
-      window.fetch = jest.fn().mockResolvedValue(fetch403Response);
+      window.fetch.catch(403);


This is really easier to follow with the next get calls and everything!

src/test/store/receive-profile.test.js

canova · 2022-05-02T10:01:54Z

src/test/store/receive-profile.test.js

+        .get(expectedUrl, makeProfileSerializable(_getSimpleProfile()), {
+          overwriteRoutes: false,
+        });


Ah yeah, this looks interesting. Thanks for filing an issue.

canova · 2022-05-02T10:04:13Z

src/test/store/receive-profile.test.js

@@ -1069,7 +1050,7 @@ describe('actions/receive-profile', function () {

    it('fails in case the fetch returns a server error', async function () {
      const hash = 'c5e53f9ab6aecef926d4be68c84f2de550e2ac2f';
-      window.fetch = jest.fn().mockResolvedValue(fetch500Response);
+      window.fetch.any(500);


It took me a while why we needed to use any here (because we have another .catch call in beforeEach block. I was going to recommend using catch but then realized this one. Maybe it could be good to add a comment that explains this here?

The fundamental difference between catch and any is that when catch is reached a warning will be emitted (which is configurable). Ideally I'd like to avoid that, so ideally it would be good to have routes for every request that's supposed to happen in a test. I think that in the future I'd rather add a .catch({ throws: new Error('No route has been defined for this request') }) in setup.js...

any is different: this is an expected request (even if it catches everything). I guess I was a bit lazy at the end of the day, and could have used a get with the full expected hash... I'm changing this :-)

canova · 2022-05-02T10:09:56Z

src/test/store/receive-profile.test.js

+      window.fetch.get(
+        expectedUrl,
+        compress(serializeProfile(_getSimpleProfile())),
+        { sendAsJson: false }


Hm yeah, I don't feel strongly about either solution. Waiting and seeing works for me.

julienw commented Apr 29, 2022

View reviewed changes

src/types/libdef/npm/node-fetch_v2.x.x.js Show resolved Hide resolved

julienw commented Apr 29, 2022

View reviewed changes

package.json Show resolved Hide resolved

julienw force-pushed the fetch-mock branch from b74abae to 39ae0df Compare April 29, 2022 07:51

julienw commented Apr 29, 2022

View reviewed changes

yarn.lock Show resolved Hide resolved

julienw force-pushed the fetch-mock branch from 39ae0df to 510bc30 Compare April 29, 2022 07:54

julienw commented Apr 29, 2022

View reviewed changes

src/test/components/Root-history.test.js Show resolved Hide resolved

julienw commented Apr 29, 2022

View reviewed changes

julienw force-pushed the fetch-mock branch from 510bc30 to d4a1262 Compare April 29, 2022 08:16

julienw commented Apr 29, 2022

View reviewed changes

julienw requested a review from canova April 29, 2022 08:21

julienw mentioned this pull request Apr 29, 2022

Add new tests for the SourceView component #4022

Merged

canova approved these changes May 2, 2022

View reviewed changes

julienw added 3 commits May 2, 2022 14:43

Install the new packages

4b55922

Setup the fetch-mock-jest library

c5de1c0

Update existing tests

acb64c9

julienw force-pushed the fetch-mock branch from d4a1262 to acb64c9 Compare May 2, 2022 12:43

julienw enabled auto-merge May 2, 2022 12:44

julienw merged commit 7f522e4 into firefox-devtools:main May 2, 2022

julienw mentioned this pull request May 3, 2022

Convert to TypeScript mozilla/perfcompare#88

Merged

canova mentioned this pull request May 3, 2022

Deploy May 3, 2022 #4027

Merged

julienw mentioned this pull request Jun 1, 2022

implement add revision mozilla/perfcompare#121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the fetch-mock-jest library to replace the manual mocking of fetch #4020

Add the fetch-mock-jest library to replace the manual mocking of fetch #4020

julienw commented Apr 29, 2022 •

edited

codecov bot commented Apr 29, 2022 •

edited

julienw Apr 29, 2022 •

edited

julienw Apr 29, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022

canova May 2, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022 •

edited

canova May 2, 2022

julienw Apr 29, 2022 •

edited

canova May 2, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022

julienw Apr 29, 2022

canova left a comment

canova May 2, 2022

canova May 2, 2022

canova May 2, 2022

canova May 2, 2022

canova May 2, 2022

julienw May 2, 2022

canova May 2, 2022

		@@ -1,55 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Add the fetch-mock-jest library to replace the manual mocking of fetch #4020

Add the fetch-mock-jest library to replace the manual mocking of fetch #4020

Conversation

julienw commented Apr 29, 2022 • edited

codecov bot commented Apr 29, 2022 • edited

Codecov Report

julienw Apr 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw Apr 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw Apr 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Apr 29, 2022 •

edited

codecov bot commented Apr 29, 2022 •

edited

julienw Apr 29, 2022 •

edited

julienw Apr 29, 2022 •

edited

julienw Apr 29, 2022 •

edited