Make the unittest's breath test a bit more resilient against timing issue.

Make the unittest's breath test a bit more resilient against timing issue.

BUG=
R=kustermann@google.com, sgjesse@google.com

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

[wibling, 2014-11-17 11:58:52]

wibling@google.com changed reviewers:
+ kevmoo@google.com, kustermann@google.com, sgjesse@google.com

[kustermann, 2014-11-17 12:32:02]

https://codereview.chromium.org/734023002/diff/1/pkg/unittest/test/breath_tes...
File pkg/unittest/test/breath_test.dart (right):

https://codereview.chromium.org/734023002/diff/1/pkg/unittest/test/breath_tes...
pkg/unittest/test/breath_test.dart:40: // timer scheduled in the 'initial' test.
This is not completely correct.

If test 'initial' takes BREATH_INTERVAL/2 + epsilon and 'starve' blocks for
another BREATH_INTERVAL/2 + epsilon, then you go out of the while loop and the
next test (i.e. 'breathed') will also be run inside a microtask. This will be
running before the 'Timer.run(() sentinel =1)' task, making this test fail.

So if I see this right you need to do something like this:

DateTime start = new DateTime.now().millisecondsSinceEpoch;
test('init', () {
  Timer.run(() {
    sentinel = 1;
  });
});

At this point, it depends on how long it took this piece of code. If it took
longer than BREATH_INTERVAL it will end up scheduling the next test in the event
loop, otherwise in a microtask queue.

The next test, is doing this then:

test('starve', () {
  var now = new DateTime.now().millisecondsSinceEpoch;
  if (now - start < BREATH_INTERVAL) {
    // now we are certain that it *MUST* this test must have been scheduled as a
microtask
    expect(sentinel, 0);
  }
  busySleep(BREATH_INTERVAL+epsilon);
});

Now since the 'starve' test is sleeping *more than* BREATH_INTERVAL, the next
test *must* be scheduled in the event loop. 


test('breathed', () {
  expect(sentinel, 1);
});

[wibling, 2014-11-17 13:13:35]

On 2014/11/17 12:32:02, kustermann wrote:
>
https://codereview.chromium.org/734023002/diff/1/pkg/unittest/test/breath_tes...
> File pkg/unittest/test/breath_test.dart (right):
> 
>
https://codereview.chromium.org/734023002/diff/1/pkg/unittest/test/breath_tes...
> pkg/unittest/test/breath_test.dart:40: // timer scheduled in the 'initial'
test.
> This is not completely correct.
> 
> If test 'initial' takes BREATH_INTERVAL/2 + epsilon and 'starve' blocks for
> another BREATH_INTERVAL/2 + epsilon, then you go out of the while loop and the
> next test (i.e. 'breathed') will also be run inside a microtask. This will be
> running before the 'Timer.run(() sentinel =1)' task, making this test fail.
> 
> So if I see this right you need to do something like this:
> 
> DateTime start = new DateTime.now().millisecondsSinceEpoch;
> test('init', () {
>   Timer.run(() {
>     sentinel = 1;
>   });
> });
> 
> At this point, it depends on how long it took this piece of code. If it took
> longer than BREATH_INTERVAL it will end up scheduling the next test in the
event
> loop, otherwise in a microtask queue.
> 
> The next test, is doing this then:
> 
> test('starve', () {
>   var now = new DateTime.now().millisecondsSinceEpoch;
>   if (now - start < BREATH_INTERVAL) {
>     // now we are certain that it *MUST* this test must have been scheduled as
a
> microtask
>     expect(sentinel, 0);
>   }
>   busySleep(BREATH_INTERVAL+epsilon);
> });
> 
> Now since the 'starve' test is sleeping *more than* BREATH_INTERVAL, the next
> test *must* be scheduled in the event loop. 
> 
> 
> test('breathed', () {
>   expect(sentinel, 1);
> });

Nice catch! I have updated the test to be even more conservative to guarantee
the correct amount of time has either not elapsed or elapsed depending on which
expect you talk about:)

[kustermann, 2014-11-17 13:16:46]

LGTM

[Søren Gjesse, 2014-11-17 13:20:08]

lgtm

https://codereview.chromium.org/734023002/diff/20001/pkg/unittest/test/breath...
File pkg/unittest/test/breath_test.dart (right):

https://codereview.chromium.org/734023002/diff/20001/pkg/unittest/test/breath...
pkg/unittest/test/breath_test.dart:35: if (testStartStopwatch.elapsed <=
BREATH_INTERVAL)
Please add {}'s to the if.

I guess there are no way of testing that this if is not always false.

[wibling, 2014-11-17 13:25:59]

On 2014/11/17 13:20:08, Søren Gjesse wrote:
> lgtm
> 
>
https://codereview.chromium.org/734023002/diff/20001/pkg/unittest/test/breath...
> File pkg/unittest/test/breath_test.dart (right):
> 
>
https://codereview.chromium.org/734023002/diff/20001/pkg/unittest/test/breath...
> pkg/unittest/test/breath_test.dart:35: if (testStartStopwatch.elapsed <=
> BREATH_INTERVAL)
> Please add {}'s to the if.

Done.

> I guess there are no way of testing that this if is not always false.

No, not that I can think of :/

[wibling, 2014-11-17 14:14:55]

Committed patchset #4 (id:60001) manually as 41774 (presubmit successful).