code review 8366044: runtime/*: deflake tests

runtime/*: deflake tests

[albert.strasheim, 2013-04-04 20:36:25]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[dvyukov, 2013-04-04 20:41:16]

On 2013/04/04 20:36:25, albert.strasheim wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Looks good, but please describe what the problem is and wait for approval from
somebody from core team, I don't know what the commit policy currently is.

[bradfitz, 2013-04-04 20:45:35]

LGTM but put a comment above fixEnv which isn't the best-named function.
 On Apr 4, 2013 1:36 PM, <fullung@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> runtime, runtime/race: exclude GOGCTRACE from subprocess test
> environment.
>
> Please review this at
https://codereview.appspot.**com/8366044/<https://codereview.appspot.com/8366...
>
> Affected files:
>   M src/pkg/runtime/crash_test.go
>   M src/pkg/runtime/race/race_**test.go
>
>
> Index: src/pkg/runtime/crash_test.go
> ==============================**==============================**=======
> --- a/src/pkg/runtime/crash_test.**go
> +++ b/src/pkg/runtime/crash_test.**go
> @@ -14,6 +14,16 @@
>         "text/template"
>  )
>
> +func fixEnv(cmd *exec.Cmd) *exec.Cmd {
> +       for _, env := range os.Environ() {
> +               if strings.HasPrefix(env, "GOGCTRACE=") {
> +                       continue
> +               }
> +               cmd.Env = append(cmd.Env, env)
> +       }
> +       return cmd
> +}
> +
>  func executeTest(t *testing.T, templ string, data interface{}) string {
>         checkStaleRuntime(t)
>
> @@ -37,13 +47,13 @@
>         }
>         f.Close()
>
> -       got, _ := exec.Command("go", "run", src).CombinedOutput()
> +       got, _ := fixEnv(exec.Command("go", "run", src)).CombinedOutput()
>         return string(got)
>  }
>
>  func checkStaleRuntime(t *testing.T) {
>         // 'go run' uses the installed copy of runtime.a, which may be out
> of date.
> -       out, err := exec.Command("go", "list", "-f", "{{.Stale}}",
> "runtime").CombinedOutput()
> +       out, err := fixEnv(exec.Command("go", "list", "-f", "{{.Stale}}",
> "runtime")).CombinedOutput()
>         if err != nil {
>                 t.Fatalf("failed to execute 'go list': %v\n%v", err,
> string(out))
>         }
> Index: src/pkg/runtime/race/race_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/runtime/race/race_**test.go
> +++ b/src/pkg/runtime/race/race_**test.go
> @@ -147,7 +147,7 @@
>         // It is required because the tests contain a lot of data races on
> the same addresses
>         // (the tests are simple and the memory is constantly reused).
>         for _, env := range os.Environ() {
> -               if strings.HasPrefix(env, "GOMAXPROCS=") {
> +               if strings.HasPrefix(env, "GOMAXPROCS=") ||
> strings.HasPrefix(env, "GOGCTRACE=") {
>                         continue
>                 }
>                 cmd.Env = append(cmd.Env, env)
>
>
> --
>
> ---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...
> .
>
>
>

[albert.strasheim, 2013-04-05 08:36:45]

PTAL

Renamed fixEnv to testEnv with a comment. I'm bad at naming stuff like this.

I found another issue in TestReadGCStats: sometimes the garbage collector
actually does run, although the test assumes it doesn't.

The best thing I could come up with was to skip the test in that case.

[bradfitz, 2013-04-05 16:24:38]

https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/crash_test.go
File src/pkg/runtime/crash_test.go (right):

https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/crash_test.g...
src/pkg/runtime/crash_test.go:21: for _, env := range os.Environ() {
if cmd.Env != nil {
    panic("environment already set")
}

before this

https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/debug/garbag...
File src/pkg/runtime/debug/garbage_test.go (right):

https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/debug/garbag...
src/pkg/runtime/debug/garbage_test.go:29: t.Skipf("skipping due to GC during
ReadGCStats; stats.NumGC = %d, but mstats.NumGC = %d", stats.NumGC,
mstats.NumGC)
this is unrelated to this CL.  and also seems wrong.

seems better to loop a few times until you get a ReadGCStats without a GC during
it, or to just turn off the GC before it and restore it afterwards.

but not in this CL.

[albert.strasheim, 2013-04-05 17:34:17]

On 2013/04/05 16:24:38, bradfitz wrote:
>
https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/debug/garbag...
> src/pkg/runtime/debug/garbage_test.go:29: t.Skipf("skipping due to GC during
> ReadGCStats; stats.NumGC = %d, but mstats.NumGC = %d", stats.NumGC,
> mstats.NumGC)
> this is unrelated to this CL.  and also seems wrong.
> seems better to loop a few times until you get a ReadGCStats without a GC
during
> it, or to just turn off the GC before it and restore it afterwards.

The issue is that with GOGC=1 (or maybe it's GOGC=0, or both), you will always
get a GC.

Is there a way to disable GC at run time? I'm not familiar with such a toggle.

Thanks.

Albert

[bradfitz, 2013-04-07 18:00:47]

On Fri, Apr 5, 2013 at 10:34 AM, <fullung@gmail.com> wrote:

> On 2013/04/05 16:24:38, bradfitz wrote:
>
> https://codereview.appspot.**com/8366044/diff/8001/src/pkg/**
>
runtime/debug/garbage_test.go#**newcode29<https://codereview.appspot.com/8366044/diff/8001/src/pkg/runtime/debug/garbage_test.go#newcode29>
>
>> src/pkg/runtime/debug/garbage_**test.go:29: t.Skipf("skipping due to GC
>>
> during
>
>> ReadGCStats; stats.NumGC = %d, but mstats.NumGC = %d", stats.NumGC,
>> mstats.NumGC)
>> this is unrelated to this CL.  and also seems wrong.
>> seems better to loop a few times until you get a ReadGCStats without a
>>
> GC during
>
>> it, or to just turn off the GC before it and restore it afterwards.
>>
>
> The issue is that with GOGC=1 (or maybe it's GOGC=0, or both), you will
> always get a GC.
>
> Is there a way to disable GC at run time? I'm not familiar with such a
> toggle.
>

http://tip.golang.org/pkg/runtime/debug/#SetGCPercent

"SetGCPercent sets the garbage collection target percentage: a collection
is triggered when the ratio of freshly allocated data to live data
remaining after the previous collection reaches this percentage.
SetGCPercent returns the previous setting. The initial setting is the value
of the GOGC environment variable at startup, or 100 if the variable is not
set. A negative percentage disables garbage collection."

[albert.strasheim, 2013-04-07 18:32:24]

PTAL

You learn something new every day. Thanks.

[bradfitz, 2013-04-07 18:33:26]

LGTM

[bradfitz, 2013-04-07 18:37:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=3bec344c9c55 ***

runtime, runtime/debug, runtime/race: deflake tests

R=golang-dev, dvyukov, bradfitz
CC=golang-dev
https://codereview.appspot.com/8366044

Committer: Brad Fitzpatrick <bradfitz@golang.org>