-
Notifications
You must be signed in to change notification settings - Fork 7.3k
test: Environment varible to specify directory for pipes #9381
Conversation
@@ -35,7 +35,11 @@ if (process.platform === 'win32') { | |||
exports.PIPE = '\\\\.\\pipe\\libuv-test'; | |||
exports.opensslCli += '.exe'; | |||
} else { | |||
exports.PIPE = exports.tmpDir + '/test.sock'; | |||
if (typeof process.env.NODE_PIPE_DIR == 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace the typeof
checks with direct comparisons to undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ===
.
@mdawsonibm As discussed during the core team meeting, please add this to the 0.12.1 or 0.12.2 milestone as you see fit. |
Updated per comments and all tests pass on linux with/without NODE_PIPE_DIR in a shallow directory. In a deep directory all pass with NODE_PIPE_DIR and without some fail as expected. |
@@ -53,11 +53,15 @@ def AfterRun(self, result): | |||
# delete the whole tmp dir | |||
try: | |||
rmtree(self.tmpdir) | |||
if "NODE_PIPE_DIR" in os.environ: | |||
rmtree(join(os.environ["NODE_PIPE_DIR"], 'NodePipeTmp')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the repetition of join(os.environ["NODE_PIPE_DIR"], 'NodePipeTmp')
by having a self.pipeTmpDir
member variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
@mdawsonibm Once this comment: https://github.com/joyent/node/pull/9381/files#r26866770 is addressed, LGTM 👍 Thank you very much! |
When I ran the accept job test-cluster-eaccess failed on windows 32bit. I think its because what's used for common.PIPE ('\.\pipe\libuv-test') is special and can't be used by fs.writeFileSync and we end up with this error: #Error: ENOENT, no such file or directory '.\pipe\libuv-test' It seems like test already special cases for windows 32 in that it expects a different error. I think it makes the most sense to special case what we use for socketPatch for windows. In this way we share the common solution to the depth paths on non-windows were we say the original problem while keeping the test on windows running in the way it did perviously |
Pushed additional change to address failure seen on windows |
// for windows pipes are handled specially and we can't write to | ||
// common.PIPE with fs.writeFileSync like we can on other platforms | ||
socketPath = path.join(common.fixturesDir, 'socket-path'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this test was not failing before when run in a deep directory and we can't use common.PIPE
on all platforms, then I suggest reverting to the old behavior of using path.join(common.fixturesDir, 'socket-path');
on all platforms. Sorry for the confusion.
Added two comments, can you please take a look and let me know what you think? Also, we'll need to squash these commits into just one before we merge them with the node-accept-pull-request Jenkins job. |
It was failing on unix due to the deep paths, that is why I modified it in the first patch. On windows it would have been ok because its not actually a pipe but a directory that's being used. |
@mdawsonibm OK, thank you for the clarification. Did you have some time to take a look at this comment: #9381 (comment)? |
Sorry did not look far up enough to see that one. Will update and squash into one commit. |
Ok updated to reflect that comment and squashed into 1 commit |
Thank you @mdawsonibm! The first line of the commit should be 50 columns or less, as indicated in the contribution guidelines. Otherwise, LGTM 👍 |
At the uv layer pipes are connected with uv_pipe_connect. The current spec for this method indicates that the maximum length is limited to the size of length of sizeof(sockaddr_un.sun_path), typically between 92 and 108 bytes. Anything longer than that just gets truncated. The simple testsuite currently creates pipes in directories under the directory where node was built. In our jenkins jobs this sometimes ends up being a deep enough path that the path for the pipes is getting truncated. The result is that tests using pipes fail with errors that don't make it obvious what the problem is. Even if the errors were helpful, we still need a way to avoid the truncation. This patch adds the environment variable NODE_PIPE_DIR. If set the tests create pipes in this directory instead of the current defaults. In addition the test harness is updated to remove/delete this directory before/after each test is run. modified: test/common.js modified: test/simple/test-net-pipe-connect-errors.js modified: test/testpy/__init__.py modified: test/simple/test-cluster-eaccess.js
Ok shrunk length of first line, will land now |
At the uv layer pipes are connected with uv_pipe_connect. The current spec for this method indicates that the maximum length is limited to the size of length of sizeof(sockaddr_un.sun_path), typically between 92 and 108 bytes. Anything longer than that just gets truncated. The simple testsuite currently creates pipes in directories under the directory where node was built. In our jenkins jobs this sometimes ends up being a deep enough path that the path for the pipes is getting truncated. The result is that tests using pipes fail with errors that don't make it obvious what the problem is. Even if the errors were helpful, we still need a way to avoid the truncation. This patch adds the environment variable NODE_PIPE_DIR. If set the tests create pipes in this directory instead of the current defaults. In addition the test harness is updated to remove/delete this directory before/after each test is run. modified: test/common.js modified: test/simple/test-net-pipe-connect-errors.js modified: test/testpy/__init__.py modified: test/simple/test-cluster-eaccess.js Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PR-URL: #9381
landed as 5dd5ce7 |
At the uv layer pipes are connected with uv_pipe_connect.
The current spec for this method indicates that the maximum
length is limited to the size of length of
sizeof(sockaddr_un.sun_path), typically between 92 and
108 bytes. Anything longer than that just gets truncated.
The simple testsuite currently creates pipes in directories
under the directory where node was built. In our jenkins
jobs this sometimes ends up being a deep enough path that
the path for the pipes is getting truncated. The result
is that tests using pipes fail with errors that don't
make it obvious what the problem is.
Even if the errors were helpful, we still need a way
to avoid the truncation.
This patch adds the environment variable NODE_PIPE_DIR.
If set the tests create pipes in this directory instead of
the current defaults. In addition the test harness is
updated to remove/delete this directory before/after
each test is run.