Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: remove invalid part of stream2-stderr-sync #6884

Closed
wants to merge 1 commit into from
Closed

test: remove invalid part of stream2-stderr-sync #6884

wants to merge 1 commit into from

Conversation

orangemocha
Copy link
Contributor

One test case in test-stream2-stderr-sync.js was creating a TTY
object using an undocumented constructor and passing in fd 2.
However, this is running in a child process and fd 2 is actually
a pipe, not a TTY.

The constructor fails on Windows and causes the handle type to be
left uninitialized, which later causes an assert to fail.

On Unix, the constructor fails to retrieve the windows size but unlike
on Windows, it just leaves the size fields undefined and continues
with initializing the stream type, yielding a semi-usable object.

I could make the Windows version match Unix behavior, but it
seems to me that the test is relying on an implementation detail of
an undocumented API, and the Unix behavior is not necessarily more
correct than the Windows one. Thus it makes more sense to remove this
test.

One test case in test-stream2-stderr-sync.js was creating a TTY
object using an undocumented constructor and passing in fd 2.
However, this is running in a child process and fd 2 is actually
a pipe, not a TTY.

The constructor fails on Windows and causes the handle type to be
left uninitialized, which later causes an assert to fail.

On Unix, the constructor fails to retrieve the windows size but unlike
on Windows, it just leaves the size fields undefined and continues
with initializing the stream type, yielding a semi-usable object.

I could make the Windows version match Unix behavior, but it
seems to me that the test is relying on an implementation detail of
an undocumented API, and the Unix behavior is not necessarily more
correct than the Windows one. Thus it makes more sense to remove this
test.
@trevnorris
Copy link

@indutny @tjfontaine thoughts on whether to remove the test that relies on an undocumented implementation detail, or to, as @orangemocha mentions, make the Windows side match?

@indutny
Copy link
Member

indutny commented Jan 17, 2014

+1 for removing it.

@tjfontaine
Copy link

sorry I didn't update this issue after the call on thursday, but @orangemocha and I had a conversation, he decided to post the pr as a mechanism to start a conversation around the concept being broken behavior. It may be that we simply can't test for this feature, but it is important to remember we do have an issue with the sync behavior (or lack thereof) of stdio on windows. Where #3584 has more information.

@orangemocha
Copy link
Contributor Author

Right, after removing this part the test still fails because of #3584#3584. I am planning to address that separately. I thought about testing the TTY part by not redirecting the child stdio but there is no good way to validate whether that’s sync or not.

@tjfontaine
Copy link

this was landed in c1bb886

@tjfontaine tjfontaine closed this Mar 10, 2014
@orangemocha orangemocha deleted the orangemocha-Stream2StderrSync branch April 10, 2014 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants