test: enable linting for tests and fix tests #1721

silverwind · 2015-05-17T17:13:42Z

This enables eslint for the test subdirectory. Noteable changes:

Put all tests in strict mode except a handful of exceptions
debugger and unreachable code are allowed in tests
Enabled forOf in global .eslintrc, as one test made use of it

Had to fix a ton of whitespace, semicolons and line wrappings to get all tests to pass the linter.

rlidwka · 2015-05-17T17:19:05Z

Put all tests in strict mode except a handful of exceptions

Why? The only reason for using strict mode in core was --use_strict flag, and we don't run tests with it.

debugger and unreachable code are allowed in tests

What for?

silverwind · 2015-05-17T17:31:20Z

Why? The only reason for using strict mode in core was --use_strict flag, and we don't run tests with it.

Tests do run strict once 'use strict'; is there. Why not run them in strict?

What for?

There's at least parallel/test-vm-debug-context.js which does make use of debugger, and another in a disabled test. I'll remove the exception for debugger and make it a per-file rule.

As for unreachable, there are a few tests that do stuff after a throw:

message/throw_custom_error.js
  10:0  error  Found unexpected statement after a throw  no-unreachable

message/throw_non_error.js
  10:0  error  Found unexpected statement after a throw  no-unreachable

parallel/test-domain.js
  172:2  error  Found unexpected statement after a throw  no-unreachable

parallel/test-file-write-stream.js
  28:6  error  Found unexpected statement after a throw  no-unreachable

parallel/test-http-content-length.js
  43:6  error  Found unexpected statement after a throw  no-unreachable

parallel/test-listen-fd-detached-inherit.js
  68:4  error  Found unexpected statement after a throw  no-unreachable

I guess we could just remove the unreachable code in these.

rlidwka · 2015-05-17T19:45:45Z

Tests do run strict once 'use strict'; is there. Why not run them in strict?

Because it requires a change in all the test files, and yet yields no benefit eslint doesn't provide already. That's a good enough reason imho.

As for unreachable, there are a few tests that do stuff after a throw:

Hmm... that's unusual. Maybe still disable that rule on per-file basis, because it is useful rule generally. I wonder what purpose those statements have though.

silverwind · 2015-05-17T20:29:13Z

Examples of code after throw: 1 2 3 4. No benefit there, right?

Regarding strict mode: enabling it lead me to at least 2 cases of duplicate property or argumentsnames and to a few cases of unintentional global variable usage, which I fixed. It has some benefits.

I guess now that these issues are fixed, I could revert to non-strict if we want to keep the number of changed files low.

brendanashworth · 2015-05-17T20:51:38Z

+1. Ref: joyent/node#25280, joyent/node#25288

bnoordhuis · 2015-05-17T23:14:50Z

LGTM with a comment (see below.) For the record: I'm in favor of turning on strict mode.

@silverwind Can you make the function arguments line up in test/common.js, test/parallel/test-fs-read-stream-inherit.js, test/parallel/test-http-url.parse-only-support-http-https-protocol.js, test/parallel/test-repl-reset-event.js and test/parallel/test-timers-ordering.js? I see you've been doing that in other files so it would be best to stay consistent.

By the way, doesn't eslint complain about function () { vs. function() { or function(){foo();}? And what's up with the super-long lines in test/parallel/test-promises-unhandled-rejections.js?

silverwind · 2015-05-18T01:11:09Z

Can you make the function arguments line up

Will try. It's not always possible to line them up at ( without splitting to 3 or more lines though.

doesn't eslint complain about function () { vs. function() { or function(){foo();}?

That's not enforced yet, which style should we enforce? Rule here.

what's up with the super-long lines in test/parallel/test-promises-unhandled-rejections.js?

Didn't want to split the long strings there in two, should I go for it?

silverwind · 2015-05-18T06:16:20Z

@bnoordhuis all done. added 2 more rules for function style. PTAL

yosuke-furukawa · 2015-05-18T07:07:12Z

@silverwind
+1 for lint test.

