diff --git a/src/mb_enhanced_cover_art_uploads/fetch.ts b/src/mb_enhanced_cover_art_uploads/fetch.ts index 0b2e4da4..24c690f8 100644 --- a/src/mb_enhanced_cover_art_uploads/fetch.ts +++ b/src/mb_enhanced_cover_art_uploads/fetch.ts @@ -220,7 +220,7 @@ export class ImageFetcher { retries: 10, onFailedAttempt: (err) => { // Don't retry on 4xx status codes except for 429. Anything below 400 doesn't throw a HTTPResponseError. - if (!(err instanceof HTTPResponseError) || err.statusCode < 500 || err.statusCode !== 429) { + if (!(err instanceof HTTPResponseError) || (err.statusCode < 500 && err.statusCode !== 429)) { throw err; } diff --git a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts index e9b6fa78..4bddfe9a 100644 --- a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts +++ b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts @@ -1,15 +1,19 @@ +import type { FailedAttemptError } from 'p-retry'; +import pRetry from 'p-retry'; + import type { MaximisedImage } from '@src/mb_enhanced_cover_art_uploads/maximise'; import type { ImageContents, QueuedImage } from '@src/mb_enhanced_cover_art_uploads/types'; import { ArtworkTypeIDs } from '@lib/MB/CoverArt'; -import { gmxhr, NetworkError } from '@lib/util/xhr'; +import { gmxhr, HTTPResponseError, NetworkError } from '@lib/util/xhr'; import { ImageFetcher } from '@src/mb_enhanced_cover_art_uploads/fetch'; import { enqueueImage } from '@src/mb_enhanced_cover_art_uploads/form'; import { getMaximisedCandidates } from '@src/mb_enhanced_cover_art_uploads/maximise'; import { getProvider, getProviderByDomain } from '@src/mb_enhanced_cover_art_uploads/providers'; import { CoverArtProvider } from '@src/mb_enhanced_cover_art_uploads/providers/base'; -import { createCoverArt, createImageFile, createXhrResponse } from './test-utils/dummy-data'; +import { createCoverArt, createHttpError, createImageFile, createXhrResponse } from './test-utils/dummy-data'; +jest.mock('p-retry'); jest.mock('@lib/util/xhr'); // We need to provide a mock factory, because for some reason, either jest or // rewire is not recognising the generator, leading to `getMaximisedCandidates` @@ -20,6 +24,7 @@ jest.mock<{ getMaximisedCandidates: typeof getMaximisedCandidates }>('@src/mb_en jest.mock('@src/mb_enhanced_cover_art_uploads/providers'); jest.mock('@src/mb_enhanced_cover_art_uploads/form'); +const mockpRetry = pRetry as jest.MockedFunction; const mockXhr = gmxhr as jest.MockedFunction; const mockGetMaximisedCandidates = getMaximisedCandidates as jest.MockedFunction; const mockGetProvider = getProvider as jest.MockedFunction; @@ -77,6 +82,11 @@ function disableDummyFetch(mock: FetchImageContentsSpy): void { mock.mockRestore(); } +beforeAll(() => { + // Mock p-retry so that it doesn't retry on image content fetching failure. + mockpRetry.mockImplementation(((fn) => (fn(0))) as typeof pRetry); +}); + beforeEach(() => { mockEnqueueImage.mockClear(); mockFindImages.mockReset(); @@ -88,17 +98,54 @@ beforeEach(() => { describe('fetching image contents', () => { let fetchImageContents: typeof ImageFetcher.prototype['fetchImageContents']; + // Used to register when a retry occurs in the p-retry mock, so we can verify in the tests. + const onRetry = jest.fn(); + + beforeAll(() => { + // Mock retrying behaviour so we can test it. + mockpRetry.mockImplementation(async (fn, options) => { + try { + // eslint-disable-next-line @typescript-eslint/return-await + return await fn(0); + } catch (err) { + Object.defineProperties(err, { + attemptNumber: { value: 1, writable: false }, + retriesLeft: { value: 1, writable: false }, + }); + await options?.onFailedAttempt?.(err as FailedAttemptError); + // Call the onRetry mock so we can verify when a retry would've + // occurred. + onRetry(); + return fn(1); + } + }); + }); beforeEach(() => { const fetcher = new ImageFetcher(hooks); fetchImageContents = fetcher['fetchImageContents'].bind(fetcher); + onRetry.mockClear(); + }); + + afterAll(() => { + mockpRetry.mockImplementation((fn) => Promise.resolve(fn(0))); }); - it('rejects on HTTP error', async () => { + it('rejects on network error', async () => { mockXhr.mockRejectedValueOnce(new NetworkError(new URL('https://example.com'))); await expect(fetchImageContents(new URL('https://example.com/broken'), 'test.jpg', 0, {})) .rejects.toBeInstanceOf(NetworkError); + expect(onRetry).not.toHaveBeenCalled(); + }); + + it('rejects on HTTP 404 error', async () => { + mockXhr.mockRejectedValue(createHttpError(createXhrResponse({ status: 404 }))); + + const result = fetchImageContents(new URL('https://example.com/broken'), 'test.jpg', 0, {}); + + await expect(result).rejects.toBeInstanceOf(HTTPResponseError); + expect(onRetry).not.toHaveBeenCalled(); }); it('rejects on text response', async () => { @@ -167,6 +214,45 @@ describe('fetching image contents', () => { }); }); + it('retries on 429 response', async () => { + mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({ + status: 429, + }))); + mockXhr.mockResolvedValueOnce(createXhrResponse({ + finalUrl: 'https://example.com/working', + response: new Blob([Uint32Array.from([0x474E5089, 0xDEADBEEF])]), + })); + + await expect(fetchImageContents(new URL('https://example.com/working'), 'test.jpg', 0, {})) + .resolves.toMatchObject({ + file: { + type: 'image/png', + name: 'test.0.png', + }, + requestedUrl: { + href: 'https://example.com/working', + }, + fetchedUrl: { + href: 'https://example.com/working', + }, + wasRedirected: false, + }); + expect(onRetry).toHaveBeenCalledOnce(); + }); + + it('rejects on too many 429 responses', async () => { + mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({ + status: 429, + }))); + mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({ + status: 429, + }))); + + await expect(fetchImageContents(new URL('https://example.com/working'), 'test.jpg', 0, {})) + .rejects.toBeInstanceOf(HTTPResponseError); + expect(onRetry).toHaveBeenCalledOnce(); + }); + it('retains redirection information', async () => { mockXhr.mockResolvedValueOnce(createXhrResponse({ finalUrl: 'https://example.com/redirected', diff --git a/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts b/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts index 514cd546..69529de2 100644 --- a/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts +++ b/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts @@ -1,6 +1,7 @@ // Abstractions to create dummy data import type { CoverArt, FetchedImage } from '@src/mb_enhanced_cover_art_uploads/types'; +import { HTTPResponseError } from '@lib/util/xhr'; export interface DummyImageData { blob?: Blob; @@ -90,3 +91,16 @@ export function createXhrResponse(response?: Partial>): GM.Re responseXML: response.responseXML ?? false, }; } + +export function createHttpError(response?: Partial>): HTTPResponseError { + const xhrResponse = createXhrResponse(response); + const err = new HTTPResponseError(xhrResponse.finalUrl, xhrResponse); + // If gmxhr is mocked, the HTTP errors are too, so we need to define these + // properties manually. + Object.defineProperties(err, { + response: { value: xhrResponse }, + statusCode: { value: xhrResponse.status }, + statusText: { value: xhrResponse.statusText }, + }); + return err; +}