Retry long path tests on Linux under real tmp dir #3929

rsp · 2015-11-19T23:22:52Z

Use os.tmpdir() on Linux if it is readable/writable
but only when the original tests fail
in test-fs-long-path.js and test-require-long-path.js
to avoid failing tests on ecryptfs filesystems
See issue #2255 and comments to PR #3925

Use os.tmpdir() on Linux if it is readable/writable but *only* when the original tests fail in test-fs-long-path.js and test-require-long-path.js to avoid failing tests on ecryptfs filesystems See issue #2255 and comments to PR #3925

rvagg · 2015-11-20T05:45:28Z

test/parallel/test-fs-long-path.js

+  if (os.type() == 'Linux') {
+    fs.accessSync(os.tmpdir(), fs.R_OK | fs.W_OK);
+    var tmpDir = path.join(os.tmpdir(),
+      'node-' + process.version + '-test-' + (Math.random() * 1e6 | 0));


use a template string to shorten this

rvagg · 2015-11-20T05:46:00Z

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

I think as long as you're basically rewriting all of this you may as well replace all the vars with const and let where appropriate

See comments to PR #3929

rsp · 2015-11-20T09:24:33Z

@rvagg Good point. I used template strings and changed vars to const in fa83454 - should a new PR with a single commit be created or is it ok as it is?

rvagg · 2015-11-20T09:49:10Z

lgtm pending https://ci.nodejs.org/job/node-test-pull-request/794/

@rsp squash the commits into one and follow the format outlined in https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit for the message.

rsp · 2015-11-20T10:05:54Z

@rvagg New PR with one commit: #3936

targos · 2015-11-20T10:07:25Z

@rsp you didn't have to create a new PR. You can force push to the same branch to update this one.

rvagg · 2015-11-20T10:13:48Z

Using git push remotename branchname --force, also if you need help squashing then have a poke around in git rebase -i HEAD~2 to see your last 2 commits on the branch, the -i enters into an interactive mode where you can choose from a bunch of handy options to rearrange and edit your commits. So normally you should only need a single PR and you just mess with your branch.

Another less elegant way to do this is to delete or rename your local copy of this branch, make a new one and do what you need on it and use --force to push it your github fork and it'll end up here.

rsp · 2015-11-20T10:26:52Z

@targos @rvagg Thanks for the advice, I thought that I should avoid force pushing to public repos but if that's ok for you then I'll do that in the future. Sorry for causing a mess and thanks for your help and patience, this is my first contribution to Node.

rvagg · 2015-11-20T13:13:51Z

No problems @rsp, you'd actually be force pushing to your own fork so that doesn't create any hassles on this end. Thanks for the contribution and presuming this lands without objection in a day or two, welcome to the club of contributors, hopefully we'll be seeing more from you!

rsp · 2015-11-20T21:37:56Z

@rvagg My pleasure, thanks for your help. I hope I'll be able to contribute something useful. By the way, those two tests that I was working with were just throwing exceptions and printing long stack traces so I had to find the relevant lines in source code to see what's going on. I kept it that way but maybe it would be useful to print a more meaningful message instead of the stack trace in those two or other similar tests? I don't know if there is any guide for writing good tests that describes how a good output should look like, I haven't found it in CONTRIBUTING.md or COLLABORATOR_GUIDE.md. Maybe there is some room for improvement to make tests print few lines of relevant info instead of stack traces? If so, I'll be happy to help with that.

jbergstroem · 2015-11-20T21:45:43Z

An alternative to this would be changing path to tmpdir (#3325) but having a default state of "Just Working" is nice. LGTM.

jbergstroem · 2015-11-20T21:56:34Z

test/parallel/test-fs-long-path.js

+  testFsLongPath(common.tmpDir);
+  common.refreshTmpDir();
+} catch (e) {
+  if (os.type() == 'Linux') {


perhaps use process.platform == 'linux' instead?

@jbergstroem Yes, it would be the same. This code already uses the os module for other things so it seemed to make sense to use the normalized name for the comparison but it could test process.platform or os.platform() just as well as os.type(). I can change it if that's what's holding the PR #3936 back.

process.platform is a little more idiomatic, at least w.r.t. to the test suite.

@bnoordhuis @jbergstroem Done. It's using process.platform now. See: 2f5ca5d

rsp · 2015-11-23T17:04:15Z

@jbergstroem changing path to tmpdir would be perfect - much better than what I did here. This was actually the first thing that I tried but changing exports.tmpDir in test/common.js broke so many tests that I gave up and used a solution that changes the behavior of just two tests without messing with something like 138 places where the tmpDir is used.

But I agree that optionally changing the temp dir for all tests would be much better and breaking so many tests by changing it is probably a sign of a more serious problem (having tmp.0, tmp.1, tmp.2, tmp.3, tmp.4, tmp.5, tmp.6, tmp.7 directories left in test after make test may be a clue on why is that but that may not be the only problem).

jbergstroem · 2015-11-24T10:07:50Z

@rsp I have a few fixes I will push to PR in a day or three. Feedback appreciated!

rsp · 2015-11-24T10:42:17Z

@jbergstroem Great, thanks. By the way, I created a new PR #3936 - see comments above. It is the same as this one but with one commit as suggested in the comments here. (I didn't know that the practice here is to force push squashed commits to public PR branches, so I thought I need to create a new PR with squashed commits to avoid losing diff comments like yours.)

jbergstroem · 2015-11-24T13:41:49Z

@rsp no need to squash until its ready for merge.

rsp · 2015-11-25T07:19:41Z

@jbergstroem OK, maybe I misunderstood the previous comments. This is my first contribution here so I'm still learning how things work here. Thanks for your help.

rsp · 2015-12-01T10:00:16Z

@jbergstroem You said about fixes to this PR that you want to push a week ago. If you don't have time then let me know what needs to be fixed and I'll change it. Thanks.

bnoordhuis · 2015-12-01T15:46:25Z

I'm not sure if I'm okay with adding hacks for something as niche as ecryptfs, the more so because the workaround is quite involved.

rsp · 2015-12-01T19:02:41Z

@bnoordhuis This test is made for Windows. You don't really have to test long path support on Linux to see if the long path support is working correctly in Node - you only have long/short path issues on Windows. The problem is that those two tests while reasonable on Windows fail on default Ubuntu installations that makes the entire Node look like it hasn't compiled correctly and if you run make && make test && make install it fails. In my opinion make test shouldn't fail when the compilation was successful but happened in the "wrong" directory, but if you think it should be solved differently then please let me know. Also @jbergstroem has some fixes for this PR but I don't know what those are yet.

bnoordhuis · 2015-12-01T19:36:58Z

Wouldn't it make more sense in that case to skip the test when process.platform !== 'win32'?

jbergstroem · 2015-12-01T19:49:58Z

@rsp my fixes aren't for this PR, rather for the one I linked (#3325).

rsp · 2015-12-01T20:33:02Z

@bnoordhuis Maybe but I thought that running the test would be more preferable than skipping it, to make sure that it is indeed just the wrong place to run it instead of lacking the required functionality. What I did is to run it in real system tmp if it fails in $PWD but as always there's more than one way to do it. If there is consensus on how it should be done differently then I will be happy to change it but I thought @rvagg and @targos were ok with how it is done right now so I don't want to skip the test entirely without hearing their opinion first.

rsp · 2015-12-01T20:38:32Z

@jbergstroem Sorry, I misunderstood you. I hope your PR gets support and the system temp dir will be the default temp path for Node tests.

jbergstroem · 2015-12-01T20:47:19Z

@rsp fyi I won't change defaults - there'll probably be too many problems with that (esp cross-filesystem issues).

rsp · 2015-12-01T21:12:46Z

@jbergstroem Oh, I see. So unfortunately we would still need this PR (or skipping those two tests entirely on Linux as suggested by @bnoordhuis) to have the tests work on Ubuntu.

Personally I can live with those tests failing because I can always inspect the stack trace every time I run make test to see if only those two tests failed or maybe some other ones that are important but for some people "make test fails" == "software is broken" and it makes Node look broken, which is unfortunate.

jbergstroem · 2015-12-02T05:38:02Z

@rsp; not really -- you'd pass NODE_TEST_DIR=/tmp/foo make check

jasnell · 2016-03-22T05:22:13Z

Any updates on this one?

rsp · 2016-03-22T16:46:51Z

@jasnell The issue #2255 was closed by pr #4116 which doesn't really fix those tests but skips them instead (personally I'd prefer running the corrected tests that work fine instead of skipping them but at least now make test works on Ubuntu by default so it's better than nothing) so I think I can close this pr. Thanks for the reminder. Please let me know if I should add/remove any labels as well.

Retry long path tests on Linux under real tmp dir

7c76592

Use os.tmpdir() on Linux if it is readable/writable but *only* when the original tests fail in test-fs-long-path.js and test-require-long-path.js to avoid failing tests on ecryptfs filesystems See issue #2255 and comments to PR #3925

rsp mentioned this pull request Nov 19, 2015

Use real OS tmp dir on Linux for long path tests #3925

Closed

mscdex added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. labels Nov 19, 2015

rvagg reviewed Nov 20, 2015
View reviewed changes

Use template strings and const instead of var

fa83454

See comments to PR #3929

rsp mentioned this pull request Nov 20, 2015

test: fix long path tests to avoid issue #2255 #3936

Closed

jbergstroem reviewed Nov 20, 2015
View reviewed changes

jasnell added the lts-watch-v4.x label Nov 20, 2015

Use process.platform instead of os.type()

2f5ca5d

This was referenced Dec 2, 2015

Skip the long path tests when not on Windows #4116

Closed

Should there be a Testing WG? #3872

Closed

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016

rsp closed this Mar 22, 2016

jasnell removed the lts-watch-v4.x label Mar 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry long path tests on Linux under real tmp dir #3929

Retry long path tests on Linux under real tmp dir #3929

rsp commented Nov 19, 2015

rvagg Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

targos commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

jbergstroem commented Nov 20, 2015

jbergstroem Nov 20, 2015

rsp Nov 23, 2015

bnoordhuis Dec 1, 2015

rsp Dec 1, 2015

rsp commented Nov 23, 2015

jbergstroem commented Nov 24, 2015

rsp commented Nov 24, 2015

jbergstroem commented Nov 24, 2015

rsp commented Nov 25, 2015

rsp commented Dec 1, 2015

bnoordhuis commented Dec 1, 2015

rsp commented Dec 1, 2015

bnoordhuis commented Dec 1, 2015

jbergstroem commented Dec 1, 2015

rsp commented Dec 1, 2015

rsp commented Dec 1, 2015

jbergstroem commented Dec 1, 2015

rsp commented Dec 1, 2015

jbergstroem commented Dec 2, 2015

jasnell commented Mar 22, 2016

rsp commented Mar 22, 2016

Retry long path tests on Linux under real tmp dir #3929

Retry long path tests on Linux under real tmp dir #3929

Conversation

rsp commented Nov 19, 2015

rvagg Nov 20, 2015

Choose a reason for hiding this comment

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

targos commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

rvagg commented Nov 20, 2015

rsp commented Nov 20, 2015

jbergstroem commented Nov 20, 2015

jbergstroem Nov 20, 2015

Choose a reason for hiding this comment

rsp Nov 23, 2015

Choose a reason for hiding this comment

bnoordhuis Dec 1, 2015

Choose a reason for hiding this comment

rsp Dec 1, 2015

Choose a reason for hiding this comment

rsp commented Nov 23, 2015

jbergstroem commented Nov 24, 2015

rsp commented Nov 24, 2015

jbergstroem commented Nov 24, 2015

rsp commented Nov 25, 2015

rsp commented Dec 1, 2015

bnoordhuis commented Dec 1, 2015

rsp commented Dec 1, 2015

bnoordhuis commented Dec 1, 2015

jbergstroem commented Dec 1, 2015

rsp commented Dec 1, 2015

rsp commented Dec 1, 2015

jbergstroem commented Dec 1, 2015

rsp commented Dec 1, 2015

jbergstroem commented Dec 2, 2015

jasnell commented Mar 22, 2016

rsp commented Mar 22, 2016