Enable linting for tests #3985

bgw · 2015-05-29T18:03:25Z

Closes #3971.

After #3968, the next thing we should do is start linting our tests.
Historically we've ignored them due to lack of parser compatibility.
But that shouldn't be a problem anymore. We may want to integrate
https://www.npmjs.com/package/eslint-plugin-react to more aggressively
lint our JSX in tests.

I understand this diff touches a lot of stuff, so I tried to keep it to
a near-minimal set of changes to make eslint happy. If you're wondering
why I changed something, let me know and I'll try to map it up to the
rule that I was fixing it for.

bgw · 2015-05-29T18:04:40Z

Potential future steps:

Enable more of the eslint-plugin-react rules.
Make our grunt task smarter to be able to pattern-match __tests__, and provide alternate .eslintrc files for tests and normal code. E.g. I'd propose disabling no-unused-expressions for tests.
Fix all of the 80-column warnings (ugh)

sophiebits · 2015-05-29T18:08:33Z

npm-react/addons.js

@@ -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. ' +


Why is this necessary?

That isn't actually. My bad regexp codemod caught it, and I figured it wouldn't hurt to change it. Let me know if you want me to revert it.

I guess let's keep it as single quotes in the require but use double quotes for the outer string so we don't need the escapes.

sophiebits · 2015-05-29T18:10:52Z

src/addons/transitions/__tests__/ReactTransitionGroup-test.js

@@ -128,7 +126,7 @@ describe('ReactTransitionGroup', function() {
      },
      render: function() {
        var children = [];
-        for (var i = 0; i < this.state.count; i++) {
+        for (let i = 0; i < this.state.count; i++) {


We don't support let internally at FB yet so this can't go in like this.

It's been brought up multiple times that we shouldn't be running these tests internally anyways, so I'll look into that issue before we merge this.

We already don't run the tests internally. But we still lint internally and I wouldn't be surprised if we end up choking on let. Regardless though, let's not introduce the first let here.

sophiebits · 2015-05-29T18:28:56Z

This looks pretty good overall.

@spicyj

Removes all `/*eslint-disable no-unused-expressions */` comments. > Instead of disabling this rule, maybe we chould use the `void` > operator on each place that triggers it (which this rule's docs claim > doesn't warn). Like `void frag[key];`. facebook#3985 (comment) @spicyj

@spicyj

> Does (x) => x * 2 work here? If so, let's do that – but otherwise this > is okay. facebook#3985 (comment) @spicyj

@spicyj

facebook#3985 (diff) @spicyj

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

.eslintrc

 env:
  browser: true
  node: true

 globals:
  __DEV__: true
+  # Jest / Jasmine
+  describe: false


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__)

@zpao

facebook#3985 (comment) @zpao

@spicyj

> For this one, we could just do displayName: 'NamedComponent', inside > the class spec and drop the var assignment. facebook#3985 (comment) @spicyj

bgw · 2015-06-01T18:53:47Z

@zpao @spicyj

I think I fixed everything except https://github.com/facebook/react/pull/3985/files#r31360729 , which I'd like an opinion on, and the trailing comma issue, which I'll fix in a later PR, after this one is merged.

I could revert the comma changes I made here and disable the rule right now, but there's not really any point besides slightly shortening the diff.

Alternatively, we could disable it globally, but this is the only place it triggers in our code. > facebook#3985 (diff)

sophiebits · 2015-06-01T22:51:22Z

src/isomorphic/classic/class/__tests__/ReactClass-test.js

@@ -159,6 +159,7 @@ describe('ReactClass-spec', function() {
    }
  });

+  /*eslint-disable no-unused-vars */


I don't think you need this now?

sophiebits · 2015-06-01T22:55:00Z

lgtm – can you fix those two things? Then feel free to squash and merge.

Closes facebook#3971. > After facebook#3968, the next thing we should do is start linting our tests. > Historically we've ignored them due to lack of parser compatibility. > But that shouldn't be a problem anymore. We may want to integrate > https://www.npmjs.com/package/eslint-plugin-react to more aggressively > lint our JSX in tests. I understand this diff touches a lot of stuff, so I tried to keep it to a near-minimal set of changes to make eslint happy.

Enable linting for __tests__

bgw assigned zpao May 29, 2015

facebook-github-bot added the GH Review: review-needed label May 29, 2015

sophiebits reviewed May 29, 2015
View reviewed changes

facebook-github-bot added the CLA Signed label May 29, 2015

sophiebits reviewed May 29, 2015
View reviewed changes

bgw added a commit to bgw/react that referenced this pull request May 29, 2015

Remove intermediate f() function in update-test

411fce2

> Does (x) => x * 2 work here? If so, let's do that – but otherwise this > is okay. facebook#3985 (comment) @spicyj

bgw added a commit to bgw/react that referenced this pull request May 29, 2015

Disable no-undef-init globally

7ff45ee

facebook#3985 (diff) @spicyj

zpao reviewed May 29, 2015
View reviewed changes

bgw added a commit to bgw/react that referenced this pull request Jun 1, 2015

Change linebreak style in ReactPropTypes-test.js

b9bb438

facebook#3985 (comment) @zpao

bgw added a commit to bgw/react that referenced this pull request Jun 1, 2015

Use displayName, not assignment in ReactClass-test

3dc606a

> For this one, we could just do displayName: 'NamedComponent', inside > the class spec and drop the var assignment. facebook#3985 (comment) @spicyj

bgw added a commit to bgw/react that referenced this pull request Jun 1, 2015

Replace eslint-disable no-new for test with void

d22edf4

Alternatively, we could disable it globally, but this is the only place it triggers in our code. > facebook#3985 (diff)

sophiebits reviewed Jun 1, 2015
View reviewed changes

sophiebits added GH Review: accepted and removed GH Review: review-needed labels Jun 1, 2015

bgw force-pushed the eslint-tests branch from 540642f to 21ca3f7 Compare June 1, 2015 23:01

bgw added a commit that referenced this pull request Jun 1, 2015

Merge pull request #3985 from PiPeep/eslint-tests

13823ec

Enable linting for __tests__

bgw merged commit 13823ec into facebook:master Jun 1, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable linting for tests #3985

Enable linting for tests #3985

bgw commented May 29, 2015

bgw commented May 29, 2015

sophiebits May 29, 2015

bgw May 29, 2015

sophiebits May 29, 2015

sophiebits May 29, 2015

bgw May 29, 2015

zpao May 29, 2015

sophiebits commented May 29, 2015

zpao May 29, 2015

bgw May 29, 2015

zpao Jun 1, 2015

bgw commented Jun 1, 2015

sophiebits Jun 1, 2015

sophiebits commented Jun 1, 2015

Enable linting for __tests__ #3985

Enable linting for __tests__ #3985

Conversation

bgw commented May 29, 2015

bgw commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgw commented Jun 1, 2015

Choose a reason for hiding this comment

sophiebits commented Jun 1, 2015

Enable linting for tests #3985

Enable linting for tests #3985