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: remove obsolete TODO comments for fs.readline() #2033

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 22, 2015

The readfile/pipe tests rely on a pre-existing pipe in the system (namely, /dev/stdin). This does not exist on Windows.

There doesn't seem to be an easy way (or perhaps any way?) to do all of:

  • create a named pipe on Windows that...
  • ...can be hooked up to STDIN...
  • ...from inside Node.js

I tried a few creative-ish things (including abusing net.createServer().listen() to trick Node into creating a named pipe and then trying to get cmd.exe to write to that pipe), but no dice.

Here's the commit where this comment was initially introduced, if that helps: 1d3d02c

In the current code base, the line in lib.fs that is being changed is now https://github.com/nodejs/io.js/blob/5d2b846d1114bad5e7fbf04910ff59fc970c175e/lib/fs.js#L288

Changing -1 to pos in that line does not result in the corresponding test failing, so it's not clear that the test is addressing the change in that commit anymore (if it ever did?).

There is no reason to expect Windows to ever have this functionality.

So, in the interest of weed-whacking out all the TODOs in the tests, these should seemingly be removed. If there's consensus that this is wrong and it actually does capture useful information, perhaps it can be moved to the issue tracker rather than lurking in a code TODO.

Apologies in advance for another in a tedious series of TODO-removal PRs...

The readfile/pipe tests rely on pre-existing pipes in the system.
This arguably tests the OS functionality and not really io.js
functionality. Removing TODOs.
@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jun 22, 2015
@Trott Trott changed the title test: remove obsolete TODO comments test: remove obsolete TODO comments for fs.readline() Jun 22, 2015
@trevnorris
Copy link
Contributor

LGTM

Trott added a commit that referenced this pull request Jun 23, 2015
The readfile/pipe tests rely on pre-existing pipes in the system.
This arguably tests the OS functionality and not really io.js
functionality. Removing TODOs.

PR-URL: #2033
Reviewed-By: Trevor Norris <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jun 23, 2015

Merged in 776a65e

@Trott Trott closed this Jun 23, 2015
@rvagg rvagg mentioned this pull request Jun 30, 2015
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
The readfile/pipe tests rely on pre-existing pipes in the system.
This arguably tests the OS functionality and not really io.js
functionality. Removing TODOs.

PR-URL: nodejs#2033
Reviewed-By: Trevor Norris <[email protected]>
@Trott Trott deleted the readfile branch January 9, 2022 22:02
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants