test: add test for process.stdin.setRawMode() #10037

jmdarling · 2016-12-01T17:53:37Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

adds a basic test for process.stdin.setRawMode to ensure that the isRaw
property is properly changed

adds a basic test for process.stdin.setRawMode to ensure that the isRaw property is properly changed

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2016-12-01T21:00:17Z

test/pseudo-tty/stdin-setrawmode.js

+assert(process.stdin.isRaw);
+
+process.stdin.setRawMode(false);
+assert(!process.stdin.isRaw);


Perhaps you can use assert.strictEqual instead of plain assert, that would catch truthy-but-not-boolean values.

Replaces usages of plain assert() with assert.strictEqual() to prevent issues with "truthiness" or "falsiness".

bnoordhuis

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/5075/ (currently queued)

adds a basic test for process.stdin.setRawMode to ensure that the isRaw property is properly changed PR-URL: #10037 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-12-05T22:25:38Z

Landed in 3ae0900. Thank you for the PR and for participating in the code-and-learn!

Fishrock123 · 2016-12-06T14:57:13Z

This test doesn't exactly function correctly:

Could not find /Users/Jeremiah/Documents/node/test/pseudo-tty/stdin-setrawmode.out

You need an .out file too for pseudo-tty tests, I'm not sure if the test is actually run or not in this situation.

We should probably update the message and pseudo-tty test runners to error without an .out file, I am not familiar enough with python to do that in any speedy manner though.

jmdarling · 2016-12-06T16:18:53Z

Well that's no good. My apologies. When I ran the test individually and with the test suite on my local I didn't see any problems, but obviously I didn't look closely enough. Now that I'm looking more closely I see the same thing you're seeing.

Unfortunately, I am quite uninformed on how these pseuty-tty tests work, and am not seeing any documentation. Am I just missing it?

Judging by the other tests in that directory, since I am not writing anything to stdout, can I just create an empty .out file (just a single newline)? Doing this seems to get rid of that error but I'm not sure if it will cause other issues.

addaleax · 2016-12-06T16:19:39Z

Judging by the other tests in that directory, since I am not writing anything to stdout, can I just create an empty .out file (just a single newline)?

Yeah that should be fine

Adds an accompanying .out file for test/pseudo-tty/stdin-setrawmode.js. The test was originally merged without this file and an astute observer found that it was causing an error message. See discussion at nodejs#10037.

jmdarling · 2016-12-06T16:27:47Z

I have opened a PR to add the .out file: #10146

Instead of ignoring missing `.out` files for message/pseudo-tty tests, raise an error to indicate that something is not quite right. Ref: nodejs#10037

adds a basic test for process.stdin.setRawMode to ensure that the isRaw property is properly changed PR-URL: #10037 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of ignoring missing `.out` files for message/pseudo-tty tests, raise an error to indicate that something is not quite right. Ref: #10037 PR-URL: #10150 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Partially taken from https://linux.die.net/man/3/cfmakeraw A very simple test script: ``` if (process.argv[2] === 'raw') process.stdin.setRawMode(true) process.stdin.on('data', (chunk) => { console.log(chunk) console.log(chunk.toString()) }) ``` Refs: #10037 PR-URL: #10147 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Ref: #10037 Ref: #10146 PR-URL: #10149 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Adds an accompanying .out file for test/pseudo-tty/stdin-setrawmode.js. The test was originally merged without this file and an astute observer found that it was causing an error message. See discussion at #10037. PR-URL: #10149 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>