tools: improve js linter #5638

mscdex · 2016-03-10T03:47:18Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

tools

Description of change

This commit switches from the eslint command-line tool to a custom tool that uses eslint programmatically in order to perform linting in parallel and to display linting results incrementally instead of buffering them until the end. There is also now a progress indicator similar to that of make test.

Additionally, there is a new lint-ci rule that disables the progress indicator. I guess the command used on the linter Jenkins node will need to be changed internally as I couldn't find where it's configured to use what command.

Fixes: #5596

jbergstroem · 2016-03-10T04:03:01Z

We currently do gmake lint; so I'll change that to gmake lint-ci at some stage then?

jbergstroem · 2016-03-10T04:08:45Z

@mscdex btw, it's on my agenda to – at some stage – get tap output from the linter so we can post feedback to github. It acts differently than the test suite since lint only reports errors and we'd have an "error count"; but it's still better than no output at all. This may or may not affect your current work but I thought I'd just bring it up.

You can find my work and playing around here: jbergstroem#1

jbergstroem · 2016-03-10T04:09:53Z

@mscdex the problem with switching what command the linter should run is that we need to merge that commit to all branches and have it sit there for a while before we can switch in CI; otherwise it would call a command that wouldn't exist. My plan to address this was introducing placeholders and let them sit around for a while. I obviously forgot about it.

mscdex · 2016-03-10T06:07:58Z

@jbergstroem Does the tap output have to all be in one single group of results (e.g. only one TAP header in the output file) or does it not matter?

jbergstroem · 2016-03-10T06:30:27Z

@mscdex I recall globbing files ("collect *.tap") working just fine.

mscdex · 2016-03-10T07:00:14Z

Ok, I've updated it to allow output to file and selecting the formatter.

silverwind · 2016-03-10T13:08:03Z

Nice work, seems to run slightly faster too:

Before: make jslint 15.72s user 0.82s system 106% cpu 15.565 total
After: make jslint 33.02s user 2.56s system 287% cpu 12.388 total

Fishrock123 · 2016-03-10T15:21:23Z

Is that faster or slower?

silverwind · 2016-03-10T15:23:57Z

3s faster, but a lot more cpu intensive.

mscdex · 2016-03-10T16:33:17Z

I am getting ~7s improvement on my machine with the current list of files we lint.

I also just bumped up the max load per worker and that seemed to decrease times a tad more.

It will use more cpu time if you have multiple cores though.

jbergstroem · 2016-03-10T18:04:04Z

I again just want to make the note that we can't land this as-is seeing how we rely on make lint from ci (just want this here in case of LGTM's starting to roll in). Few options:

New PR with make lint-ci calling make lint, merge it and wait a few months before landing this
Env logic (since its easy for us to add env stuff through jenkins) for make lint -- this is how we usually get around handling multiple versions of Makefile.
I had something in mind but forgot while writing the other two :) will revisit when awake.

bnoordhuis · 2016-03-23T17:45:33Z

LGTM although there are a few conflicts. @jbergstroem What needs to happen before this can land?

jbergstroem · 2016-03-23T20:34:50Z

@bnoordhuis I suggest we do the first path; create placeholders in Makefile and make sure we get that commit into all branches we want to cover in CI, then land this.

Trott · 2016-03-23T23:15:58Z

I don't think the make lint-ci job is working correctly. If I modify .eslintrc in the root directory (say, remove the option fromcomma-dangle so that it is just 2), make jslint correctly flags errors but make lint-ci reports 0 errors.

mscdex · 2016-03-26T11:32:06Z

@Trott That message is from cpplint, not jslint-ci.

@jbergstroem What would that kind of initial change look like? I'd like to get the placeholders in sooner than later.

jbergstroem · 2016-03-26T20:46:19Z

@mscdex I'll do a PR.

Trott · 2016-03-26T21:20:15Z

@mscdex Ah, yes, you are correct, of course. The errors are showing up just fine in the tap file.

Fishrock123 · 2016-03-28T14:58:09Z

tools/jslint.js

+    const secs = padString(elapsed % 60, 2, '0');
+    const passed = padString(successes, 6, ' ');
+    const failed = padString(failures, 6, ' ');
+    var pct = Math.ceil(((totalPaths - paths.length) / totalPaths) * 100);


Percent complete

Fishrock123 · 2016-03-28T14:59:23Z

@mscdex could you comment-document the new jslint proxy tool more? I just want to make sure this isn't something we have to fight with much in the future. :)

mscdex · 2016-04-09T18:23:41Z

@Fishrock123 more comments added

jbergstroem · 2016-04-15T04:08:33Z

By "update" I just meant look at merging; sorry. I've changed targets in ci now -- this might imply that old jobs that hasn't been rebased as of late will likely fail; so lets try and keep track of that. Failed lint jobs aren't common, so it should be pretty easy to spot. LGTM.

jbergstroem · 2016-04-15T04:13:22Z

Actually; hold off passing -J in test-ci for now. I'd like to add it similar to how its done in #6208.

This commit switches from the eslint command-line tool to a custom tool that uses eslint programmatically in order to perform linting in parallel and to display linting results incrementally instead of buffering them until the end. Fixes: nodejs#5596 PR-URL: nodejs#5638 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

