tools: remove lint-js.js #30955

Trott · 2019-12-13T23:44:25Z

lint-js.js was implemented before ESLint had a caching feature. It is
now only used in CI. Let's remove it on the following grounds:

It results in occasional (and puzzling) yellow CI runs for
node-test-linter because the tap file is corrupted somehow.
Interleaved maybe? I don't know, but a simple solution is removing it
and running ESLint directly.
On my local laptop, it reduces the linting from about 75 seconds to
about 55 seconds. This kind of savings is not worth the added
complexity and the instability noted above.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-12-13T23:46:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27654/

richardlau

We will need to check the removed lines from vcbuild.bat are not used anywhere in the CI.

vcbuild.bat

Trott · 2019-12-13T23:59:20Z

We will need to check the removed lines from vcbuild.bat are not used anywhere in the CI.

Yeah, running full CI to check, although obviously that won't necessarily catch things in other jobs. Kinda doubt it would be in other jobs and not in the main one, though.

nodejs-github-bot · 2019-12-14T00:02:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27655/

mscdex · 2019-12-14T00:27:14Z

Before removing, it would be a good idea to see the performance impact on all of the CI machines (especially the ARM-based ones).

Trott · 2019-12-14T03:30:43Z

Before removing, it would be a good idea to see the performance impact on all of the CI machines (especially the ARM-based ones).

We only run lint on one machine these days.

richardlau · 2019-12-14T04:34:39Z

Looks like the linter job failed to parse the tap file with tap2junit:
https://ci.nodejs.org/job/node-test-linter/31548/console

02:42:08 + tap2junit -i test-eslint.tap -o out/test.xml
02:42:09 Traceback (most recent call last):
02:42:09   File "/usr/local/bin/tap2junit", line 11, in <module>
02:42:09     sys.exit(main())
02:42:09   File "/usr/local/lib/python2.7/dist-packages/tap2junit/__main__.py", line 44, in main
02:42:09     result = parse(os.path.splitext(args.input.name)[0], data)
02:42:09   File "/usr/local/lib/python2.7/dist-packages/tap2junit/__main__.py", line 15, in parse
02:42:09     t = TestCase(test.description, None, test.yaml['duration_ms'])
02:42:09 TypeError: 'NoneType' object has no attribute '__getitem__'
02:42:09 POST BUILD TASK : FAILURE

Trott · 2019-12-14T06:05:54Z

Looks like the linter job failed to parse the tap file with tap2junit:

That's alarming. It parses just fine when I do it locally....

Trott · 2019-12-14T06:12:12Z

Looks like the linter job failed to parse the tap file with tap2junit:

That's alarming. It parses just fine when I do it locally....

Argh, the version of tap2junit on the linter machine is different than what I get with pip install tap2junit....

Trott · 2019-12-14T06:24:37Z

tap2junit on the linter box is 0.1.4 but the current release is 0.1.5.

Trott · 2019-12-14T06:35:48Z

Oh, interesting. So I managed to replicate the bug that I mention above where I surmise that its interleaved output or something, but actually it's a bug in tap2junit 0.1.4.

Input file:

TAP version 13
1..1
ok 1 - /Users/trott/io.js/.eslintrc.js

TAP version 13
1..2
ok 1 - /Users/trott/io.js/benchmark/net/tcp-raw-pipe.js
ok 2 - /Users/trott/io.js/benchmark/net/tcp-raw-s2c.js

With tap2junit 0.1.4:

$ tap2junit -i test.tap -o foo
Traceback (most recent call last):
  File "/usr/local/bin/tap2junit", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/site-packages/tap2junit/__main__.py", line 44, in main
    result = parse(os.path.splitext(args.input.name)[0], data)
  File "/usr/local/lib/python2.7/site-packages/tap2junit/__main__.py", line 11, in parse
    tap_parser.parse(data)
  File "/usr/local/lib/python2.7/site-packages/tap2junit/tap13.py", line 124, in parse
    self._parse(StringIO.StringIO(source))
  File "/usr/local/lib/python2.7/site-packages/tap2junit/tap13.py", line 102, in _parse
    raise ValueError("Descending test id on line: %r" % line)
ValueError: Descending test id on line: 'ok 1 - /Users/trott/io.js/benchmark/net/tcp-raw-pipe.js'
$

0.1.5 processes it normally.

Any chance we want to update to 0.1.5, at least on the linter boxes? It contains the changes for Python 3 compatibility, if that's any encouragement.

