Skip to content

api-fetch: cleanup and improve unit tests #71439

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

Merged
jsnajdr merged 1 commit into from
Sep 1, 2025
Merged

api-fetch: cleanup and improve unit tests #71439

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
266 changes: 126 additions & 140 deletions packages/api-fetch/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,53 +4,53 @@
import apiFetch from '../';

/**
* Mock return value for a successful fetch JSON return value.
* Mock response value for a successful fetch.
*
* @return {Promise} Mock return value.
* @return {Response} Mock return value.
*/
const DEFAULT_FETCH_MOCK_RETURN = Promise.resolve( {
const DEFAULT_FETCH_MOCK_RETURN = {
ok: true,
status: 200,
json: () => Promise.resolve( {} ),
} );
};

describe( 'apiFetch', () => {
const originalFetch = window.fetch;
const originalFetch = globalThis.fetch;

beforeEach( () => {
window.fetch = jest.fn();
globalThis.fetch = jest.fn();
} );

afterAll( () => {
window.fetch = originalFetch;
globalThis.fetch = originalFetch;
} );

it( 'should call the API properly', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 200,
json() {
return Promise.resolve( { message: 'ok' } );
},
} )
);
it( 'should call the API properly', async () => {
globalThis.fetch.mockResolvedValue( {
ok: true,
status: 200,
async json() {
return { message: 'ok' };
},
} );

return apiFetch( { path: '/random' } ).then( ( body ) => {
expect( body ).toEqual( { message: 'ok' } );
await expect( apiFetch( { path: '/random' } ) ).resolves.toEqual( {
message: 'ok',
} );
} );

it( 'should fetch with non-JSON body', () => {
window.fetch.mockReturnValue( DEFAULT_FETCH_MOCK_RETURN );
it( 'should fetch with non-JSON body', async () => {
globalThis.fetch.mockResolvedValue( DEFAULT_FETCH_MOCK_RETURN );

const body = 'FormData';

apiFetch( {
await apiFetch( {
Copy link
Member Author

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

Example of a synchronous test where the response promise was leaking out of the test. The promise resolves only after the test has already finished running.

path: '/wp/v2/media',
method: 'POST',
body,
} );

expect( window.fetch ).toHaveBeenCalledWith(
expect( globalThis.fetch ).toHaveBeenCalledWith(
'/wp/v2/media?_locale=user',
{
credentials: 'include',
Expand All @@ -63,10 +63,10 @@ describe( 'apiFetch', () => {
);
} );

it( 'should fetch with a JSON body', () => {
window.fetch.mockReturnValue( DEFAULT_FETCH_MOCK_RETURN );
it( 'should fetch with a JSON body', async () => {
globalThis.fetch.mockResolvedValue( DEFAULT_FETCH_MOCK_RETURN );

apiFetch( {
await apiFetch( {
path: '/wp/v2/posts',
method: 'POST',
headers: {
Expand All @@ -75,7 +75,7 @@ describe( 'apiFetch', () => {
data: {},
} );

expect( window.fetch ).toHaveBeenCalledWith(
expect( globalThis.fetch ).toHaveBeenCalledWith(
'/wp/v2/posts?_locale=user',
{
body: '{}',
Expand All @@ -89,17 +89,17 @@ describe( 'apiFetch', () => {
);
} );

it( 'should respect developer-provided options', () => {
window.fetch.mockReturnValue( DEFAULT_FETCH_MOCK_RETURN );
it( 'should respect developer-provided options', async () => {
globalThis.fetch.mockResolvedValue( DEFAULT_FETCH_MOCK_RETURN );

apiFetch( {
await apiFetch( {
path: '/wp/v2/posts',
method: 'POST',
data: {},
credentials: 'omit',
} );

expect( window.fetch ).toHaveBeenCalledWith(
expect( globalThis.fetch ).toHaveBeenCalledWith(
'/wp/v2/posts?_locale=user',
{
body: '{}',
Expand All @@ -113,83 +113,86 @@ describe( 'apiFetch', () => {
);
} );

it( 'should return the error message properly', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 400,
json() {
return Promise.resolve( {
code: 'bad_request',
message: 'Bad Request',
} );
},
} )
);
it( 'should return the error message properly', async () => {
globalThis.fetch.mockResolvedValue( {
ok: false,
status: 400,
async json() {
return {
code: 'bad_request',
message: 'Bad Request',
};
},
} );

return apiFetch( { path: '/random' } ).catch( ( body ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( body ).toEqual( {
code: 'bad_request',
message: 'Bad Request',
} );
await expect( apiFetch( { path: '/random' } ) ).rejects.toEqual( {
code: 'bad_request',
message: 'Bad Request',
Copy link
Member Author

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

This removes the need to disable jest/no-conditional-expect. And also the test checks that the promise really rejects and fails otherwise. In the original code, the expect inside the catch callback would never run, and the test would finish without calling any expect.

} );
} );

it( 'should return invalid JSON error if no json response', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 200,
} )
);
it( 'should return invalid JSON error if no json response', async () => {
globalThis.fetch.mockResolvedValue( {
ok: true,
status: 200,
async json() {
return JSON.parse( '' );
},
} );

return apiFetch( { path: '/random' } ).catch( ( body ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( body ).toEqual( {
code: 'invalid_json',
message: 'The response is not a valid JSON response.',
} );
await expect( apiFetch( { path: '/random' } ) ).rejects.toEqual( {
code: 'invalid_json',
message: 'The response is not a valid JSON response.',
} );
} );

it( 'should return invalid JSON error if response is not valid', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 200,
json() {
return Promise.reject();
},
} )
);
it( 'should return invalid JSON error if response is not valid', async () => {
globalThis.fetch.mockResolvedValue( {
ok: true,
status: 200,
async json() {
return JSON.parse( '' );
},
} );

return apiFetch( { path: '/random' } ).catch( ( body ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( body ).toEqual( {
code: 'invalid_json',
message: 'The response is not a valid JSON response.',
} );
await expect( apiFetch( { path: '/random' } ) ).rejects.toEqual( {
code: 'invalid_json',
message: 'The response is not a valid JSON response.',
} );
} );

it( 'should return offline error when fetch errors', () => {
window.fetch.mockReturnValue( Promise.reject() );
it( 'should return offline error when fetch errors', async () => {
globalThis.fetch.mockRejectedValue(
new TypeError( 'Failed to fetch' )
);

return apiFetch( { path: '/random' } ).catch( ( body ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( body ).toEqual( {
code: 'fetch_error',
message: 'You are probably offline.',
} );
await expect( apiFetch( { path: '/random' } ) ).rejects.toEqual( {
code: 'fetch_error',
message: 'You are probably offline.',
} );
} );

it( 'should throw AbortError when fetch aborts', async () => {
const abortError = new Error();
abortError.name = 'AbortError';
abortError.code = 20;

window.fetch.mockReturnValue( Promise.reject( abortError ) );
globalThis.fetch.mockImplementation(
( path, { signal } ) =>
new Promise( ( _, reject ) => {
if ( ! signal ) {
reject( new Error( 'Expected signal as argument' ) );
return;
}

signal.throwIfAborted();
signal.addEventListener(
'abort',
( e ) => {
reject( e.target.reason );
},
{ once: true }
);
} )
);
Copy link
Member Author

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

This is a realistic implementation of a fetch handler that:

  1. Returns a promise that never resolves.
  2. Aborts when someone calls controller.abort().

The original version of the test just mocked fetch to reject with AbortError and then tested that this error was really thrown. The signal and the controller didn't play any role at all.


const controller = new window.AbortController();
const controller = new globalThis.AbortController();

const promise = apiFetch( {
path: '/random',
Expand All @@ -198,78 +201,61 @@ describe( 'apiFetch', () => {

controller.abort();

let error;

try {
await promise;
} catch ( err ) {
error = err;
}

expect( error.name ).toBe( 'AbortError' );
await expect( promise ).rejects.toMatchObject( {
name: 'AbortError',
} );
} );

it( 'should return null if response has no content status code', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 204,
} )
);

return apiFetch( { path: '/random' } ).catch( ( body ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( body ).toEqual( null );
it( 'should return null if response has no content status code', async () => {
globalThis.fetch.mockResolvedValue( {
ok: true,
status: 204,
} );

await expect( apiFetch( { path: '/random' } ) ).resolves.toBe( null );
Copy link
Member Author

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

This test was wrong! It expected that apiFetch throws null, but in reality it returns null. The "204 No content" response doesn't have any body, so apiFetch returns null. And also avoids calling response.json() because that would throw.

} );

it( 'should not try to parse the response', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 200,
} )
);
it( 'should not try to parse the response', async () => {
const mockResponse = {
ok: true,
status: 200,
};

return apiFetch( { path: '/random', parse: false } ).then(
( response ) => {
expect( response ).toEqual( {
status: 200,
} );
}
);
globalThis.fetch.mockResolvedValue( mockResponse );

await expect(
apiFetch( { path: '/random', parse: false } )
).resolves.toBe( mockResponse );
} );

it( 'should not try to parse the error', () => {
window.fetch.mockReturnValue(
Promise.resolve( {
status: 400,
} )
);
it( 'should not try to parse the error', async () => {
const mockResponse = {
ok: false,
status: 400,
};

return apiFetch( { path: '/random', parse: false } ).catch(
( response ) => {
// eslint-disable-next-line jest/no-conditional-expect
expect( response ).toEqual( {
status: 400,
} );
}
);
globalThis.fetch.mockResolvedValue( mockResponse );

await expect(
apiFetch( { path: '/random', parse: false } )
).rejects.toBe( mockResponse );
} );

it( 'should not use the default fetch handler when using a custom fetch handler', () => {
it( 'should not use the default fetch handler when using a custom fetch handler', async () => {
const customFetchHandler = jest.fn();

apiFetch.setFetchHandler( customFetchHandler );

apiFetch( { path: '/random' } );
await apiFetch( { path: '/random' } );

expect( window.fetch ).not.toHaveBeenCalled();
expect( globalThis.fetch ).not.toHaveBeenCalled();

expect( customFetchHandler ).toHaveBeenCalledWith( {
path: '/random?_locale=user',
} );
} );

it( 'should run the last-registered user-defined middleware first', () => {
it( 'should run the last-registered user-defined middleware first', async () => {
// This could potentially impact other tests in that a lingering
// middleware is left. For the purposes of this test, it is sufficient
// to ensure that the last-registered middleware receives the original
Expand All @@ -286,6 +272,6 @@ describe( 'apiFetch', () => {
return next( actualOptions );
} );

apiFetch( expectedOptions );
await apiFetch( expectedOptions );
} );
} );
Loading