nodejs/node-convergence-archive

[Converge] test: Env variable to specify directory for pipes

Closed this issue · 7 comments

jasnell commented

See nodejs/node-v0.x-archive@5dd5ce7
/cc: @mhdawson

Original commit message:

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
Fishrock123 commented

Interesting, @mhdawson do you remember which tests failed prior to this?

bnoordhuis commented

io.js has NODE_COMMON_PIPE for the same reason, see nodejs/node@05daf5f7 and nodejs/node@9fb7aee7.

jasnell commented

Ok, so similar change has already landed in io.js. @mhdawson, can you take a look and confirm if the io.js commits address the original issue? Also, do you see any reason to still port this for backwards compatibility with joyent/node?

Fishrock123 commented

Ping @mhdawson (does he get these notifications?)

mhdawson commented

I do get the notifications but I have so much email I don't manage to read it all in a timely manner sorry for the late reply.

I've used the equivalent change in my builds of io.js next on pLinux and it looks fine to me. I think we can assume that the io.js commits address the original issue.

In addition to the base change there were 2 tests changed by this PR but I think one was to make code more consistent and in the other case the test has changed such modification in this PR no longer applies.

Fishrock123 commented

Ok, sounds like this has already been solved then!