Make basic_debugger_test less flaky

tests/standalone/debugger/debug_lib.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 // Library used by debugger wire protocol tests (standalone VM debugging).
 
 library DartDebugger;
 
 import "dart:io";
 import "dart:math";
@@ skipping 136 matching lines @@
         isMatch = false;
       }
     } else {
       isMatch = false;
     }
   });
   return isMatch;
 }
 
 
-class Matcher {
-  void match(Debugger debugger);
-}
-
-
 class Command {
   var template;
 
   void send(Debugger debugger) {
     debugger.sendMessage(template);
   }
 
   void matchResponse(Debugger debugger) {
     Map response = debugger.currentMessage;
     var id = template["id"];
@@ skipping 249 matching lines @@
   }
 
   bool get errorsDetected => errors.length > 0;
 
   // Record error message.
   void error(String s) {
     errors.add(s);
   }
 
   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();
             } catch(e, trace) {
               print("Unexpected exception:\n$e\n$trace");
               close(killDebugee: true);
             }
           },
           onDone: () {
             print("Connection closed by debug target");
             close(killDebugee: true);
           },
           onError: (e) {
             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:

   }
 
   void close({killDebugee: false}) {
     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");
     }
     Expect.isTrue(!errorsDetected);
     exit(errors.length);
   }
 }
 
 
@@ skipping 33 matching lines @@
       if (++retries >= 3) { 
         print('unable to find unused port: $e');
         return -1; 
       } else {
         // Retry with another random port.
         RunScript(script);
       }
     });
   return true;
 }