code review 5689076: time: run full TestTicker test even during short tests

time: run full TestTicker test even during short tests

[brainman, 2012-02-23 00:21:08]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2012-02-23 00:24:00]

LGTM i suppose

http://codereview.appspot.com/5689076/diff/3/src/pkg/time/tick_test.go
File src/pkg/time/tick_test.go (right):

http://codereview.appspot.com/5689076/diff/3/src/pkg/time/tick_test.go#newcode29
src/pkg/time/tick_test.go:29: t.Fatalf("%d %s ticks took %s; too fast", Count,
Delta, dt)
s/fast/quick/

http://codereview.appspot.com/5689076/diff/3/src/pkg/time/tick_test.go#newcode33
src/pkg/time/tick_test.go:33: t.Fatalf("%d %s ticks took %s; too long", Count,
Delta, dt)
s/long/slow/

[bradfitz, 2012-02-23 00:24:06]

This is a misuse of testing.Short().

If the test is flaky, the test is flaky and it should be removed.

But don't conditionally hide flakiness just to make the dashboard prettier.


On Thu, Feb 23, 2012 at 11:21 AM, <alex.brainman@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> time: do not care if TestTicker is too slow during short tests
>
> Please review this at
http://codereview.appspot.com/**5689076/<http://codereview.appspot.com/5689076/>
>
> Affected files:
>  M src/pkg/time/tick_test.go
>
>
> Index: src/pkg/time/tick_test.go
> ==============================**==============================**=======
> --- a/src/pkg/time/tick_test.go
> +++ b/src/pkg/time/tick_test.go
> @@ -25,8 +25,13 @@
>        dt := t1.Sub(t0)
>        target := Delta * Count
>        slop := target * 2 / 10
> -       if dt < target-slop || dt > target+slop {
> -               t.Fatalf("%d %s ticks took %s, expected [%s,%s]", Count,
> Delta, dt, target-slop, target+slop)
> +       if dt < target-slop {
> +               t.Fatalf("%d %s ticks took %s; too fast", Count, Delta, dt)
> +       }
> +       if dt > target+slop {
> +               if !testing.Short() { // otherwise it fails when Delta is
> small.
> +                       t.Fatalf("%d %s ticks took %s; too long", Count,
> Delta, dt)
> +               }
>        }
>        // Now test that the ticker stopped
>        Sleep(2 * Delta)
>
>
>

[brainman, 2012-02-23 00:29:41]

On 2012/02/23 00:24:06, bradfitz wrote:
> This is a misuse of testing.Short().
> 
> If the test is flaky, the test is flaky and it should be removed.
> 

Do you propose we delete TestTicker altogether?

Alex

[bradfitz, 2012-02-23 00:41:12]

On Thu, Feb 23, 2012 at 11:29 AM, <alex.brainman@gmail.com> wrote:

> On 2012/02/23 00:24:06, bradfitz wrote:
>
>> This is a misuse of testing.Short().
>>
>
>  If the test is flaky, the test is flaky and it should be removed.
>>
>
>
> Do you propose we delete TestTicker altogether?
>

The lower bound seems reliable and all that we promise anyway.  That's
worth keeping.

I might try the upper bound test separately, re-trying it up to 10 times
and only fail if all 10 go way too long.

But that's just my opinion.

[r2, 2012-02-23 00:51:47]

yeah, make the test more robust, not more forgiving of flakiness.

-rob

[brainman, 2012-02-23 01:00:04]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-02-23 01:04:04]

oh yes, this test. i remember this test. this was the test that, when edited to
add those lines, i said would break.

LGTM

[brainman, 2012-02-23 01:08:06]

*** Submitted as http://code.google.com/p/go/source/detail?r=61839ceca21d ***

time: run full TestTicker test even during short tests

R=golang-dev, r, bradfitz, r
CC=golang-dev
http://codereview.appspot.com/5689076