From b42d1062ceff72f82ffee30fb35d34cf6c918652 Mon Sep 17 00:00:00 2001 From: Simba Date: Thu, 23 Jul 2020 22:00:52 -0700 Subject: [PATCH 1/6] Rename ResourceList to ResourceListView Redux logic will be added in a ResourceList component which wraps the ResourceListView component. --- .../app/__snapshots__/App.test.js.snap | 22 +++++++++---------- .../{ResourceList.js => ResourceListView.js} | 4 ++-- ...eList.test.js => ResourceListView.test.js} | 6 ++--- .../src/components/resource-list/index.js | 2 +- 4 files changed, 17 insertions(+), 17 deletions(-) rename dashboard/src/components/resource-list/{ResourceList.js => ResourceListView.js} (89%) rename dashboard/src/components/resource-list/{ResourceList.test.js => ResourceListView.test.js} (69%) diff --git a/dashboard/src/components/app/__snapshots__/App.test.js.snap b/dashboard/src/components/app/__snapshots__/App.test.js.snap index 7b82561..6d5aff0 100644 --- a/dashboard/src/components/app/__snapshots__/App.test.js.snap +++ b/dashboard/src/components/app/__snapshots__/App.test.js.snap @@ -442,7 +442,7 @@ exports[`App hasn't changed 1`] = ` key="/sources" path="/sources" > - @@ -450,7 +450,7 @@ exports[`App hasn't changed 1`] = ` key="/views" path="/views" > - @@ -458,7 +458,7 @@ exports[`App hasn't changed 1`] = ` key="/features" path="/features" > - @@ -466,7 +466,7 @@ exports[`App hasn't changed 1`] = ` key="/feature-sets" path="/feature-sets" > - @@ -474,7 +474,7 @@ exports[`App hasn't changed 1`] = ` key="/training-sets" path="/training-sets" > - @@ -482,7 +482,7 @@ exports[`App hasn't changed 1`] = ` key="/metrics" path="/metrics" > - @@ -490,7 +490,7 @@ exports[`App hasn't changed 1`] = ` key="/deployment" path="/deployment" > - @@ -498,7 +498,7 @@ exports[`App hasn't changed 1`] = ` key="/users" path="/users" > - @@ -506,7 +506,7 @@ exports[`App hasn't changed 1`] = ` key="/settings" path="/settings" > - @@ -514,7 +514,7 @@ exports[`App hasn't changed 1`] = ` key="/billing" path="/billing" > - @@ -522,7 +522,7 @@ exports[`App hasn't changed 1`] = ` key="/help" path="/help" > - diff --git a/dashboard/src/components/resource-list/ResourceList.js b/dashboard/src/components/resource-list/ResourceListView.js similarity index 89% rename from dashboard/src/components/resource-list/ResourceList.js rename to dashboard/src/components/resource-list/ResourceListView.js index 1aec58d..3a6b9c8 100644 --- a/dashboard/src/components/resource-list/ResourceList.js +++ b/dashboard/src/components/resource-list/ResourceListView.js @@ -10,7 +10,7 @@ const useStyles = makeStyles((theme) => ({ }, })); -export const ResourceList = ({ title }) => { +export const ResourceListView = ({ title }) => { const classes = useStyles(); return ( // MaterialTable doesn't support className, so we wrap it in a Box: @@ -28,4 +28,4 @@ export const ResourceList = ({ title }) => { ); }; -export default ResourceList; +export default ResourceListView; diff --git a/dashboard/src/components/resource-list/ResourceList.test.js b/dashboard/src/components/resource-list/ResourceListView.test.js similarity index 69% rename from dashboard/src/components/resource-list/ResourceList.test.js rename to dashboard/src/components/resource-list/ResourceListView.test.js index 9b61029..16033d4 100644 --- a/dashboard/src/components/resource-list/ResourceList.test.js +++ b/dashboard/src/components/resource-list/ResourceListView.test.js @@ -4,12 +4,12 @@ import "jest-canvas-mock"; import { configure, shallow } from "enzyme"; import Adapter from "enzyme-adapter-react-16"; -import { ResourceList } from "./ResourceList.js"; +import { ResourceListView } from "./ResourceListView.js"; configure({ adapter: new Adapter() }); -describe("ResourceList", () => { - const sources = shallow(); +describe("ResourceListView", () => { + const sources = shallow(); it("passes title to child prop", () => { expect(sources.children().props().title).toEqual("abc"); diff --git a/dashboard/src/components/resource-list/index.js b/dashboard/src/components/resource-list/index.js index 65f6e17..460436e 100644 --- a/dashboard/src/components/resource-list/index.js +++ b/dashboard/src/components/resource-list/index.js @@ -1 +1 @@ -export { default } from "./ResourceList.js"; +export { default } from "./ResourceListView.js"; From 8c7be18c1ae440b7337005032e491857f3065f2e Mon Sep 17 00:00:00 2001 From: Simba Date: Tue, 28 Jul 2020 13:40:48 -0700 Subject: [PATCH 2/6] Add Enum package to dashboard This will be used initially to specify the resource types. --- dashboard/package-lock.json | 5 +++++ dashboard/package.json | 1 + 2 files changed, 6 insertions(+) diff --git a/dashboard/package-lock.json b/dashboard/package-lock.json index 52a70d8..473c595 100644 --- a/dashboard/package-lock.json +++ b/dashboard/package-lock.json @@ -5163,6 +5163,11 @@ "resolved": "https://registry.npmjs.org/entities/-/entities-2.0.3.tgz", "integrity": "sha512-MyoZ0jgnLvB2X3Lg5HqpFmn1kybDiIfEQmKzTb5apr51Rb+T3KdmMiqa70T+bhGnyv7bQ6WMj2QMHpGMmlrUYQ==" }, + "enum": { + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/enum/-/enum-3.0.4.tgz", + "integrity": "sha512-X+VaPeIut6Vh0nJq1sfSBQQuqVmjUpiheoR5RokqeRm4xkzsNZjOqK+gpSnW+BrEs5d9q7Wl+nf1LpolFDYp4g==" + }, "enzyme": { "version": "3.11.0", "resolved": "https://registry.npmjs.org/enzyme/-/enzyme-3.11.0.tgz", diff --git a/dashboard/package.json b/dashboard/package.json index 714e3f6..b155e2c 100644 --- a/dashboard/package.json +++ b/dashboard/package.json @@ -9,6 +9,7 @@ "@testing-library/jest-dom": "^4.2.4", "@testing-library/react": "^9.5.0", "@testing-library/user-event": "^7.2.1", + "enum": "^3.0.4", "jest-canvas-mock": "^2.2.0", "material-table": "^1.66.0", "react": "^16.13.1", From 0f08e43683facdef35474d3c8f4f0bd135c76cc7 Mon Sep 17 00:00:00 2001 From: Simba Date: Wed, 29 Jul 2020 00:17:26 -0700 Subject: [PATCH 3/6] Add deferred to dashboard dependencies Deferred is used to set ordering in specific ways to test async code without any chance of race conditions. --- dashboard/package-lock.json | 30 ++++++++++++++++++++++++++++++ dashboard/package.json | 1 + 2 files changed, 31 insertions(+) diff --git a/dashboard/package-lock.json b/dashboard/package-lock.json index 473c595..4acedfe 100644 --- a/dashboard/package-lock.json +++ b/dashboard/package-lock.json @@ -4692,6 +4692,18 @@ "ip-regex": "^2.1.0" } }, + "deferred": { + "version": "0.7.11", + "resolved": "https://registry.npmjs.org/deferred/-/deferred-0.7.11.tgz", + "integrity": "sha512-8eluCl/Blx4YOGwMapBvXRKxHXhA8ejDXYzEaK8+/gtcm8hRMhSLmXSqDmNUKNc/C8HNSmuyyp/hflhqDAvK2A==", + "requires": { + "d": "^1.0.1", + "es5-ext": "^0.10.50", + "event-emitter": "^0.3.5", + "next-tick": "^1.0.0", + "timers-ext": "^0.1.7" + } + }, "define-properties": { "version": "1.1.3", "resolved": "https://registry.npmjs.org/define-properties/-/define-properties-1.1.3.tgz", @@ -5848,6 +5860,15 @@ "resolved": "https://registry.npmjs.org/etag/-/etag-1.8.1.tgz", "integrity": "sha1-Qa4u62XvpiJorr/qg6x9eSmbCIc=" }, + "event-emitter": { + "version": "0.3.5", + "resolved": "https://registry.npmjs.org/event-emitter/-/event-emitter-0.3.5.tgz", + "integrity": "sha1-34xp7vFkeSPHFXuc6DhAYQsCzDk=", + "requires": { + "d": "1", + "es5-ext": "~0.10.14" + } + }, "eventemitter3": { "version": "4.0.4", "resolved": "https://registry.npmjs.org/eventemitter3/-/eventemitter3-4.0.4.tgz", @@ -13436,6 +13457,15 @@ "setimmediate": "^1.0.4" } }, + "timers-ext": { + "version": "0.1.7", + "resolved": "https://registry.npmjs.org/timers-ext/-/timers-ext-0.1.7.tgz", + "integrity": "sha512-b85NUNzTSdodShTIbky6ZF02e8STtVVfD+fu4aXXShEELpozH+bCpJLYMPZbsABN2wDH7fJpqIoXxJpzbf0NqQ==", + "requires": { + "es5-ext": "~0.10.46", + "next-tick": "1" + } + }, "timsort": { "version": "0.3.0", "resolved": "https://registry.npmjs.org/timsort/-/timsort-0.3.0.tgz", diff --git a/dashboard/package.json b/dashboard/package.json index b155e2c..60f0e3f 100644 --- a/dashboard/package.json +++ b/dashboard/package.json @@ -9,6 +9,7 @@ "@testing-library/jest-dom": "^4.2.4", "@testing-library/react": "^9.5.0", "@testing-library/user-event": "^7.2.1", + "deferred": "^0.7.11", "enum": "^3.0.4", "jest-canvas-mock": "^2.2.0", "material-table": "^1.66.0", From c8ec2917afc745c63c90cf9fdb3f0d7430ffa6b9 Mon Sep 17 00:00:00 2001 From: Simba Date: Wed, 29 Jul 2020 00:19:37 -0700 Subject: [PATCH 4/6] Add newTestStore fn to redux store This function can be used to create a redux store to use for certain tests. Typically unit-tests will test the reducer directly, but it makes sense to include a store when testing an async thunk. --- dashboard/src/components/redux/store/ReduxStore.js | 5 +++++ dashboard/src/components/redux/store/index.js | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dashboard/src/components/redux/store/ReduxStore.js b/dashboard/src/components/redux/store/ReduxStore.js index ee4be5c..8c437b8 100644 --- a/dashboard/src/components/redux/store/ReduxStore.js +++ b/dashboard/src/components/redux/store/ReduxStore.js @@ -5,4 +5,9 @@ const store = configureStore({ reducer: rootReducer, }); +export const newTestStore = () => + configureStore({ + reducer: rootReducer, + }); + export default store; diff --git a/dashboard/src/components/redux/store/index.js b/dashboard/src/components/redux/store/index.js index d60f40f..6246587 100644 --- a/dashboard/src/components/redux/store/index.js +++ b/dashboard/src/components/redux/store/index.js @@ -1 +1 @@ -export { default } from "./ReduxStore.js"; +export { default, newTestStore } from "./ReduxStore.js"; From 22ce545daf4b3eac4303108600cf174eea7e48f5 Mon Sep 17 00:00:00 2001 From: Simba Date: Wed, 29 Jul 2020 00:21:27 -0700 Subject: [PATCH 5/6] Add ResourceSlice to dashboard A ResourceSlice handles all the redux state for every resource list in the dashboard. Currently this just includes a single async thunk that returns all the resources for a given type. --- .../components/resource-list/ResourceSlice.js | 73 ++++++++ .../resource-list/ResourceSlice.test.js | 158 ++++++++++++++++++ .../src/components/resource-list/index.js | 1 + 3 files changed, 232 insertions(+) create mode 100644 dashboard/src/components/resource-list/ResourceSlice.js create mode 100644 dashboard/src/components/resource-list/ResourceSlice.test.js diff --git a/dashboard/src/components/resource-list/ResourceSlice.js b/dashboard/src/components/resource-list/ResourceSlice.js new file mode 100644 index 0000000..5915c41 --- /dev/null +++ b/dashboard/src/components/resource-list/ResourceSlice.js @@ -0,0 +1,73 @@ +import { createAsyncThunk, createSlice } from "@reduxjs/toolkit"; +import Enum from "enum"; + +export const fetchResources = createAsyncThunk( + "resourceList/fetchByType", + async ({ api, type }, { signal }) => { + const response = await api.fetchResources(type, signal); + return response.data; + }, + { + condition: ({ api, type }, { getState }) => { + const { resources, loading } = getState().resourceList[type]; + if (loading || resources) { + return false; + } + }, + } +); + +export const resourceTypes = new Enum([ + "DATA_SOURCE", + "MATERIALIZED_VIEW", + "FEATURE", + "FEATURE_SET", + "TRAINING_SET", +]); + +const reduceFn = (map, type) => { + map[type] = {}; + return map; +}; +const reduceFnInitial = {}; +export const initialState = resourceTypes.enums.reduce( + reduceFn, + reduceFnInitial +); + +const resourceSlice = createSlice({ + name: "resourceList", + // initialState is a map between each resource type to an empty object. + initialState: initialState, + extraReducers: { + [fetchResources.pending]: (state, action) => { + const type = action.meta.arg.type; + const requestId = action.meta.requestId; + state[type].resources = null; + state[type].requestId = requestId; + state[type].loading = true; + state[type].failed = false; + }, + [fetchResources.fulfilled]: (state, action) => { + const type = action.meta.arg.type; + const requestId = action.meta.requestId; + if (requestId !== state[type].requestId) { + return; + } + state[type].resources = action.payload; + state[type].loading = false; + state[type].failed = false; + }, + [fetchResources.rejected]: (state, action) => { + const type = action.meta.arg.type; + const requestId = action.meta.requestId; + if (requestId !== state[type].requestId) { + return; + } + state[type].loading = false; + state[type].failed = true; + }, + }, +}); + +export default resourceSlice.reducer; diff --git a/dashboard/src/components/resource-list/ResourceSlice.test.js b/dashboard/src/components/resource-list/ResourceSlice.test.js new file mode 100644 index 0000000..2931b19 --- /dev/null +++ b/dashboard/src/components/resource-list/ResourceSlice.test.js @@ -0,0 +1,158 @@ +import "jest-canvas-mock"; +import deferred from "deferred"; +import { configureStore } from "@reduxjs/toolkit"; +import { newTestStore } from "components/redux/store"; +import { + initialState, + fetchResources, + default as resourceReducer, + resourceTypes, +} from "./ResourceSlice.js"; + +const dataType = resourceTypes.get("DATA_SOURCE"); + +describe("fetchResourcesThunk", () => { + const wrapInPromise = (arr) => Promise.resolve({ data: arr }); + + it("fetches resources with dispatch", async () => { + const reduxStore = newTestStore(); + const mockApi = { + fetchResources: jest.fn(() => wrapInPromise(["abc"])), + }; + const data = await reduxStore.dispatch( + fetchResources({ api: mockApi, type: dataType }) + ); + expect(data.payload).toEqual(["abc"]); + }); + + it("sets resources state with dispatch", async () => { + const reduxStore = newTestStore(); + const mockApi = { + fetchResources: jest.fn(() => wrapInPromise(["abc"])), + }; + // fulfilled (or rejected) will be called after this returns. Not waiting + // for this results in a race condition. + await reduxStore.dispatch(fetchResources({ api: mockApi, type: dataType })); + const state = reduxStore.getState(); + const resources = state.resourceList[dataType].resources; + expect(resources).toEqual(["abc"]); + }); + + it("doesn't run a new request when it has data", async () => { + const reduxStore = newTestStore(); + const mockFetchResources = jest.fn(); + mockFetchResources + .mockReturnValueOnce(wrapInPromise(["def"])) + .mockReturnValueOnce(wrapInPromise(["abc"])); + const mockApi = { + fetchResources: mockFetchResources, + }; + await reduxStore.dispatch(fetchResources({ api: mockApi, type: dataType })); + // This one should be a no-op due to the asyncThunk condition. + await reduxStore.dispatch(fetchResources({ api: mockApi, type: dataType })); + const state = reduxStore.getState(); + const resources = state.resourceList[dataType].resources; + expect(resources).toEqual(["def"]); + }); + + it("doesn't run a new request when loading", async () => { + const reduxStore = newTestStore(); + const defer = deferred(); + const mockFetchResources = jest.fn(); + mockFetchResources + .mockReturnValueOnce(defer.promise) + .mockReturnValueOnce(wrapInPromise(["abc"])); + const mockApi = { + fetchResources: mockFetchResources, + }; + // Don't await here since it'll wait for us to resolve the promise, hence + // deadlock. + const origDispatch = reduxStore.dispatch( + fetchResources({ api: mockApi, type: dataType }) + ); + // The second promise is resolved, so we can await it. + await reduxStore.dispatch(fetchResources({ api: mockApi, type: dataType })); + defer.resolve({ data: ["def"] }); + origDispatch.then(() => { + const state = reduxStore.getState(); + const resources = state.resourceList[dataType].resources; + expect(resources).toEqual(["def"]); + }); + }); +}); + +describe("ResourceReducers", () => { + it("sets state to loading on pending", () => { + const action = fetchResources.pending("requestID", { type: dataType }); + const state = resourceReducer({ [dataType]: {} }, action); + expect(state[dataType].loading).toEqual(true); + }); + + it("unsets data on pending", () => { + const action = fetchResources.pending("requestId", { type: dataType }); + const state = resourceReducer({ [dataType]: { data: [] } }, action); + expect(state[dataType].resources).toEqual(null); + }); + + it("sets data on success", () => { + const payload = ["abc"]; + const requestId = "123"; + const action = fetchResources.fulfilled(payload, requestId, { + type: dataType, + }); + const state = resourceReducer( + { [dataType]: { requestId: requestId } }, + action + ); + expect(state[dataType].resources).toEqual(payload); + }); + + it("sets failed on rejected", () => { + const requestId = "123"; + const action = fetchResources.rejected(null, requestId, { type: dataType }); + const state = resourceReducer( + { [dataType]: { requestId: requestId } }, + action + ); + expect(state[dataType].failed).toEqual(true); + }); + + it("clears failed on pending", () => { + const payload = ["abc"]; + const requestId = "123"; + const action = fetchResources.pending(requestId, { type: dataType }); + const state = resourceReducer( + { [dataType]: { requestId: requestId, failed: true } }, + action + ); + expect(state[dataType].failed).toEqual(false); + }); + + it("ignore old request on fulfilled", () => { + const payload = ["abc"]; + const oldRequestId = "456"; + const newRequestId = "123"; + const action = fetchResources.fulfilled(payload, oldRequestId, { + type: dataType, + }); + const state = resourceReducer( + { [dataType]: { loading: true, requestId: newRequestId } }, + action + ); + expect(state[dataType].loading).toEqual(true); + }); + + it("ignore old request on rejected", () => { + const payload = ["abc"]; + const oldRequestId = "456"; + const newRequestId = "123"; + const action = fetchResources.rejected(payload, oldRequestId, { + type: dataType, + }); + const state = resourceReducer( + { [dataType]: { loading: true, requestId: newRequestId } }, + action + ); + expect(state[dataType].loading).toEqual(true); + }); +}); diff --git a/dashboard/src/components/resource-list/index.js b/dashboard/src/components/resource-list/index.js index 460436e..28120b2 100644 --- a/dashboard/src/components/resource-list/index.js +++ b/dashboard/src/components/resource-list/index.js @@ -1 +1,2 @@ export { default } from "./ResourceListView.js"; +export { default as resourceReducer } from "./ResourceSlice.js"; From acc22ee9c63105b031e3b8a69a99406741fdd796 Mon Sep 17 00:00:00 2001 From: Simba Date: Wed, 29 Jul 2020 00:25:02 -0700 Subject: [PATCH 6/6] Add ResourceSlice to rootReducers This adds ResourceSlice's reducer to the default redux store singleton. --- dashboard/src/reducers/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dashboard/src/reducers/index.js b/dashboard/src/reducers/index.js index 07c5096..26607af 100644 --- a/dashboard/src/reducers/index.js +++ b/dashboard/src/reducers/index.js @@ -1,6 +1,8 @@ import { combineReducers } from "redux"; import navSectionsReducer from "components/nav/slice"; +import { resourceReducer } from "components/resource-list"; export default combineReducers({ navSections: navSectionsReducer, + resourceList: resourceReducer, });