test: preloading common module in tests

[thefourtheye]

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

[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:

[silverwind]

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

[thefourtheye]

@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 :(

[silverwind]

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

[silverwind]

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?

[thefourtheye]

@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.

[thefourtheye]

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

[silverwind]

What was the issue with it?

[thefourtheye]

@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.

[silverwind]

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

I'm think fine with removing it.

[silverwind]

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

[thefourtheye]

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

[silverwind]

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.

[Fishrock123]

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.

[thefourtheye]

@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)

[Fishrock123]

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

[thefourtheye]

@silverwind make lint complains about Arrow functions :-/

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

[thefourtheye]

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/

[silverwind]

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

[rvagg]

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

[bnoordhuis]

-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.

[thefourtheye]

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