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: fix test-debug-port-from-cmdline #2186

Closed

Conversation

joaocgreis
Copy link
Member

test-debug-port-from-cmdline.js has been failing occasionally with unclear errors in Windows and silently (false positive) in Linux.

Errors found in Windows:

Error: Not enough storage is available to process this command.
Error: Access is denied.
Error: The system cannot find the file specified.

Note that in case of success it should always display two lines of output:

> Starting debugger agent.
> Debugger listening on port 12346

This test was failing because the spawned process was terminated before anything could be done, by calling child.stdin.end. With this change, the child's stdin is no longer closed. When the stdin is not a tty, io.js waits for the whole input before starting, so the child must be run with --interactive to process the command sent by the parent. The child is killed explicitly by the parent before it exits.

This test was failing silently because the asserts were not called if nothing was received from the child. This fix moves assertOutputLines to always run on exit.

Fixes #2094
Fixes #2177

This test was failing because the spawned process was terminated before
anything could be done, by calling child.stdin.end. With this change,
the child's stdin is no longer closed. When the stdin is not a tty,
io.js waits for the whole input before starting, so the child must be
run with --interactive to process the command sent by the parent. The
child is killed explicitly by the parent before it exits.

This test was failing silently because the asserts were not called if
nothing was received from the child. This fix moves assertOutputLines to
always run on exit.

Fixes nodejs#2094
Fixes nodejs#2177
@cjihrig
Copy link
Contributor

cjihrig commented Jul 15, 2015

Thanks for the detailed explanation. I'll start the CI.

UPDATE: CI https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/152/

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Jul 15, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Jul 15, 2015

CI is happy, with regards to this test anyway.

@rvagg
Copy link
Member

rvagg commented Jul 16, 2015

fantastic @joaocgreis, great investigative work here LGTM

there are 2 main persistent errors and one intermittent that are stopping greens in CI for this but there are unrelated to this change

@jbergstroem
Copy link
Member

Awesome, this thing has been annoying me for a while.

@orangemocha
Copy link
Contributor

Also LGTM. Nice work @joaocgreis 👍

@jbergstroem
Copy link
Member

LGTM from me too.

cjihrig pushed a commit that referenced this pull request Jul 16, 2015
This test was failing because the spawned process was terminated before
anything could be done, by calling child.stdin.end. With this change,
the child's stdin is no longer closed. When the stdin is not a tty,
io.js waits for the whole input before starting, so the child must be
run with --interactive to process the command sent by the parent. The
child is killed explicitly by the parent before it exits.

This test was failing silently because the asserts were not called if
nothing was received from the child. This fix moves assertOutputLines to
always run on exit.

Fixes: #2177
Refs: #2094
PR-URL: #2186
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Alexis Campailla <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Jul 16, 2015

Thanks! Landed in 2b4b600

@cjihrig cjihrig closed this Jul 16, 2015
joaocgreis added a commit to nodejs/node-v0.x-archive that referenced this pull request Jul 23, 2015
This change is a backport of 2b4b600
from io.js.

Original commit message:

  This test was failing because the spawned process was terminated
  before anything could be done, by calling child.stdin.end. With this
  change, the child's stdin is no longer closed. When the stdin is not
  a tty, io.js waits for the whole input before starting, so the child
  must be run with --interactive to process the command sent by the
  parent. The child is killed explicitly by the parent before it exits.

  This test was failing silently because the asserts were not called if
  nothing was received from the child. This fix moves assertOutputLines
  to always run on exit.

  Fixes: nodejs/node#2177
  Refs: nodejs/node#2094
  PR-URL: nodejs/node#2186
  Reviewed-By: Colin Ihrig <[email protected]>
  Reviewed-By: Rod Vagg <[email protected]>
  Reviewed-By: Johan Bergström <[email protected]>
  Reviewed-By: Alexis Campailla <[email protected]>

Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #25748
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
This change is a backport of 2b4b600
from io.js.

Original commit message:

  This test was failing because the spawned process was terminated
  before anything could be done, by calling child.stdin.end. With this
  change, the child's stdin is no longer closed. When the stdin is not
  a tty, io.js waits for the whole input before starting, so the child
  must be run with --interactive to process the command sent by the
  parent. The child is killed explicitly by the parent before it exits.

  This test was failing silently because the asserts were not called if
  nothing was received from the child. This fix moves assertOutputLines
  to always run on exit.

  Fixes: nodejs/node#2177
  Refs: nodejs/node#2094
  PR-URL: nodejs/node#2186
  Reviewed-By: Colin Ihrig <[email protected]>
  Reviewed-By: Rod Vagg <[email protected]>
  Reviewed-By: Johan Bergström <[email protected]>
  Reviewed-By: Alexis Campailla <[email protected]>

Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#25748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
7 participants