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

test: fix test-debug-port-from-cmdline #25748

Closed
wants to merge 1 commit into from
Closed

test: fix test-debug-port-from-cmdline #25748

wants to merge 1 commit into from

Conversation

joaocgreis
Copy link
Member

test-debug-port-from-cmdline became an issue in io.js in nodejs/node#2094 , when it was noticed that it failed frequently on Windows 2008. The error messages displayed were not clear and threw investigation off from the real problem.

The problem turned out to be with the test itself, and it failed also on Linux but silently, which is also the case for v0.12. For example here: http://jenkins.nodejs.org/job/node-test-commit-unix/132/DESTCPU=ia32,label=linux/tapTestReport/simple.tap-126/ the output should have been 2 lines:

> Starting debugger agent.
> Debugger listening on port 12346

This fix changes the test so that it runs as expected and always asserts the expected result. Further details in the original PR: nodejs/node#2186

/cc @joyent/node-collaborators

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]>
@orangemocha
Copy link
Contributor

👍

@cjihrig
Copy link

cjihrig commented Jul 22, 2015

LGTM

joaocgreis added a commit 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
@joaocgreis
Copy link
Member Author

Thanks! Landed in ceb6a8c.

@joaocgreis joaocgreis closed this Jul 23, 2015
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 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.

3 participants