jbergstroem · 2016-04-15T04:42:11Z

(Still LGTM after you added -j)

jbergstroem · 2016-04-15T05:21:13Z

FYI: Went from spending >50sec to <30sec in our CI on linting as a result of this PR 🎉

This commit switches from the eslint command-line tool to a custom tool that uses eslint programmatically in order to perform linting in parallel and to display linting results incrementally instead of buffering them until the end. Fixes: #5596 PR-URL: #5638 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

MylesBorins · 2016-04-21T06:35:30Z

@mscdex do you think we should backport this to v4?

mscdex · 2016-04-21T14:11:26Z

@thealphanerd AFAIK it should work as long as a2ca347 is also applied.

This commit switches from the eslint command-line tool to a custom tool that uses eslint programmatically in order to perform linting in parallel and to display linting results incrementally instead of buffering them until the end. Fixes: #5596 PR-URL: #5638 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

MylesBorins · 2016-05-17T22:42:25Z

I'm going to mark this as do not land for lts right now

@mscdex feel free to prepare a backport of this change if you think it would be beneficial

mscdex added the tools Issues and PRs related to the tools directory. label Mar 10, 2016

mscdex force-pushed the tools-improve-linter branch from 61174ff to d4be90c Compare March 10, 2016 06:58

mscdex force-pushed the tools-improve-linter branch from d4be90c to 6c4e3e1 Compare March 10, 2016 07:01

mscdex force-pushed the tools-improve-linter branch from 6c4e3e1 to 4f6c8ad Compare March 10, 2016 16:19

mscdex force-pushed the tools-improve-linter branch from 4f6c8ad to b7c5f6d Compare March 10, 2016 20:57

mscdex mentioned this pull request Mar 11, 2016

eslint hangs on /bechmark #5669

Closed

mscdex force-pushed the tools-improve-linter branch from b7c5f6d to 72994e6 Compare March 26, 2016 11:26

jbergstroem mentioned this pull request Mar 27, 2016

build: introduce ci targets for lint/benchmark #5921

Closed

2 tasks

Fishrock123 reviewed Mar 28, 2016
View reviewed changes

mscdex mentioned this pull request Apr 9, 2016

tools, test: update ESLint to 2.7.0 #6132

Closed

2 tasks

mscdex force-pushed the tools-improve-linter branch from 72994e6 to e13cad2 Compare April 9, 2016 18:23

mscdex force-pushed the tools-improve-linter branch from e13cad2 to 3d3ef39 Compare April 15, 2016 04:38

mscdex force-pushed the tools-improve-linter branch from 3d3ef39 to 81fd458 Compare April 15, 2016 04:40

mscdex merged commit 81fd458 into nodejs:master Apr 15, 2016

mscdex deleted the tools-improve-linter branch April 15, 2016 04:51

jbergstroem mentioned this pull request Apr 15, 2016

build: allow test-ci to run tests in parallel #6208

Merged

2 tasks

MylesBorins mentioned this pull request Apr 20, 2016

V5.10.2 proposal #6311

Closed

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

Audit commits not found on v4.x #5647

Closed

MylesBorins added the lts-watch-v4.x label Apr 21, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels May 17, 2016

gibfahn mentioned this pull request May 13, 2017

lint job doesn't show tap result nodejs/build#720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: improve js linter #5638

tools: improve js linter #5638

mscdex commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

mscdex commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

mscdex commented Mar 10, 2016

silverwind commented Mar 10, 2016

Fishrock123 commented Mar 10, 2016

silverwind commented Mar 10, 2016

mscdex commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

bnoordhuis commented Mar 23, 2016

jbergstroem commented Mar 23, 2016

Trott commented Mar 23, 2016

mscdex commented Mar 26, 2016

jbergstroem commented Mar 26, 2016

Trott commented Mar 26, 2016

Fishrock123 Mar 28, 2016

mscdex Mar 28, 2016

Fishrock123 commented Mar 28, 2016

mscdex commented Apr 9, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

MylesBorins commented Apr 21, 2016

mscdex commented Apr 21, 2016

MylesBorins commented May 17, 2016

tools: improve js linter #5638

tools: improve js linter #5638

Conversation

mscdex commented Mar 10, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

jbergstroem commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

mscdex commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

mscdex commented Mar 10, 2016

silverwind commented Mar 10, 2016

Fishrock123 commented Mar 10, 2016

silverwind commented Mar 10, 2016

mscdex commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

bnoordhuis commented Mar 23, 2016

jbergstroem commented Mar 23, 2016

Trott commented Mar 23, 2016

mscdex commented Mar 26, 2016

jbergstroem commented Mar 26, 2016

Trott commented Mar 26, 2016

Fishrock123 Mar 28, 2016

Choose a reason for hiding this comment

mscdex Mar 28, 2016

Choose a reason for hiding this comment

Fishrock123 commented Mar 28, 2016

mscdex commented Apr 9, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

jbergstroem commented Apr 15, 2016

MylesBorins commented Apr 21, 2016

mscdex commented Apr 21, 2016

MylesBorins commented May 17, 2016