Enable linting for tests #3985

zpao · 2015-05-29T20:44:45Z

Any difference between true and false here? If not let's be consistent (I don't care what the value is if it works)

To configure global variables inside of a configuration file, use the globals key and indicate the global variables you want to use. Set each global variable name equal to true to allow the variable to be overwritten or false to disallow overwriting.

http://eslint.org/docs/user-guide/configuring.html

EDIT: I can comment something to that effect in the eslintrc if you want.

Ah no, I just didn't realize there was a difference. I might have known back when I first did this (I think there's a test that toggles __DEV__)

sophiebits · 2015-05-29T18:09:07Z

I'd rather just turn off the no trailing commas rule.

I agree, and think we should actually enforce trailing commas, but I think that should be another commit, and could be done with a codemod.

In this commit can we turn off the rule and not remove all the trailing commas (just to add them again a little later)? Also happy for us to add them everywhere before landing this PR if you'd rather do that.

I'll open another PR reversing this rule, and rebase this PR on top of it after it merges. That seems like the least amount of overall effort.

Okay. I've changed my mind here. I'll fix the dangling commas after this PR lands. I hacked together a fancy codemod tool that hooks into eslint's node API to fix the code. (which is awesome because we get zero false positives, and it obeys eslintignore/eslintrc files)

zpao · 2015-05-29T20:47:53Z

Eh, not sure how I feel about this. I don't have a good alternative though

The alternative, IMHO is to disable no-unused-expression in __tests__ only, by extending the grunt task to do the pattern matching itself, or by supporting multiple sets of eslintrc and eslintignore files.

sophiebits · 2015-05-29T18:11:12Z

(Why this change?)

componentWillLeave also defined cb so there was shadowing. Additionally, the existence of _cb and cb seemed a bit ugly. This seemed like the smallest modification I could make that didn't sacrifice readability.

sophiebits · 2015-05-29T18:12:49Z

Later in the code, there's the test:

