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-ttywrap.writestream.js test always passes #28304

Closed
Trott opened this issue Jun 19, 2019 · 3 comments
Closed

test-ttywrap.writestream.js test always passes #28304

Trott opened this issue Jun 19, 2019 · 3 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.

Comments

@Trott
Copy link
Member

Trott commented Jun 19, 2019

  • Version: v13.0.0-pre
  • Platform: macOS Mojave
  • Subsystem: test, async-hooks

In test/async-hooks/test-ttywrap.writestream.js, there is delayedOnCloseHandler() which is supposed to check the async-hooks graph. When I run directly with the node executable, the test fails. When run with tools/test.py, though, the test passes. On my computer, at least, I can change the contents of delayedOnCloseHandler() to anything. It can just be a throw statement. The test will fail as expected when run directly with node but will pass when run with tools/test.py.

First, can someone else confirm this so that we can label this with confirmed-bug (or find that they don't experience this behavior in which case I can look more closely at exactly what I'm doing wrong).

Second, does anyone know why this would be the case?

@nodejs/testing @nodejs/async_hooks

@Trott Trott added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jun 19, 2019
@addaleax
Copy link
Member

When run with tools/test.py, though, the test passes.

That’s because the test isn’t run in that case, I assume; it’s skip’ed when stdout does not refer to a TTY. (This also means that it should be in test/pseudo-tty if we keep it, probably.)

As for the failure, I assume the missing before/after hooks would have been corresponding to the .destroy() call. I think the test should have been updated when we started making ._destroy() for stdio streams throw an error, and later be a no-op; that was forever ago, so it’s quite possible that the test never worked (I didn’t check).

Tbh, I’d just remove the test file.

@Trott
Copy link
Member Author

Trott commented Jun 20, 2019

Tbh, I’d just remove the test file.

I can get behind that: #28316

Trott added a commit to Trott/io.js that referenced this issue Jun 22, 2019
The test is never run in CI and may have never worked.

Refs: nodejs#28304
PR-URL: nodejs#28316
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jul 2, 2019
The test is never run in CI and may have never worked.

Refs: #28304
PR-URL: #28316
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca
Copy link
Member

lpinca commented Jul 14, 2019

Closing as the test was removed in b448db3.

@lpinca lpinca closed this as completed Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

3 participants