refactor categorical controls state (#1549)

* refactor categorical controls state

* lint

* fix race condition in tests

* fix typo

* add missing update on subset

* remove obsolete code

* update jest and puppeteer major version; update all minors

* update when label changes

* remove lint from tests; increase timeouts in e2e tests

* changes in response to PR review

* lint

* more PR comment changes

* more PR comment fixes

* lint

* more PR comment resolutions

client/__tests__/e2e/cellxgeneActions.js

@@ -1,6 +1,6 @@
 import { strict as assert } from "assert";
 
-export const cellxgeneActions = (page, utils) => ({
+const cellxgeneActions = (page, utils) => ({
   async drag(testId, start, end, lasso = false) {
     const layout = await utils.waitByID(testId);
     const elBox = await layout.boxModel();
@@ -31,14 +31,16 @@ export const cellxgeneActions = (page, utils) => ({
 
   async getAllHistograms(testclass, testIds) {
     const histTestIds = testIds.map((tid) => `histogram-${tid}`);
-    // these load asynchronously, so we need to wait for each histogram individually
-    await utils.waitForAllByIds(histTestIds);
+    // these load asynchronously, so we need to wait for each histogram individually,
+    // and they may be quite slow in some cases.
+    await utils.waitForAllByIds(histTestIds, { timeout: 240000 });
     const allHistograms = await utils.getAllByClass(testclass);
     return allHistograms.map((hist) => hist.replace(/^histogram-/, ""));
   },
 
   async getAllCategoriesAndCounts(category) {
-    await utils.waitByClass("categorical-row");
+    // these load asynchronously, so we have to wait for the specific category.
+    await utils.waitByID(`category-${category}`);
     return page.$$eval(
       `[data-testid="category-${category}"] [data-testclass='categorical-row']`,
       (rows) =>
@@ -222,3 +224,5 @@ export const cellxgeneActions = (page, utils) => ({
     await page.keyboard.press("Enter");
   },
 });
+
+export default cellxgeneActions;

client/__tests__/e2e/config.js

@@ -1,10 +1,11 @@
 export const jestEnv = process.env.JEST_ENV;
 export const appPort = process.env.CXG_SERVER_PORT;
 export const appUrlBase =
-  process.env.CXG_URL_BASE || `http://localhost:${appPort}`;
+	process.env.CXG_URL_BASE || `http://localhost:${appPort}`;
 export const DEV = jestEnv === "dev";
 export const DEBUG = jestEnv === "debug";
 export const DATASET = "pbmc3k";
 
-if (DEBUG) jest.setTimeout(100000);
-if (DEV) jest.setTimeout(10000);
+if (DEBUG) jest.setTimeout(2 * 60 * 1000);
+if (DEV) jest.setTimeout(30 * 1000);
+if (!DEBUG && !DEV) jest.setTimeout(10 * 1000);

client/__tests__/e2e/e2e.test.js

@@ -3,8 +3,8 @@ Smoke test suite that will be run in Travis CI
 
 Tests included in this file are expected to be relatively stable and test core features
  */
-import { appUrlBase, DATASET } from "./config";
-import { setupTestBrowser } from "./testBrowser";
+import { appUrlBase, DATASET, DEBUG } from "./config";
+import setupTestBrowser from "./testBrowser";
 import { datasets } from "./data";
 
 let browser;
@@ -22,7 +22,7 @@ beforeEach(async () => {
 });
 
 afterAll(() => {
-  if (browser !== undefined) browser.close();
+  if (!DEBUG && browser !== undefined) browser.close();
 });
 
 describe("did launch", () => {

client/__tests__/e2e/e2eAnnotations.test.js

@@ -1,8 +1,8 @@
 /*
 Tests included in this file are specific to annotation features
 */
-import { appUrlBase, DATASET } from "./config";
-import { setupTestBrowser } from "./testBrowser";
+import { appUrlBase, DATASET, DEBUG } from "./config";
+import setupTestBrowser from "./testBrowser";
 import { datasets } from "./data";
 
 let browser;
@@ -15,8 +15,8 @@ beforeAll(async () => {
   [browser, page, utils, actions] = await setupTestBrowser();
 });
 
-afterAll(() => {
-  if (browser !== undefined) browser.close();
+afterAll(async () => {
+  if (!DEBUG && browser !== undefined) await browser.close();
 });
 
 describe.each([
@@ -28,6 +28,7 @@ describe.each([
 
   beforeEach(async () => {
     await page.goto(appUrlBase);
+
     // wait for the page to load
     await utils.waitByClass("autosave-complete");
     // setup the test fixtures
@@ -238,13 +239,9 @@ describe.each([
   }
 
   async function deleteCategoryIfExists(categoryName) {
-    try {
-      const category = await page.waitForSelector(
-        `[data-testid='${categoryName}:category-expand']`,
-        { timeout: 200 }
-      );
-      if (category !== null) return await actions.deleteCategory(categoryName);
-    } catch {}
-    return null;
+    const handle = await page.$(
+      `[data-testid='${categoryName}:category-expand']`
+    );
+    if (handle) await actions.deleteCategory(categoryName);
   }
 });

client/__tests__/e2e/puppeteerUtils.js

@@ -1,15 +1,17 @@
-export const puppeteerUtils = (page) => ({
-  async waitByID(testId, props = {}) {
+const puppeteerUtils = (page) => ({
+  waitByID(testId, props = {}) {
     return page.waitForSelector(`[data-testid='${testId}']`, props);
   },
 
-  async waitByClass(testClass, props = {}) {
+  waitByClass(testClass, props = {}) {
     return page.waitForSelector(`[data-testclass='${testClass}']`, props);
   },
 
-  async waitForAllByIds(testIds) {
-    await Promise.all(
-      testIds.map((testId) => page.waitForSelector(`[data-testid='${testId}']`))
+  async waitForAllByIds(testIds, props = {}) {
+    return Promise.all(
+      testIds.map((testId) =>
+        page.waitForSelector(`[data-testid='${testId}']`, props)
+      )
     );
   },
 
@@ -67,3 +69,5 @@ export const puppeteerUtils = (page) => ({
     });
   },
 });
+
+export default puppeteerUtils;

client/__tests__/e2e/testBrowser.js

@@ -1,9 +1,9 @@
 import puppeteer from "puppeteer";
 import { DEBUG, DEV } from "./config";
-import { puppeteerUtils } from "./puppeteerUtils";
-import { cellxgeneActions } from "./cellxgeneActions";
+import puppeteerUtils from "./puppeteerUtils";
+import cellxgeneActions from "./cellxgeneActions";
 
-export async function setupTestBrowser() {
+export default async function setupTestBrowser() {
   const browserViewport = { width: 1280, height: 960 };
   const browserParams = DEV
     ? {
@@ -35,21 +35,30 @@ export async function setupTestBrowser() {
   if (DEV || DEBUG) {
     page.on("console", async (msg) => {
       // If there is a console.error but an error is not thrown, this will ensure the test fails
+      console.log(`PAGE LOG: ${msg.text()}`);
       if (msg.type() === "error") {
         // TODO: chromium does not currently support the CSP directive on the
         // line below, so we swallow this error. Remove this when the test
         // suite uses a browser version that supports this directive.
-        if (msg.text() === "Unrecognized Content-Security-Policy directive 'require-trusted-types-for'.\n") return;
+        if (
+          msg.text() ===
+          "Unrecognized Content-Security-Policy directive 'require-trusted-types-for'.\n"
+        )
+          return;
         const errorMsgText = await Promise.all(
           // TODO can we do this without internal properties?
           msg.args().map((arg) => arg._remoteObject.description)
         );
         throw new Error(`Console error: ${errorMsgText}`);
       }
-      console.log(`PAGE LOG: ${msg.text()}`);
     });
   }
   page.on("pageerror", (err) => {
+    console.log(`PAGE LOG: ${msg.text()}`);
+    throw new Error(`Console error: ${err}`);
+  });
+  page.on("error", (err) => {
+    console.log(`PAGE LOG: ${msg.text()}`);
     throw new Error(`Console error: ${err}`);
   });
   const utils = puppeteerUtils(page);

client/__tests__/util/centroid.test.js

@@ -35,19 +35,15 @@ describe("centroid", () => {
 
     // Create categorical selection from world
     categoricalSelection = CH.createCategoricalSelection(
-      world,
-      CH.selectableCategoryNames(world.schema, CH.maxCategoryItems(REST.config))
+      CH.selectableCategoryNames(world.schema)
     );
   });
 
   test("field4 (categorical obsAnnotation)", () => {
     const centroidResult = calcCentroid(
-      world.obsAnnotations,
-      world.obsLayout,
+      world,
       "field4",
-      ["umap_0", "umap_1"],
-      categoricalSelection,
-      world.schema.annotations.obsByName
+      ["umap_0", "umap_1"]
     );
 
     // Check to see that a centroid has been calculated for every categorical value
@@ -69,12 +65,9 @@ describe("centroid", () => {
 
   test("field3 (boolean obsAnnotation)", () => {
     const centroidResult = calcCentroid(
-      world.obsAnnotations,
-      world.obsLayout,
+      world,
       "field3",
-      ["umap_0", "umap_1"],
-      categoricalSelection,
-      world.schema.annotations.obsByName
+      ["umap_0", "umap_1"]
     );
 
     // Check to see that a centroid has been calculated for every categorical value