Skip to content

Conversation

@julienw
Copy link
Contributor

@julienw julienw commented Jun 11, 2025

This PR updates the obsolete fetch-mock-jest to its new version @fetch-mock/jest.
Unfortunately this comes with lots of syntax changes. In particular, previously fetch was both the mock and the configuration object, but now they're 2 different objects.

There's a useful migration page: https://www.wheresrhys.co.uk/fetch-mock/docs/Usage/upgrade-guide

The fact this patch also removes node-fetch brought other problems, I believe due to the interaction with jsdom or jest's vm mechanism, with different cross-realm constructors, so I had to minimally change some app code as well.

Also due to jsdom's not implementing fetch and other fetch-related objects that are now available in nodejs by default (even if not exactly the same), I had to supply a custom environment.

I tried to split the PR by logical commits.

Tell me what you think!

Comment on lines +23 to +24
fetchMock.mockGlobal();
global.fetchMock = fetchMock;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was afraid that this could be an issue when running tests in parallel, but I couldn't make it fail. I think that nowadays jest (or is it the jsdom environment, or just new versions of jsdom) isolates the globals better than it used to.

@julienw julienw requested a review from mstange June 11, 2025 13:01
@codecov
Copy link

codecov bot commented Jun 11, 2025

Codecov Report

Attention: Patch coverage is 38.09524% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.11%. Comparing base (452ec65) to head (7aa6c5f).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/test/custom-environment.js 0.00% 10 Missing and 2 partials ⚠️
src/profile-logic/mozilla-symbolication-api.js 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5488      +/-   ##
==========================================
- Coverage   86.15%   86.11%   -0.04%     
==========================================
  Files         307      308       +1     
  Lines       29637    29652      +15     
  Branches     7999     8001       +2     
==========================================
+ Hits        25533    25536       +3     
- Misses       3516     3526      +10     
- Partials      588      590       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

// in jest apparently.
// Still let's double check that they're from the global scope as expected, so
// that this can be removed once it's implemented.
if ('TextDecoder' in this.global) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not fully sure this condition works, TBH... I don't know if this.global contains all the globals, or contains only the new things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.

Copy link
Member

Choose a reason for hiding this comment

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

In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.

Nice, thanks for checking!

I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.

Ah, do we have TextDecoder in v30?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextDecoder I don't know, but Request, Response, fetch are there (but not ReadableStream surprisingly!)

@julienw julienw requested a review from canova June 12, 2025 12:13
import { TestEnvironment } from 'jest-environment-jsdom';
import { TextDecoder, TextEncoder } from 'util';

export default class CustomTestEnvironment extends TestEnvironment {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Looks good to me. But wow, that's surprisingly a lot of changes. I always get annoyed when a utility/testing package has this big of a breaking change (insert "old man yells at cloud" meme). To be fair, technically it's a different package, but still... At least I'm happy that I didn't have to deal with this, so thank you! 😄

I have some questions but they are mostly so I understand better, not blocking.

],
"transformIgnorePatterns": [
"/node_modules/(?!(query-string|decode-uri-component|split-on-first|filter-obj)/)"
"/node_modules/(?!(query-string|decode-uri-component|split-on-first|filter-obj|@fetch-mock/jest|fetch-mock)/)"
Copy link
Member

Choose a reason for hiding this comment

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

Hm I can't seem to remember this. Why do we need this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because our current configuration doesn't deal with esm import and export when running in jest (I think it should be possible to make it work easily nowadays...).

This configuration property configures jest to not run babel on some files, by default this is all of node_modules. We override this line to run babel on some npm packages that are using export/import.

// in jest apparently.
// Still let's double check that they're from the global scope as expected, so
// that this can be removed once it's implemented.
if ('TextDecoder' in this.global) {
Copy link
Member

Choose a reason for hiding this comment

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

In perfcompare I'm using a newer jsdom that has Request, the followng check actually worked, so it looks like this works after all.

Nice, thanks for checking!

I see jest v30 was just released 2 days ago, this file will probably need to be adjusted with that update.

Ah, do we have TextDecoder in v30?

Comment on lines +98 to +102
const isObject = (subject) =>
Object.prototype.toString.call(subject) === '[object Object]';
Copy link
Member

Choose a reason for hiding this comment

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

Why did we need a function like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is that the object we get inherits from the Object object in another js environment.
Also the previous code didn't work for an object without a prototype (such as when created with Object.create(null)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of wonder if this might be a bug in jest though.

I've seen this in jest's node environment => https://github.com/jestjs/jest/blob/42c8e7de64d1a3f8ee9b7f3b309332ac8f88e8b1/packages/jest-environment-node/src/index.ts#L157-L161
but it's not in the jsdom environment.
This might be a red herring though.

@julienw julienw force-pushed the update-fetch-mock branch from 3f5f7e5 to 7aa6c5f Compare June 16, 2025 09:17
@julienw julienw merged commit 530446e into firefox-devtools:main Jun 16, 2025
13 of 15 checks passed
@canova canova mentioned this pull request Jul 11, 2025
canova added a commit that referenced this pull request Jul 11, 2025
Changes:

[Nazım Can Altınova] Fix issues related to the track borders (#5484)
[Nazım Can Altınova] Remove the active tab and origins views (#5483)
[Julien Wajsberg] Automatically request reviews for dependency updates
(#5490)
[Julien Wajsberg] Update fetch-mock-jest to @fetch-mock/jest (#5488)
[Steve Fink] Document marker filter syntax (#5493)
[Nazım Can Altınova] Update all of jest 29.7.0 → 30.0.0 (major) (#5495)
[Nazım Can Altınova] Order global tracks by activity and select the most
active non-parent process by default (#5491)
[Paul Adenot] Allow searching for HTTP response status in marker views
(#5504)
[Nazım Can Altınova] Expose a `totalMarkerDuration` function in console
(#5507)
[Nazım Can Altınova] 🔃 Sync: l10n -> main (July 11, 2025) (#5510)

And thanks to our localizers:

el: Jim Spentzos
kab: ZiriSut
tr: Grk
tr: Selim Şumlu
uk: Artem Polivanchuk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants