Skip to content

Commit

Permalink
Rewrite getPage retry logic and fix associated tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@MonkeyDo
MonkeyDo committed Mar 15, 2021
1 parent f967494 commit d157100
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 63 deletions.
123 changes: 81 additions & 42 deletions listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx
View file
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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<LastFmImporter>(<LastFmImporter {...props} />);
instance = wrapper.instance();
Expand All @@ -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(() => {
Expand All @@ -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 () => {
Expand All @@ -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"]);
});
});

Expand Down
65 changes: 44 additions & 21 deletions listenbrainz/webserver/static/js/src/LastFMImporter.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Array<Listen> | null> {
async getPage(
page: number,
retries: number
): Promise<Array<Listen> | undefined | null> {
const { lastfmUsername } = this.state;

const retry = async (reason: string): Promise<Array<Listen> | 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
Expand All @@ -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) {
Expand Down Expand Up @@ -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 = (
<p>
We were unable to import from LastFM, please try again.
<br />
If the problem persists please contact us.
</p>
);
this.setState({ msg });
return;
}
if (payload) {
// Submit only if response is valid
this.submitPage(payload);
Expand Down

0 comments on commit d157100

Please sign in to comment.