From d1571005d945f8ce07a0b2c30273da0e488af479 Mon Sep 17 00:00:00 2001 From: Monkey Do Date: Mon, 15 Mar 2021 18:22:50 +0100 Subject: [PATCH] Rewrite getPage retry logic and fix associated tests --- .../static/js/src/LastFMImporter.test.tsx | 123 ++++++++++++------ .../static/js/src/LastFMImporter.tsx | 65 ++++++--- 2 files changed, 125 insertions(+), 63 deletions(-) diff --git a/listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx b/listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx index e7f8aeaf51..718a76c9ed 100644 --- a/listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx +++ b/listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx @@ -1,6 +1,6 @@ import * as React from "react"; import { mount, shallow } from "enzyme"; -import LastFmImporter, {LASTFM_RETRIES} from "./LastFMImporter"; +import LastFmImporter, { LASTFM_RETRIES } from "./LastFMImporter"; // Mock data to test functions import * as page from "./__mocks__/page.json"; import * as getInfo from "./__mocks__/getInfo.json"; @@ -133,6 +133,21 @@ describe("getTotalNumberOfScrobbles", () => { }); describe("getPage", () => { + beforeAll(() => { + // The timeout we use in getPage's implementation does not work with Jest fake timers + // so we disable fake timers for this section + // and give enough time for the real timeouts to run + // see https://stackoverflow.com/questions/50783013/how-to-timeout-promises-in-jest + jest.useRealTimers(); + jest.setTimeout(3000 * (LASTFM_RETRIES + 2)); + }); + + afterAll(() => { + // Back to default + jest.setTimeout(5000); + jest.useFakeTimers(); + }); + beforeEach(() => { const wrapper = shallow(); instance = wrapper.instance(); @@ -154,15 +169,6 @@ describe("getPage", () => { ); }); - it("should call encodeScrobbles", async () => { - // Mock function for encodeScrobbles - LastFmImporter.encodeScrobbles = jest.fn(() => ["foo", "bar"]); - - const data = await instance.getPage(1, LASTFM_RETRIES); - expect(LastFmImporter.encodeScrobbles).toHaveBeenCalledTimes(1); - expect(data).toEqual(["foo", "bar"]); - }); - it("should retry if 50x error is recieved", async () => { // Mock function for fetch window.fetch = jest.fn().mockImplementation(() => { @@ -173,17 +179,41 @@ describe("getPage", () => { }); const getPageSpy = jest.spyOn(instance, "getPage"); - await instance.getPage(1, LASTFM_RETRIES); - - // we run the timer sufficient number of times to ensure retries are not exceeded or undetected due to timeouts - for (let i = 0; i < LASTFM_RETRIES + 2; i += 1) { - jest.runAllTimers(); - // make timers and promises play well with jest - // eslint-disable-next-line no-await-in-loop - await Promise.resolve(); - } + + const finalValue = await instance.getPage(1, LASTFM_RETRIES); expect(getPageSpy).toHaveBeenCalledTimes(1 + LASTFM_RETRIES); + expect(finalValue).toBeNull(); + }); + + it("should return the expected value if retry is successful", async () => { + // Mock function for fetch + window.fetch = jest + .fn() + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: false, + status: 503, + }); + }) + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: false, + status: 503, + }); + }) + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: true, + json: () => Promise.resolve(page), + }); + }); + + const getPageSpy = jest.spyOn(instance, "getPage"); + const finalValue = await instance.getPage(1, LASTFM_RETRIES); + + expect(getPageSpy).toHaveBeenCalledTimes(3); + expect(finalValue).toEqual(encodeScrobbleOutput); }); it("should skip the page if 40x is recieved", async () => { @@ -195,40 +225,49 @@ describe("getPage", () => { }); }); const getPageSpy = jest.spyOn(instance, "getPage"); - await instance.getPage(1, LASTFM_RETRIES); - - // we run the timer sufficient number of times to ensure retries are not exceeded or undetected due to timeouts - for (let i = 0; i < LASTFM_RETRIES + 2; i += 1) { - jest.runAllTimers(); - // make timers and promises play well with jest - // eslint-disable-next-line no-await-in-loop - await Promise.resolve(); - } + const finalValue = await instance.getPage(1, LASTFM_RETRIES); expect(getPageSpy).toHaveBeenCalledTimes(1); + expect(finalValue).toEqual(undefined); }); it("should retry if there is any other error", async () => { // Mock function for fetch - window.fetch = jest.fn().mockImplementation(() => { - return Promise.resolve({ - ok: true, - json: () => Promise.reject(), + window.fetch = jest + .fn() + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: true, + json: () => Promise.reject(), + }); + }) + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: false, + status: 600, + }); + }) + .mockImplementationOnce(() => { + return Promise.resolve({ + ok: true, + json: () => Promise.resolve(page), + }); }); - }); const getPageSpy = jest.spyOn(instance, "getPage"); + const finalValue = await instance.getPage(1, LASTFM_RETRIES); - await instance.getPage(1, LASTFM_RETRIES); - // we run the timer sufficient number of times to ensure retries are not exceeded or undetected due to timeouts - for (let i = 0; i < LASTFM_RETRIES + 2; i += 1) { - jest.runAllTimers(); - // make timers and promises play well with jest - // eslint-disable-next-line no-await-in-loop - await Promise.resolve(); - } + expect(getPageSpy).toHaveBeenCalledTimes(3); + expect(finalValue).toEqual(encodeScrobbleOutput); + }); - expect(getPageSpy).toHaveBeenCalledTimes(1 + LASTFM_RETRIES); + it("should call encodeScrobbles", async () => { + // Mock function for encodeScrobbles + LastFmImporter.encodeScrobbles = jest.fn(() => ["foo", "bar"]); + + const data = await instance.getPage(1, LASTFM_RETRIES); + expect(LastFmImporter.encodeScrobbles).toHaveBeenCalledTimes(1); + expect(data).toEqual(["foo", "bar"]); }); }); diff --git a/listenbrainz/webserver/static/js/src/LastFMImporter.tsx b/listenbrainz/webserver/static/js/src/LastFMImporter.tsx index 0b000724b2..2a2da13fcc 100644 --- a/listenbrainz/webserver/static/js/src/LastFMImporter.tsx +++ b/listenbrainz/webserver/static/js/src/LastFMImporter.tsx @@ -154,23 +154,15 @@ export default class LastFmImporter extends React.Component< * @param {number} page - the page to fetch from Last.fm * @param {number} retries - number times to retry in case of errors other than 40x * Fetch page from Last.fm + * @returns Returns an array of Listens if successful, null if it exceeds the max number of retries (consider that an error) + * and undefined if we receive a 40X error for the page */ - async getPage(page: number, retries: number): Promise | null> { + async getPage( + page: number, + retries: number + ): Promise | undefined | null> { const { lastfmUsername } = this.state; - - const retry = async (reason: string): Promise | null> => { - // eslint-disable-next-line no-console - const timeout = 3000; - console.warn( - `${reason} while fetching last.fm page=${page}, retrying in - ${timeout / 1000}s` - ); - if (retries > 0) { - await new Promise((resolve) => setTimeout(resolve, timeout)); - await this.getPage(page, retries - 1); - } - return null; - }; + const timeout = 3000; const url = `${ this.lastfmURL @@ -196,16 +188,36 @@ export default class LastFmImporter extends React.Component< this.countReceived += payload.length; return payload; } - if (/^5/.test(response.status.toString())) { - return await retry(`Got ${response.status}`); + // ignore 40x errors + if (!/^4/.test(response.status.toString())) { + // eslint-disable-next-line no-console + console.warn( + `Got ${ + response.status + } while fetching last.fm page=${page}, retrying in + ${timeout / 1000}s` + ); + if (retries <= 0) { + return null; + } + await new Promise((resolve) => setTimeout(resolve, timeout)); + return await this.getPage(page, retries - 1); } - // ignore 40x - // console.warn(`Got ${response.status} while fetching page last.fm page=${page}, skipping`); } catch { // Retry if there is a network error - await retry("Network error"); + // eslint-disable-next-line no-console + console.warn( + `Network error while fetching last.fm page=${page}, retrying in + ${timeout / 1000}s` + ); + if (retries <= 0) { + return null; + } + await new Promise((resolve) => setTimeout(resolve, timeout)); + // eslint-disable-next-line no-return-await + return await this.getPage(page, retries - 1); } - return null; + return undefined; } static getlastImportedString(listen: Listen) { @@ -283,6 +295,17 @@ export default class LastFmImporter extends React.Component< // Fixing no-await-in-loop will require significant changes to the code, ignoring for now this.lastImportedString = "..."; const payload = await this.getPage(this.page, LASTFM_RETRIES); // eslint-disable-line + if (payload === null) { + const msg = ( +

+ We were unable to import from LastFM, please try again. +
+ If the problem persists please contact us. +

+ ); + this.setState({ msg }); + return; + } if (payload) { // Submit only if response is valid this.submitPage(payload);