fix: timer component, fixes #10849, closes #11002 (#11004)

superset-frontend/cypress-base/cypress/integration/sqllab/query.test.ts

@@ -16,43 +16,80 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import shortid from 'shortid';
+import * as shortid from 'shortid';
 import { selectResultsTab, assertSQLLabResultsAreEqual } from './sqllab.helper';
 
+function parseClockStr(node: JQuery) {
+  return Number.parseFloat(node.text().replace(/:/g, ''));
+}
+
 describe('SqlLab query panel', () => {
   beforeEach(() => {
     cy.login();
     cy.server();
     cy.visit('/superset/sqllab');
-
-    cy.route('POST', '/superset/sql_json/').as('sqlLabQuery');
   });
 
   it.skip('supports entering and running a query', () => {
     // row limit has to be < ~10 for us to be able to determine how many rows
     // are fetched below (because React _Virtualized_ does not render all rows)
-    const rowLimit = 3;
+    let clockTime = 0;
+
+    const sampleResponse = {
+      status: 'success',
+      data: [{ '?column?': 1 }],
+      columns: [{ name: '?column?', type: 'INT', is_date: false }],
+      selected_columns: [{ name: '?column?', type: 'INT', is_date: false }],
+      expanded_columns: [],
+    };
+
+    cy.route({
+      method: 'POST',
+      url: '/superset/sql_json/',
+      delay: 1000,
+      response: () => sampleResponse,
+    }).as('mockSQLResponse');
+
+    cy.get('.TableSelector .Select:eq(0)').click();
+    cy.get('.TableSelector .Select:eq(0) input[type=text]')
+      .focus()
+      .type('{enter}');
 
     cy.get('#brace-editor textarea')
-      .clear({ force: true })
-      .type(
-        `{selectall}{backspace}SELECT ds, gender, name, num FROM main.birth_names LIMIT ${rowLimit}`,
-        { force: true },
-      );
-    cy.get('#js-sql-toolbar button').eq(0).click();
+      .focus()
+      .clear()
+      .type(`{selectall}{backspace}SELECT 1`);
 
-    cy.wait('@sqlLabQuery');
+    cy.get('#js-sql-toolbar button:eq(0)').eq(0).click();
 
-    selectResultsTab()
-      .eq(0) // ensures results tab in case preview tab exists
-      .then(tableNodes => {
-        const [header, bodyWrapper] = tableNodes[0].childNodes;
-        const body = bodyWrapper.childNodes[0];
-        const expectedColCount = header.childNodes.length;
-        const expectedRowCount = body.childNodes.length;
-        expect(expectedColCount).to.equal(4);
-        expect(expectedRowCount).to.equal(rowLimit);
-      });
+    // wait for 300 milliseconds
+    cy.wait(300);
+
+    // started timer
+    cy.get('.sql-toolbar .label-success').then(node => {
+      clockTime = parseClockStr(node);
+      // should be longer than 0.2s
+      expect(clockTime).greaterThan(0.2);
+    });
+
+    cy.wait('@mockSQLResponse');
+
+    // timer is increasing
+    cy.get('.sql-toolbar .label-success').then(node => {
+      const newClockTime = parseClockStr(node);
+      expect(newClockTime).greaterThan(0.9);
+      clockTime = newClockTime;
+    });
+
+    // rerun the query
+    cy.get('#js-sql-toolbar button:eq(0)').eq(0).click();
+
+    // should restart the timer
+    cy.get('.sql-toolbar .label-success').contains('00:00:00');
+    cy.wait('@mockSQLResponse');
+    cy.get('.sql-toolbar .label-success').then(node => {
+      expect(parseClockStr(node)).greaterThan(0.9);
+    });
   });
 
   it.skip('successfully saves a query', () => {
@@ -64,7 +101,7 @@ describe('SqlLab query panel', () => {
     const savedQueryTitle = `CYPRESS TEST QUERY ${shortid.generate()}`;
 
     // we will assert that the results of the query we save, and the saved query are the same
-    let initialResultsTable = null;
+    let initialResultsTable: HTMLElement | null = null;
     let savedQueryResultsTable = null;
 
     cy.get('#brace-editor textarea')

superset-frontend/cypress-base/cypress/integration/sqllab/tabs.test.js

@@ -39,6 +39,7 @@ describe('SqlLab query tabs', () => {
         .contains(`Untitled Query ${initialTabCount + 2}`);
     });
   });
+
   it('allows you to close a tab', () => {
     cy.get('[data-test="sql-editor-tabs"]')
       .children()

superset-frontend/cypress-base/cypress/support/index.d.ts

@@ -30,7 +30,7 @@ declare namespace Cypress {
      */
     login(): void;
 
-    visitChartByParams(params: string | object): cy;
+    visitChartByParams(params: string | Record<string, unknown>): cy;
     visitChartByName(name: string): cy;
     visitChartById(id: number): cy;
 

superset-frontend/cypress-base/tsconfig.json

@@ -5,7 +5,7 @@
     "lib": ["ES5", "ES2015", "DOM"],
     "types": ["cypress"],
     "allowJs": true,
-    "noEmit": true
+    "noEmit": true,
   },
   "files": ["cypress/support/index.d.ts"],
   "include": ["node_modules/cypress", "cypress/**/*.ts"]

superset-frontend/spec/javascripts/sqllab/Timer_spec.jsx

@@ -34,17 +34,15 @@ describe('Timer', () => {
     wrapper = mount(<Timer {...mockedProps} />);
   });
 
-  it('is a valid element', () => {
+  it('renders correctly', () => {
     expect(React.isValidElement(<Timer {...mockedProps} />)).toBe(true);
+    expect(wrapper.find('span').hasClass('label-warning')).toBe(true);
   });
 
-  it('useEffect starts timer after 30ms and sets state of clockStr', async () => {
+  it('should start timer and sets clockStr', async () => {
+    expect.assertions(2);
     expect(wrapper.find('span').text()).toBe('');
     await new Promise(r => setTimeout(r, 35));
     expect(wrapper.find('span').text()).not.toBe('');
   });
-
-  it('renders a span with the correct class', () => {
-    expect(wrapper.find('span').hasClass('label-warning')).toBe(true);
-  });
 });

superset-frontend/src/components/Label/index.tsx

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React from 'react';
+import React, { CSSProperties } from 'react';
 import { Label as BootstrapLabel } from 'react-bootstrap';
 import { styled } from '@superset-ui/core';
 import cx from 'classnames';
@@ -31,7 +31,7 @@ export interface LabelProps {
   placement?: string;
   onClick?: OnClickHandler;
   bsStyle?: string;
-  style?: BootstrapLabel.LabelProps['style'];
+  style?: CSSProperties;
   children?: React.ReactNode;
 }
 

superset-frontend/src/components/Timer.tsx

@@ -16,10 +16,11 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useEffect, useState } from 'react';
+import React, { useEffect, useRef, useState } from 'react';
+import { styled } from '@superset-ui/core';
 import Label from 'src/components/Label';
 
-import { now, fDuration } from '../modules/dates';
+import { now, fDuration } from 'src/modules/dates';
 
 interface TimerProps {
   endTime?: number;
@@ -28,6 +29,11 @@ interface TimerProps {
   status?: string;
 }
 
+const TimerLabel = styled(Label)`
+  width: 80px;
+  text-align: right;
+`;
+
 export default function Timer({
   endTime,
   isRunning,
@@ -35,46 +41,31 @@ export default function Timer({
   status = 'success',
 }: TimerProps) {
   const [clockStr, setClockStr] = useState('');
-  const [timer, setTimer] = useState<NodeJS.Timeout>();
-
-  const stopTimer = () => {
-    if (timer) {
-      clearInterval(timer);
-      setTimer(undefined);
-    }
-  };
-
-  const stopwatch = () => {
-    if (startTime) {
-      const endDttm = endTime || now();
-      if (startTime < endDttm) {
-        setClockStr(fDuration(startTime, endDttm));
-      }
-      if (!isRunning) {
-        stopTimer();
-      }
-    }
-  };
-
-  const startTimer = () => {
-    setTimer(setInterval(stopwatch, 30));
-  };
+  const timer = useRef<NodeJS.Timeout>();
 
   useEffect(() => {
-    if (isRunning) {
-      startTimer();
-    }
-  }, [isRunning]);
-
-  useEffect(() => {
-    return () => {
-      stopTimer();
+    const stopTimer = () => {
+      if (timer.current) {
+        clearInterval(timer.current);
+        timer.current = undefined;
+      }
     };
-  });
 
-  return (
-    <Label id="timer" bsStyle={status}>
-      {clockStr}
-    </Label>
-  );
+    if (isRunning) {
+      timer.current = setInterval(() => {
+        if (startTime) {
+          const endDttm = endTime || now();
+          if (startTime < endDttm) {
+            setClockStr(fDuration(startTime, endDttm));
+          }
+          if (!isRunning) {
+            stopTimer();
+          }
+        }
+      }, 30);
+    }
+    return stopTimer;
+  }, [endTime, isRunning, startTime]);
+
+  return <TimerLabel bsStyle={status}>{clockStr}</TimerLabel>;
 }

superset-frontend/stylesheets/superset.less

@@ -93,11 +93,6 @@ input[type='checkbox'] {
   margin-right: 5px;
 }
 
-#timer {
-  width: 80px;
-  text-align: right;
-}
-
 .notbtn {
   cursor: default;
   box-shadow: none;