Make basic_debugger_test less flaky

Make basic_debugger_test less flaky

There is a race condition between debugger and debugee. Detect if the debugger is attempting to connect to the debugee to soon and simply skip running the debug test
rather than failing the test. 
Committed: https://code.google.com/p/dart/source/detail?r=20894

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

lgtm

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
File tests/standalone/debugger/debug_lib.dart (right):

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:457: if (socket != null)
socket.close();
Braces surrounding socket.close()? I don't know if there's an official Dart
style guide requiring them, though.

[hausner, 2013-04-03 21:33:00]

Committed patchset #1 manually as r20894 (presubmit successful).

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

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
File tests/standalone/debugger/debug_lib.dart (right):

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:426: Socket.connect("127.0.0.1",
portNumber).then((s) {
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.)

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:450: });
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]

Thanks for taking a look Martin. If you know how to add the timer, let me know
or feel free to add it.

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
File tests/standalone/debugger/debug_lib.dart (right):

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:426: Socket.connect("127.0.0.1",
portNumber).then((s) {
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:
> 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.)

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:450: });
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:
> 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.

https://codereview.chromium.org/13555003/diff/1/tests/standalone/debugger/deb...
tests/standalone/debugger/debug_lib.dart:457: if (socket != null)
socket.close();
On 2013/04/03 21:22:26, Tom Ball wrote:
> Braces surrounding socket.close()? I don't know if there's an official Dart
> style guide requiring them, though.
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.)