Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: allow specifying more known globals via ENV #15187

Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Sep 4, 2017

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 4, 2017
@refack refack self-assigned this Sep 4, 2017
@refack refack changed the title test: allow JetBrain leaked global test: allow JetBrains leaked global Sep 4, 2017
@refack refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 8c172a9 to d496037 Compare September 4, 2017 15:33
@@ -355,7 +355,7 @@ let knownGlobals = [
process,
setImmediate,
setInterval,
setTimeout
setTimeout,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this change is somewhat unrelated ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mscdex
Copy link
Contributor

mscdex commented Sep 4, 2017

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

@refack
Copy link
Contributor Author

refack commented Sep 4, 2017

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. 🤷‍♂️

mscdex
mscdex previously requested changes Sep 4, 2017
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it explicit

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bnoordhuis
Copy link
Member

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

@Fishrock123
Copy link
Contributor

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

@mscdex
Copy link
Contributor

mscdex commented Sep 5, 2017

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

@Fishrock123
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

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.

@bnoordhuis
Copy link
Member

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

@BridgeAR
Copy link
Member

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.

@TimothyGu
Copy link
Member

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.

@Trott
Copy link
Member

Trott commented Sep 11, 2017

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.

@addaleax
Copy link
Member

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.

@refack
Copy link
Contributor Author

refack commented Sep 11, 2017

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

@refack
Copy link
Contributor Author

refack commented Sep 11, 2017

@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

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

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more reasonable to me.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much cleaner 👍

@BridgeAR
Copy link
Member

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

@mscdex mscdex dismissed their stale review September 13, 2017 18:34

@BridgeAR it's a better solution

@mscdex
Copy link
Contributor

mscdex commented Sep 13, 2017

It still shouldn't be necessary though...

@refack
Copy link
Contributor Author

refack commented Sep 13, 2017

@refack refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 17e2716 to 21cb0ef Compare September 13, 2017 19:55
PR-URL: nodejs#15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack force-pushed the test-common-ignore-global-_jb_debug_helper branch from 21cb0ef to b8d532c Compare September 13, 2017 21:08
@refack refack merged commit b8d532c into nodejs:master Sep 13, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
PR-URL: nodejs/node#15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack deleted the test-common-ignore-global-_jb_debug_helper branch September 18, 2017 01:01
jasnell pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
PR-URL: nodejs/node#15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
PR-URL: #15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
PR-URL: #15187
Refs: https://youtrack.jetbrains.com/issue/WEB-27528
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.