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

Skip the long path tests when not on Windows #4116

Closed
wants to merge 1 commit into from
Closed

Skip the long path tests when not on Windows #4116

wants to merge 1 commit into from

Conversation

rsp
Copy link
Contributor

@rsp rsp commented Dec 2, 2015

This is another attempt to fix the tests failing on Ubuntu (see issue #2255)
This time by skipping the tests when not on Windows (as advised by @bnoordhuis)

Other PRs that solve the same issue:

See also comments to PRs #3925 and #3929 for more context.

@@ -1,4 +1,7 @@
'use strict';

if (process.platform !== 'win32') process.exit();
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the TAP-compatible approach we use e.g. here?

(I forgot about common.isWindows, it's fairly new.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the test skipping in 5502b9b - is that what you mean?

@bnoordhuis
Copy link
Member

LGTM, thanks. Can you squash and write a commit log in the style that CONTRIBUTING.md wants?

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

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Dec 2, 2015
If not running on Windows it skips the long path tests in:
* test-fs-long-path.js
* test-require-long-path.js

It fixes issue #2255 that is currently: wontfix

For context see comments to PRs:
#3925
#3929
#4116
@rsp
Copy link
Contributor Author

rsp commented Dec 2, 2015

@bnoordhuis I rebased it on fresh master, squashed into e07b145 with new message and force pushed here. I can't access the CI link ("rsp is missing the Overall/Read permission") so I don't know if it tests the forced update automatically or not. Thanks for your help.

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

LGTM

@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

As @rsp updated, new CI : https://ci.nodejs.org/job/node-test-pull-request/932/

@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

LGTM if new CI is happy :)

bnoordhuis pushed a commit that referenced this pull request Dec 7, 2015
If not running on Windows it skips the long path tests in:

* test-fs-long-path.js
* test-require-long-path.js

Fixes: #2255
PR-URL: #4116
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@bnoordhuis
Copy link
Member

Thanks, landed in 19e06d7.

@bnoordhuis bnoordhuis closed this Dec 7, 2015
rvagg pushed a commit that referenced this pull request Dec 8, 2015
If not running on Windows it skips the long path tests in:

* test-fs-long-path.js
* test-require-long-path.js

Fixes: #2255
PR-URL: #4116
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@rsp
Copy link
Contributor Author

rsp commented Dec 8, 2015

@jasnell @JungMinu @bnoordhuis @rvagg Thanks. My pleasure. I think the wontfix label can be removed from issue #2255 now.

@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
If not running on Windows it skips the long path tests in:

* test-fs-long-path.js
* test-require-long-path.js

Fixes: #2255
PR-URL: #4116
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
If not running on Windows it skips the long path tests in:

* test-fs-long-path.js
* test-require-long-path.js

Fixes: #2255
PR-URL: #4116
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
If not running on Windows it skips the long path tests in:

* test-fs-long-path.js
* test-require-long-path.js

Fixes: nodejs#2255
PR-URL: nodejs#4116
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants