-
Notifications
You must be signed in to change notification settings - Fork 7.3k
debugger: fix unhandled error in setBreakpoint #6460
Conversation
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. Commit bajtos/node@357f4bb has the following error(s):
The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
@tjfontaine @trevnorris Who is the best person to review this pull request, now that Ben is gone? |
There a test you could include? |
unfortunately the test would probably be flakey, this seems fairly reasonable, I'm not necessarily thrilled with the message in general, the question doesn't seem necessary? |
Shall I rephrase? Or keep it short but not very helpful?
I can write a new one from scratch, but as TJ pointed out, it may cost more on maintenance that it will bring back as regression guard. I don't mind either way -i f you think there should be a test then I'll add it. |
@tjfontaine @trevnorris ping, what changes are needed to get this patch landed? |
I'd say this would be an acceptable way to phrase the error. If there are no strong objections by EOD tomorrow, I'll merge. |
Fix Interface.setBreakpoint() to correctly handle an attempt to set a breakpoint in the current script when there is no current script. This usually happens when the debugged process is not paused.
@chrisdickinson thank you. I have rebased the commit on top of the current master and reworded the message. |
Thanks @chrisdickinson landed this in e93ff4f |
Fix Interface.setBreakpoint() to correctly handle an attempt to set a
breakpoint in the current script when there is no current script. This usually
happens when the debugged process is not paused.
Close #6453
@bnoordhuis Please review. Is it worth adding a unit test for this change? It's more than just adding few lines to
test/simple/test-debugger-repl.js
, a new test fixture is needed.