Merge pull request #180 from pi-hole/tweak/list-tests

Improve list tests

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<AlertProps> = (props: AlertProps) => {
+const Alert = (props: AlertProps) => {
+  const dismissClass = props.dismissible ? "alert-dismissible" : "";
+
   return (
-    <div
-      className={"alert alert-" + props.type + " alert-dismissible fade show"}
-    >
-      <button type="button" className="close" onClick={props.onClick}>
-        &times;
-      </button>
+    <div className={`alert alert-${props.type} ${dismissClass} fade show`}>
+      {props.dismissible ? (
+        <button type="button" className="close" onClick={props.onClick}>
+          &times;
+        </button>
+      ) : null}
       {props.message}
     </div>
   );
 };
 
 Alert.defaultProps = {
-  onClick: () => {}
+  onClick: () => {},
+  dismissible: true
 };
 
 export default Alert;

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 = (
-      <div className="alert alert-info" role="alert">
-        {t("There are no domains in this list")}
-      </div>
+      <Alert
+        type="info"
+        message={t("There are no domains in this list")}
+        dismissible={false}
+      />
     );
   }
 

src/components/list/ListPage.tsx

@@ -23,9 +23,9 @@ export interface ListPageProps extends WithNamespaces {
   title: string;
   note?: {} | string;
   placeholder: string;
-  add: (domain: string) => Promise<any | never>;
-  refresh: () => Promise<any | never>;
-  remove: (domain: string) => Promise<any | never>;
+  onAdd: (domain: string) => Promise<any | never>;
+  onRefresh: () => Promise<any | never>;
+  onRemove: (domain: string) => Promise<any | never>;
   isValid: (domain: string) => boolean;
   validationErrorMsg: string;
 }