it('should warn if a fragment is accessed', function() { spyOn(console, 'error'); var child = React.createElement('span'); var frag = ReactChildren.map([child, child], function(c) { return c; }); ...

Which means there's shadowing. Since frag was just a stub method used in only two places, I figured it'd be better to just get rid of it rather than renaming things to avoid a conflict.

-Original file line number
+Diff line change
@@ -1,9 +1,5 @@
     # We can probably lint these later but not important at this point
     src/shared/vendor
-    # eslint uses JSX* node types to determine if using JSX. esprima-fb still uses
-    # XJS* nodes. When we fix that (https://github.com/facebook/esprima/pull/85) we
-    # can enable linting the tests and fix those errors.
-    src/**/__tests__/**
     # This should be enabled but that folder has too much in it that doesn't belong
     src/test
     test/the-files-to-test.generated.js
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,12 +1,27 @@
     ---
     parser: babel-eslint
+    plugins:
+      - react
     env:
       browser: true
       node: true
     globals:
       __DEV__: true
+      # Jest / Jasmine
+      describe: false
+      xdescribe: false
+      beforeEach: false
+      afterEach: false
+      it: false
+      xit: false
+      jest: false
+      pit: false
+      expect: false
+      spyOn: false
+      jasmine: false
     rules:
       # ERRORS
@@ Expand All / @@ -26,7 +41,7 @@ rules: @@
       dot-location: [2, property]
       consistent-return: 2
       no-unused-vars: [2, args: none]
-      quotes: [2, single]
+      quotes: [2, single, avoid-escape]
       no-shadow: 2
       no-multi-spaces: 2
@@ Expand All / @@ -50,7 +65,13 @@ rules: @@
       key-spacing: 0
       # It's nice to be able to leave catch blocks empty
       no-empty: 0
+      # It makes code more readable to make this explicit sometimes
+      no-undef-init: 0
       # BROKEN. We'd like to turn these back on.
       # causes a ton of noise, eslint is too picky?
       block-scoped-var: 0
+      # JSX
+      react/jsx-uses-react: 2
+      react/jsx-quotes: [2, "double"]

-Original file line number
+Diff line change
@@ -1,8 +1,8 @@
     var warning = require('./lib/warning');
     warning(
       false,
-      'require("react/addons") is deprecated. ' +
-      'Access using require("react/addons/{addon}") instead.'
+      "require('react/addons') is deprecated. " +
+      "Access using require('react/addons/{addon}') instead."
     );
     module.exports = require('./lib/ReactWithAddons');

-Original file line number
+Diff line change
@@ Expand Up / @@ -40,6 +40,7 @@ @@
         "envify": "^3.0.0",
         "es5-shim": "^4.0.0",
         "eslint": "^0.21.2",
+        "eslint-plugin-react": "^2.3.0",
         "grunt": "~0.4.2",
         "grunt-cli": "^0.1.13",
         "grunt-compare-size": "~0.4.0",
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -50,20 +50,20 @@ describe('ReactComponentWithPureRenderMixin', function() { @@
           getInitialState: function() {
             return {
               cut: false,
-              slices: 1,
+              slices: 1
             };
           },
           cut: function() {
             this.setState({
               cut: true,
-              slices: 10,
+              slices: 10
             });
           },
           eatSlice: function() {
             this.setState({
-              slices: this.state.slices - 1,
+              slices: this.state.slices - 1
             });
           },
@@ Expand Down Expand Up / @@ -101,7 +101,7 @@ describe('ReactComponentWithPureRenderMixin', function() { @@
         function getInitialState() {
           return {
             foo: [1, 2, 3],
-            bar: {a: 4, b: 5, c: 6},
+            bar: {a: 4, b: 5, c: 6}
           };
         }
@@ Expand All / @@ -127,7 +127,7 @@ describe('ReactComponentWithPureRenderMixin', function() { @@
         // Do not re-render if state is equal
         var settings = {
           foo: initialSettings.foo,
-          bar: initialSettings.bar,
+          bar: initialSettings.bar
         };
         instance.setState(settings);
         expect(renderCalls).toBe(1);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -27,7 +27,7 @@ describe('ReactFragment', function() { @@
           x: <span />,
           y: <span />
         };
-        <div>{children}</div>;
+        void <div>{children}</div>;
         expect(console.error.calls.length).toBe(1);
         expect(console.error.calls[0].args[0]).toContain(
           'Any use of a keyed object'
@@ Expand All / @@ -37,7 +37,7 @@ describe('ReactFragment', function() { @@
           x: <span />,
           y: <span />
         };
-        <div>{sameChildren}</div>;
+        void <div>{sameChildren}</div>;
         expect(console.error.calls.length).toBe(1);
       });
@@ Expand Down Expand Up / @@ -65,7 +65,7 @@ describe('ReactFragment', function() { @@
           y: <span />
         };
         var frag = ReactFragment.create(children);
-        frag.x;
+        void frag.x;
         frag.y = 10;
         expect(console.error.calls.length).toBe(1);
         expect(console.error.calls[0].args[0]).toContain(
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -33,7 +33,7 @@ describe('renderSubtreeIntoContainer', function() { @@
         var Parent = React.createClass({
           childContextTypes: {
-            foo: React.PropTypes.string.isRequired,
+            foo: React.PropTypes.string.isRequired
           },
           getChildContext: function() {
@@ Expand Down Expand Up / @@ -72,7 +72,7 @@ describe('renderSubtreeIntoContainer', function() { @@
         var Parent = React.createClass({
           childContextTypes: {
-            foo: React.PropTypes.string.isRequired,
+            foo: React.PropTypes.string.isRequired
           },
           getChildContext: function() {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -72,7 +72,7 @@ describe('update', function() { @@
       });
       it('should support apply', function() {
-        expect(update(2, {$apply: function(x) { return x * 2; }})).toEqual(4);
+        expect(update(2, {$apply: (x) => x * 2})).toEqual(4);
         expect(update.bind(null, 2, {$apply: 123})).toThrow(
           'Invariant Violation: update(): expected spec of $apply to be a ' +
           'function; got 123.'
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -16,13 +16,11 @@ @@
     describe('LinkedStateMixin', function() {
       var LinkedStateMixin;
       var React;
-      var ReactLink;
       var ReactTestUtils;
       beforeEach(function() {
         LinkedStateMixin = require('LinkedStateMixin');
         React = require('React');
-        ReactLink = require('ReactLink');
         ReactTestUtils = require('ReactTestUtils');
       });
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -13,7 +13,6 @@ @@
     var React;
     var ReactCSSTransitionGroup;
-    var mocks;
     // Most of the real functionality is covered in other unit tests, this just
     // makes sure we're wired up correctly.
@@ Expand All / @@ -23,7 +22,6 @@ describe('ReactCSSTransitionGroup', function() { @@
       beforeEach(function() {
         React = require('React');
         ReactCSSTransitionGroup = require('ReactCSSTransitionGroup');
-        mocks = require('mocks');
         container = document.createElement('div');
         spyOn(console, 'error');
@@ Expand Down Expand Up / @@ -52,7 +50,7 @@ describe('ReactCSSTransitionGroup', function() { @@
         // For some reason jst is adding extra setTimeout()s and grunt test isn't,
         // so we need to do this disgusting hack.
-        for (var i = 0 ; i < setTimeout.mock.calls.length; i++) {
+        for (var i = 0; i < setTimeout.mock.calls.length; i++) {
           if (setTimeout.mock.calls[i][1] === 5000) {
             setTimeout.mock.calls[i][0]();
             break;
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -13,7 +13,6 @@ @@
     var React;
     var ReactTransitionGroup;
-    var mocks;
     // Most of the real functionality is covered in other unit tests, this just
     // makes sure we're wired up correctly.
@@ Expand All / @@ -23,7 +22,6 @@ describe('ReactTransitionGroup', function() { @@
       beforeEach(function() {
         React = require('React');
         ReactTransitionGroup = require('ReactTransitionGroup');
-        mocks = require('mocks');
         container = document.createElement('div');
       });
@@ Expand Down Expand Up / @@ -94,15 +92,15 @@ describe('ReactTransitionGroup', function() { @@
       it('should handle enter/leave/enter/leave correctly', function() {
         var log = [];
-        var cb;
+        var willEnterCb;
         var Child = React.createClass({
           componentDidMount: function() {
             log.push('didMount');
           },
-          componentWillEnter: function(_cb) {
+          componentWillEnter: function(cb) {
             log.push('willEnter');
-            cb = _cb;
+            willEnterCb = cb;
           },
           componentDidEnter: function() {
             log.push('didEnter');
@@ Expand Down Expand Up / @@ -139,12 +137,13 @@ describe('ReactTransitionGroup', function() { @@
         expect(log).toEqual(['didMount']);
         instance.setState({count: 2});
         expect(log).toEqual(['didMount', 'didMount', 'willEnter']);
-        for (var i = 0; i < 5; i++) {
+        for (var k = 0; k < 5; k++) {
           instance.setState({count: 2});
           expect(log).toEqual(['didMount', 'didMount', 'willEnter']);
           instance.setState({count: 1});
         }
-        cb();
+        // other animations are blocked until willEnterCb is called
+        willEnterCb();
         expect(log).toEqual([
           'didMount', 'didMount', 'willEnter',
           'didEnter', 'willLeave', 'didLeave', 'willUnmount'
@@ Expand All / @@ -153,15 +152,15 @@ describe('ReactTransitionGroup', function() { @@
       it('should handle enter/leave/enter correctly', function() {
         var log = [];
-        var cb;
+        var willEnterCb;
         var Child = React.createClass({
           componentDidMount: function() {
             log.push('didMount');
           },
-          componentWillEnter: function(_cb) {
+          componentWillEnter: function(cb) {
             log.push('willEnter');
-            cb = _cb;
+            willEnterCb = cb;
           },
           componentDidEnter: function() {
             log.push('didEnter');
@@ Expand Down Expand Up / @@ -198,20 +197,19 @@ describe('ReactTransitionGroup', function() { @@
         expect(log).toEqual(['didMount']);
         instance.setState({count: 2});
         expect(log).toEqual(['didMount', 'didMount', 'willEnter']);
-        for (var i = 0; i < 5; i++) {
+        for (var k = 0; k < 5; k++) {
           instance.setState({count: 1});
           expect(log).toEqual(['didMount', 'didMount', 'willEnter']);
           instance.setState({count: 2});
         }
-        cb();
+        willEnterCb();
         expect(log).toEqual([
           'didMount', 'didMount', 'willEnter', 'didEnter'
         ]);
       });
       it('should handle entering/leaving several elements at once', function() {
         var log = [];
-        var cb;
         var Child = React.createClass({
           componentDidMount: function() {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -22,10 +22,6 @@ describe('ReactChildren', function() { @@
         ReactFragment = require('ReactFragment');
       });
-      function frag(obj) {
-        return ReactFragment.create(obj);
-      }
       function nthChild(mappedChildren, n) {
         var result = null;
         ReactChildren.forEach(mappedChildren, function(child, index) {
@@ Expand Down Expand Up / @@ -227,7 +223,7 @@ describe('ReactChildren', function() { @@
         var instance = (
           <div>{
-            [frag({
+            [ReactFragment.create({
               firstHalfKey: [zero, one, two],
               secondHalfKey: [three, four],
               keyFive: five
@@ Expand Down Expand Up / @@ -345,7 +341,9 @@ describe('ReactChildren', function() { @@
         var zero = <div key="something"/>;
         var one = <span key="something" />;
-        var mapFn = function(component) { return component; };
+        var mapFn = function(component) {
+          return component;
+        };
         var instance = (
           <div>{zero}{one}</div>
         );
@@ Expand Down Expand Up / @@ -410,7 +408,7 @@ describe('ReactChildren', function() { @@
         var instance = (
           <div>{
-            [frag({
+            [ReactFragment.create({
               firstHalfKey: [zero, one, two],
               secondHalfKey: [three, four],
               keyFive: five
@@ Expand Down Expand Up / @@ -438,7 +436,7 @@ describe('ReactChildren', function() { @@
           return c;
         });
         for (var key in frag) {
-          frag[key];
+          void frag[key];
           break;
         }
         expect(console.error.calls.length).toBe(1);
@@ Expand All / @@ -447,8 +445,8 @@ describe('ReactChildren', function() { @@
         var frag2 = ReactChildren.map([child, child], function(c) {
           return c;
         });
-        for (var key in frag2) {
-          frag2[key] = 123;
+        for (var key2 in frag2) {
+          frag2[key2] = 123;
           break;
         }
         expect(console.error.calls.length).toBe(2);
@@ Expand Down @@

Enable linting for __tests__ #3985

Uh oh!

Enable linting for __tests__ #3985

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Enable linting for tests #3985

Enable linting for tests #3985