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.xml02: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 main02: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 parse02: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 131..1ok 1 - /Users/trott/io.js/.eslintrc.jsTAP version 131..2ok 1 - /Users/trott/io.js/benchmark/net/tcp-raw-pipe.jsok 2 - /Users/trott/io.js/benchmark/net/tcp-raw-s2c.js

With tap2junit 0.1.4:

$ tap2junit -i test.tap -o fooTraceback (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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 addaleax and apapirovski December 13, 2019 23:44

richardlau requested changes Dec 13, 2019
View reviewed changes

vcbuild.bat Outdated Show resolvedHide 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 5b4d652Compare 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 2935f72Compare 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 bfab964Compare 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

Uh oh!

tools: remove lint-js.js#30955

tools: remove lint-js.js #30955

Uh oh!

Conversation

Trott commented Dec 13, 2019

Checklist

Uh oh!

nodejs-github-bot commented Dec 13, 2019

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Trott commented Dec 13, 2019

Uh oh!

nodejs-github-bot commented Dec 14, 2019

Uh oh!

mscdex commented Dec 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

richardlau commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

cclauss commented Dec 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Dec 14, 2019

Uh oh!

Trott commented Dec 14, 2019

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

rvagg commented Dec 20, 2019

Uh oh!

BridgeAR commented Dec 24, 2019

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

nodejs-github-bot commented Jan 27, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

nodejs-github-bot commented Feb 1, 2020• edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jun 25, 2020

Uh oh!

nodejs-github-bot commented Jun 26, 2020• edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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