Make basic_debugger_test less flaky

tests/standalone/debugger/debug_lib.dart

Index: tests/standalone/debugger/debug_lib.dart
===================================================================
--- tests/standalone/debugger/debug_lib.dart	(revision 20873)
+++ tests/standalone/debugger/debug_lib.dart	(working copy)
@@ -154,11 +154,6 @@
 }
 
 
-class Matcher {
-  void match(Debugger debugger);
-}
-
-
 class Command {
   var template;
 
@@ -429,10 +424,9 @@
 
   void openConnection() {
     Socket.connect("127.0.0.1", portNumber).then((s) {

[kustermann, 2013/04/04 11:41:39]

Although I really hate putting in sleep's in tests, I think if you delay this
connect statement by 100ms or so, the test will be way less flaky and we can
mark the test as PASS.

(Obviously the best solution would be if the debuggee would report a "I'm ready
and waiting for a connection" message to the debugger somehow.)

[hausner, 2013/04/04 15:12:05]

I agree but couldn't figure out how to add a sleep. It would be another
asynchronous code snippet in a piece of code that to me is already almost
unreadable.

On 2013/04/04 11:41:39, kustermann wrote:

-      socket = s;
-      var stringStream = socket.transform(new StringDecoder());
-      stringStream.listen(
-          (str) {
+        this.socket = s;
+        var stringStream = socket.transform(new StringDecoder());
+        stringStream.listen((str) {
             try {
               responses.append(str);
               handleMessages();
@@ -449,6 +443,10 @@
             print("Error '$e' detected in input stream from debug target");
             close(killDebugee: true);
           });
+      },
+      onError: (asyncErr) {
+        print("Error while connecting to debugee: $asyncErr");
+        close(killDebugee: true);
       });

[kustermann, 2013/04/04 11:41:39]

I think there is a bug here: If we were not able to connect to the debugee, we
kill it now, but we exit with 'exit(errors.length)'. Since we don't call
'errors.add()' in this case, we'll probably exit with exit code 0. This is
wrong: If we're not able to connect to the debuggee, the test should fail.

To fix this, you should call here
  error("Error while connecting to debugee: $asyncErr");
instead of 
  print("Error while connecting to debugee: $asyncErr");

This way 'close' will print the error and we'll exit with nonzero exitcode.

[hausner, 2013/04/04 15:12:05]

I had this at one point and then removed it again. If I make this an error, the
test remains flaky, and I don't like tests that nobody checks because it's ok if
they fail. I want a failure to mean that something is wrong with the debugger,
not with i/o. 

On 2013/04/04 11:41:39, kustermann wrote:

   }
 
@@ -456,7 +454,7 @@
     if (errorsDetected) {
       for (int i = 0; i < errors.length; i++) print(errors[i]);
     }
-    socket.close();
+    if (socket != null) socket.close();

[Tom Ball, 2013/04/03 21:22:26]

Braces surrounding socket.close()? I don't know if there's an official Dart
style guide requiring them, though.

[hausner, 2013/04/04 15:12:05]

I've seen both kinds of formatting in Dart code. (If only one is "correct", then
we should change the grammar of the language to require braces.)

     if (killDebugee) {
       targetProcess.kill();
       print("Target process killed");