Eliminate nondeterminism from analysis server unit tests.

Eliminate nondeterminism from analysis server unit tests.

Previously, we tested for non-responsiveness in the analysis server
using timeout(new Duration(seconds: 1)).  This caused sporadic
failures on slower buildbots.  Now, we simply let the event loop run
20 times--this should produce consistent results regardless of machine
speed.

R=brianwilkerson@google.com, danrubel@google.com

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

[Paul Berry, 2014-04-21 14:11:07]

FYI, this technique isn't new--I cribbed it from the barback unit tests (see, in
particular, the original pumpEventQueue function in
pkg/barback/lib/src/utils.dart).

[Brian Wilkerson, 2014-04-21 14:18:05]

My only concern is if we are loosing some protection against blocked or runaway
processes by getting rid of the timeout. Other than that, LGTM.

https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mocks.dart (right):

https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mocks.dart:87: /** The request that was not responded
to. */
I don't know for sure, but I suspect that this should either be formatted as
  /**
   * The request...
   */
or
  /// The request...

[Paul Berry, 2014-04-21 15:10:12]

On 2014/04/21 14:18:05, Brian Wilkerson wrote:
> My only concern is if we are loosing some protection against blocked or
runaway
> processes by getting rid of the timeout. Other than that, LGTM.

I believe this CL doesn't change anything for (synchronously) blocked or runaway
processes.  Timeouts are events, so they're only serviced after control flow
returns to the event loop.  So either way, blocked or runaway processes lead to
a hang :(

> 
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/moc...
> File pkg/analysis_server/test/mocks.dart (right):
> 
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/moc...
> pkg/analysis_server/test/mocks.dart:87: /** The request that was not responded
> to. */
> I don't know for sure, but I suspect that this should either be formatted as
>   /**
>    * The request...
>    */
> or
>   /// The request...

Done.

[danrubel, 2014-04-21 16:59:09]

LGTM once comment has been addressed

https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
File pkg/analysis_server/test/analysis_server_test.dart (left):

https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
pkg/analysis_server/test/analysis_server_test.dart:41: .timeout(new
Duration(seconds: 1))
If for some reason, the server completes normally or throws an exception but
does not send a response, then this test will hang without this timeout. If we
want to assume in our tests that the server will always send a response, then
I'm ok with this change.

[Paul Berry, 2014-04-21 17:43:49]

On 2014/04/21 16:59:09, danrubel wrote:
> LGTM once comment has been addressed
> 
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
> File pkg/analysis_server/test/analysis_server_test.dart (left):
> 
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
> pkg/analysis_server/test/analysis_server_test.dart:41: .timeout(new
> Duration(seconds: 1))
> If for some reason, the server completes normally or throws an exception but
> does not send a response, then this test will hang without this timeout. If we
> want to assume in our tests that the server will always send a response, then
> I'm ok with this change.

Actually, we are ok.  If the server throws an exception, that will be caught by
the test framework and reported as a test failure (even if the exception is
thrown from an asynchronous callback).  If the server completes normally but
does not send a response, then this code:

+    pumpEventQueue().then((_) => responseController.addError(
+        new NoResponseException(request)));

will cause the future returned by sendRequest() to complete with an error after
20 executions of the event loop, and that error will be caught by the test
framework and reported as a test failure.

I've double-checked that both of these cases actually work.

[danrubel, 2014-04-21 18:28:51]

On 2014/04/21 17:43:49, Paul Berry wrote:
> On 2014/04/21 16:59:09, danrubel wrote:
> > LGTM once comment has been addressed
> > 
> >
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
> > File pkg/analysis_server/test/analysis_server_test.dart (left):
> > 
> >
>
https://codereview.chromium.org/243703008/diff/1/pkg/analysis_server/test/ana...
> > pkg/analysis_server/test/analysis_server_test.dart:41: .timeout(new
> > Duration(seconds: 1))
> > If for some reason, the server completes normally or throws an exception but
> > does not send a response, then this test will hang without this timeout. If
we
> > want to assume in our tests that the server will always send a response,
then
> > I'm ok with this change.
> 
> Actually, we are ok.  If the server throws an exception, that will be caught
by
> the test framework and reported as a test failure (even if the exception is
> thrown from an asynchronous callback).  If the server completes normally but
> does not send a response, then this code:
> 
> +    pumpEventQueue().then((_) => responseController.addError(
> +        new NoResponseException(request)));
> 
> will cause the future returned by sendRequest() to complete with an error
after
> 20 executions of the event loop, and that error will be caught by the test
> framework and reported as a test failure.
> 
> I've double-checked that both of these cases actually work.

SGTM!

[Paul Berry, 2014-04-21 19:48:20]

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