Improve visualize modal test coverage (#2811)

add unit tests for VisualizeModal component

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];

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',
+};

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' });
 

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(<VisualizeModal {...mockedProps} />, {
       context: { store },
@@ -66,47 +78,234 @@ describe('VisualizeModal', () => {
     expect(wrapper.find(Modal)).to.have.length(1);
   });
 
+  describe('setStateFromProps', () => {
+    const wrapper = getVisualizeModalWrapper();
+    const sampleQuery = queries[0];
+
+    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);
+  });
+
+  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,
+    });
+  });
+
   describe('visualize', () => {
     const wrapper = getVisualizeModalWrapper();
-
+    const mockOptions = { attr: 'mockOptions' };
     wrapper.setState({
       chartType: mockChartTypeBarChart,
       columns: mockColumns,
       datasourceName: 'mockDatasourceName',
     });
 
-    const vizOptions = {
-      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;
-
+    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);
     });
   });
 });

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 },