test: allow specifying more known globals via ENV by refack · Pull Request #15187 · nodejs/node · GitHub

update
In order to provide a smoother test dev experience, with less false positives, we should allow specifying more known global symbols. This implementation uses an ENV var for said specification.

Example

end update

When debugging /test/ using JetBrains' debugger sometimes it leaked a global symbol _jb_debug_helper:

C:\bin\dev\node\node.exe --inspect-brk=58577 D:\code\node\test\parallel\test-setproctitle.js
Debugger listening on ws://127.0.0.1:58577/bd9a5439-00dd-4040-8f4d-a9c8f983834e
For help see https://nodejs.org/en/docs/inspector
Debugger attached.
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Unexpected global(s) found: _jb_debug_helper
    at process.<anonymous> (D:\code\node\test\common\index.js:453:12)
    at emitOne (events.js:120:20)
    at process.emit (events.js:210:7)
Waiting for the debugger to disconnect...

Process finished with exit code 1

A bug report was opened upstream for said IDE
Refs: https://youtrack.jetbrains.com/issue/WEB-27528

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

btw, this change is somewhat unrelated ;)

Yes, I thought for a second I could add it here, but it's not the right place.
Reverted the comma.

I don't think we should make it a habit of adding miscellaneous globals from IDEs.

I don't understand the need for this for running the test suite. To me, this seems like something JetBrains should fix, not us.

I don't think we should make it a habit of adding miscellaneous globals from IDEs.

I don't understand the need for this for running the test suite. To me, this seems like something JetBrains should fix, not us.

I added a reference to the Issue I opened for JetBrains, meanwhile it's a nuisance for devs who want to debug the tests with a JetBrains tool.
On some level this could be equated with excluding compiler warnings, or adding a known kruft pattern to .gitignore, i.e. it does not directly relevant to the runtime but has the benefit of making devs' life a little bit easier. 🤷‍♂️

Making it explicit

I think it is a reasonable exception in order to make a dev's life easier and we shouldn't be purist about it -it's helpful and there is no harm.

Given how unlikely it is to have a legitimate _jb_debug_helper global leaking from our code, I'd say LGTM.

I'm with Brian and Colin: if your tool of choice is broken, fix it or use a better one, don't add hacks.

I don't think we can guarantee inspectors won't inject things. Perhaps we could have a way to force code tampering off?

@Fishrock123 If you do that, then you won't know when non-IDE globals start leaking...

It is my understanding that the global leak checks do not use the inspector, (or at the very least they pre-date the inspector)?

Another possible alternative solution to just saying no would be to introduce an environment variable that test/common pays attention to, allowing globals like this to be set. There would be zero impact on the default case and CI would not be impacted.

I won't strenuously object but that's still adding complexity to work around a broken tool.

What is the conclusion here? We have a couple of persons pro and a couple of persons against it. I personally have no strong opinion about this but I am somewhat with @cjihrig etc that this should be fixed in JetBrains and not in Node.

Even though I'm in favor of this PR, I think it should be closed given the number of 👎's this is getting. My two cents.

Even though I'm in favor of this PR, I think it should be closed given the number of 👎's this is getting. My two cents.

Yeah, if I'm not mistaken, there's only two ways this could land:

Convince all the people with objections that it's actually a good idea (or that some similar implementation is a good idea). Seems very unlikely.
Escalate to the TSC for a decision.

I’d leave that decision to @refack. I’m not terribly happy about this patch either, but if @refack says it makes his working on Node easier, then I think that’s worth 6 more lines of code.

IMHO It's not worth escalating.
I'll switch implementation to an environment variable in place of the hardcoded exception and ask again.

@mscdex and other objectors PTAL
I switched impl to be a configurable env var:

if (process.env.NODE_TEST_KNOWN_GLOBALS) {
  const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
  allowGlobals(...knownFromEnv);
}

/cc @ulitink @segrey

Still LGTM, but now Node.js is not aware of hard coded debugger values - so that's nice.

This seems more reasonable to me.

This is much cleaner 👍

@mscdex are you still against this with the current change?

@BridgeAR it's a better solution

It still shouldn't be necessary though...

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

PR-URL: nodejs#15187 Refs: https://youtrack.jetbrains.com/issue/WEB-27528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15187 Refs: https://youtrack.jetbrains.com/issue/WEB-27528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #15187 Refs: https://youtrack.jetbrains.com/issue/WEB-27528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15187 Refs: https://youtrack.jetbrains.com/issue/WEB-27528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #15187 Refs: https://youtrack.jetbrains.com/issue/WEB-27528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 4, 2017

refack self-assigned this Sep 4, 2017

refack changed the title test: allow JetBrain leaked global test: allow JetBrains leaked global Sep 4, 2017

refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 8c172a9 to d496037 Compare September 4, 2017 15:33

addaleax approved these changes Sep 4, 2017

View reviewed changes

addaleax reviewed Sep 4, 2017

View reviewed changes

refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from d496037 to 6baa612 Compare September 4, 2017 15:36

cjihrig reviewed Sep 4, 2017

View reviewed changes

mscdex previously requested changes Sep 4, 2017

View reviewed changes

benjamingr approved these changes Sep 4, 2017

View reviewed changes

TimothyGu approved these changes Sep 4, 2017

View reviewed changes

refack changed the title test: allow JetBrains leaked global test: allow specifying more known globals via ENV Sep 11, 2017

cjihrig approved these changes Sep 11, 2017

View reviewed changes

BridgeAR approved these changes Sep 11, 2017

View reviewed changes

refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 17e2716 to 21cb0ef Compare September 13, 2017 19:55

refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 21cb0ef to b8d532c Compare September 13, 2017 21:08

refack merged commit b8d532c into nodejs:master Sep 13, 2017

refack deleted the test-common-ignore-global-_jb_debug_helper branch September 18, 2017 01:01

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

refack removed their assignment Oct 12, 2018

test: allow specifying more known globals via ENV by refack · Pull Request #15187 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: allow specifying more known globals via ENV - #15187

test: allow specifying more known globals via ENV#15187refack merged 1 commit intonodejs:masternodejs/node:masterfrom refack:test-common-ignore-global-_jb_debug_helperrefack/node:test-common-ignore-global-_jb_debug_helperCopy head branch name to clipboard

Conversation

refack commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Checklist

Affected core subsystem(s)

Uh oh!

addaleax Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

refack Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 4, 2017

Uh oh!

refack commented Sep 4, 2017

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 5, 2017

Uh oh!

Fishrock123 commented Sep 5, 2017

Uh oh!

mscdex commented Sep 5, 2017

Uh oh!

Fishrock123 commented Sep 5, 2017

Uh oh!

jasnell commented Sep 7, 2017

Uh oh!

bnoordhuis commented Sep 7, 2017

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

TimothyGu commented Sep 11, 2017

Uh oh!

Trott commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 11, 2017

Uh oh!

refack commented Sep 11, 2017

Uh oh!

refack commented Sep 11, 2017

Uh oh!

benjamingr commented Sep 11, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

mscdex commented Sep 13, 2017

Uh oh!

refack commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

test: allow specifying more known globals via ENV#15187
refack merged 1 commit into
nodejs:masterfrom
refack:test-common-ignore-global-_jb_debug_helper

refack commented Sep 4, 2017 •
edited
Loading

benjamingr left a comment •
edited
Loading

Trott commented Sep 11, 2017 •
edited
Loading