/ping @nodejs/python

Trott · 2019-12-14T06:37:42Z

(And, naturally, it seems exceedingly likely that the other problem we're seeing now with the tap file from this PR is also a bug in tap2junit 0.1.4. The file is processed without a problem by tap2junit 0.1.5.)

Trott · 2019-12-14T06:38:01Z

Any chance we want to update to 0.1.5, at least on the linter boxes? It contains the changes for Python 3 compatibility, if that's any encouragement.

@nodejs/build ^^^^^^

cclauss · 2019-12-14T06:55:53Z

StringIO.StringIO(source) in Python2 would be io.StringIO(source) in Python 3 so I would strongly encourage the use of the latest version of tap2junit.

https://github.com/nodejs/tap2junit/search?q=stringio&unscoped_q=stringio

mscdex · 2019-12-14T13:35:56Z

So we should just upgrade tap2junit instead of making the changes in this PR?

Trott · 2019-12-14T13:39:48Z

So we should just upgrade tap2junit instead of making the changes in this PR?

Perhaps. There's still the issue of cost/benefit ratio where cost is code complexity and extra time spent troubleshooting issues like this. But yeah, this PR suddenly becomes less compelling for sure.

apapirovski

Whether or not this fixes the issue, given the minimal improvements in run time and extra incurred complexity, I think removing this is actually still a good thing.

rvagg · 2019-12-20T04:33:45Z

test-softlayer-ubuntu1604-x64-1 too, it's just offline for now cause it's wonky and needs some of my time

https://ci.nodejs.org/label/jenkins-workspace/ are the machines.

BridgeAR · 2019-12-24T16:13:48Z

Ref: eslint/eslint#3565
Ref: eslint/eslint#12191

I suggest to remove this as soon as the feature landed in eslint itself but not before? It should hopefully not take much more time to be implemented.

Trott · 2020-01-27T06:06:59Z

Ref: eslint/eslint#3565
Ref: eslint/eslint#12191

I suggest to remove this as soon as the feature landed in eslint itself but not before? It should hopefully not take much more time to be implemented.

Going the other way, the CLIEngine class used by lint-js.js will be deprecated in ESLint 7.0.0 (in alpha right now). So that's an argument for doing it sooner. We can always enable the parallel linting when it becomes available in ESLint.

Trott · 2020-01-27T06:28:33Z

tap2junit updated for Python 2 on test-packetnet-ubuntu1604-x64-2. It was already on the correct version on test-packetnet-ubuntu1604-x64-1. Removing blocked label.

nodejs-github-bot · 2020-01-27T06:31:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28709/

Trott · 2020-01-27T06:44:15Z

tap2junit updated for Python 2 on test-packetnet-ubuntu1604-x64-2.

test-softlayer-ubuntu1604-x64-1 too.

nodejs-github-bot · 2020-02-01T03:01:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28832/ ✅

jasnell · 2020-06-25T21:49:29Z

Ping @Trott

lint-js.js was implemented before ESLint had a caching feature. It is now only used in CI. Let's remove it on the following grounds: * It results in occasional (and puzzling) yellow CI runs for node-test-linter because the tap file is corrupted somehow. Interleaved maybe? I don't know, but a simple solution is removing it and running ESLint directly. * On my local laptop, it reduces the linting from about 75 seconds to about 55 seconds. This kind of savings is not worth the added complexity and the instability noted above.

nodejs-github-bot · 2020-06-26T02:27:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32098/ ✔️

mhdawson

LGTM

lint-js.js was implemented before ESLint had a caching feature. It is now only used in CI. Let's remove it on the following grounds: * It results in occasional (and puzzling) yellow CI runs for node-test-linter because the tap file is corrupted somehow. Interleaved maybe? I don't know, but a simple solution is removing it and running ESLint directly. * On my local laptop, it reduces the linting from about 75 seconds to about 55 seconds. This kind of savings is not worth the added complexity and the instability noted above. PR-URL: #30955 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2020-07-03T21:57:00Z

Landed in 92f8781

lint-js.js was implemented before ESLint had a caching feature. It is now only used in CI. Let's remove it on the following grounds: * It results in occasional (and puzzling) yellow CI runs for node-test-linter because the tap file is corrupted somehow. Interleaved maybe? I don't know, but a simple solution is removing it and running ESLint directly. * On my local laptop, it reduces the linting from about 75 seconds to about 55 seconds. This kind of savings is not worth the added complexity and the instability noted above. PR-URL: #30955 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Dec 13, 2019

Trott requested review from apapirovski and addaleax December 13, 2019 23:44

richardlau requested changes Dec 13, 2019

View reviewed changes

vcbuild.bat Show resolved Hide resolved

richardlau approved these changes Dec 14, 2019

View reviewed changes

gengjiawen approved these changes Dec 14, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2019

watilde approved these changes Dec 14, 2019

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2019

This was referenced Dec 14, 2019

tap2junit fails on test input when tests time out nodejs/build#1847

Closed

freebsd couldn't convert tap to xml nodejs/build#1830

Closed

Update tap2junit from 0.1.4 to 0.1.5 nodejs/build#2092

Closed

apapirovski approved these changes Dec 14, 2019

View reviewed changes

Trott force-pushed the simplify-lint branch from b598aee to 5b4d652 Compare January 27, 2020 06:11

Trott removed the blocked PRs that are blocked by other issues or PRs. label Jan 27, 2020

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

Trott force-pushed the simplify-lint branch from 5b4d652 to bfab964 Compare June 26, 2020 02:24

Trott removed the stalled Issues and PRs that are stalled. label Jun 26, 2020

mhdawson approved these changes Jul 3, 2020

View reviewed changes

jasnell approved these changes Jul 3, 2020

View reviewed changes

jasnell closed this Jul 3, 2020

MylesBorins mentioned this pull request Jul 14, 2020

v14.6.0 proposal #34371

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

Trott deleted the simplify-lint branch April 14, 2022 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: remove lint-js.js #30955

tools: remove lint-js.js #30955

Trott commented Dec 13, 2019

nodejs-github-bot commented Dec 13, 2019

richardlau left a comment

Trott commented Dec 13, 2019

nodejs-github-bot commented Dec 14, 2019

mscdex commented Dec 14, 2019 •

edited

Loading

Trott commented Dec 14, 2019

richardlau commented Dec 14, 2019

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019 •

edited

Loading

Trott commented Dec 14, 2019 •

edited

Loading

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019

cclauss commented Dec 14, 2019 •

edited

Loading

mscdex commented Dec 14, 2019

Trott commented Dec 14, 2019

apapirovski left a comment

rvagg commented Dec 20, 2019

BridgeAR commented Dec 24, 2019

Trott commented Jan 27, 2020

Trott commented Jan 27, 2020

nodejs-github-bot commented Jan 27, 2020

Trott commented Jan 27, 2020

nodejs-github-bot commented Feb 1, 2020 •

edited by Trott

Loading

jasnell commented Jun 25, 2020

nodejs-github-bot commented Jun 26, 2020 •

edited by Trott

Loading

mhdawson left a comment

jasnell commented Jul 3, 2020

tools: remove lint-js.js #30955

tools: remove lint-js.js #30955

Conversation

Trott commented Dec 13, 2019

Checklist

nodejs-github-bot commented Dec 13, 2019

richardlau left a comment

Choose a reason for hiding this comment

Trott commented Dec 13, 2019

nodejs-github-bot commented Dec 14, 2019

mscdex commented Dec 14, 2019 • edited Loading

Trott commented Dec 14, 2019

richardlau commented Dec 14, 2019

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019 • edited Loading

Trott commented Dec 14, 2019 • edited Loading

Trott commented Dec 14, 2019

Trott commented Dec 14, 2019

cclauss commented Dec 14, 2019 • edited Loading

mscdex commented Dec 14, 2019

Trott commented Dec 14, 2019

apapirovski left a comment

Choose a reason for hiding this comment

rvagg commented Dec 20, 2019

BridgeAR commented Dec 24, 2019

Trott commented Jan 27, 2020

Trott commented Jan 27, 2020

nodejs-github-bot commented Jan 27, 2020

Trott commented Jan 27, 2020

nodejs-github-bot commented Feb 1, 2020 • edited by Trott Loading

jasnell commented Jun 25, 2020

nodejs-github-bot commented Jun 26, 2020 • edited by Trott Loading

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Jul 3, 2020

mscdex commented Dec 14, 2019 •

edited

Loading

Trott commented Dec 14, 2019 •

edited

Loading

Trott commented Dec 14, 2019 •

edited

Loading

cclauss commented Dec 14, 2019 •

edited

Loading

nodejs-github-bot commented Feb 1, 2020 •

edited by Trott

Loading

nodejs-github-bot commented Jun 26, 2020 •

edited by Trott

Loading