And I would like to upgrade our eslint from v0.20 to v0.21 before our eslint changes.
In eslint v0.21, we can use eslint config inheritance feature.
http://eslint.org/docs/user-guide/configuring#extending-configuration-files

So we don't modify <io.js root>/.eslintrc, we can add <io.js root>/test/.eslintrc .
And we can loosen/strengthen eslint rules in <io.js root>/test/.eslintrc like this.

    extends: "../.eslintrc"
    ecmaFeatures:  
       "forOf": true

    rules: 
        space-before-function-paren: [2, "never"]
        space-before-blocks: [2, "always"]

But I have not confirmed the eslint config inheritance yet... If I finished to confirm the feature then, I will pull request it.

silverwind · 2015-05-18T07:10:09Z

@yosuke-furukawa I was under the impression that test/.eslintrc already extends .eslintrc in the current version.

yosuke-furukawa · 2015-05-18T07:44:23Z

@silverwind In my understanding, we can inherit the config, but we don't apply the differenct configuration on a directory. that is eslint v0.21 feature. current version is v0.20.

silverwind · 2015-05-18T07:48:55Z

I doubt extends is needed with the way configuration cascading works by default.

yosuke-furukawa · 2015-05-18T08:33:13Z

OK, I will check it.

yosuke-furukawa · 2015-05-18T18:28:56Z

@silverwind
you are correct. I misunderstood that.

+1 for .eslintrc rules more stricter, and add forOf feature.
But we would be better to loosen the test/.eslintrc rules.

I think the following code style checks are not needed currently.

  quotes: 0
  semi: 0
  indent: 0
  max-len: 0

current chages are huge, I cannot check the chages are proper or not...

silverwind · 2015-05-18T18:42:04Z

Yeah, a shame GitHub cuts the diff here. Except maybe 5 cases, all changes are of stylistic nature. If you like, I can highlight the actual changes to code, if that makes the review easier. But I think @bnoordhuis already did an overall check.

yosuke-furukawa · 2015-05-19T01:18:38Z

I also browsed the changes roughly.

I would like to know why this change is needed.

 }, function(err) {
   if (err instanceof Error) {
-    assert.strictEqual(err.message,
-        'Protocol "file:" not supported. Expected "http:".');
+    assert.strictEqual(err.message, 'Protocol "file:" not supported.' +
+                       ' Expected "http:".');
     return true;
   }
 });

I don't think this is readable but eslint warns some messages ??

silverwind · 2015-05-19T01:37:28Z

Well, the normal recommendation on how to split lines like this that go over 80 chars is to align additional lines to the function parens like this:

assert.strictEqual(err.message,
                   'Protocol "file:" not supported. Expected "http:".');

Now the issue in this case is, the second line goes above 80 chars as well, which forced me to split the string unfortunately.

yosuke-furukawa · 2015-05-19T03:10:29Z

@silverwind
That is the my concern.
test scripts have many text checks and explain the test code using comment // xxxx.
Line length goes over 80 easily.
So I recommend you to uncheck the max-len and indent for test scripts.

silverwind · 2015-05-19T04:48:12Z

I'm not sure it's worth the time to change back now again. Tests do pass with the same strict rules like lib, which I think is a huge benefit. Lifting the unreachable rule is fine with me because you never know if the dead code might be of use at some point.

The 80 chars rule is there for an valid, albeit archaic reason, and I think @bnoordhuis would agree to keeping it in place.

yosuke-furukawa · 2015-05-19T05:51:58Z

I also think the huge benefit for linting test codes.
And I also agree with most rules without max-len/indent on test code.
BUT It's OK, I don't have so strong opinion that.

but If i were you, i would change the code like this.

   if (err instanceof Error) {
-    assert.strictEqual(err.message,
-        'Protocol "file:" not supported. Expected "http:".');
+    assert.strictEqual(
+        err.message, 'Protocol "file:" not supported. Expected "http:".'); //75 chars
     return true;
   }

