Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

print 'ok' after connection #25

Merged
merged 1 commit into from
Feb 18, 2017
Merged

Conversation

ofrobots
Copy link
Contributor

This change makes inspect more compatible with the old debugger. There
are some Node.js core test-cases that are expecting the 'ok' to show up
here.

I am investigating if we can make node debug an alias of node inspect.

Copy link
Collaborator

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran npm test locally - it fails because of commit message conventions (it expects a prefix like fix: ).

The change itself LGTM and the actual test suite passes. 👍

This change makes inspect more compatible with the old debugger. There
are some Node.js core test-cases that are expecting the 'ok' to show up
here.
@ofrobots
Copy link
Contributor Author

Commit message fixed.

@jkrems
Copy link
Collaborator

jkrems commented Feb 15, 2017

Verified npm test locally. Not sure what the official flow with node CI is. I'm guessing a member of the diagnostics WG would now start a CI run against this branch..?

@ofrobots
Copy link
Contributor Author

Hmm.. let me see if I did this right.. https://ci.nodejs.org/view/x%20-%20Diagnostics/job/node-inspect-continuous-integration/6/.

/cc @mhdawson does @jkrems need to be a collaborator to get access to the CI?

@jkrems
Copy link
Collaborator

jkrems commented Feb 15, 2017

Looks like there's some inherent instability in the Windows build. Maybe caused by different stdout flushing behavior/timing..? Don't think that's connected to this PR though.

@mhdawson
Copy link
Member

Anybody who is part of the diagnostics team should be able to kick of the node-inspect job. Looking at the list I don't see jkrems on that list but given his involvement in node-inspect my feeling is that he should be but I think we need members of that WG to chime in. @joshgav @nodejs/diagnostics

@mhdawson
Copy link
Member

And specifically, he does not need to be a collaborator, just a member of the team configured for the diagnostics workgroup.

@mhdawson
Copy link
Member

@jkrems I did open some issues for failures in the CI. My guess is that we'll find they are issues with the tests, most likely related to timing. May of the failures we've seen when broadening out the platform coverage tests run on fall into this category. Do you have cycles to look at those failures ?

@joshgav
Copy link
Contributor

joshgav commented Feb 16, 2017

Yeah we need to manage membership of the Diag WG better, I've been waiting for the changes in the TSC to land though. PR to add Jan: nodejs/diagnostics#87

@mhdawson
Copy link
Member

@jkrems at this point unless you see new failures in the CI jobs that were not in the previous runs without the commit, I think this can go ahead and land.

We'll need to work the failures to get to green on all platforms @CurryKitten is starting to look at the zLinux/AIX failures. @joshgav do you have anybody you can nominate to look at the windows failures ?

@jkrems jkrems merged commit 37cc9b7 into nodejs:master Feb 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants