tools: add debuglog eslint rule #32161

BridgeAR · 2020-03-09T21:20:28Z

Please have a look at the commit messages for details.

Refs: #32078

Checklist

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

Trott · 2020-03-29T20:47:54Z

@BridgeAR @jasnell For the long-term, what do you think about changing console.debug() so it only prints something when NODE_DEBUG is set? My reading of the spec is that this would be spec-compliant both in word and spirit.

jasnell · 2020-03-29T20:51:23Z

Not generally a fan of that approach given that we would lose the categorization that debuglog() provides. e.g. debuglog('test') vs debuglog('http2').

nodejs-github-bot · 2020-04-20T13:38:03Z

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

nodejs-github-bot · 2020-05-09T12:57:07Z

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

targos · 2020-05-09T13:01:54Z

14:57:47 not ok 21 - /home/iojs/build/workspace/node-test-linter/test/doctool/test-doctool-versions.js
14:57:47   ---
14:57:47   message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47   severity: error
14:57:47   data:
14:57:47     line: 32
14:57:47     column: 1
14:57:47     ruleId: no-restricted-syntax
14:57:47   messages:
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 35
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 36
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 41
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47   ...
14:57:47 not ok 12 - /home/iojs/build/workspace/node-test-linter/test/parallel/test-macos-signed-deps.js
14:57:47   ---
14:57:47   message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47   severity: error
14:57:47   data:
14:57:47     line: 24
14:57:47     column: 3
14:57:47     ruleId: no-restricted-syntax
14:57:47   messages:
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 26
14:57:47         column: 3
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 27
14:57:47         column: 3
14:57:47         ruleId: no-restricted-syntax
14:57:47   ...
14:57:47 + exit 1
14:57:47 Build step 'Execute shell' marked build as failure

test/.eslintrc.yaml

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument.

These rules only apply for the test folder and will already be checked for.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2020-05-09T23:59:52Z

@targos thanks, I updated the rule only to match variable assignments from now on.

@Trott

For the long-term, what do you think about changing console.debug() so it only prints something when NODE_DEBUG is set? My reading of the spec is that this would be spec-compliant both in word and spirit.

I think that would be fine. Using debuglog(foo) would allow a more fine grained way of debugging while console.debug() would at least follow the actual "debug" intention the console name embodies.

BridgeAR · 2020-05-10T00:01:20Z

Note: I do not run a full CI on this, since the eslint rules are already run by our github actions and there's no other change involved.

Trott · 2020-05-10T03:09:04Z

Note: I do not run a full CI on this, since the eslint rules are already run by our github actions and there's no other change involved.

To whoever lands this: Please do a make lint-js before pushing to upstream master to check that something hasn't landed between the last GitHub Actions run and now that would get flagged by this new rule. (I'm pretty sure #33329 will not violate this rule, but that's an example of something that might if it lands before this gets merged.)

targos · 2020-05-10T12:01:19Z

Landed in c252f6c...1858472

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR requested review from jasnell and Trott March 9, 2020 21:20

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 9, 2020

Trott approved these changes Mar 10, 2020

View reviewed changes

jasnell approved these changes Mar 10, 2020

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2020

trivikr approved these changes Mar 18, 2020

View reviewed changes

juanarbol approved these changes Apr 20, 2020

View reviewed changes

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2020

Trott reviewed May 9, 2020

View reviewed changes

test/.eslintrc.yaml Outdated Show resolved Hide resolved

BridgeAR added 3 commits May 10, 2020 01:37

tools: add eslint rule to only pass through 'test' to debuglog

9b74eca

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument.

tools: remove obsolete no-restricted-syntax eslint rules

39e869d

These rules only apply for the test folder and will already be checked for.

fixup! tools: add eslint rule to only pass through 'test' to debuglog

a5144ed

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR force-pushed the 2020-03-09-debuglog-eslint-rule branch from 5714228 to a5144ed Compare May 9, 2020 23:59

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2020

targos approved these changes May 10, 2020

View reviewed changes

targos closed this May 10, 2020

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add debuglog eslint rule #32161

tools: add debuglog eslint rule #32161

BridgeAR commented Mar 9, 2020

Trott commented Mar 29, 2020

jasnell commented Mar 29, 2020

nodejs-github-bot commented Apr 20, 2020

nodejs-github-bot commented May 9, 2020

targos commented May 9, 2020

BridgeAR commented May 9, 2020

BridgeAR commented May 10, 2020

Trott commented May 10, 2020 •

edited

Loading

targos commented May 10, 2020

tools: add debuglog eslint rule #32161

tools: add debuglog eslint rule #32161

Conversation

BridgeAR commented Mar 9, 2020

Checklist

Trott commented Mar 29, 2020

jasnell commented Mar 29, 2020

nodejs-github-bot commented Apr 20, 2020

nodejs-github-bot commented May 9, 2020

targos commented May 9, 2020

BridgeAR commented May 9, 2020

BridgeAR commented May 10, 2020

Trott commented May 10, 2020 • edited Loading

targos commented May 10, 2020

Trott commented May 10, 2020 •

edited

Loading