Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
"@babel/preset-env": "^7.27.2",
"@babel/preset-flow": "^7.27.1",
"@babel/preset-react": "^7.27.1",
"@fetch-mock/jest": "^0.2.15",
"@testing-library/dom": "^10.4.0",
"@testing-library/jest-dom": "^6.6.3",
"@testing-library/react": "^16.3.0",
Expand Down Expand Up @@ -146,7 +147,6 @@
"eslint-plugin-testing-library": "^7.2.1",
"espree": "^10.3.0",
"fake-indexeddb": "^6.0.1",
"fetch-mock-jest": "^1.5.1",
"file-loader": "^6.2.0",
"flow-bin": "^0.96.0",
"glob": "^11.0.2",
Expand All @@ -159,7 +159,6 @@
"local-web-server": "^5.4.0",
"lockfile-lint": "^4.14.1",
"mkdirp": "^3.0.1",
"node-fetch": "^2.6.11",
"npm-run-all2": "^8.0.4",
"open": "^10.1.2",
"postcss": "^8.5.4",
Expand Down Expand Up @@ -188,7 +187,7 @@
"jsx"
],
"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
Copy Markdown
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
Copy Markdown
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.

],
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga|ftl)$": "<rootDir>/src/test/fixtures/mocks/file-mock.js",
Expand All @@ -205,7 +204,7 @@
"escapeString": true,
"printBasicPrototype": true
},
"testEnvironment": "jsdom",
"testEnvironment": "./src/test/custom-environment",
"verbose": false
},
"husky": {
Expand Down
8 changes: 7 additions & 1 deletion src/actions/receive-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1030,8 +1030,14 @@ async function _extractZipFromResponse(
reportError: (...data: Array<any>) => void
): Promise<JSZip> {
const buffer = await response.arrayBuffer();
// Workaround for https://github.com/Stuk/jszip/issues/941
// When running this code in tests, `buffer` doesn't inherits from _this_
// realm's ArrayBuffer object, and this breaks JSZip which doesn't account for
// this case. We workaround the issue by wrapping the buffer in an Uint8Array
// that comes from this realm.
const typedBuffer = new Uint8Array(buffer);
try {
const zip = await JSZip.loadAsync(buffer);
const zip = await JSZip.loadAsync(typedBuffer);
// Catch the error if unable to load the zip.
return zip;
} catch (error) {
Expand Down
16 changes: 11 additions & 5 deletions src/profile-logic/mozilla-symbolication-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,13 @@ type APIResultV5 = {
// Make sure that the JSON blob we receive from the API conforms to our flow
// type definition.
function _ensureIsAPIResultV5(result: MixedObject): APIResultV5 {
if (!(result instanceof Object) || !('results' in result)) {
// It's possible (especially when running tests with Jest) that the parameter
// inherits from a `Object` global from another realm. By using toString
// this issue is solved wherever the parameter comes from.
const isObject = (subject) =>
Object.prototype.toString.call(subject) === '[object Object]';
Comment on lines +101 to +102
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.


if (!isObject(result) || !('results' in result)) {
throw new Error('Expected an object with property `results`');
}
const results = result.results;
Expand All @@ -104,7 +110,7 @@ function _ensureIsAPIResultV5(result: MixedObject): APIResultV5 {
}
for (const jobResult of results) {
if (
!(jobResult instanceof Object) ||
!isObject(jobResult) ||
!('found_modules' in jobResult) ||
!('stacks' in jobResult)
) {
Expand All @@ -113,7 +119,7 @@ function _ensureIsAPIResultV5(result: MixedObject): APIResultV5 {
);
}
const found_modules = jobResult.found_modules;
if (!(found_modules instanceof Object)) {
if (!isObject(found_modules)) {
throw new Error('Expected `found_modules` to be an object');
}
const stacks = jobResult.stacks;
Expand All @@ -125,7 +131,7 @@ function _ensureIsAPIResultV5(result: MixedObject): APIResultV5 {
throw new Error('Expected `stack` to be an array');
}
for (const frameInfo of stack) {
if (!(frameInfo instanceof Object)) {
if (!isObject(frameInfo)) {
throw new Error('Expected `frameInfo` to be an object');
}
if (
Expand Down Expand Up @@ -165,7 +171,7 @@ function _ensureIsAPIResultV5(result: MixedObject): APIResultV5 {
throw new Error('Expected `inlines` to be an array');
}
for (const inlineFrame of inlines) {
if (!(inlineFrame instanceof Object)) {
if (!isObject(inlineFrame)) {
throw new Error('Expected `inlineFrame` to be an object');
}
}
Expand Down
20 changes: 10 additions & 10 deletions src/test/components/AppLocalizationProvider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ describe('AppLocalizationProvider', () => {
jest.spyOn(window.navigator, 'languages', 'get').mockReturnValue(languages);

const translatedText = (language) => `This is ${language} Text`;
const fetchUrlRe = /^\/locales\/(?<language>[^/]+)\/app.ftl$/;
window.fetch
const fetchUrlRe = /\/locales\/(?<language>[^/]+)\/app.ftl$/;
window.fetchMock
.catch(404) // catchall
.get(fetchUrlRe, (fetchUrl) => {
const matchUrlResult = fetchUrlRe.exec(fetchUrl);
.get(fetchUrlRe, ({ url }) => {
const matchUrlResult = fetchUrlRe.exec(url);
if (matchUrlResult) {
// $FlowExpectError Our Flow doesn't know about named groups.
const { language } = matchUrlResult.groups;
Expand Down Expand Up @@ -188,15 +188,15 @@ describe('AppLocalizationProvider', () => {

expect(await screen.findByText(translatedText('de'))).toBeInTheDocument();
expect(document.documentElement).toHaveAttribute('lang', 'de');
expect(window.fetch).toHaveBeenCalledWith('/locales/de/app.ftl', {
expect(window.fetch).toHaveFetched('/locales/de/app.ftl', {
credentials: 'include',
mode: 'no-cors',
});
expect(window.fetch).toHaveBeenCalledWith('/locales/en-US/app.ftl', {
expect(window.fetch).toHaveFetched('/locales/en-US/app.ftl', {
credentials: 'include',
mode: 'no-cors',
});
expect(window.fetch).toHaveBeenCalledTimes(2);
expect(window.fetch).toHaveFetchedTimes(2);
});

it('falls back properly on en-US if the primary locale lacks a string', async () => {
Expand All @@ -220,14 +220,14 @@ describe('AppLocalizationProvider', () => {
await screen.findByText(translatedText('en-US'))
).toBeInTheDocument();
expect(document.documentElement).toHaveAttribute('lang', 'de');
expect(window.fetch).toHaveBeenCalledWith('/locales/de/app.ftl', {
expect(window.fetch).toHaveFetched('/locales/de/app.ftl', {
credentials: 'include',
mode: 'no-cors',
});
expect(window.fetch).toHaveBeenCalledWith('/locales/en-US/app.ftl', {
expect(window.fetch).toHaveFetched('/locales/en-US/app.ftl', {
credentials: 'include',
mode: 'no-cors',
});
expect(window.fetch).toHaveBeenCalledTimes(2);
expect(window.fetch).toHaveFetchedTimes(2);
});
});
2 changes: 1 addition & 1 deletion src/test/components/BottomBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('BottomBox', () => {

const revision = '997f00815e6bc28806b75448c8829f0259d2cb28';
const filepath = 'widget/cocoa/nsAppShell.mm';
window.fetch
window.fetchMock
.post('http://127.0.0.1:8000/source/v1', 500)
.get(
`https://hg.mozilla.org/mozilla-central/raw-file/${revision}/${filepath}`,
Expand Down
30 changes: 13 additions & 17 deletions src/test/components/FooterLinks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,22 @@ import { render, screen, fireEvent } from '@testing-library/react';
beforeEach(() => {
// Implement the fetch operation for local language files, so that we can test
// switching languages.
const fetchUrlRe = /^\/locales\/(?<language>[^/]+)\/app.ftl$/;
window.fetch
const fetchUrlRe = /\/locales\/(?<language>[^/]+)\/app.ftl$/;
window.fetchMock
.catch(404) // catchall
.get(
fetchUrlRe,
(fetchUrl) => {
const matchUrlResult = fetchUrlRe.exec(fetchUrl);
if (matchUrlResult) {
// $FlowExpectError Our Flow doesn't know about named groups.
const { language } = matchUrlResult.groups;
const path = `locales/${language}/app.ftl`;
if (fs.existsSync(path)) {
return fs.readFileSync(path);
}
.get(fetchUrlRe, ({ url }) => {
const matchUrlResult = fetchUrlRe.exec(url);
if (matchUrlResult) {
// $FlowExpectError Our Flow doesn't know about named groups.
const { language } = matchUrlResult.groups;
const path = `locales/${language}/app.ftl`;
if (fs.existsSync(path)) {
return fs.readFileSync(path);
}
}

return 404;
},
{ sendAsJson: false }
);
return 404;
});
});

afterEach(function () {
Expand Down
12 changes: 6 additions & 6 deletions src/test/components/ListOfPublishedProfiles.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,20 +285,20 @@ describe('ListOfPublishedProfiles', () => {
endpointUrl: string,
jwtToken: string,
}) {
window.fetch
window.fetchMock
.catch(404) // Catchall
.mock(endpointUrl, (urlString, options) => {
.route(endpointUrl, ({ options }) => {
const { method, headers } = options;
if (method !== 'DELETE') {
if (method !== 'delete') {
return new Response(null, {
status: 405,
statusText: 'Method not allowed',
});
}

if (
headers['Content-Type'] !== 'application/json' ||
headers.Accept !==
headers['content-type'] !== 'application/json' ||
headers.accept !==
'application/vnd.firefox-profiler+json;version=1.0'
) {
return new Response(null, {
Expand All @@ -307,7 +307,7 @@ describe('ListOfPublishedProfiles', () => {
});
}

if (headers.Authorization !== `Bearer ${jwtToken}`) {
if (headers.authorization !== `Bearer ${jwtToken}`) {
return new Response(null, {
status: 401,
statusText: 'Forbidden',
Expand Down
2 changes: 1 addition & 1 deletion src/test/components/Root-history.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ describe('Root with history', function () {
});

function mockFetchProfileAtUrl(url: string, profile: Profile): void {
window.fetch
window.fetchMock
.catch(404) // catchall
.get(url, profile);
}
8 changes: 4 additions & 4 deletions src/test/components/UrlManager.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ describe('UrlManager', function () {
});

it(`sets the data source to public and doesn't change the URL when there's a fetch error`, async function () {
window.fetch.getAny({ throws: new Error('Simulated network error') });
window.fetchMock.get('*', { throws: new Error('Simulated network error') });
const urlPath = '/public/FAKE_HASH/marker-chart';
const { getState, createUrlManager, waitUntilUrlSetupPhase } =
setup(urlPath);
Expand All @@ -163,7 +163,7 @@ describe('UrlManager', function () {
});

it(`sets the data source to public and doesn't change the URL when there's a URL upgrading error`, async function () {
window.fetch.getAny(getSerializableProfile());
window.fetchMock.get('*', getSerializableProfile());

const urlPath = '/public/FAKE_HASH/calltree';
const searchString = '?v=' + (CURRENT_URL_VERSION + 1);
Expand All @@ -185,7 +185,7 @@ describe('UrlManager', function () {
});

it(`fetches profile and sets the phase to done when everything works`, async function () {
window.fetch.getAny(getSerializableProfile());
window.fetchMock.get('*', getSerializableProfile());

const urlPath = '/public/FAKE_HASH/';
const expectedResultingPath = urlPath + 'calltree/';
Expand All @@ -206,7 +206,7 @@ describe('UrlManager', function () {
});

it('allows navigating back and forward when changing view options', async () => {
window.fetch.getAny(getSerializableProfile());
window.fetchMock.get('*', getSerializableProfile());

const urlPath = '/public/FAKE_HASH/calltree/';
const searchString = 'v=' + CURRENT_URL_VERSION;
Expand Down
44 changes: 44 additions & 0 deletions src/test/custom-environment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
// @flow
//
import { TestEnvironment } from 'jest-environment-jsdom';
import { TextDecoder, TextEncoder } from 'util';

// This class registers various globals coming from node in test environments.
export default class CustomTestEnvironment extends TestEnvironment {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

async setup() {
await super.setup();

Check warning on line 12 in src/test/custom-environment.js

View check run for this annotation

Codecov / codecov/patch

src/test/custom-environment.js#L11-L12

Added lines #L11 - L12 were not covered by tests

// Register TextDecoder and TextEncoder with the global scope.
// These are now available globally in nodejs, but not when running with jsdom
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!)

throw new Error(

Check warning on line 20 in src/test/custom-environment.js

View check run for this annotation

Codecov / codecov/patch

src/test/custom-environment.js#L20

Added line #L20 was not covered by tests
'TextDecoder is already present in the global scope, please update custom-environment.js.'
);
}

this.global.TextDecoder = TextDecoder;
this.global.TextEncoder = TextEncoder;

Check warning on line 26 in src/test/custom-environment.js

View check run for this annotation

Codecov / codecov/patch

src/test/custom-environment.js#L25-L26

Added lines #L25 - L26 were not covered by tests

// Register Request and friends with the global scope.
// These are now available globally in nodejs, but not when running with jsdom
// 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 ('Request' in this.global) {
throw new Error(

Check warning on line 34 in src/test/custom-environment.js

View check run for this annotation

Codecov / codecov/patch

src/test/custom-environment.js#L34

Added line #L34 was not covered by tests
'Request is already present in the global scope, please update custom-environment.js.'
);
}

this.global.fetch = fetch;
this.global.Request = Request;
this.global.Response = Response;
this.global.ReadableStream = ReadableStream;

Check warning on line 42 in src/test/custom-environment.js

View check run for this annotation

Codecov / codecov/patch

src/test/custom-environment.js#L39-L42

Added lines #L39 - L42 were not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('symbolicator-cli tool', function () {
'src/test/integration/symbolicator-cli/symbol-server-response.json'
);

window.fetch.post(
window.fetchMock.post(
'http://symbol.server/symbolicate/v5',
new Response(symbolsJson)
);
Expand Down
28 changes: 3 additions & 25 deletions src/test/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
import '@testing-library/jest-dom';

// This installs jest matchers as a side effect as well.
import fetchMock from 'fetch-mock-jest';
import { Headers, Request, Response } from 'node-fetch';
import { TextDecoder, TextEncoder } from 'util';
import fetchMock from '@fetch-mock/jest';
import crypto from 'crypto';

jest.mock('../utils/worker-factory');
Expand All @@ -22,28 +20,8 @@ if (process.env.TZ !== 'UTC') {
throw new Error('Jest must be run from `yarn test`');
}

// Register TextDecoder and TextEncoder with the global scope.
// These are now available globally in nodejs, but not when running with jsdom
// 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 global) {
throw new Error(
'TextDecoder is already present in the global scope, please update setup.js.'
);
}

global.TextDecoder = TextDecoder;
global.TextEncoder = TextEncoder;

beforeEach(function () {
// Install fetch and fetch-related objects globally.
// Using the sandbox ensures that parallel tests run properly.
global.fetch = fetchMock.sandbox();
global.Headers = Headers;
global.Request = Request;
global.Response = Response;
});
fetchMock.mockGlobal();
global.fetchMock = fetchMock;
Comment on lines +23 to +24
Copy link
Copy Markdown
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.


afterEach(function () {
// This `__shutdownWorkers` function only exists in the mocked test environment,
Expand Down
Loading