test: use realpath for NODE_TEST_DIR in common.js

[gibfahn]

If you don't specify NODE_TEST_DIR, common.tmpDir will resolve to the
realpath of node/test.

If you do specify NODE_TEST_DIR (with the environment variable or by
running tools/test.py --test-dir=x), common.tmpDir (which
is resolved from testRoot) uses the symbolic path (doesn't resolve
symlinks). This uses fs.realpathSync() to fix that.

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

Affected core subsystem(s)

test, common

[Fishrock123]

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

[gibfahn]

To reproduce the issue:

git clone https://github.com/nodejs/node.git && cd node && ./configure && make -j8
ln -s node symnode
cd symnode
export NODE_TEST_DIR=$PWD/tmp
tools/test.py parallel/test-process-chdir

[gibfahn]

So I noticed another possible issue when running tools/test.py --temp-dir $PWD/tmp parallel/test-process-chdir.

Looking at these lines:

  tempdir = os.environ.get('NODE_TEST_DIR') or options.temp_dir
  if tempdir:
    try:
      os.makedirs(tempdir)
      os.environ['NODE_TEST_DIR'] = tempdir
    except OSError as exception:
      if exception.errno != errno.EEXIST:
        print "Could not create the temporary directory", options.temp_dir
        sys.exit(1)

If tempdir already exists, the os.makedirs(tempdir) fails with errno.EEXIST, which is ignored. However this means that it doesn't ever run the os.environ['NODE_TEST_DIR'] = tempdir line.

Fixed by swapping os.makedirs(tempdir) and os.environ['NODE_TEST_DIR'] = tempdir. This means that NODE_TEST_DIR gets set even if the makedirs fails. If makedir fails because tempdir already existed then we still want NODE_TEST_DIR, if it fails for some other reason tools/test.py will exit, so there shouldn't be a downside to this change.

[gibfahn]

CI 2: https://ci.nodejs.org/job/node-test-commit/7145/

[gibfahn]

cc/ @nodejs/python

[thefourtheye]

I would say, move this out of the try...except block.

[gibfahn]

Makes sense, done.

[gibfahn]

CI 3: https://ci.nodejs.org/job/node-test-commit/7156/

[mhdawson]

LGTM

[mhdawson]

Failure on arm looks like a machine/connection issue so CI run looks good.

[jasnell]

Landed in dd2d3d3 and 57f6a10

[gibfahn]

Would be good to get these in v4 and v6, as otherwise we get several test failures in our CI.

[MylesBorins]

landed in lts staging :D