Do not crash when unsetting scrollToRow (schrodinger#62)

* Do not crash when unsetting scrollToRow * Adding semicolons * Adding more tests and fixing for scrollTop * CR feedback
lior035 · Oct 11, 2016 · 79db986 · 79db986
1 parent 53df937
commit 79db986
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 22 deletions.
diff --git a/build_helpers/test-globals.js b/build_helpers/test-globals.js
@@ -0,0 +1,29 @@
+'use strict'
+
+var jsdom = require('jsdom');
+
+// setup the simplest document possible
+var doc = jsdom.jsdom('<!doctype html><html><body></body></html>');
+
+// get the window object out of the document
+var win = doc.defaultView;
+
+// set globals for mocha that make access to document and window feel
+// natural in the test environment
+global.document = doc;
+global.window = win;
+
+// take all properties of the window object and also attach it to the
+// mocha global object
+propagateToGlobal(win);
+
+// from mocha-jsdom https://github.com/rstacruz/mocha-jsdom/blob/master/index.js#L80
+function propagateToGlobal (window) {
+  for (let key in window) {
+    if (!window.hasOwnProperty(key) || key in global) {
+      continue;
+    }
+
+    global[key] = window[key];
+  }
+}
diff --git a/package.json b/package.json
@@ -26,6 +26,7 @@
     "file-loader": "^0.8.1",
     "glob": "^4.3.5",
     "html-loader": "^0.2.3",
+    "jsdom": "^9.6.0",
     "less": "^2.2.0",
     "less-loader": "^2.0.0",
     "marked": "^0.3.2",
@@ -39,6 +40,7 @@
     "react-addons-test-utils": "^15.2.1",
     "react-dimensions": "^1.3.0",
     "react-docgen": "^1.2.0",
+    "react-dom": "^15.3.2",
     "react-tools": "^0.12.2",
     "react-tooltip": "^3.2.1",
     "sinon": "^2.0.0-pre.2",
@@ -55,8 +57,8 @@
     "build-docs": "./build_helpers/buildAPIDocs.sh",
     "publish-site": "./build_helpers/publishStaticSite.sh",
     "publish-package": "./build_helpers/publishPackage.sh",
-    "test": "mocha-webpack --webpack-config webpack.config-test.js \"src/**/*-test.js\"",
-    "test:watch": "mocha-webpack --webpack-config webpack.config-test.js --watch \"src/**/*-test.js\"",
+    "test": "mocha-webpack --webpack-config webpack.config-test.js \"src/**/*-test.js\" --require build_helpers/test-globals.js",
+    "test:watch": "mocha-webpack --webpack-config webpack.config-test.js --watch \"src/**/*-test.js\" --require build_helpers/test-globals.js",
     "test:server": "webpack-dev-server --config webpack.config-test.js --hot --inline"
   },
   "repository": {

diff --git a/src/FixedDataTable.js b/src/FixedDataTable.js
@@ -912,15 +912,15 @@ var FixedDataTable = React.createClass({
     }
 
     var lastScrollToRow  = oldState ? oldState.scrollToRow : undefined;
-    if (props.scrollToRow !== lastScrollToRow) {
+    if (props.scrollToRow != null && props.scrollToRow !== lastScrollToRow) {
       scrollState = this._scrollHelper.scrollRowIntoView(props.scrollToRow);
       firstRowIndex = scrollState.index;
       firstRowOffset = scrollState.offset;
       scrollY = scrollState.position;
     }
 
     var lastScrollTop = oldState ? oldState.scrollTop : undefined;
-    if (props.scrollTop !== lastScrollTop) {
+    if (props.scrollTop != null && props.scrollTop !== lastScrollTop) {
       scrollState = this._scrollHelper.scrollTo(props.scrollTop);
       firstRowIndex = scrollState.index;
       firstRowOffset = scrollState.offset;

diff --git a/src/FixedDataTableRoot-test.js b/src/FixedDataTableRoot-test.js
@@ -5,7 +5,7 @@
 import { assert } from 'chai';
 import React from 'react';
 import FixedDataTable from './FixedDataTableRoot';
-import { createRenderer, isElement } from 'react-addons-test-utils';
+import { createRenderer, isElement, renderIntoDocument, findRenderedComponentWithType } from 'react-addons-test-utils';
 
 const { Table, Column } = FixedDataTable;
 
@@ -33,16 +33,32 @@ describe('FixedDataTableRoot', function() {
     });
   });
 
-  describe('initial render', function() {
-    const renderTable = (optionalProps = {}) => {
-      let table = (
+  class TestTable extends React.Component {
+    constructor(props) {
+      super(props);
+
+      this.state = {...props}
+    }
+
+    /**
+     * Returns state from FDT.Table object for unit testing
+     *
+     * @return {!Object}
+     */
+    getTableState() {
+      return this.refs['table'].state;
+    }
+
+    render() {
+      return (
         <Table
+          ref="table"
           width={600}
           height={400}
           rowsCount={50}
           rowHeight={100}
           headerHeight={50}
-          {...optionalProps}
+          {...this.state}
         >
           <Column width={300} />
           <Column width={300} />
@@ -51,33 +67,66 @@ describe('FixedDataTableRoot', function() {
           <Column width={300} />
         </Table>
       );
-      let renderer = createRenderer();
-      renderer.render(table);
-      return renderer.getMountedInstance();
-    };
+    }
+  }
 
+  const renderTable = (optionalProps = {}) => {
+    let renderedTree = renderIntoDocument(<TestTable {...optionalProps} />);
+    return findRenderedComponentWithType(renderedTree, TestTable);
+  };
+
+  describe('initial render', function() {
     it('should set scrollLeft correctly', function() {
       let table = renderTable({scrollLeft: 300});
-      assert.equal(table.state.scrollX, 300, 'should set scrollX to 300');
+      assert.equal(table.getTableState().scrollX, 300, 'should set scrollX to 300');
     });
 
     it('should set scrollTop correctly', function() {
       let table = renderTable({scrollTop: 600});
-      assert.equal(table.state.scrollY, 600, 'should set scrollY to 600');
+      assert.equal(table.getTableState().scrollY, 600, 'should set scrollY to 600');
     });
 
     it('should set scrollToColumn correctly', function() {
       let table = renderTable({scrollToColumn: 3});
-      assert.equal(table.state.scrollX, 300 * 2, 'should be third visible column');
+      assert.equal(table.getTableState().scrollX, 300 * 2, 'should be third visible column');
     });
 
     it('should set scrollToRow correctly', function() {
       let table = renderTable({scrollToRow: 30, height: 300});
       //scrollToRow is considered valid if row is visible. Test to make sure that row is somewhere in between
-      assert.isBelow(table.state.scrollY, 30 * 100, 'should be below first row');
-      assert.isAbove(table.state.scrollY, 30 * 100 - 300, 'should be above last row');
+      assert.isBelow(table.getTableState().scrollY, 30 * 100, 'should be below first row');
+      assert.isAbove(table.getTableState().scrollY, 30 * 100 - 300, 'should be above last row');
     });
+  });
 
+  describe('update render', function() {
+
+    it('should not blow up when unsetting the scrollLeft property', function() {
+      let table = renderTable({scrollLeft: 300});
+      assert.doesNotThrow(function() {
+        table.setState({scrollLeft: undefined});
+      });
+    });
+
+    it('should not blow up when unsetting the scrollTop property', function() {
+      let table = renderTable({scrollTop: 600});
+      //assert.doesNotThrow(function() {
+        table.setState({scrollTop: undefined});
+      //});
+    });
+
+    it('should not blow up when unsetting the scrollToColumn property', function() {
+      let table = renderTable({scrollToColumn: 3});
+      assert.doesNotThrow(function() {
+        table.setState({scrollToColumn: undefined});
+      });
+    });
+
+    it('should not blow up when unsetting the scrollToRow property', function() {
+      let table = renderTable({scrollToRow: 30});
+      assert.doesNotThrow(function() {
+        table.setState({scrollToRow: undefined});
+      });
+    });
   });
 });
-
diff --git a/src/vendor_upstream/core/shallowEqual.js b/src/vendor_upstream/core/shallowEqual.js
@@ -13,8 +13,6 @@
 
 'use strict';
 
-var hasOwnProperty = Object.prototype.hasOwnProperty;
-
 /**
  * Performs equality by iterating through keys on an object and returning false
  * when any key has values which are not strictly equal between the arguments.
@@ -38,7 +36,7 @@ function shallowEqual(objA: mixed, objB: mixed): boolean {
   }
 
   // Test for A's keys different from B.
-  var bHasOwnProperty = hasOwnProperty.bind(objB);
+  var bHasOwnProperty = Object.prototype.hasOwnProperty.bind(objB);
   for (var i = 0; i < keysA.length; i++) {
     if (!bHasOwnProperty(keysA[i]) || objA[keysA[i]] !== objB[keysA[i]]) {
       return false;