Fix for an issue w/ breakpointResolved events not being sent.

Fix for an issue w/ breakpointResolved events not being sent.
Added tests for breakpointResolved events.
Added tests for handling toString() invocations throwing exceptions, and extended debug_lib.dart to supported verifying values of local variables.

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24164

[devoncarew, 2013-06-18 16:34:46]

Hey Matthias, I did a bit more work in debug_lib.dart then I was expecting - let
me know what you think.

[hausner, 2013-06-18 17:30:25]

I would appreciate if the order of breakpointResolved events were not
significant for the success of tests. See my comment below.

If you don't mind I'd like to take a second look before you check in.

Thank you for implementing the "check locals" functionality!

https://codereview.chromium.org/17112010/diff/9001/tests/standalone/debugger/...
File tests/standalone/debugger/breakpoint_resolved_test.dart (right):

https://codereview.chromium.org/17112010/diff/9001/tests/standalone/debugger/...
tests/standalone/debugger/breakpoint_resolved_test.dart:26:
ExpectEvent("breakpointResolved"),
I think this test might be quite brittle because it assumes a lot about the
asynchronous nature of the breakpointResolved events.  It should not matter WHEN
the breakpointResolved events are received exactly, only that they HAVE been
received.

Maybe it would be better to keep a list of breakpoints that were set during the
test, and at the end of the test make sure that for each one of them we received
a breakpointResolved event at SOME point.

https://codereview.chromium.org/17112010/diff/9001/tests/standalone/debugger/...
File tests/standalone/debugger/debug_lib.dart (right):

https://codereview.chromium.org/17112010/diff/9001/tests/standalone/debugger/...
tests/standalone/debugger/debug_lib.dart:255: MatchFrames(List<String>
functionNames) {
How about an optional parameter here that takes a map of expected local names
values? That would eliminate the need for matchLocals() and the IMO ugly
cascaded calls to it.

Same for MatchFrame above.

Even better, you could create a separate matcher class for locals. That would
make the tests more readable:
  MatchFrames(['foo', 'bar', 'main']);
  MatchLocals(0, {'var1': 0, 'var2': null});  // locals in function 'foo'.
  MatchLocals(2, {'m1': 'hello'});  // locals in function 'main'.

[devoncarew, 2013-06-18 20:37:21]

Here's the changes, mostly what you outlined in your review, w/ some tweaks to 
the event matching. PTAL -

[hausner, 2013-06-18 21:05:09]

LGTM w/Comment

I think you could save the test writer the ExpectEvent entries in the script if
you remember every SetBreakpoint command that was issued. When the ACK message
arrives from the VM, you could add the ExpectEvent to the end of the script
automatically. Just an idea.

You could also add some MatchLocals to the basic_debugger_test to cover that
functionality. But feel free to do that in another changelist.

https://codereview.chromium.org/17112010/diff/16001/tests/standalone/debugger...
File tests/standalone/debugger/debug_lib.dart (right):

https://codereview.chromium.org/17112010/diff/16001/tests/standalone/debugger...
tests/standalone/debugger/debug_lib.dart:280: void matchEvent(Debugger debugger)
{
If an event was matched, it might make sense to take it out of the events list
so you don't accidentally match the same event twice.

[devoncarew, 2013-06-18 21:55:19]

On 2013/06/18 21:05:09, hausner wrote:
> LGTM w/Comment
> 
> I think you could save the test writer the ExpectEvent entries in the script
if
> you remember every SetBreakpoint command that was issued. When the ACK message
> arrives from the VM, you could add the ExpectEvent to the end of the script
> automatically. Just an idea.

I'd be worried in this case that we'd run across legitimate cases where we set a
breakpoint but never receive a breakpointResolved event. Like if the method w/
the bp was never executed. It would make sense to make this change if it was a
convention that any breakpoint set in a test was expected to be hit (or at least
resolved). I'll submit w/o that change, but feel free to add it if you think it
makes sense.
 
> You could also add some MatchLocals to the basic_debugger_test to cover that
> functionality. But feel free to do that in another changelist.

Will do! I'll add it in another CL.
 
>
https://codereview.chromium.org/17112010/diff/16001/tests/standalone/debugger...
> File tests/standalone/debugger/debug_lib.dart (right):
> 
>
https://codereview.chromium.org/17112010/diff/16001/tests/standalone/debugger...
> tests/standalone/debugger/debug_lib.dart:280: void matchEvent(Debugger
debugger)
> {
> If an event was matched, it might make sense to take it out of the events list
> so you don't accidentally match the same event twice.

[devoncarew, 2013-06-18 21:56:06]

Committed patchset #5 manually as r24164 (presubmit successful).