@@ -60,7 +60,7 @@ export class ListPage extends Component<ListPageProps, ListPageState> {
       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<ListPageProps, ListPageState> {
     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<ListPageProps, ListPageState> {
   };
 
   onRefresh = () => {
-    this.refreshHandler = makeCancelable(this.props.refresh());
+    this.refreshHandler = makeCancelable(this.props.onRefresh());
     this.refreshHandler.promise
       .then(data => {
         this.setState({ domains: data });

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(<DomainList domains={[]} onRemove={jest.fn()} />);
 
-  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", () => {
     <DomainList domains={domains} onRemove={jest.fn()} />
   );
 
-  expect(
-    wrapper
-      .find("ul")
-      .childAt(0)
-      .find("button")
-  ).not.toExist();
+  expect(wrapper.find("Button")).not.toExist();
 });
 
 it("has a delete button when logged in", () => {

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;
@@ -32,9 +35,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,15 +53,15 @@ it("shows the placeholder", () => {
       title=""
       placeholder={placeholder}
       note=""
-      add={ignoreAPI}
-      refresh={ignoreAPI}
-      remove={ignoreAPI}
+      onAdd={ignoreAPI}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
   );
 
-  expect(wrapper.find("DomainInput")).toHaveProp("placeholder", placeholder);
+  expect(wrapper.find(DomainInput)).toHaveProp("placeholder", placeholder);
 });
 
 it("shows the note", () => {
@@ -68,9 +71,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,15 +88,105 @@ 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=""
     />
   );
 
-  expect(wrapper.find("Alert")).toHaveLength(0);
+  expect(wrapper.find(Alert)).not.toExist();
+});
+
+it("hides the alert if closed", () => {
+  const wrapper: ListPageWrapper = shallow(
+    <ListPage
+      title=""
+      placeholder=""
+      note=""
+      onAdd={ignoreAPI}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
+      isValid={jest.fn()}
+      validationErrorMsg=""
+    />
+  );
+
+  // Show an error message
+  wrapper.instance().onAlreadyAdded("domain");
+
+  // Now the alert is shown
+  const alert = wrapper.find(Alert);
+  expect(alert).toExist();
+
+  // Hide the alert
+  alert.props().onClick();
+
+  expect(wrapper.find(Alert)).not.toExist();
+});
+
+it("cancels requests when un-mounting", async () => {
+  const wrapper: ListPageWrapper = shallow(
+    <ListPage
+      title=""
+      placeholder=""
+      note=""
+      onAdd={ignoreAPI}
+      onRefresh={() => 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(
+    <ListPage
+      title=""
+      placeholder=""
+      note=""
+      onAdd={ignoreAPI}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
+      isValid={jest.fn()}
+      validationErrorMsg={validationError}
+    />
+  );
+
+  wrapper
+    .find(DomainInput)
+    .props()
+    .onValidationError();
+
+  const alert = wrapper.find(Alert);
+  expect(alert).toExist();
+  expect(alert.props().message).toEqual(validationError);
+  expect(alert.props().type).toEqual("danger");
 });
 
 it("loads domains after mounting", async () => {
@@ -103,42 +196,38 @@ 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=""
     />
   );
 
   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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={ignoreAPI}
-      refresh={() => Promise.resolve(domains)}
-      remove={ignoreAPI}
+      onAdd={ignoreAPI}
+      onRefresh={() => Promise.resolve(domains)}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       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,15 +235,15 @@ 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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={add}
-      refresh={ignoreAPI}
-      remove={ignoreAPI}
+      onAdd={onAdd}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
@@ -162,27 +251,25 @@ it("calls the add prop when adding a domain", () => {
 
   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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={ignoreAPI}
-      refresh={ignoreAPI}
-      remove={ignoreAPI}
+      onAdd={ignoreAPI}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
   );
+  const onAdding = jest.spyOn(wrapper.instance(), "onAdding");
 
-  wrapper.instance().onAdding = onAdding;
-  wrapper.update();
   wrapper.instance().onEnter(domain);
 
   expect(onAdding).toHaveBeenCalledWith(domain);
@@ -190,22 +277,20 @@ 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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={() => Promise.resolve()}
-      refresh={ignoreAPI}
-      remove={ignoreAPI}
+      onAdd={() => Promise.resolve()}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
   );
+  const onAdded = jest.spyOn(wrapper.instance(), "onAdded");
 
-  wrapper.instance().onAdded = onAdded;
-  wrapper.update();
   wrapper.instance().onEnter(domain);
   await tick();
 
@@ -214,22 +299,20 @@ 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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={() => Promise.reject({})}
-      refresh={ignoreAPI}
-      remove={ignoreAPI}
+      onAdd={() => Promise.reject({})}
+      onRefresh={ignoreAPI}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
   );
+  const onAddFailed = jest.spyOn(wrapper.instance(), "onAddFailed");
 
-  wrapper.instance().onAddFailed = onAddFailed;
-  wrapper.update();
   wrapper.instance().onEnter(domain);
   await tick();
 
@@ -243,16 +326,15 @@ 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=""
     />
   );
 
   wrapper.instance().onEnter(domain);
-  wrapper.update();
   await tick();
 
   expect(wrapper.state().domains).toEqual([domain]);
@@ -265,60 +347,84 @@ 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=""
     />
   );
 
   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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={ignoreAPI}
-      refresh={() => Promise.resolve(domains)}
-      remove={ignoreAPI}
+      onAdd={ignoreAPI}
+      onRefresh={() => Promise.resolve(domains)}
+      onRemove={ignoreAPI}
       isValid={jest.fn()}
       validationErrorMsg=""
     />
   );
 
-  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(
+    <ListPage
+      title=""
+      placeholder=""
+      note=""
+      onAdd={ignoreAPI}
+      onRefresh={() => 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(
     <ListPage
       title=""
       placeholder=""
       note=""
-      add={ignoreAPI}
-      refresh={() => Promise.resolve(domains)}
-      remove={ignoreAPI}
+      onAdd={ignoreAPI}
+      onRefresh={() => Promise.resolve(domains)}
+      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);
 });

src/views/Blacklist.tsx

@@ -21,9 +21,9 @@ const Blacklist: FunctionComponent<WithNamespaces> = props => {
     <ListPage
       title={`${t("Blacklist")} (${t("Exact")})`}
       placeholder={t("Add a domain or hostname (example.com or example)")}
-      add={api.addBlacklist}
-      remove={api.removeBlacklist}
-      refresh={api.getBlacklist}
+      onAdd={api.addBlacklist}
+      onRemove={api.removeBlacklist}
+      onRefresh={api.getBlacklist}
       isValid={isValidHostname}
       validationErrorMsg={t("Not a valid hostname")}
       {...props}

src/views/Regexlist.tsx

@@ -21,9 +21,9 @@ const Regexlist: FunctionComponent<WithNamespaces> = props => {
     <ListPage
       title={`${t("Blacklist")} (${t("Regex")})`}
       placeholder={t("Input a regular expression")}
-      add={api.addRegexlist}
-      remove={api.removeRegexlist}
-      refresh={api.getRegexlist}
+      onAdd={api.addRegexlist}
+      onRemove={api.removeRegexlist}
+      onRefresh={api.getRegexlist}
       isValid={isValidRegex}
       validationErrorMsg={t("Not a valid regular expression")}
       {...props}

src/views/Whitelist.tsx

@@ -21,9 +21,9 @@ const Whitelist: FunctionComponent<WithNamespaces> = props => {
     <ListPage
       title={t("Whitelist")}
       placeholder={t("Add a domain or hostname (example.com or example)")}
-      add={api.addWhitelist}
-      remove={api.removeWhitelist}
-      refresh={api.getWhitelist}
+      onAdd={api.addWhitelist}
+      onRemove={api.removeWhitelist}
+      onRefresh={api.getWhitelist}
       isValid={isValidHostname}
       validationErrorMsg={t("Not a valid hostname")}
       {...props}