test: do not use `more` command on Windows #11953

vsemozhetbyt · 2017-03-21T01:06:58Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

Fixes: #11469

Refs: #11469 Fixes: #11469

vsemozhetbyt · 2017-03-21T01:12:42Z

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

jasnell · 2017-03-21T15:26:11Z

@nodejs/platform-windows

joaocgreis

LGTM

joaocgreis · 2017-03-21T15:58:33Z

cc @nodejs/testing

gibfahn · 2017-03-21T16:09:36Z

Is cat included by default on windows machines? I know you get it through Git Bash, which we require, but I didn't think it came with CMD by default.

vsemozhetbyt · 2017-03-21T16:28:09Z

@gibfahn I don't think cat is included. I just thought the build requirements was a safe condition for it.

joaocgreis · 2017-03-21T16:49:48Z

@gibfahn no, not by default. We already require Git Bash, tests already fail if the Unix tools are not installed, so I think it's better to make it consistent.

@vsemozhetbyt you have the same issue in Refs: and Fixes:, you can leave only the Fixes: line.

gibfahn · 2017-03-21T16:51:35Z

@vsemozhetbyt Yep, that was the context I hadn't seen.

I think it's fine to assume the developer will have cat.exe on the system due to it being included in Git Bash, the only issue is that the developer might not have the path to the Git command tools in their CMD path. I guess the answer is to document that you will need to add C:\Program Files\Git\usr\bin to your system path if it's not there already.

vsemozhetbyt · 2017-03-21T17:12:36Z

@gibfahn It seems something like that is already present in the doc:

Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

seishun · 2017-03-21T17:12:45Z

the only issue is that the developer might not have the path to the Git command tools in their CMD path

That's the same with other tests too.

mhdawson

LGTM

Trott

LGTM. Happy to see stuff being removed from the common.js monolith!

PR-URL: #11953 Fixes: #11469 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

vsemozhetbyt · 2017-03-23T03:56:58Z

Landed in 2dff3a2

PR-URL: #11953 Fixes: #11469 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins · 2017-04-18T20:20:46Z

This is causing failures for OSX testing on v6.x. did not cherry-pick. LMK if it should land

refack · 2017-04-18T20:33:27Z

post-mortem

I pro-uniformity (i.e. anti-disparity). (so I'm not suggesting we use the built in Windows type which is the equivalent to cat)
I don't have cat in my Path, also adding ...\Git\usr\bin or any other unix tools folder, as it has some non-trivial side effects.
I have no problem with special setup for building/testing node.

bottom-line

But in case changes like this are added I would like to ask for a quick failing sanity check to be added to run early to remind us if something in the environment is missing
/cc @nodejs/platform-windows

refack · 2017-04-18T20:35:38Z

Such a test will also be a defacto documentation of what the following process is dependent on.

gibfahn · 2017-04-18T21:13:54Z

also adding ...\Git\usr\bin or any other unix tools folder, as it has some non-trivial side effects.

Out of interest, what side effects are those?

I would like to ask for a quick failing sanity check to be added to run early to remind us if something in the environment is missing

A test that just runs through all the tools test.py expects makes sense to me, and not just for Windows.

benjamingr · 2017-04-18T21:22:51Z

The post-morgen tag is for post mortem analysis related issues like core dump problems.

Also, not sure why something not landing cleanly on an old version requires post-mortem analysis?

refack · 2017-04-18T21:58:53Z

The post-morgen tag is for post mortem analysis related issues like core dump problems.

Sorry. it's not documented (or I couldn't find where).

Also, not sure why something not landing cleanly on an old version requires post-mortem analysis?

Since this has already landed, I could only consider this a post-mortem for it creating a regression on Windows. After my #12493 I didn't want to tag in "regression" since it's not such a big deal.

refack · 2017-04-18T22:02:11Z

also adding ...\Git\usr\bin or any other unix tools folder, as it has some non-trivial side effects.

Out of interest, what side effects are those?

For some reason if sh.exe is in the path, make can't run properly. Also it overrides find and if you don't order your %Path% correctly, it overrides some ported tools, like curl or wget.

refack · 2017-04-18T22:17:25Z

@gibfahn Ref: the Git4Windows installer https://github.com/git-for-windows/build-extra/blob/master/installer/install.iss#L863

