Merge pull request #1306 from prymitive/fetch-refactor

chore(ui): refactor fetch retry config and tests

ui/src/Common/Fetch.js

@@ -3,12 +3,13 @@ import merge from "lodash/merge";
 import promiseRetry from "promise-retry";
 
 const CommonOptions = {
+  mode: "cors",
   credentials: "include",
   redirect: "follow"
 };
 
 const FetchRetryConfig = {
-  retries: 10,
+  retries: 9,
   minTimeout: 2000,
   maxTimeout: 5000
 };
@@ -21,13 +22,12 @@ const FetchGet = async (uri, options, beforeRetry) =>
         merge(
           {},
           {
-            method: "GET",
-            mode:
-              number <= Math.round(FetchRetryConfig.retries * 0.8)
-                ? "cors"
-                : "no-cors"
+            method: "GET"
           },
           CommonOptions,
+          {
+            mode: number <= FetchRetryConfig.retries ? "cors" : "no-cors"
+          },
           options
         )
       ).catch(err => {

ui/src/Common/Fetch.test.js

@@ -1,4 +1,10 @@
-import { CommonOptions, FetchGet, FetchPost, FetchDelete } from "./Fetch";
+import {
+  CommonOptions,
+  FetchGet,
+  FetchPost,
+  FetchDelete,
+  FetchRetryConfig
+} from "./Fetch";
 
 import merge from "lodash/merge";
 
@@ -60,24 +66,29 @@ describe("Fetch", () => {
     });
   }
 
-  it("FetchGet switches to no-cors after 80% failures", async () => {
+  it("FetchGet switches to no-cors for the last retry", async () => {
     fetch.mockReject(new Error("Fetch error"));
 
     const request = FetchGet("http://example.com", {});
     await expect(request).rejects.toThrow("Fetch error");
 
-    expect(fetch).toHaveBeenCalledTimes(11);
-    expect(fetch).toHaveBeenCalledWith("http://example.com", {
-      method: "GET",
-      credentials: "include",
-      mode: "no-cors",
-      redirect: "follow"
+    expect(fetch).toHaveBeenCalledTimes(FetchRetryConfig.retries + 1);
+    expect(fetch.mock.calls.map(r => r[1])).toMatchObject(
+      Array.from(Array(FetchRetryConfig.retries + 1).keys(), i => ({
+        mode: i < FetchRetryConfig.retries ? "cors" : "no-cors",
+        credentials: "include"
+      }))
+    );
+    // ensure that the the second to last call was with cors
+    expect(fetch.mock.calls[fetch.mock.calls.length - 2][1]).toMatchObject({
+      mode: "cors",
+      credentials: "include"
+    });
+    // ensure that the last call was with no-cors
+    expect(fetch.mock.calls[fetch.mock.calls.length - 1][1]).toMatchObject({
+      mode: "no-cors",
+      credentials: "include"
     });
-    for (let i = 0; i <= 10; i++) {
-      expect(fetch.mock.calls[i][1]).toMatchObject({
-        mode: i < 8 ? "cors" : "no-cors"
-      });
-    }
   });
 
   it("FetchGet calls beforeRetry before each retry", async () => {
@@ -88,9 +99,9 @@ describe("Fetch", () => {
     const request = FetchGet("http://example.com", {}, beforeRetrySpy);
     await expect(request).rejects.toThrow("Fetch error");
 
-    expect(beforeRetrySpy).toHaveBeenCalledTimes(11);
+    expect(beforeRetrySpy).toHaveBeenCalledTimes(FetchRetryConfig.retries + 1);
 
-    for (let i = 0; i < 10; i++) {
+    for (let i = 0; i < FetchRetryConfig.retries + 1; i++) {
       expect(beforeRetrySpy.mock.calls[i][0]).toBe(i + 1);
     }
   });

ui/src/Components/NavBar/FilterInput/index.test.js

@@ -203,7 +203,7 @@ describe("<FilterInput Autosuggest />", () => {
     tree.find("input").simulate("change", { target: { value: "bar" } });
     await WaitForFetch(tree);
 
-    expect(fetch.mock.calls).toHaveLength(11);
+    expect(fetch).toHaveBeenCalledTimes(10);
     expect(fetch.mock.calls[0]).toContain("./autocomplete.json?term=bar");
     expect(instance.inputStore.suggestions).toHaveLength(0);
   });

ui/src/Stores/AlertStore.test.js

@@ -375,7 +375,7 @@ describe("AlertStore.fetch", () => {
     const store = new AlertStore([]);
     await expect(store.fetch()).resolves.toHaveProperty("error");
 
-    expect(global.fetch).toHaveBeenCalledTimes(11);
+    expect(global.fetch).toHaveBeenCalledTimes(10);
     expect(store.status.value).toEqual(AlertStoreStatuses.Failure);
     expect(store.info.version).toBe("unknown");
     // there should be a trace of the error
@@ -388,7 +388,7 @@ describe("AlertStore.fetch", () => {
 
     fetch.mockReject(new Error("Fetch error"));
     await expect(store.fetch()).resolves.toHaveProperty("error");
-    expect(global.fetch).toHaveBeenCalledTimes(11);
+    expect(global.fetch).toHaveBeenCalledTimes(10);
   });
 
   it("fetch() retry counter is reset after successful fetch", async () => {
@@ -397,16 +397,16 @@ describe("AlertStore.fetch", () => {
 
     fetch.mockReject(new Error("Fetch error"));
     await expect(store.fetch()).resolves.toHaveProperty("error");
-    expect(global.fetch).toHaveBeenCalledTimes(11);
+    expect(global.fetch).toHaveBeenCalledTimes(10);
 
     const response = EmptyAPIResponse();
     fetch.mockResponse(JSON.stringify(response));
     await expect(store.fetch()).resolves.toBeUndefined();
-    expect(global.fetch).toHaveBeenCalledTimes(12);
+    expect(global.fetch).toHaveBeenCalledTimes(11);
 
     fetch.mockReject(new Error("Fetch error"));
     await expect(store.fetch()).resolves.toHaveProperty("error");
-    expect(global.fetch).toHaveBeenCalledTimes(23);
+    expect(global.fetch).toHaveBeenCalledTimes(21);
   });
 
   it("fetch() reloads the page after if auth middleware is detected", async () => {