From fdbb3d495c68adef1f25dd3362a0e0c4bfa318de Mon Sep 17 00:00:00 2001 From: briannguyen4 Date: Wed, 26 Jan 2022 12:40:18 -0500 Subject: [PATCH 1/8] converted file to functional component --- .../components/QueryAutoRefresh/index.jsx | 84 +++++++++---------- 1 file changed, 40 insertions(+), 44 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx index b54936b691efe..267b80a303512 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ + import React from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; @@ -28,31 +29,11 @@ const QUERY_UPDATE_BUFFER_MS = 5000; const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; -class QueryAutoRefresh extends React.PureComponent { - constructor(props) { - super(props); - this.state = { - offline: props.offline, - }; - } - - UNSAFE_componentWillMount() { - this.startTimer(); - } - - componentDidUpdate(prevProps) { - if (prevProps.offline !== this.state.offline) { - this.props.actions.setUserOffline(this.state.offline); - } - } +const QueryAutoRefresh = ({ offline, queries, queriesLastUpdate, actions }) => { + const [offlineState, setOfflineState] = useState(offline); - componentWillUnmount() { - this.stopTimer(); - } - - shouldCheckForQueries() { + const shouldCheckForQueries = () => { // if there are started or running queries, this method should return true - const { queries } = this.props; const now = new Date().getTime(); const isQueryRunning = q => ['running', 'started', 'pending', 'fetching'].indexOf(q.state) >= 0; @@ -60,46 +41,61 @@ class QueryAutoRefresh extends React.PureComponent { return Object.values(queries).some( q => isQueryRunning(q) && now - q.startDttm < MAX_QUERY_AGE_TO_POLL, ); - } + }; - startTimer() { - if (!this.timer) { - this.timer = setInterval(this.stopwatch.bind(this), QUERY_UPDATE_FREQ); + const startTimer = () => { + if (!timer) { + timer = setInterval(stopwatch(), QUERY_UPDATE_FREQ); } - } + }; - stopTimer() { - clearInterval(this.timer); - this.timer = null; - } + const stopTimer = () => { + clearInterval(timer); + timer = null; + }; - stopwatch() { + const stopwatch = () => { // only poll /superset/queries/ if there are started or running queries - if (this.shouldCheckForQueries()) { + if (shouldCheckForQueries()) { SupersetClient.get({ endpoint: `/superset/queries/${ - this.props.queriesLastUpdate - QUERY_UPDATE_BUFFER_MS + queriesLastUpdate - QUERY_UPDATE_BUFFER_MS }`, timeout: QUERY_TIMEOUT_LIMIT, }) .then(({ json }) => { if (Object.keys(json).length > 0) { - this.props.actions.refreshQueries(json); + actions.refreshQueries(json); } - this.setState({ offline: false }); + + setOfflineState(false); }) .catch(() => { - this.setState({ offline: true }); + setOfflineState(true); }); } else { - this.setState({ offline: false }); + setOfflineState(false); } - } + }; + + useEffect(() => { + console.log('component mounted!'); + startTimer(); + }, []); + + useEffect(() => { + return () => { + stopTimer(); + }; + }, []); + + useEffect(() => { + actions.setUserOffline(offline); + }, [offline]); + + return null; +}; - render() { - return null; - } -} QueryAutoRefresh.propTypes = { offline: PropTypes.bool.isRequired, queries: PropTypes.object.isRequired, From 8694429ea0f0e56827e7d9763ed8eff25adcc310 Mon Sep 17 00:00:00 2001 From: briannguyen4 Date: Wed, 26 Jan 2022 14:16:21 -0500 Subject: [PATCH 2/8] refactor: convert class component to functional component --- .../components/QueryAutoRefresh/index.jsx | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx index 267b80a303512..d658bf6b45671 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx @@ -16,8 +16,7 @@ * specific language governing permissions and limitations * under the License. */ - -import React from 'react'; +import React, { useState, useEffect } from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; import { connect } from 'react-redux'; @@ -29,8 +28,9 @@ const QUERY_UPDATE_BUFFER_MS = 5000; const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; -const QueryAutoRefresh = ({ offline, queries, queriesLastUpdate, actions }) => { +function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { const [offlineState, setOfflineState] = useState(offline); + let timer = null; const shouldCheckForQueries = () => { // if there are started or running queries, this method should return true @@ -43,17 +43,6 @@ const QueryAutoRefresh = ({ offline, queries, queriesLastUpdate, actions }) => { ); }; - const startTimer = () => { - if (!timer) { - timer = setInterval(stopwatch(), QUERY_UPDATE_FREQ); - } - }; - - const stopTimer = () => { - clearInterval(timer); - timer = null; - }; - const stopwatch = () => { // only poll /superset/queries/ if there are started or running queries if (shouldCheckForQueries()) { @@ -78,23 +67,30 @@ const QueryAutoRefresh = ({ offline, queries, queriesLastUpdate, actions }) => { } }; - useEffect(() => { - console.log('component mounted!'); - startTimer(); - }, []); + const startTimer = () => { + if (!timer) { + timer = setInterval(stopwatch(), QUERY_UPDATE_FREQ); + } + }; + + const stopTimer = () => { + clearInterval(timer); + timer = null; + }; useEffect(() => { + startTimer(); return () => { stopTimer(); }; }, []); useEffect(() => { - actions.setUserOffline(offline); - }, [offline]); + actions.setUserOffline(offlineState); + }, [offlineState]); return null; -}; +} QueryAutoRefresh.propTypes = { offline: PropTypes.bool.isRequired, From 2a09ab7c114a3c1d5db07bdcbebc28b31ce02fb4 Mon Sep 17 00:00:00 2001 From: briannguyen4 Date: Fri, 28 Jan 2022 16:28:45 -0500 Subject: [PATCH 3/8] refactor: convert class component to functional" --- .../src/SqlLab/components/QueryAutoRefresh/index.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx index d658bf6b45671..43f6c5d8a7d6e 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useState, useEffect } from 'react'; +import { useState, useEffect } from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; import { connect } from 'react-redux'; From dd1726a354c9b0c8426d62424a675b2f01282d19 Mon Sep 17 00:00:00 2001 From: Josue Lugaro Date: Wed, 2 Feb 2022 16:24:17 -0600 Subject: [PATCH 4/8] Working on converting the shouldCheckForQueries test to RTL --- .../QueryAutoRefresh.test.jsx | 63 ++++++++++++------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx index 06bf187e1185a..77ef0dd6d2d29 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx @@ -18,6 +18,8 @@ */ import React from 'react'; import { shallow } from 'enzyme'; +import { render, screen } from 'spec/helpers/testing-library'; +import { ThemeProvider, supersetTheme } from '@superset-ui/core'; import sinon from 'sinon'; import thunk from 'redux-thunk'; import configureStore from 'redux-mock-store'; @@ -36,33 +38,52 @@ describe('QueryAutoRefresh', () => { const state = { ...initialState, sqlLab, + setUserOffline: jest.fn(), }; const store = mockStore(state); - const getWrapper = () => - shallow() - .dive() - .dive(); - let wrapper; + const setup = (overrides = {}) => ( + + + + ); it('shouldCheckForQueries', () => { - wrapper = getWrapper(); - expect(wrapper.instance().shouldCheckForQueries()).toBe(true); + render(setup()); + + expect(state.setUserOffline).toHaveBeenCalled(); }); - it('setUserOffline', () => { - wrapper = getWrapper(); - const spy = sinon.spy(wrapper.instance().props.actions, 'setUserOffline'); + // const getWrapper = () => + // shallow() + // .dive() + // .dive(); + // let wrapper; - // state not changed - wrapper.setState({ - offline: false, - }); - expect(spy.called).toBe(false); + // Just need to render for this + // spy on setUserOffline for the actual test + // eslint-disable-next-line jest/no-commented-out-tests + // it('shouldCheckForQueries', () => { + // wrapper = getWrapper(); + // expect(wrapper.instance().shouldCheckForQueries()).toBe(true); + // }); - // state is changed - wrapper.setState({ - offline: true, - }); - expect(spy.callCount).toBe(1); - }); + // Change the props passed into the render, setting offline to on or off + // eslint-disable-next-line jest/no-commented-out-tests + // it('setUserOffline', () => { + // wrapper = getWrapper(); + // // calls action.setUserOffline, and keeps track of whether it has been called or not + // const spy = sinon.spy(wrapper.instance().props.actions, 'setUserOffline'); + + // // state not changed + // wrapper.setState({ + // offline: false, + // }); + // expect(spy.called).toBe(false); + + // // state is changed + // wrapper.setState({ + // offline: true, + // }); + // expect(spy.callCount).toBe(1); + // }); }); From b79f455a814b96d1247e80d39ba981cca4a84990 Mon Sep 17 00:00:00 2001 From: Josue Lugaro Date: Wed, 9 Feb 2022 14:40:34 -0600 Subject: [PATCH 5/8] Working on converting first test to RTL --- .../QueryAutoRefresh/QueryAutoRefresh.test.jsx | 13 +++++++++---- .../SqlLab/components/QueryAutoRefresh/index.jsx | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx index 77ef0dd6d2d29..88b457f7c1013 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx @@ -25,6 +25,8 @@ import thunk from 'redux-thunk'; import configureStore from 'redux-mock-store'; import QueryAutoRefresh from 'src/SqlLab/components/QueryAutoRefresh'; import { initialState, runningQuery } from 'src/SqlLab/fixtures'; +import fetchMock from 'fetch-mock'; +import * as Actions from 'src/SqlLab/actions/sqlLab'; describe('QueryAutoRefresh', () => { const middlewares = [thunk]; @@ -38,19 +40,22 @@ describe('QueryAutoRefresh', () => { const state = { ...initialState, sqlLab, - setUserOffline: jest.fn(), }; const store = mockStore(state); const setup = (overrides = {}) => ( - + ); + const mockFetch = fetchMock.get('glob:*/superset/queries/*', {}); + it('shouldCheckForQueries', () => { - render(setup()); + render(setup(), { + useRedux: true, + }); - expect(state.setUserOffline).toHaveBeenCalled(); + expect(mockFetch.calls).toHaveBeenCalledTimes(1); }); // const getWrapper = () => diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx index 43f6c5d8a7d6e..c9277d9056d29 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx @@ -29,6 +29,7 @@ const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { + console.log(actions, 'ACTIONS'); const [offlineState, setOfflineState] = useState(offline); let timer = null; @@ -45,6 +46,11 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { const stopwatch = () => { // only poll /superset/queries/ if there are started or running queries + console.log( + shouldCheckForQueries(), + '<====SHOULD CHECK=====================', + ); + console.log(offlineState, 'OFFLINE BEFORE'); if (shouldCheckForQueries()) { SupersetClient.get({ endpoint: `/superset/queries/${ @@ -53,6 +59,7 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { timeout: QUERY_TIMEOUT_LIMIT, }) .then(({ json }) => { + console.log('INSIDE THEN STATEMENT'); if (Object.keys(json).length > 0) { actions.refreshQueries(json); } @@ -60,11 +67,14 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { setOfflineState(false); }) .catch(() => { + console.log('INSIDE CATCH STATEMENT'); setOfflineState(true); }); } else { setOfflineState(false); } + + console.log(offlineState, 'OFFLINE AFTER'); }; const startTimer = () => { @@ -80,12 +90,14 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { useEffect(() => { startTimer(); + console.log(offlineState, 'OFFLINE STATE IN USE EFFECT'); return () => { stopTimer(); }; }, []); useEffect(() => { + console.log('OFFLINE STATE CHANGED =============================='); actions.setUserOffline(offlineState); }, [offlineState]); From 6eeed7ab89a0886c3b3d5073b2295932bcdecff6 Mon Sep 17 00:00:00 2001 From: Josue Lugaro Date: Wed, 16 Feb 2022 13:33:25 -0600 Subject: [PATCH 6/8] Working on first test for queryAutoRefresh --- .../components/QueryAutoRefresh/QueryAutoRefresh.test.jsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx index 88b457f7c1013..da6037c165a66 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx @@ -55,6 +55,8 @@ describe('QueryAutoRefresh', () => { useRedux: true, }); + screen.logTestingPlaygroundURL(); + expect(mockFetch.calls).toHaveBeenCalledTimes(1); }); From fcc0c9cb14133b6185da87a3629b7a2c3a38eef4 Mon Sep 17 00:00:00 2001 From: Josue Lugaro Date: Thu, 24 Feb 2022 13:53:52 -0600 Subject: [PATCH 7/8] Finished Tests, pushing for review --- .../QueryAutoRefresh.test.jsx | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx index da6037c165a66..17b288fdf0f1a 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx @@ -17,16 +17,14 @@ * under the License. */ import React from 'react'; -import { shallow } from 'enzyme'; -import { render, screen } from 'spec/helpers/testing-library'; +import { render } from 'spec/helpers/testing-library'; import { ThemeProvider, supersetTheme } from '@superset-ui/core'; -import sinon from 'sinon'; import thunk from 'redux-thunk'; import configureStore from 'redux-mock-store'; import QueryAutoRefresh from 'src/SqlLab/components/QueryAutoRefresh'; import { initialState, runningQuery } from 'src/SqlLab/fixtures'; import fetchMock from 'fetch-mock'; -import * as Actions from 'src/SqlLab/actions/sqlLab'; +import * as actions from 'src/SqlLab/actions/sqlLab'; describe('QueryAutoRefresh', () => { const middlewares = [thunk]; @@ -55,9 +53,17 @@ describe('QueryAutoRefresh', () => { useRedux: true, }); - screen.logTestingPlaygroundURL(); + expect(mockFetch.called()).toBe(true); + }); + + it('setUserOffline', () => { + const spy = jest.spyOn(actions, 'setUserOffline'); + + render(setup(), { + useRedux: true, + }); - expect(mockFetch.calls).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalled(); }); // const getWrapper = () => @@ -74,6 +80,7 @@ describe('QueryAutoRefresh', () => { // expect(wrapper.instance().shouldCheckForQueries()).toBe(true); // }); + // is it possible to call actions.setUserOffline to change the mock state of the component causing the value of offline to change? // Change the props passed into the render, setting offline to on or off // eslint-disable-next-line jest/no-commented-out-tests // it('setUserOffline', () => { From 172f1784c1b0376cd1f875ec56a264bd28205498 Mon Sep 17 00:00:00 2001 From: Josue Lugaro Date: Sat, 26 Feb 2022 19:58:23 -0600 Subject: [PATCH 8/8] Cleaned up comments and console logs --- .../QueryAutoRefresh.test.jsx | 35 ------------------- .../components/QueryAutoRefresh/index.jsx | 12 ------- 2 files changed, 47 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx index 17b288fdf0f1a..93cea6d08d65f 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.jsx @@ -65,39 +65,4 @@ describe('QueryAutoRefresh', () => { expect(spy).toHaveBeenCalled(); }); - - // const getWrapper = () => - // shallow() - // .dive() - // .dive(); - // let wrapper; - - // Just need to render for this - // spy on setUserOffline for the actual test - // eslint-disable-next-line jest/no-commented-out-tests - // it('shouldCheckForQueries', () => { - // wrapper = getWrapper(); - // expect(wrapper.instance().shouldCheckForQueries()).toBe(true); - // }); - - // is it possible to call actions.setUserOffline to change the mock state of the component causing the value of offline to change? - // Change the props passed into the render, setting offline to on or off - // eslint-disable-next-line jest/no-commented-out-tests - // it('setUserOffline', () => { - // wrapper = getWrapper(); - // // calls action.setUserOffline, and keeps track of whether it has been called or not - // const spy = sinon.spy(wrapper.instance().props.actions, 'setUserOffline'); - - // // state not changed - // wrapper.setState({ - // offline: false, - // }); - // expect(spy.called).toBe(false); - - // // state is changed - // wrapper.setState({ - // offline: true, - // }); - // expect(spy.callCount).toBe(1); - // }); }); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx index c9277d9056d29..43f6c5d8a7d6e 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.jsx @@ -29,7 +29,6 @@ const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { - console.log(actions, 'ACTIONS'); const [offlineState, setOfflineState] = useState(offline); let timer = null; @@ -46,11 +45,6 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { const stopwatch = () => { // only poll /superset/queries/ if there are started or running queries - console.log( - shouldCheckForQueries(), - '<====SHOULD CHECK=====================', - ); - console.log(offlineState, 'OFFLINE BEFORE'); if (shouldCheckForQueries()) { SupersetClient.get({ endpoint: `/superset/queries/${ @@ -59,7 +53,6 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { timeout: QUERY_TIMEOUT_LIMIT, }) .then(({ json }) => { - console.log('INSIDE THEN STATEMENT'); if (Object.keys(json).length > 0) { actions.refreshQueries(json); } @@ -67,14 +60,11 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { setOfflineState(false); }) .catch(() => { - console.log('INSIDE CATCH STATEMENT'); setOfflineState(true); }); } else { setOfflineState(false); } - - console.log(offlineState, 'OFFLINE AFTER'); }; const startTimer = () => { @@ -90,14 +80,12 @@ function QueryAutoRefresh({ offline, queries, queriesLastUpdate, actions }) { useEffect(() => { startTimer(); - console.log(offlineState, 'OFFLINE STATE IN USE EFFECT'); return () => { stopTimer(); }; }, []); useEffect(() => { - console.log('OFFLINE STATE CHANGED =============================='); actions.setUserOffline(offlineState); }, [offlineState]);