mscdex · 2017-04-18T23:12:20Z

FWIW the contributing guide mentions that the building guide should be read when running tests and the building guide does mention in the Windows section that basic unix tools are required for some tests and that they can be installed via Git for Windows.

refack · 2017-04-18T23:38:48Z

@mscdex aware, agree, but supporting docs with automation make for a time saver :-D

test: do not use more command on Windows

4428101

Refs: #11469 Fixes: #11469

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 21, 2017

vsemozhetbyt mentioned this pull request Mar 21, 2017

Tests that call more fail in Windows with Unicode shell codepage #11469

Closed

vsemozhetbyt added doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. labels Mar 21, 2017

joaocgreis approved these changes Mar 21, 2017

View reviewed changes

gibfahn approved these changes Mar 21, 2017

View reviewed changes

mhdawson approved these changes Mar 21, 2017

View reviewed changes

Trott approved these changes Mar 22, 2017

View reviewed changes

vsemozhetbyt closed this Mar 23, 2017

vsemozhetbyt deleted the more-to-cat branch March 23, 2017 03:57

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the dont-land-on-v6.x label Apr 18, 2017

refack added the post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. label Apr 18, 2017

gibfahn removed the post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. label Apr 18, 2017

gibfahn mentioned this pull request Jun 10, 2017

test: test for issue #5574 #5841

Closed

4 tasks

vsemozhetbyt mentioned this pull request Oct 16, 2017

test: skip process-kill-null if cat unavailable #16228

Closed

2 tasks

richardlau mentioned this pull request Aug 3, 2018

test: fix a problem that the test fails at the windows7 command prompt #22102

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: do not use `more` command on Windows #11953

test: do not use `more` command on Windows #11953

vsemozhetbyt commented Mar 21, 2017 •

edited

Loading

vsemozhetbyt commented Mar 21, 2017

jasnell commented Mar 21, 2017

joaocgreis left a comment

joaocgreis commented Mar 21, 2017

gibfahn commented Mar 21, 2017

vsemozhetbyt commented Mar 21, 2017 •

edited

Loading

joaocgreis commented Mar 21, 2017

gibfahn commented Mar 21, 2017 •

edited

Loading

vsemozhetbyt commented Mar 21, 2017

seishun commented Mar 21, 2017

mhdawson left a comment

Trott left a comment

vsemozhetbyt commented Mar 23, 2017

MylesBorins commented Apr 18, 2017

refack commented Apr 18, 2017 •

edited

Loading

refack commented Apr 18, 2017

gibfahn commented Apr 18, 2017

benjamingr commented Apr 18, 2017

refack commented Apr 18, 2017

refack commented Apr 18, 2017

refack commented Apr 18, 2017

mscdex commented Apr 18, 2017 •

edited

Loading

refack commented Apr 18, 2017

test: do not use more command on Windows #11953

test: do not use more command on Windows #11953

Conversation

vsemozhetbyt commented Mar 21, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Mar 21, 2017

jasnell commented Mar 21, 2017

joaocgreis left a comment

Choose a reason for hiding this comment

joaocgreis commented Mar 21, 2017

gibfahn commented Mar 21, 2017

vsemozhetbyt commented Mar 21, 2017 • edited Loading

joaocgreis commented Mar 21, 2017

gibfahn commented Mar 21, 2017 • edited Loading

vsemozhetbyt commented Mar 21, 2017

seishun commented Mar 21, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Mar 23, 2017

MylesBorins commented Apr 18, 2017

refack commented Apr 18, 2017 • edited Loading

post-mortem

bottom-line

refack commented Apr 18, 2017

gibfahn commented Apr 18, 2017

benjamingr commented Apr 18, 2017

refack commented Apr 18, 2017

refack commented Apr 18, 2017

refack commented Apr 18, 2017

mscdex commented Apr 18, 2017 • edited Loading

refack commented Apr 18, 2017

test: do not use `more` command on Windows #11953

test: do not use `more` command on Windows #11953

vsemozhetbyt commented Mar 21, 2017 •

edited

Loading

vsemozhetbyt commented Mar 21, 2017 •

edited

Loading

gibfahn commented Mar 21, 2017 •

edited

Loading

refack commented Apr 18, 2017 •

edited

Loading

mscdex commented Apr 18, 2017 •

edited

Loading