Synced basic_debugger_test start on new "debugger initialized" message.

Synced basic_debugger_test start on new "debugger initialized" message.
Committed: https://code.google.com/p/dart/source/detail?r=20997

[hausner, 2013-04-04 23:17:45]

LGTM.

https://codereview.chromium.org/13597004/diff/3001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/13597004/diff/3001/runtime/vm/debugger.cc#new...
runtime/vm/debugger.cc:1580: OS::Print("Debugger initialized\n");
I think the handshake should even work without this message, since there is
stdout output when the debugger reaches the breakpoint at the beginning of
main().

[Tom Ball, 2013-04-04 23:24:56]

https://codereview.chromium.org/13597004/diff/3001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/13597004/diff/3001/runtime/vm/debugger.cc#new...
runtime/vm/debugger.cc:1580: OS::Print("Debugger initialized\n");
On 2013/04/04 23:17:45, hausner wrote:
> I think the handshake should even work without this message, since there is
> stdout output when the debugger reaches the breakpoint at the beginning of
> main().

I tried removing it and the test hangs. You're right that once the test starts
running then stdout messages are printed, but the race condition we're facing is
that the test starts running before the debuggee is ready.

[devoncarew, 2013-04-05 00:48:23]

lgtm

As an fyi, when we start up a VM process from the editor, we poll the debugging
port every 25 ms until the debugger responds. Depending on the amount of source
in the user's application it can take a little while for the debugger to start
listening on its port.

[kustermann, 2013-04-05 08:20:08]

lgtm - and thanks a lot for fixing this.

Since we have only one test testing the debugger interface it is crucial that we
have coverage here (and this test was broken for a long time now).

Please also remove the SKIP marker in the status file.

(As a side note: Currently the test is marked as SKIP. Since this test doesn't
leave processes behind anymore [it kills the debugee if it can't connect] we
should NOT mark it as SKIP [as it is now] but rather as PASS, FAIL [aka flaky].
SKIP should only be used for tests that either are not supposed to be run at all
on a given configuration or if a tests causes issues on the bots -- otherwise we
should mark it as flaky.).

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

https://codereview.chromium.org/13597004/diff/3001/tests/standalone/debugger/...
tests/standalone/debugger/debug_lib.dart:306: }
I'd actually test if 'line == "Debugger initialized"' -- in case somebody else
adds another OS::Print statement which is executed earlier, we could end up
calling 'openConnection' before the debuggee is ready.

Additionally, I think it wouldn't hurt if we print the "Debugger initialized"
message (as we do with all other stdout/stderr lines).

https://codereview.chromium.org/13597004/diff/3001/tests/standalone/debugger/...
tests/standalone/debugger/debug_lib.dart:448: print("Error while connecting to
debugee: $asyncErr");
Would be nice if you could change that from
  print("Error while connecting to debugee: $asyncErr");
to
  error("Error while connecting to debugee: $asyncErr");
otherwise the test will be reported as passing even if it couldn't connect to
the debuggee.

[Tom Ball, 2013-04-05 20:55:15]

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

[Tom Ball, 2013-04-05 21:28:52]

I had to leave basic_debugger_test disabled, since Florian broke it separately
with a bad expect import change (issue 9703). I changed its "skip" to "pass
fail" as you suggested, however.

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

https://codereview.chromium.org/13597004/diff/3001/tests/standalone/debugger/...
tests/standalone/debugger/debug_lib.dart:306: }
On 2013/04/05 08:20:08, kustermann wrote:
> I'd actually test if 'line == "Debugger initialized"' -- in case somebody else
> adds another OS::Print statement which is executed earlier, we could end up
> calling 'openConnection' before the debuggee is ready.

Done.

> Additionally, I think it wouldn't hurt if we print the "Debugger initialized"
> message (as we do with all other stdout/stderr lines).

It was being displayed twice, because a debugger is initialized for each
isolate. I moved the initialized message to main.cc, and removed this test.

https://codereview.chromium.org/13597004/diff/3001/tests/standalone/debugger/...
tests/standalone/debugger/debug_lib.dart:448: print("Error while connecting to
debugee: $asyncErr");
On 2013/04/05 08:20:08, kustermann wrote:
> Would be nice if you could change that from
>   print("Error while connecting to debugee: $asyncErr");
> to
>   error("Error while connecting to debugee: $asyncErr");
> otherwise the test will be reported as passing even if it couldn't connect to
> the debuggee.

Done.