or

   if (err instanceof Error) {
-    assert.strictEqual(err.message,
-        'Protocol "file:" not supported. Expected "http:".');
+    assert.strictEqual(
+        err.message, 
+        'Protocol "file:" not supported. Expected "http:".'); // down indent
     return true;
   }

rlidwka · 2015-05-19T10:27:27Z

I do not understand what is wrong with the original code here:

    assert.strictEqual(err.message,
        'Protocol "file:" not supported. Expected "http:".');

Breaking a line and starting a new one with +4 spaces indent is somewhat a standard in this case.

If linter doesn't complain, don't change the existing code. If linter complains about this particular case, fix the linter. In any case I don't think it should be changed.

edit: nevermind, the diff above confused me.

bnoordhuis · 2015-05-19T10:37:55Z

The 80 chars rule is there for an valid, albeit archaic reason, and I think @bnoordhuis would agree to keeping it in place.

THE 80 CHARACTER LIMIT IS INVIOLATE!

(In all seriousness, there needs to be a limit and 80 chars is traditional.)

Breaking a line and starting a new one with +4 spaces indent is somewhat a standard in this case.

io.js / node.js follows a lisp-y style where arguments need to line up.

bnoordhuis · 2015-05-19T10:43:13Z

Had a quick look and rubber-stamp LGTM. It needs a rebase and you should run the CI after that just in case but it looks good to go to me.

Enable linting for the test directory. A number of changes was made so all tests conform the current rules used by lib and src directories. The only exception for tests is that unreachable (dead) code is allowed.

silverwind · 2015-05-19T11:11:47Z

Fixed up test/parallel/test-repl-tab, rebased and squashed.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/686/

silverwind · 2015-05-19T12:39:53Z

A few unrelated failures were from #1707. One new failure on windows boxes:

not ok 215 - test-fs-non-number-arguments-throw.js
# 
# assert.js:88
# throw new assert.AssertionError({
# ^
# AssertionError: read uire('assert' instead of ire('assert')
# at ReadStream.<anonymous> (c:\workspace\iojs+any-pr+multi\nodes\win2012r2\test\parallel\test-fs-non-number-arguments-throw.js:23:10)
# at emitOne (events.js:77:13)
# at ReadStream.emit (events.js:169:7)
# at readableAddChunk (_stream_readable.js:145:16)
# at ReadStream.Readable.push (_stream_readable.js:109:10)
# at onread (fs.js:1734:12)
# at FSReqWrap.wrapper [as oncomplete] (fs.js:576:17)

The test reads itself, and for some reason reads one position to low on both win2008 and win2012. Strangely, I can't reproduce on my Windows 7 box.

bnoordhuis · 2015-05-19T13:05:16Z

Curious. I wonder if the test has been broken all along. For posterity, here is the diff:

diff --git a/test/parallel/test-fs-non-number-arguments-throw.js b/test/parallel/test-fs-non-number-arguments-throw.js
index 7a1e7bd..4e7af86 100644
--- a/test/parallel/test-fs-non-number-arguments-throw.js
+++ b/test/parallel/test-fs-non-number-arguments-throw.js
@@ -1,23 +1,24 @@
+'use strict';
 var assert = require('assert'),
     fs = require('fs'),
     saneEmitter,
     sanity = 'ire(\'assert\')';

-saneEmitter = fs.createReadStream(__filename, { start: 17, end: 29 });
+saneEmitter = fs.createReadStream(__filename, { start: 31, end: 43 });

