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
Debugger - Filter out the ending of the Module wrapper when using the list command #9773
Debugger - Filter out the ending of the Module wrapper when using the list command #9773
Conversation
Could you provide a test case please. |
sure |
06d78d3
to
5c9da75
Compare
5c9da75
to
758059b
Compare
@cjihrig Sorry for the delay(Thanksgiving Holiday), I've added a test case here |
561c191
to
d93bfa1
Compare
and rebased |
const proc = spawn(process.execPath, args, { stdio: 'pipe' }); | ||
proc.stdout.setEncoding('utf8'); | ||
|
||
var stdout = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be let
.
|
||
var stdout = ''; | ||
|
||
var sentCommand = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As can these.
proc.stdout.on('data', (data) => { | ||
stdout += data; | ||
if (!sentCommand && stdout.includes('> 1')) { | ||
return sentCommand = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split this into two separate lines. Same for the other similar lines below.
return sentEmpty = true; | ||
} | ||
if (!sentExit && sentCommand && sentEmpty) { | ||
setTimeout(() => {proc.stdin.write('\n\n\n.exit\n\n\n');}, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use setImmediate()
above and setTimeout()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i copied and pasted from another test, so to be honest, thats what happened. is there a preferences for me to switch too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works with setImmediate()
, or plain synchronous code, that should be preferred over setTimeout()
.
false, | ||
'the last line of the debugger should not have the module wrapping ending' | ||
); | ||
console.log(stdout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this.
d93bfa1
to
3b08903
Compare
@cjihrig ok, i think i got all the changes, and i also rebased against master too |
e712633
to
ed4eec9
Compare
rebased again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it would be nice if we could reuse the test/debugger/helper-debugger-repl.js
file. I'm not sure how feasible it is or not though. Thoughts @nodejs/testing?
process.on('exit', (exitCode) => { | ||
assert.strictEqual(exitCode, 0); | ||
// No module wrapping at the first line | ||
assert.equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this strictEqual()
'debugger should not have the module wrapper' | ||
); | ||
// No module wrapping at the end | ||
assert.equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strictEqual()
again please.
} | ||
}); | ||
|
||
process.on('exit', (exitCode) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be on the child process close
event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense
ed4eec9
to
fbd1287
Compare
@cjihrig updated |
} | ||
}); | ||
|
||
proc.on('close', (exitCode) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you assert the signal
too (the second argument to the close
handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added that too. it happens to be null, i was sort of expecting it to not be though
fbd1287
to
94f4a3f
Compare
looks like some failures, not sure whats going on in those platforms though |
94f4a3f
to
338c052
Compare
i rebased, maybe there was something missing |
} | ||
}); | ||
|
||
proc.on('exit', (exitCode, signal) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be wrapped in common.mustCall()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lance made the update and rebased
7460b4a
to
df6f0fe
Compare
Let's get a fresh CI and see what happens: https://ci.nodejs.org/job/node-test-pull-request/5959/ |
i see that the ARM is reporting as failed, but the link to CI looks like it passed. Also, looks like the smartOS 14-32 isn't failing anymore, but 14-64 still is |
The ARM failure on GitHub can be ignored. If you look at the actual Jenkins link, only SmartOS failed. |
df6f0fe
to
0800565
Compare
@cjihrig can this PR be merged or does it need some more work? |
The last time we ran the CI, the test in this PR was failing on SmartOS. |
The old CI URL seems to have expired. Let's try again then: https://ci.nodejs.org/job/node-test-pull-request/6926/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI seems to be passing on SmartOS this time around.
@lholmquist, can you fix this nit and @cjihrig's review comment here (in other words, reverting 8f10635360ffbdd158d3c088a0e19e066ac8cd8c), so we can merge this PR?
|
||
if (!sentEmpty) { | ||
setImmediate(() => { proc.stdin.write('list(10)\n'); }); | ||
sentEmpty = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the variable name to something that is more applicable to this test, like sentList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu sure
0800565
to
edaba7d
Compare
@TimothyGu @cjihrig I've reverted back that commit, and fixed that small nit. I've also rebased against master too. |
edaba7d
to
bb314f9
Compare
… list command When doing the list command using a number that is more than the amount lines in that files, ex: list(10), the ending of the Module wrapper was still showing. This removes that line. Fixes: nodejs#9768
bb314f9
to
acbc8bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: the subsystem should perhaps be lib:
, and the commit message adhere to the guidelines. (I can make those changes when merging after all reviews have been approved)
@cjihrig @TimothyGu Would it be alright if I land this? |
Looks like there are still related test failures. |
I'm running the test on windows to see if I can track down the issue. |
This commit attempts to fix a failure that has been reported by CI and that I was able to reproduce locally as well. Will run another CI and see if this takes care of the issue.
This needs a rebase. @danbev did your commit solve the failure? |
I don't think so. I'll close this. |
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
Debbuger
Description of change
PR for #9768.
When inside the debugger, doing a
list(10)
on a file with only say 5 lines, would print the end of the module wrapper on the last line.There is code currently to make sure that the first part of the Module wrapper is filtered out. that is located here: https://github.com/nodejs/node/blob/master/lib/_debugger.js#L1107
The code added filters out the very last line of the lines array, which is the ending of the module wrapper.
I did not add a test for this. I don't believe there was one that tests List anyways. If there needs to be one, i will try and look at the other debugger tests as an example