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

test: stdin is not always a net.Socket #5935

Merged

Conversation

Fishrock123
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

test

Description of change

Refs: #5916 (comment)

<-ing a file into stdin actually results in a fs.ReadStream, rather an a tty.ReadStream, and as such does not inherit from net.Socket, unlike the other possible stdin options:

case 'FILE':
var fs = require('fs');
stdin = new fs.ReadStream(null, { fd: fd, autoClose: false });
break;

cc @Trott I think this should cover the known issue.

@Fishrock123 Fishrock123 added test Issues and PRs related to the tests. process Issues and PRs related to the process subsystem. labels Mar 28, 2016
const spawn = require('child_process').spawn;

if (process.argv[2] === 'child') {
// Refs: https://github.com/nodejs/node/pull/5916
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this comment up to line 2.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 28, 2016

LGTM with a couple comments.

@Fishrock123
Copy link
Contributor Author

Hmmm, doing this reliably in a must-fail environment is quite tricky.

@Fishrock123 Fishrock123 force-pushed the known-issue-stdin-not-net.socket branch from f9486f0 to f384f83 Compare March 30, 2016 17:36
@Fishrock123
Copy link
Contributor Author

CI on windows: https://ci.nodejs.org/job/node-test-known-issues/2/ (That ci says to not run with all?)

@Fishrock123
Copy link
Contributor Author

@cjihrig LGTY now?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

Yea, LGTM

`<`-ing a file into stdin actually results in a `fs.ReadStream`, rather
than a `tty.ReadStream`, and as such does not inherit from net.Socket,
unlike the other possible stdin options.

Refs: nodejs#5916
PR-URL: nodejs#5935
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123 Fishrock123 force-pushed the known-issue-stdin-not-net.socket branch from f384f83 to d6c9f64 Compare March 31, 2016 18:11
@Fishrock123 Fishrock123 merged commit d6c9f64 into nodejs:master Mar 31, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

@Fishrock123 is there a particular issue on the issue tracker that this maps to? I noticed you Refs'ed a PR. I've been adding the repro-exists label to the referenced issues, but I'm not not if that would apply to a PR.

@phillipj
Copy link
Member

@Fishrock123 did this pass linting for you? I'm getting this locally:

➜ make lint
./node tools/eslint/bin/eslint.js benchmark lib src test tools/doc \
        tools/eslint-rules --rulesdir tools/eslint-rules

/Users/phillipj/node/test/known_issues/test-stdin-is-always-net.socket.js
  14:1  error  Line 14 exceeds the maximum line length of 80  max-len

✖ 1 problem (1 error, 0 warnings)

@thefourtheye
Copy link
Contributor

#5980 Fixes the linter error

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Mar 31, 2016
Refer: nodejs#5935
PR-URL: nodejs#5980
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
thefourtheye added a commit that referenced this pull request Mar 31, 2016
Refer: #5935
PR-URL: #5980
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

Is this applicable to v4?

@Fishrock123
Copy link
Contributor Author

@jasnell do we put known-issue tests there? if so, yes this applies back as far as I can remember.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Yep, we can port the known-issue tests back to v4 now

MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
`<`-ing a file into stdin actually results in a `fs.ReadStream`, rather
than a `tty.ReadStream`, and as such does not inherit from net.Socket,
unlike the other possible stdin options.

Refs: #5916
PR-URL: #5935
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
Refer: #5935
PR-URL: #5980
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
This was referenced Apr 5, 2016
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
`<`-ing a file into stdin actually results in a `fs.ReadStream`, rather
than a `tty.ReadStream`, and as such does not inherit from net.Socket,
unlike the other possible stdin options.

Refs: #5916
PR-URL: #5935
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Refer: #5935
PR-URL: #5980
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
@addaleax addaleax added the known limitation Issues that are identified as known limitations. label May 18, 2018
@addaleax
Copy link
Member

@Fishrock123 I know this was a long, long time ago, but what exactly is the issue that this test is catching? It sounds like the underlying idea is that stdio streams should always inherit from net.Socket, but that isn’t really made explicit anywhere, is it?

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented May 18, 2018

@addaleax There was this thing at one point where sometimes stdin wasnt a net.Socket... I think this checks when stdin doesn't actually exist that it still makes a socket?

It was supposed to be a consistency check.


Edit:

It sounds like the underlying idea is that stdio streams should always inherit from net.Socket

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
known limitation Issues that are identified as known limitations. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants