test: using common.isWindows consistently #2269
Conversation
thefourtheye
force-pushed
the
isWindows-consistent-fix
branch
3 times, most recently
from
abd0432
to
0c6c1c4
Compare
|
LGTM if the CI is happy. You might want to change some of the |
|
Rubber-stamp LGTM if CI is happy. Maybe change to commit line to something like "test: use common.isWindows() check everywhere" (or "consistently"), I think that explains it just a little bit better. |
|
@cjihrig Actually I am working on another PR which will replace all the @bnoordhuis Sure, I ll change it when I am landing it. |
| @@ -4,7 +4,7 @@ var assert = require('assert'); | |||
|
|
|||
| var spawn = require('child_process').spawn; | |||
|
|
|||
| var is_windows = process.platform === 'win32'; | |||
| var is_windows = common.isWindows; | |||
There was a problem hiding this comment.
Can we just use common.isWindows on line 28?
|
Mostly LGTM |
|
@Fishrock123 You want to replace the variables with |
|
@thefourtheye might as well. |
thefourtheye
force-pushed
the
isWindows-consistent-fix
branch
2 times, most recently
from
89422b1
to
0dba4ed
Compare
thefourtheye
force-pushed
the
isWindows-consistent-fix
branch
from
0dba4ed
to
dcb2c68
Compare
|
@Fishrock123 I did that change and I had to rebase, so that I can pull in #2265. @bnoordhuis I fixed the commit line, PTAL. CI run 2: Edit: Sorry, I pushed few more files. Restarting the CI and the latest is at iojs+any-pr+multi/215 |
thefourtheye
force-pushed
the
isWindows-consistent-fix
branch
from
dcb2c68
to
9d2cfcf
Compare
thefourtheye
changed the title
|
I'd s/using/use/, that's the prevalent tense in commit messages. Ideally, you'd add one or two lines describing the what and why of the change but in this case it's pretty self-explanatory. |
|
@bnoordhuis ah, your original comment had use only. My bad. I ll fix it while landing :-) |
|
CI is almost green and about the SmartOS failure, @targos and @bnoordhuis are working on it in #2220 @cjihrig Oh sorry, I totally misunderstood what you were saying. BTW, as per Fishrock123's comments, I removed all those variables. Does it look fine to you now? cc @Fishrock123 |
|
Still LGTM |
| @@ -39,7 +39,7 @@ var dgram = require('dgram'); | |||
| // supported while using cluster, though servers still cause the master to error | |||
| // with ENOTSUP. | |||
|
|
|||
| var windows = process.platform === 'win32'; | |||
| var windows = common.isWindows; | |||
There was a problem hiding this comment.
Might as well also do it here then?
There was a problem hiding this comment.
And then I thought I covered everything :D
|
LGTM. You didn't have to change the ones that were used a bunch of times in the file, but, either way |
|
CI run 3: iojs+any-pr+multi/216/ I'll land this, if it turns green. |
In the tests, we use "process.platform === 'win32'" in some places. This patch replaces them with the "common.isWindows" for consistency. PR-URL: #2269 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
|
Thanks people. Landed at d5ab92b |
No description provided.