-assert.throws(function () {
-  fs.createReadStream(__filename, { start: "17", end: 29 });
+assert.throws(function() {
+  fs.createReadStream(__filename, { start: '31', end: 43 });
 }, "start as string didn't throw an error for createReadStream");

-assert.throws(function () {
-  fs.createReadStream(__filename, { start: 17, end: "29" });
+assert.throws(function() {
+  fs.createReadStream(__filename, { start: 31, end: '43' });
 }, "end as string didn't throw an error");

-assert.throws(function () {
-  fs.createWriteStream(__filename, { start: "17" });
+assert.throws(function() {
+  fs.createWriteStream(__filename, { start: '31' });
 }, "start as string didn't throw an error for createWriteStream");

-saneEmitter.on('data', function (data) {
+saneEmitter.on('data', function(data) {
   // a sanity check when using numbers instead of strings
   assert.strictEqual(sanity, data.toString('utf8'), 'read ' +
                      data.toString('utf8') + ' instead of ' + sanity);

end is inclusive. The test expects to read "ire('assert')" but when I double-check with python, I get this:

# v1.x
>>> open('test/parallel/test-fs-non-number-arguments-throw.js').read()[17:29]
"ire('assert'"
# pr/1721
open('test/parallel/test-fs-non-number-arguments-throw.js').read()[31:43]
"ire('assert'"

It passes both times on my Linux machine, however.

silverwind · 2015-05-19T13:07:42Z

Guessing it's related to the newline introduced.

Started off another CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/688/

silverwind · 2015-05-19T13:15:15Z

Works fine in both python and iojs on my Windows machine:

> fs.createReadStream('test/parallel/test-fs-non-number-arguments-throw.js', { start: 31, end: 43 }).pipe(process.stdout)
> ire('assert')

C:\git\iojs>python
Python 2.7.9 (default, Dec 10 2014, 12:24:55) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> open('test/parallel/test-fs-non-number-arguments-throw.js').read()[31:43]
"ire('assert'"

silverwind · 2015-05-19T14:22:08Z

Working fine on a Windows 8.1 VM I just set up:

branch master

C:\io.js>iojs -e "require('fs').createReadStream('test/parallel/test-fs-non
-number-arguments-throw.js', {start: 17, end: 29}).pipe(process.stdout)"
ire('assert')

branch test-lint

C:\io.js>iojs -e "require('fs').createReadStream('test/parallel/test-fs-non
-number-arguments-throw.js', {start: 31, end: 43}).pipe(process.stdout)"
ire('assert')

Either this is CI-specific or Windows Server specific. I'm thinking that this could maybe point us to the reason of the other test failures on Windows too.

Could someone with access to the Windows CI machines check why createReadStream reads one position too early the second example above? @rvagg maybe?

silverwind · 2015-05-19T18:07:22Z

For giggles, I also spun up a Windows 2012 VM, unable to reproduce:

C:\io.js>git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.

C:\io.js>iojs -e "require('fs').createReadStream('test/parallel/test-
fs-non-number-arguments-throw.js', { start: 17, end: 29}).pipe(process.stdout)"
ire('assert')

C:\io.js>git checkout test-lint
Checking out files: 100% (5578/5578), done.
Switched to branch 'test-lint'
Your branch is up-to-date with 'origin/test-lint'.

C:\io.js>iojs -e "require('fs').createReadStream('test/parallel/test-fs
-non-number-arguments-throw.js', { start: 31, end: 43}).pipe(process.stdout)"
ire('assert')

At this point, I'm certain that this is another issue with the Windows CI and it didn't manifest before because the test only read from the first line.

silverwind · 2015-05-19T18:13:15Z

I'll revert changes on that test and add it to .eslintignore so it doesn't block the landing here. Will open a second issue for CI.

silverwind · 2015-05-19T18:18:44Z

CI:
~~https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/692/~~
https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/693/

silverwind · 2015-05-19T19:18:00Z

No new failures in the CI now, noteably a few build targets like CoreOS and CentOS didn't start up correctly, but I'm confident the changes here are fine now.

Enable linting for the test directory. A number of changes was made so all tests conform the current rules used by lib and src directories. The only exception for tests is that unreachable (dead) code is allowed. test-fs-non-number-arguments-throw had to be excluded from the changes because of a weird issue on Windows CI. PR-URL: #1721 Reviewed-By: Ben Noordhuis <[email protected]>

silverwind · 2015-05-19T19:28:34Z

Landed in f29762f
createReadStream issue at #1737

Enable linting for the test directory. A number of changes was made so all tests conform the current rules used by lib and src directories. The only exception for tests is that unreachable (dead) code is allowed. test-fs-non-number-arguments-throw had to be excluded from the changes because of a weird issue on Windows CI. PR-URL: nodejs#1721 Reviewed-By: Ben Noordhuis <[email protected]> Conflicts: test/parallel/test-arm-math-exp-regress-1376.js test/parallel/test-net-dns-custom-lookup.js test/parallel/test-net-dns-lookup-skip.js test/parallel/test-repl-mode.js test/parallel/test-repl-options.js test/parallel/test-repl-tab.js test/parallel/test-util-inspect.js

Enable linting for the test directory. A number of changes was made so all tests conform the current rules used by lib and src directories. The only exception for tests is that unreachable (dead) code is allowed. test-fs-non-number-arguments-throw had to be excluded from the changes because of a weird issue on Windows CI. PR-URL: nodejs/node#1721 Reviewed-By: Ben Noordhuis <[email protected]>

silverwind added the test Issues and PRs related to the tests. label May 17, 2015

silverwind force-pushed the test-lint branch 2 times, most recently from 0daf392 to 234eb90 Compare May 17, 2015 17:37

silverwind mentioned this pull request May 17, 2015

test: Fix make jslint to include test scripts too. nodejs/node-v0.x-archive#25280

Closed

test: enable linting for tests

5a8d7ca

Enable linting for the test directory. A number of changes was made so all tests conform the current rules used by lib and src directories. The only exception for tests is that unreachable (dead) code is allowed.

silverwind force-pushed the test-lint branch from 68a270e to 5a8d7ca Compare May 19, 2015 11:07

bnoordhuis mentioned this pull request May 19, 2015

net: persist net.Socket options before connect #1518

Closed

Revert and blacklist test-fs-non-number-arguments-throw

bb1db51

silverwind mentioned this pull request May 19, 2015

test: fs.createReadStream weirdness on Windows CI #1737

Closed

silverwind closed this May 19, 2015

rvagg mentioned this pull request May 23, 2015

Release proposal v2.1.0 #1777

Closed

silverwind mentioned this pull request Jun 28, 2015

tools: update eslint and re-enable comma-spacing rule #2072

Closed

silverwind mentioned this pull request Aug 20, 2015

test: remove unreachable code #2289

Closed

snyk-bot mentioned this pull request Jan 20, 2023

[Snyk] Upgrade commander from 2.11.0 to 9.4.1 Supermajestcd/node#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: enable linting for tests and fix tests #1721

test: enable linting for tests and fix tests #1721

silverwind commented May 17, 2015

rlidwka commented May 17, 2015

silverwind commented May 17, 2015

rlidwka commented May 17, 2015

silverwind commented May 17, 2015

brendanashworth commented May 17, 2015

bnoordhuis commented May 17, 2015

silverwind commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 19, 2015

silverwind commented May 19, 2015

yosuke-furukawa commented May 19, 2015

silverwind commented May 19, 2015

yosuke-furukawa commented May 19, 2015

rlidwka commented May 19, 2015

bnoordhuis commented May 19, 2015

bnoordhuis commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

bnoordhuis commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

test: enable linting for tests and fix tests #1721

test: enable linting for tests and fix tests #1721

Conversation

silverwind commented May 17, 2015

rlidwka commented May 17, 2015

silverwind commented May 17, 2015

rlidwka commented May 17, 2015

silverwind commented May 17, 2015

brendanashworth commented May 17, 2015

bnoordhuis commented May 17, 2015

silverwind commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 18, 2015

yosuke-furukawa commented May 18, 2015

silverwind commented May 18, 2015

yosuke-furukawa commented May 19, 2015

silverwind commented May 19, 2015

yosuke-furukawa commented May 19, 2015

silverwind commented May 19, 2015

yosuke-furukawa commented May 19, 2015

rlidwka commented May 19, 2015

bnoordhuis commented May 19, 2015

bnoordhuis commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

bnoordhuis commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

branch master

branch test-lint

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015

silverwind commented May 19, 2015