test: preloading common module in tests by thefourtheye · Pull Request #2836 · nodejs/node · GitHub

1st commit:

Preloading common module will make sure that we don't have to
explicitly import it in the tests unless necessary. This module
has few stuff, like global variables leakage checking, clearing
common PIPEs etc, which are necessary for most of the tests.

2nd commit:

As the test-runner preloads common module, importing it in the test
is not necessary unless its properties are used. This commit removes
all such unnecessary imports.

Related: #2828

cc: @silverwind

Oh, that's nice! I assume you need to add the common 'global' here now so the linter passes:

node/test/.eslintrc

Line 9 in de05175

globals:

Also, I'm curious if this has a visible effect on the test suite runtime.

@silverwind It just preloads, so common cannot be used unless explicitly imported and it will not be in global space.

I am not sure about the impact on the run time :(

Pretty much zero impact here:

master:

make test  247.69s user 47.25s system 218% cpu 2:14.68 total

This PR:

make test  248.15s user 46.91s system 219% cpu 2:14.73 total

It seems a bit suspicious thought that the run time is the same. Are we sure the leak check is happening when the module is just preloaded instead of required?

@silverwind Ya, I am sure it is happening. Additional changes in thefourtheye@8e191cb are because the build was failing because of the leakage. So I had to explicitly fix them.

I removed this test, as it doesn't make much sense to keep it.

What was the issue with it?

@silverwind Actually, I learnt something new when I was debugging this. When we add something to Object.prototype, it is getting added to the global as well.

> process.versions
{ http_parser: '2.5.0',
  node: '5.0.0-pre',
  v8: '4.5.103.30',
  uv: '1.7.3',
  zlib: '1.2.8',
  ares: '1.10.1-DEV',
  modules: '46',
  openssl: '1.0.2d' }
> Object.prototype.silverwind = 1;
1
> global.silverwind
1

So, the property xadsadsdasasdxx is leaked to global. But, this doesn't have any reference to why this is necessary. So I thought that it may not be necessary to keep it around.

It looks to be ancient and related to c-ares: cc9d5ab

I'm think fine with removing it.

Also, that's quite interesting, I didn't expect the global object to inherit from Object.prototype.

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

Unrelated EADDRINUSE errors on freebsd and a timeout issue on ARM for which I landed the fix now.

LGTM, maybe @Fishrock123 can also sign this off.

LGTM if it works I suppose. Is there some way to test that this is working?

cc @nodejs/build since this alters our testing a bit.

@Fishrock123 You mean something like this?

➜  io.js git:(master) ✗ echo "leak_this_into_global_space = 1;" > test.js
➜  io.js git:(master) ✗ ./node test.js 
➜  io.js git:(master) ✗ ./node -r ./test/common test.js
Unknown globals: leak_this_into_global_space

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: Unknown global found
    at process.<anonymous> (/home/thefourtheye/git/io.js/test/common.js:323:12)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)

Hmm, yeah. Maybe spawn a child process with that and assert that it throws?

@silverwind make lint complains about Arrow functions :-/

test/parallel/test-common-detect-global-leak.js
  18:31  error  Unexpected token >

Preloading `common` module will make sure that we don't have to explicitly import it in the tests unless necessary. This module has few stuff, like global variables leakage checking, clearing common PIPEs etc, which are necessary for most of the tests.

As the test-runner preloads `common` module, importing it in the test is not necessary unless its properties are used. This commit removes all such unnecessary imports.

Okay, switched to normal functions and force-pushed.

@Fishrock123 I included a test in test-common-detect-global-leak.js, as per your suggestion. Thanks :-)

I thought it would be better to have another CI run, so started one. https://ci.nodejs.org/job/node-test-pull-request/286/

Add arrowFunctions: true here in a seperate commit (might as well sort that list alphabetically).

ummmm .. +0, I guess this means we enforce common.js preloading which should be a good thing

-100. It's singularly annoying that you have to pass -r test/common.js every time you run a test standalone, doubly annoying that you have to do the same thing in tests that spawn child processes and triply annoying that it subtly drops the common.js checks from cluster workers.

@bnoordhuis All are valid points. I didn't think of any of them. Thanks for pointing out :-)

common.js contains code that checks for variables leaking into the global namespace. Load common.js in all tests that do not intentionally leak variables. PR-URL: #3095 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

thefourtheye added the test Issues and PRs related to the tests. label Sep 12, 2015

thefourtheye reviewed Sep 12, 2015
View reviewed changes

thefourtheye force-pushed the test-preload-common branch from 215bc0a to 91a5d7b Compare September 12, 2015 20:29

thefourtheye added 2 commits September 13, 2015 02:06

test: remove redundant common import

1151652

As the test-runner preloads `common` module, importing it in the test is not necessary unless its properties are used. This commit removes all such unnecessary imports.

thefourtheye force-pushed the test-preload-common branch from 91a5d7b to 1151652 Compare September 12, 2015 20:36

thefourtheye mentioned this pull request Sep 12, 2015

tools: enable arrow functions in .eslintrc #2840

Closed

thefourtheye closed this Sep 13, 2015

thefourtheye deleted the test-preload-common branch March 12, 2016 15:49

thefourtheye mentioned this pull request Jun 6, 2016

test: fix tests for non-crypto builds #7056

Closed

2 tasks

test: preloading common module in tests by thefourtheye · Pull Request #2836 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: preloading common module in tests - #2836

test: preloading common module in tests#2836thefourtheye wants to merge 2 commits intonodejs:masternodejs/node:masterfrom thefourtheye:test-preload-commonthefourtheye/io.js:test-preload-commonCopy head branch name to clipboard

Conversation

thefourtheye commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

thefourtheye Sep 12, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind Sep 12, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheye Sep 12, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind Sep 12, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind Sep 12, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

Fishrock123 commented Sep 12, 2015

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

Fishrock123 commented Sep 12, 2015

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

thefourtheye commented Sep 12, 2015

Uh oh!

silverwind commented Sep 12, 2015

Uh oh!

rvagg commented Sep 13, 2015

Uh oh!

bnoordhuis commented Sep 13, 2015

Uh oh!

thefourtheye commented Sep 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: preloading common module in tests#2836
thefourtheye wants to merge 2 commits into
nodejs:masterfrom
thefourtheye:test-preload-common