code review 10146043: net/http/httputil: disable flaky TestChunkReaderAllocs

net/http/httputil: disable flaky TestChunkReaderAllocs

This test has been flaky for almost 2 weeks.

Update issue 5000
Update issue 5608

[albert.strasheim, 2013-06-10 11:56:53]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-06-10 15:50:19]

Why not just do a t.Skip when GOMAXPROCS > 1 instead, printing a message
out with a reference to the open bug(s)?



On Mon, Jun 10, 2013 at 1:56 PM, <fullung@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> net/http/httputil: disable flaky TestChunkReaderAllocs
>
> This test has been flaky for almost 2 weeks.
>
> Update issue 5000
> Update issue 5608
>
> Please review this at
https://codereview.appspot.**com/10146043/<https://codereview.appspot.com/101...
>
> Affected files:
>   M src/pkg/net/http/httputil/**chunked_test.go
>
>
> Index: src/pkg/net/http/httputil/**chunked_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/httputil/**chunked_test.go
> +++ b/src/pkg/net/http/httputil/**chunked_test.go
> @@ -43,7 +43,7 @@
>         }
>  }
>
> -func TestChunkReaderAllocs(t *testing.T) {
> +func DisabledTestChunkReaderAllocs(**t *testing.T) {
>         // temporarily set GOMAXPROCS to 1 as we are testing memory
> allocations
>         defer runtime.GOMAXPROCS(runtime.**GOMAXPROCS(1))
>         var buf bytes.Buffer
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[iant, 2013-06-10 17:03:25]

On Mon, Jun 10, 2013 at 8:50 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Why not just do a t.Skip when GOMAXPROCS > 1 instead, printing a message out
> with a reference to the open bug(s)?

The test already sets GOMAXPROCS to 1, doesn't it?

But I agree that renaming the function is not the right way to disable a test.

Ian

> On Mon, Jun 10, 2013 at 1:56 PM, <fullung@gmail.com> wrote:
>>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> net/http/httputil: disable flaky TestChunkReaderAllocs
>>
>> This test has been flaky for almost 2 weeks.
>>
>> Update issue 5000
>> Update issue 5608
>>
>> Please review this at https://codereview.appspot.com/10146043/
>>
>> Affected files:
>>   M src/pkg/net/http/httputil/chunked_test.go
>>
>>
>> Index: src/pkg/net/http/httputil/chunked_test.go
>> ===================================================================
>> --- a/src/pkg/net/http/httputil/chunked_test.go
>> +++ b/src/pkg/net/http/httputil/chunked_test.go
>> @@ -43,7 +43,7 @@
>>         }
>>  }
>>
>> -func TestChunkReaderAllocs(t *testing.T) {
>> +func DisabledTestChunkReaderAllocs(t *testing.T) {
>>         // temporarily set GOMAXPROCS to 1 as we are testing memory
>> allocations
>>         defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1))
>>         var buf bytes.Buffer
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[albert.strasheim, 2013-06-10 18:52:50]

Hello

On 2013/06/10 17:03:25, iant wrote:
> On Mon, Jun 10, 2013 at 8:50 AM, Brad Fitzpatrick <mailto:bradfitz@golang.org>
wrote:
> > Why not just do a t.Skip when GOMAXPROCS > 1 instead, printing a message out
> > with a reference to the open bug(s)?
> The test already sets GOMAXPROCS to 1, doesn't it?
> But I agree that renaming the function is not the right way to disable a test.

So basically this test can't pass reliably at present.

So how about:

t.Skip("skipped; see issue 5000 and issue 5608")

?

[iant, 2013-06-10 19:41:46]

On Mon, Jun 10, 2013 at 11:52 AM,  <fullung@gmail.com> wrote:
>
> So basically this test can't pass reliably at present.
>
> So how about:
>
> t.Skip("skipped; see issue 5000 and issue 5608")
>
> ?

Recently testing.AllocsPerRun was changed to be more stable.  Can we
rewrite this test to use AllocsPerRun?

I would be OK with disabling the test if there were an issue to fix
some underlying code that this test relies on.  But the issues you
mention are issues about the test and the test framework, not the
underlying code.  I don't see the point of disabling a test when the
problem is with the test, because it will never be fixed.  If we're
going to do that, we may as well simply delete the test.

Ian

[bradfitz, 2013-06-17 20:26:55]

Q=wait

[gobot, 2013-12-20 16:25:57]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2013-12-26 20:42:54]

R=close

I haven't seen this be flaky or at least lately.  If it's flaky, I'd like to
understand why and fix the real reason.