From bd9b47b4eb64b84c577579a78349d4c38a40521d Mon Sep 17 00:00:00 2001 From: Mcat12 Date: Mon, 15 Apr 2019 18:47:33 -0700 Subject: [PATCH 1/4] Rename ListPage's event handler props to onX Signed-off-by: Mcat12 --- src/components/list/ListPage.tsx | 12 +-- .../list/__tests__/ListPage.test.tsx | 84 +++++++++---------- src/views/Blacklist.tsx | 6 +- src/views/Regexlist.tsx | 6 +- src/views/Whitelist.tsx | 6 +- 5 files changed, 57 insertions(+), 57 deletions(-) diff --git a/src/components/list/ListPage.tsx b/src/components/list/ListPage.tsx index 336beca..6450a1d 100644 --- a/src/components/list/ListPage.tsx +++ b/src/components/list/ListPage.tsx @@ -23,9 +23,9 @@ export interface ListPageProps extends WithNamespaces { title: string; note?: {} | string; placeholder: string; - add: (domain: string) => Promise; - refresh: () => Promise; - remove: (domain: string) => Promise; + onAdd: (domain: string) => Promise; + onRefresh: () => Promise; + onRemove: (domain: string) => Promise; isValid: (domain: string) => boolean; validationErrorMsg: string; } @@ -60,7 +60,7 @@ export class ListPage extends Component { const prevDomains = this.state.domains.slice(); // Try to add the domain - this.addHandler = makeCancelable(this.props.add(domain)); + this.addHandler = makeCancelable(this.props.onAdd(domain)); this.addHandler.promise .then(() => { this.onAdded(domain); @@ -117,7 +117,7 @@ export class ListPage extends Component { if (this.state.domains.includes(domain)) { const prevDomains = this.state.domains.slice(); - this.removeHandler = makeCancelable(this.props.remove(domain)); + this.removeHandler = makeCancelable(this.props.onRemove(domain)); this.removeHandler.promise.catch(ignoreCancel).catch(() => { this.onRemoveFailed(domain, prevDomains); }); @@ -127,7 +127,7 @@ export class ListPage extends Component { }; onRefresh = () => { - this.refreshHandler = makeCancelable(this.props.refresh()); + this.refreshHandler = makeCancelable(this.props.onRefresh()); this.refreshHandler.promise .then(data => { this.setState({ domains: data }); diff --git a/src/components/list/__tests__/ListPage.test.tsx b/src/components/list/__tests__/ListPage.test.tsx index 5f42384..e4103f3 100644 --- a/src/components/list/__tests__/ListPage.test.tsx +++ b/src/components/list/__tests__/ListPage.test.tsx @@ -32,9 +32,9 @@ it("shows the title", () => { title={title} placeholder="" note="" - add={ignoreAPI} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -50,9 +50,9 @@ it("shows the placeholder", () => { title="" placeholder={placeholder} note="" - add={ignoreAPI} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -68,9 +68,9 @@ it("shows the note", () => { title="" placeholder="" note={note} - add={ignoreAPI} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -85,9 +85,9 @@ it("starts with no alerts shown", () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -103,9 +103,9 @@ it("loads domains after mounting", async () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={() => Promise.resolve(domains)} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={() => Promise.resolve(domains)} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -125,9 +125,9 @@ it("checks if the domain was already added", async () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={() => Promise.resolve(domains)} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={() => Promise.resolve(domains)} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -152,9 +152,9 @@ it("calls the add prop when adding a domain", () => { title="" placeholder="" note="" - add={add} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={add} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -173,9 +173,9 @@ it("calls onAdding when adding a domain", () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -196,9 +196,9 @@ it("calls onAdded after API request succeeds", async () => { title="" placeholder="" note="" - add={() => Promise.resolve()} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={() => Promise.resolve()} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -220,9 +220,9 @@ it("calls onAddFailed after API request fails", async () => { title="" placeholder="" note="" - add={() => Promise.reject({})} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={() => Promise.reject({})} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -243,9 +243,9 @@ it("adds the domain in onAdded", async () => { title="" placeholder="" note="" - add={() => Promise.resolve()} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={() => Promise.resolve()} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -265,9 +265,9 @@ it("resets the domains when adding failed", async () => { title="" placeholder="" note="" - add={() => Promise.reject({})} - refresh={ignoreAPI} - remove={ignoreAPI} + onAdd={() => Promise.reject({})} + onRefresh={ignoreAPI} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -288,9 +288,9 @@ it("removes the domain when onRemoved is called", async () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={() => Promise.resolve(domains)} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={() => Promise.resolve(domains)} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> @@ -310,9 +310,9 @@ it("resets the domains when removal failed", () => { title="" placeholder="" note="" - add={ignoreAPI} - refresh={() => Promise.resolve(domains)} - remove={ignoreAPI} + onAdd={ignoreAPI} + onRefresh={() => Promise.resolve(domains)} + onRemove={ignoreAPI} isValid={jest.fn()} validationErrorMsg="" /> diff --git a/src/views/Blacklist.tsx b/src/views/Blacklist.tsx index f24479b..8a1db81 100644 --- a/src/views/Blacklist.tsx +++ b/src/views/Blacklist.tsx @@ -21,9 +21,9 @@ const Blacklist: FunctionComponent = props => { = props => { = props => { Date: Mon, 15 Apr 2019 19:52:49 -0700 Subject: [PATCH 2/4] Bring list folder coverage to 100% by adding/improving ListPage tests Also used `jest.spyOn` where possible to avoid directly manipulating the component's methods. Signed-off-by: Mcat12 --- .../list/__tests__/ListPage.test.tsx | 172 ++++++++++++++---- 1 file changed, 139 insertions(+), 33 deletions(-) diff --git a/src/components/list/__tests__/ListPage.test.tsx b/src/components/list/__tests__/ListPage.test.tsx index e4103f3..71528f7 100644 --- a/src/components/list/__tests__/ListPage.test.tsx +++ b/src/components/list/__tests__/ListPage.test.tsx @@ -15,6 +15,9 @@ import ListPage, { ListPageProps, ListPageState } from "../ListPage"; +import Alert from "../../common/Alert"; +import DomainInput from "../DomainInput"; +import DomainList from "../DomainList"; const ignoreAPI = global.ignoreAPI; const tick = global.tick; @@ -58,7 +61,7 @@ it("shows the placeholder", () => { /> ); - expect(wrapper.find("DomainInput")).toHaveProp("placeholder", placeholder); + expect(wrapper.find(DomainInput)).toHaveProp("placeholder", placeholder); }); it("shows the note", () => { @@ -93,7 +96,97 @@ it("starts with no alerts shown", () => { /> ); - expect(wrapper.find("Alert")).toHaveLength(0); + expect(wrapper.find(Alert)).toHaveLength(0); +}); + +it("hides the alert if closed", () => { + const wrapper: ListPageWrapper = shallow( + + ); + + // Show an error message + wrapper.instance().onAlreadyAdded("domain"); + + // Now the alert is shown + const alert = wrapper.find(Alert); + expect(alert).toHaveLength(1); + + // Hide the alert + alert.props().onClick(); + + expect(wrapper.find(Alert)).toHaveLength(0); +}); + +it("cancels requests when un-mounting", async () => { + const wrapper: ListPageWrapper = shallow( + Promise.resolve(["domain"])} + onRemove={ignoreAPI} + isValid={jest.fn()} + validationErrorMsg="" + /> + ); + + // Load the domains into state + await tick(); + + // Initiate requests to refresh, add, and remove domains + wrapper.instance().onRefresh(); + wrapper.instance().onEnter("domain2"); + wrapper.instance().onRemove("domain"); + + // Spy on the handlers + // Casting instance to any to access private fields + const instance = wrapper.instance() as any; + const cancelRefreshSpy = jest.spyOn(instance.refreshHandler, "cancel"); + const cancelAddSpy = jest.spyOn(instance.addHandler, "cancel"); + const cancelRemoveSpy = jest.spyOn(instance.removeHandler, "cancel"); + + // Unmount, which should cancel the requests + wrapper.unmount(); + + expect(cancelRefreshSpy).toHaveBeenCalled(); + expect(cancelAddSpy).toHaveBeenCalled(); + expect(cancelRemoveSpy).toHaveBeenCalled(); +}); + +it("shows a validation message as an error", () => { + const validationError = "test message"; + const wrapper = shallow( + + ); + + wrapper + .find(DomainInput) + .props() + .onValidationError(); + + const alert = wrapper.find(Alert); + expect(alert).toHaveLength(1); + expect(alert.props().message).toEqual(validationError); + expect(alert.props().type).toEqual("danger"); }); it("loads domains after mounting", async () => { @@ -112,14 +205,12 @@ it("loads domains after mounting", async () => { ); await tick(); - wrapper.update(); - expect(wrapper.find("DomainList")).toHaveProp("domains", domains); + expect(wrapper.find(DomainList)).toHaveProp("domains", domains); }); it("checks if the domain was already added", async () => { const domains = ["domain1", "domain2.com", "domain3.net"]; - const onAlreadyAdded = jest.fn(); const wrapper: ListPageWrapper = shallow( { validationErrorMsg="" /> ); + const onAlreadyAdded = jest.spyOn(wrapper.instance(), "onAlreadyAdded"); - // Setup with domains (wait for promise to resolve) and mock function + // Setup with domains (wait for promise to resolve) await tick(); - wrapper.instance().onAlreadyAdded = onAlreadyAdded; - wrapper.update(); - // Test onEnter wrapper.instance().onEnter(domains[0]); expect(onAlreadyAdded).toHaveBeenCalledWith(domains[0]); @@ -146,13 +235,13 @@ it("checks if the domain was already added", async () => { it("calls the add prop when adding a domain", () => { const domain = "domain"; - const add = jest.fn(ignoreAPI); + const onAdd = jest.fn(ignoreAPI); const wrapper: ListPageWrapper = shallow( { wrapper.instance().onEnter(domain); - expect(add).toHaveBeenCalledWith(domain); + expect(onAdd).toHaveBeenCalledWith(domain); }); it("calls onAdding when adding a domain", () => { const domain = "domain"; - const onAdding = jest.fn(); const wrapper: ListPageWrapper = shallow( { validationErrorMsg="" /> ); + const onAdding = jest.spyOn(wrapper.instance(), "onAdding"); - wrapper.instance().onAdding = onAdding; - wrapper.update(); wrapper.instance().onEnter(domain); expect(onAdding).toHaveBeenCalledWith(domain); @@ -190,7 +277,6 @@ it("calls onAdding when adding a domain", () => { it("calls onAdded after API request succeeds", async () => { const domain = "domain"; - const onAdded = jest.fn(); const wrapper: ListPageWrapper = shallow( { validationErrorMsg="" /> ); + const onAdded = jest.spyOn(wrapper.instance(), "onAdded"); - wrapper.instance().onAdded = onAdded; - wrapper.update(); wrapper.instance().onEnter(domain); await tick(); @@ -214,7 +299,6 @@ it("calls onAdded after API request succeeds", async () => { it("calls onAddFailed after API request fails", async () => { const domain = "domain"; - const onAddFailed = jest.fn(); const wrapper: ListPageWrapper = shallow( { validationErrorMsg="" /> ); + const onAddFailed = jest.spyOn(wrapper.instance(), "onAddFailed"); - wrapper.instance().onAddFailed = onAddFailed; - wrapper.update(); wrapper.instance().onEnter(domain); await tick(); @@ -252,7 +335,6 @@ it("adds the domain in onAdded", async () => { ); wrapper.instance().onEnter(domain); - wrapper.update(); await tick(); expect(wrapper.state().domains).toEqual([domain]); @@ -274,15 +356,14 @@ it("resets the domains when adding failed", async () => { ); wrapper.instance().onEnter(domain); - wrapper.update(); await tick(); expect(wrapper.state().domains).toEqual([]); }); -it("removes the domain when onRemoved is called", async () => { - const domain = "domain"; - const domains = [domain]; +it("does not remove the domain if it is not present", async () => { + const domain = "domain1"; + const domains = ["domain2"]; const wrapper: ListPageWrapper = shallow( { /> ); - wrapper.instance().onRemoved(domain); - wrapper.update(); + await tick(); + wrapper.instance().onRemove(domain); - expect(wrapper.state().domains).toEqual([]); + expect(wrapper.state().domains).toEqual(domains); }); -it("resets the domains when removal failed", () => { +it("removes the domain from state when onRemove is called", async () => { const domain = "domain"; + const domain2 = "domain2"; + const domains = [domain, domain2]; + const wrapper: ListPageWrapper = shallow( + Promise.resolve(domains)} + onRemove={() => Promise.resolve()} + isValid={jest.fn()} + validationErrorMsg="" + /> + ); + + await tick(); + wrapper.instance().onRemove(domain); + + expect(wrapper.state().domains).toEqual([domain2]); +}); + +it("resets the domains when removal failed", async () => { + const domain = "domain"; const domains = [domain]; const wrapper: ListPageWrapper = shallow( { note="" onAdd={ignoreAPI} onRefresh={() => Promise.resolve(domains)} - onRemove={ignoreAPI} + onRemove={() => Promise.reject()} isValid={jest.fn()} validationErrorMsg="" /> ); - wrapper.instance().onRemoveFailed(domain, domains); - wrapper.update(); + + await tick(); + wrapper.instance().onRemove(domain); + await tick(); expect(wrapper.state().domains).toEqual(domains); }); From ab6455d48b033eba2bf02a3b65abea76444eeb72 Mon Sep 17 00:00:00 2001 From: Mcat12 Date: Mon, 15 Apr 2019 20:08:46 -0700 Subject: [PATCH 3/4] Allow an Alert to be non-dismissible and use it in DomainList Also improved DomainList tests. Signed-off-by: Mcat12 --- src/components/common/Alert.tsx | 22 +++++++++++-------- src/components/list/DomainList.tsx | 9 +++++--- .../list/__tests__/DomainList.test.tsx | 16 ++++++-------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/components/common/Alert.tsx b/src/components/common/Alert.tsx index a36f131..414252e 100644 --- a/src/components/common/Alert.tsx +++ b/src/components/common/Alert.tsx @@ -8,7 +8,7 @@ * This file is copyright under the latest version of the EUPL. * Please see LICENSE file for your rights under this license. */ -import React, { FunctionComponent } from "react"; +import React from "react"; export type AlertType = "info" | "success" | "danger"; @@ -16,23 +16,27 @@ export interface AlertProps { type: AlertType; onClick: () => void; message: string; + dismissible: boolean; } -const Alert: FunctionComponent = (props: AlertProps) => { +const Alert = (props: AlertProps) => { + const dismissClass = props.dismissible ? "alert-dismissible" : ""; + return ( -
- +
+ {props.dismissible ? ( + + ) : null} {props.message}
); }; Alert.defaultProps = { - onClick: () => {} + onClick: () => {}, + dismissible: true }; export default Alert; diff --git a/src/components/list/DomainList.tsx b/src/components/list/DomainList.tsx index ab9e4cf..4863803 100644 --- a/src/components/list/DomainList.tsx +++ b/src/components/list/DomainList.tsx @@ -12,6 +12,7 @@ import React from "react"; import { WithNamespaces, withNamespaces } from "react-i18next"; import api from "../../util/api"; import { Button } from "reactstrap"; +import Alert from "../common/Alert"; export interface DomainListProps extends WithNamespaces { domains: string[]; @@ -54,9 +55,11 @@ const DomainList = ({ domains, onRemove, t }: DomainListProps) => { body = domains.map(mapDomainsToListItems); } else { body = ( -
- {t("There are no domains in this list")} -
+ ); } diff --git a/src/components/list/__tests__/DomainList.test.tsx b/src/components/list/__tests__/DomainList.test.tsx index eb9b828..c66681c 100644 --- a/src/components/list/__tests__/DomainList.test.tsx +++ b/src/components/list/__tests__/DomainList.test.tsx @@ -12,6 +12,7 @@ import React from "react"; import { shallow } from "enzyme"; import DomainList from "../DomainList"; import api from "../../../util/api"; +import Alert from "../../common/Alert"; const domains = ["domain1.com", "domain2.com", "domain3.com"]; @@ -26,9 +27,11 @@ it("shows a list of domains", () => { it("shows an alert if there are no domains", () => { const wrapper = shallow(); - expect(wrapper.find("li")).toHaveLength(0); - expect(wrapper.find("ul").childAt(0)).toHaveClassName("alert-info"); - expect(wrapper).toIncludeText("There are no domains in this list"); + expect(wrapper.find("li")).not.toExist(); + expect(wrapper.find(Alert)).toExist(); + expect(wrapper.find(Alert).props().message).toEqual( + "There are no domains in this list" + ); }); it("does not have a delete button when not logged in", () => { @@ -36,12 +39,7 @@ it("does not have a delete button when not logged in", () => { ); - expect( - wrapper - .find("ul") - .childAt(0) - .find("button") - ).not.toExist(); + expect(wrapper.find("Button")).not.toExist(); }); it("has a delete button when logged in", () => { From fef0e9d2d0a9e76f2e6e82809204d17e9cd5b341 Mon Sep 17 00:00:00 2001 From: Mcat12 Date: Mon, 15 Apr 2019 20:10:28 -0700 Subject: [PATCH 4/4] Change ListPage tests to use toExist instead of toHaveLength Signed-off-by: Mcat12 --- src/components/list/__tests__/ListPage.test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/list/__tests__/ListPage.test.tsx b/src/components/list/__tests__/ListPage.test.tsx index 71528f7..4f1a013 100644 --- a/src/components/list/__tests__/ListPage.test.tsx +++ b/src/components/list/__tests__/ListPage.test.tsx @@ -96,7 +96,7 @@ it("starts with no alerts shown", () => { /> ); - expect(wrapper.find(Alert)).toHaveLength(0); + expect(wrapper.find(Alert)).not.toExist(); }); it("hides the alert if closed", () => { @@ -118,12 +118,12 @@ it("hides the alert if closed", () => { // Now the alert is shown const alert = wrapper.find(Alert); - expect(alert).toHaveLength(1); + expect(alert).toExist(); // Hide the alert alert.props().onClick(); - expect(wrapper.find(Alert)).toHaveLength(0); + expect(wrapper.find(Alert)).not.toExist(); }); it("cancels requests when un-mounting", async () => { @@ -184,7 +184,7 @@ it("shows a validation message as an error", () => { .onValidationError(); const alert = wrapper.find(Alert); - expect(alert).toHaveLength(1); + expect(alert).toExist(); expect(alert.props().message).toEqual(validationError); expect(alert.props().type).toEqual("danger"); });