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: use realpath for NODE_TEST_DIR in common.js #10723

Closed
wants to merge 2 commits into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jan 10, 2017

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

@gibfahn gibfahn added the test Issues and PRs related to the tests. label Jan 10, 2017
@Fishrock123
Copy link
Contributor

@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

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
Copy link
Member Author

gibfahn commented Jan 10, 2017

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
Copy link
Member Author

gibfahn commented Jan 10, 2017

@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

cc/ @nodejs/python

@@ -1629,8 +1629,8 @@ def Main():
tempdir = os.environ.get('NODE_TEST_DIR') or options.temp_dir
if tempdir:
try:
os.makedirs(tempdir)
os.environ['NODE_TEST_DIR'] = tempdir
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, done.

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 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.
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.
@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

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

jasnell pushed a commit that referenced this pull request Jan 12, 2017
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 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.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell pushed a commit that referenced this pull request Jan 12, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member

jasnell commented Jan 12, 2017

Landed in dd2d3d3 and 57f6a10

@jasnell jasnell closed this Jan 12, 2017
@gibfahn gibfahn deleted the common-testroot branch January 12, 2017 22:18
@gibfahn
Copy link
Member Author

gibfahn commented Jan 16, 2017

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

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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 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.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
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 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.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
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 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.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
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 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.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: nodejs#10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
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 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.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
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 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.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor

landed in lts staging :D

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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 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.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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 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.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
If a temp-dir is specified and already exists, the NODE_TEST_DIR
environment variable will never be set. This fixes that.

PR-URL: #10723
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
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.

9 participants