From 1e7773eb169d29607991d0c4619ee930104e18de Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Tue, 30 May 2017 18:16:22 -0700 Subject: [PATCH] Improve visualize modal test coverage (#2811) add unit tests for VisualizeModal component --- .../SqlLab/components/VisualizeModal.jsx | 18 +- .../assets/javascripts/SqlLab/constants.js | 7 + .../javascripts/explorev2/actions_spec.js | 2 +- .../sqllab/VisualizeModal_spec.jsx | 233 ++++++++++++++++-- .../spec/javascripts/sqllab/fixtures.js | 12 +- 5 files changed, 244 insertions(+), 28 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index da0a19cb7c5b7..8fd2ba0dfefb0 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -10,6 +10,7 @@ import { Table } from 'reactable'; import shortid from 'shortid'; import { getExploreUrl } from '../../explore/exploreUtils'; import * as actions from '../actions'; +import { VISUALIZE_VALIDATION_ERRORS } from '../constants'; const CHART_TYPES = [ { value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false }, @@ -49,7 +50,7 @@ class VisualizeModal extends React.PureComponent { this.setStateFromProps(nextProps); } setStateFromProps(props) { - if ( + if (!props || !props.query || !props.query.results || !props.query.results.columns) { @@ -87,7 +88,7 @@ class VisualizeModal extends React.PureComponent { } }); if (this.state.chartType === null) { - hints.push('Pick a chart type!'); + hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_CHART_TYPE); } else if (this.state.chartType.requiresTime) { let hasTime = false; for (const colName in cols) { @@ -97,9 +98,7 @@ class VisualizeModal extends React.PureComponent { } } if (!hasTime) { - hints.push( - 'To use this chart type you need at least one column ' + - 'flagged as a date'); + hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_TIME); } } this.setState({ hints }); @@ -118,16 +117,17 @@ class VisualizeModal extends React.PureComponent { } return columns; } - visualize() { - const vizOptions = { + buildVizOptions() { + return { chartType: this.state.chartType.value, datasourceName: this.state.datasourceName, columns: this.state.columns, sql: this.props.query.sql, dbId: this.props.query.dbId, }; - - this.props.actions.createDatasource(vizOptions, this) + } + visualize() { + this.props.actions.createDatasource(this.buildVizOptions(), this) .done(() => { const columns = Object.keys(this.state.columns).map(k => this.state.columns[k]); const mainMetric = columns.filter(d => d.agg)[0]; diff --git a/superset/assets/javascripts/SqlLab/constants.js b/superset/assets/javascripts/SqlLab/constants.js index 37e32f7087016..2a9327509e3f8 100644 --- a/superset/assets/javascripts/SqlLab/constants.js +++ b/superset/assets/javascripts/SqlLab/constants.js @@ -22,3 +22,10 @@ export const TIME_OPTIONS = [ '90 days ago', '1 year ago', ]; + +export const VISUALIZE_VALIDATION_ERRORS = { + REQUIRE_CHART_TYPE: 'Pick a chart type!', + REQUIRE_TIME: 'To use this chart type you need at least one column flagged as a date', + REQUIRE_DIMENSION: 'To use this chart type you need at least one dimension', + REQUIRE_AGGREGATION_FUNCTION: 'To use this chart type you need at least one aggregation function', +}; diff --git a/superset/assets/spec/javascripts/explorev2/actions_spec.js b/superset/assets/spec/javascripts/explorev2/actions_spec.js index 346e5edda9f92..7f5e3d3d3fe07 100644 --- a/superset/assets/spec/javascripts/explorev2/actions_spec.js +++ b/superset/assets/spec/javascripts/explorev2/actions_spec.js @@ -23,7 +23,7 @@ describe('reducers', () => { describe('runQuery', () => { it('should handle query timeout', () => { const dispatch = sinon.spy(); - const urlStub = sinon.stub(exploreUtils, 'getExploreUrl', () => ('mockURL')); + const urlStub = sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); const ajaxStub = sinon.stub($, 'ajax'); ajaxStub.yieldsTo('error', { statusText: 'timeout' }); diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 169a020dbc492..aa0be16ec80b2 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -9,8 +9,11 @@ import { expect } from 'chai'; import sinon from 'sinon'; import $ from 'jquery'; +import shortid from 'shortid'; import { queries } from './fixtures'; import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; +import * as actions from '../../../javascripts/SqlLab/actions'; +import { VISUALIZE_VALIDATION_ERRORS } from '../../../javascripts/SqlLab/constants'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; import * as exploreUtils from '../../../javascripts/explore/exploreUtils'; @@ -47,7 +50,16 @@ describe('VisualizeModal', () => { requiresTime: false, value: 'dist_bar', }; - + const mockChartTypeTB = { + label: 'Time Series - Bar Chart', + requiresTime: true, + value: 'bar', + }; + const mockEvent = { + target: { + value: 'mock event value', + }, + }; const getVisualizeModalWrapper = () => ( shallow(, { context: { store }, @@ -66,47 +78,234 @@ describe('VisualizeModal', () => { expect(wrapper.find(Modal)).to.have.length(1); }); - describe('visualize', () => { + describe('setStateFromProps', () => { const wrapper = getVisualizeModalWrapper(); + const sampleQuery = queries[0]; - wrapper.setState({ - chartType: mockChartTypeBarChart, - columns: mockColumns, - datasourceName: 'mockDatasourceName', + it('should require valid props parameters', () => { + const spy = sinon.spy(wrapper.instance(), 'setState'); + wrapper.instance().setStateFromProps(); + expect(spy.callCount).to.equal(0); + spy.restore(); + }); + it('should set columns state', () => { + wrapper.instance().setStateFromProps({ query: sampleQuery }); + expect(wrapper.state().columns).to.deep.equal(mockColumns); + }); + }); + + describe('datasourceName', () => { + const wrapper = getVisualizeModalWrapper(); + let stub; + beforeEach(() => { + stub = sinon.stub(shortid, 'generate').returns('abcd'); + }); + afterEach(() => { + stub.restore(); + }); + + it('should generate data source name from query', () => { + const sampleQuery = queries[0]; + const name = wrapper.instance().datasourceName(); + expect(name).to.equal(`${sampleQuery.user}-${sampleQuery.db}-${sampleQuery.tab}-abcd`); + }); + it('should generate data source name with empty query', () => { + wrapper.setProps({ query: {} }); + const name = wrapper.instance().datasourceName(); + expect(name).to.equal('undefined-abcd'); + }); + }); + + describe('mergedColumns', () => { + const wrapper = getVisualizeModalWrapper(); + const oldColumns = { + ds: 1, + gender: 2, + }; + + it('should merge by column name', () => { + wrapper.setState({ columns: {} }); + const mc = wrapper.instance().mergedColumns(); + expect(mc).to.deep.equal(mockColumns); + }); + it('should not override current state', () => { + wrapper.setState({ columns: oldColumns }); + + const mc = wrapper.instance().mergedColumns(); + expect(mc.ds).to.equal(oldColumns.ds); + expect(mc.gender).to.equal(oldColumns.gender); + }); + }); + + describe('validate', () => { + const wrapper = getVisualizeModalWrapper(); + let columnsStub; + beforeEach(() => { + columnsStub = sinon.stub(wrapper.instance(), 'mergedColumns'); + }); + afterEach(() => { + columnsStub.restore(); + }); + + it('should validate column name', () => { + columnsStub.returns(mockColumns); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(0); + wrapper.instance().mergedColumns.restore(); + }); + it('should hint invalid column name', () => { + columnsStub.returns({ + '&': 1, + }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + wrapper.instance().mergedColumns.restore(); }); + it('should hint empty chartType', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: null }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + expect(wrapper.state().hints[0]) + .to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_CHART_TYPE); + }); + it('should check time series', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(0); + + // no is_date columns + columnsStub.returns({ + ds: { + is_date: false, + is_dim: false, + name: 'ds', + type: 'STRING', + }, + gender: { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }, + }); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + expect(wrapper.state().hints[0]).to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_TIME); + }); + it('should validate after change checkbox', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeCheckbox('is_dim', 'gender', mockEvent); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + it('should validate after change Agg function', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeAggFunction('num', { label: 'MIN(x)', value: 'min' }); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + }); + + it('should validate after change chart type', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeChartType(mockChartTypeBarChart); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().chartType).to.equal(mockChartTypeBarChart); + }); + + it('should validate after change datasource name', () => { + const wrapper = getVisualizeModalWrapper(); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeDatasourceName(mockEvent); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().datasourceName).to.equal(mockEvent.target.value); + }); - const vizOptions = { + it('should build viz options', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'buildVizOptions'); + wrapper.instance().buildVizOptions(); + expect(spy.returnValues[0]).to.deep.equal({ chartType: wrapper.state().chartType.value, datasourceName: wrapper.state().datasourceName, columns: wrapper.state().columns, sql: wrapper.instance().props.query.sql, dbId: wrapper.instance().props.query.dbId, - }; + }); + }); - let spy; - let server; + describe('visualize', () => { + const wrapper = getVisualizeModalWrapper(); + const mockOptions = { attr: 'mockOptions' }; + wrapper.setState({ + chartType: mockChartTypeBarChart, + columns: mockColumns, + datasourceName: 'mockDatasourceName', + }); + let ajaxSpy; + let datasourceSpy; beforeEach(() => { - spy = sinon.spy($, 'ajax'); - server = sinon.fakeServer.create(); + ajaxSpy = sinon.spy($, 'ajax'); sinon.stub(JSON, 'parse').callsFake(() => ({ table_id: 107 })); sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); + sinon.stub(wrapper.instance(), 'buildVizOptions').callsFake(() => (mockOptions)); + sinon.spy(window, 'open'); + datasourceSpy = sinon.stub(actions, 'createDatasource'); }); afterEach(() => { - spy.restore(); - server.restore(); + ajaxSpy.restore(); JSON.parse.restore(); exploreUtils.getExploreUrl.restore(); + wrapper.instance().buildVizOptions.restore(); + window.open.restore(); + datasourceSpy.restore(); }); it('should build request', () => { wrapper.instance().visualize(); - expect(spy.callCount).to.equal(1); + expect(ajaxSpy.callCount).to.equal(1); - const spyCall = spy.getCall(0); + const spyCall = ajaxSpy.getCall(0); expect(spyCall.args[0].type).to.equal('POST'); expect(spyCall.args[0].url).to.equal('/superset/sqllab_viz/'); - expect(spyCall.args[0].data.data).to.equal(JSON.stringify(vizOptions)); + expect(spyCall.args[0].data.data).to.equal(JSON.stringify(mockOptions)); + }); + it('should open new window', () => { + datasourceSpy.callsFake(() => { + const d = $.Deferred(); + d.resolve('done'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(1); + }); + it('should notify error', () => { + datasourceSpy.callsFake(() => { + const d = $.Deferred(); + d.reject('error message'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + sinon.spy(notify, 'error'); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(0); + expect(notify.error.callCount).to.equal(1); }); }); }); diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js index 3e3d9c38fc07e..db638ba3c18fd 100644 --- a/superset/assets/spec/javascripts/sqllab/fixtures.js +++ b/superset/assets/spec/javascripts/sqllab/fixtures.js @@ -205,7 +205,17 @@ export const queries = [ serverId: 141, resultsKey: null, results: { - columns: ['col1', 'col2'], + columns: [{ + is_date: true, + is_dim: false, + name: 'ds', + type: 'STRING', + }, { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }], data: [ { col1: 0, col2: 1 }, { col1: 2, col2: 3 },