Goのtestingパッケージにコミットした話

• Illustrated by egonelbre.
• The Go Gopher was designed by Renee French.

こんにちは、バックエンドエンジニアの藤井脩紀です。

今回はGoにコントリビュートすることができたのでそのお話をさせていただきたいと思います！

概要

早速コミットの内容からですが、並列実行されるテストで環境変数を設定できなくするというものです。 コード的にはtestingパッケージのT.SetenvとT.Parallelを組み合わせて呼び出すとpanicを起こすようにするという変更になります。 （正確にはもとからそういった実装になっていたのですがカバーされていないケースがあったのでその対応をしました）

• testing: T.Setenv can be called after T.Parallel · Issue #55128 · golang/go · GitHub
• testing: fail if T.Setenv is called via T.Run in a parallel test · golang/go@d6ca244 · GitHub
• testing: update description of Setenv · golang/go@a14ac86 · GitHub

以下では、コミットに至った経緯やなぜこのような変更が必要だったのかを話していきたいと思います。

経緯

修正すべき問題を見つけた経緯ですが、ある日CIでテストを回してると自身の変更と関係のない部分で失敗しておりFlakyの表示が出ていたので関連しそうなメンバーに共有したところ、同じくバックエンドエンジニアの夏さんにT.SetenvとT.Parallelの組み合わせは相性が悪いことを教えていただきました。

そこで該当するコードを見てみると以下のようにpanicを起こす実装となっていました。

func (t *T) Setenv(key, value string) {
    if t.isParallel {
        panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
    }
    // 以下略
}

https://github.com/golang/go/blob/1fc83690e68de1ce252975c5fd3a232629d6a3d6/src/testing/testing.go#L1322-L1330

しかし失敗したテストではpanicは発生しておらず、以下のようにT.Runでテストを階層化したときにこのチェックをすり抜けてしまうことが判明しました。

func Test(t *testing.T) {
    t.Parallel()
    t.Run("Sub", func(t *testing.T) {
        t.Setenv("key", "value")
    })
}

この内容を共有したところ、以下のように言っていただき「確かに！」となったのがコミットに挑戦した経緯になります。

（ちなみに言われるまでtestingパッケージを内製ライブラリだと謎の誤認をしていました）

修正が必要な理由

なぜT.SetenvとT.Parallelを組み合わせたときpanicする必要があるかというと、上述の失敗したテストのようにFlakyになってしまうためです。 どうしてFlakyになるかを知るためにはT.SetenvとT.Parallelが具体的に何をしているのか、また並列テストがどのように実行されているのかを知る必要があるので簡単に説明していきます。

説明の前に

共通の認識を持ってもらうために説明に用いる用語を列挙します。 （あくまでもこの記事内での呼称で正式なものとは限りませんのでご注意ください）

用語	説明
トップレベルテスト	パッケージ直下にTestXXXのような名前で定義されるテスト
サブテスト	T.Runにより実行されるテスト
並列テスト	T.Parallelを呼び出しているテスト
非並列テスト	T.Parallelを呼び出していないテスト

また、この他にもテストをツリー的に考えて親、子、兄弟などの表現を用います。 T.Runで呼び出されるサブテストが子で呼び出し元のテストが親、共通の親を持つ子が兄弟です。 そしてトップレベルテストはRootという共通の親を持つものと考えてください。

T.Setenvの挙動

T.Setenvは以下のように内部的にはos.Setenvを呼び出しており、そのテストが終了したタイミングで環境変数の状態を呼び出し前に戻してくれるというものになっています。 （*commonのレシーバーになっていますがT.Setenvは内部的にこちらを呼び出しています）

func (c *common) Setenv(key, value string) {
    c.checkFuzzFn("Setenv")
    prevValue, ok := os.LookupEnv(key)

    if err := os.Setenv(key, value); err != nil {
        c.Fatalf("cannot set environment variable: %v", err)
    }

    if ok {
        c.Cleanup(func() {
            os.Setenv(key, prevValue)
        })
    } else {
        c.Cleanup(func() {
            os.Unsetenv(key)
        })
    }
}

https://github.com/golang/go/blob/1fc83690e68de1ce252975c5fd3a232629d6a3d6/src/testing/testing.go#L1185-L1202

環境変数はプロセス内で共有されるため同一プロセス内で並列にテストが実行されていると他のテストにも影響するのですが、 go testコマンドの実行時はパッケージ単位でプロセスが別れるのでT.Setenvが干渉するのは同一パッケージ内のテスト同士ということになります。

例えば以下のようなテストコードがあったとして、fooパッケージはファイルが分かれていても同じプロセスIDとなり、barパッケージはfooと異なるプロセスIDとなっていることが分かります。（hogeはモジュール名）

// foo/x_test.go
package foo

func TestX(t *testing.T) {
    fmt.Printf("%d\n", os.Getpid())
}

// foo/y_test.go
package foo

func TestY(t *testing.T) {
    fmt.Printf("%d\n", os.Getpid())
}

// bar/z_test.go
package bar

func TestZ(t *testing.T) {
    fmt.Printf("%d\n", os.Getpid())
}

$ go test ./... -v
=== RUN   TestZ
22634
--- PASS: TestZ (0.00s)
PASS
ok      hoge/bar        0.097s
=== RUN   TestX
22635
--- PASS: TestX (0.00s)
=== RUN   TestY
22635
--- PASS: TestY (0.00s)
PASS
ok      hoge/foo        0.284s

T.Parallelの挙動

T.Parallelは兄弟の非並列テストが全て完了するまで待機した後に並列にテストを再開するという振る舞いをします。 例えば以下のようなテストがあるときTestBとDはTestAとCが完了するまで待機した後に並列で実行されます。（図1参照）

func TestA(t *testing.T) {}
func TestB(t *testing.T) { t.Parallel() }
func TestC(t *testing.T) {}
func TestD(t *testing.T) { t.Parallel() }

図1

この振る舞いはgo testコマンドに-vオプションをつけると実際に確認できます。

• RUN: 開始
• PAUSE: 中断
• CONT: 再開
• PASS: 終了（成功）

$ go test main_test.go -v
=== RUN   TestA
--- PASS: TestA (0.00s)
=== RUN   TestB
=== PAUSE TestB
=== RUN   TestC
--- PASS: TestC (0.00s)
=== RUN   TestD
=== PAUSE TestD
=== CONT  TestB
--- PASS: TestB (0.00s)
=== CONT  TestD
--- PASS: TestD (0.00s)
PASS
ok      command-line-arguments  0.088s

また、サブテストは親の一部とみなされるため親が非並列テストであれば兄弟間でのみ並列に実行され、親が並列テストであれば子は並列テストでなくとも兄弟以外と並列に実行される可能性があります。（図2参照）

• TestC/Sub1とTestC/Sub3が並列に実行されている間に他のテストが並列に実行されることはない
• TestD/Sub2はTestBと並列に実行されうる

func TestA(t *testing.T) {}
func TestB(t *testing.T) { t.Parallel() }
func TestC(t *testing.T) {
    t.Run("Sub1", func(t *testing.T) { t.Parallel() })
    t.Run("Sub2", func(t *testing.T) {})
    t.Run("Sub3", func(t *testing.T) { t.Parallel() })
}
func TestD(t *testing.T) {
    t.Parallel()
    t.Run("Sub1", func(t *testing.T) { t.Parallel() })
    t.Run("Sub2", func(t *testing.T) {})
    t.Run("Sub3", func(t *testing.T) { t.Parallel() })
}

図2

そして、今回問題となるのは図3のようなケースです。 TestAとBそれぞれのサブテストは自身がT.Parallelを呼び出していないためT.Setenvを呼び出すことができてしまいますが実際には他のテストと並列に実行されうるためこれを許容するとFlakyなテストになってしまう恐れがあります。

図3

以下にFlakyなテストコード例を示します。以下のTestA/SubとTestB/Subは並列に実行されうるのでタイミング次第で互いのT.Setenvが干渉してしまい結果がランダムになってしまいます。

var rnd *rand.Rand = rand.New(rand.NewSource(time.Now().UnixNano()))

func TestA(t *testing.T) {
    t.Parallel()
    t.Run("Sub", func(t *testing.T) {
        setAndGetEnv(t, "A")
    })
}

func TestB(t *testing.T) {
    t.Parallel()
    t.Run("Sub", func(t *testing.T) {
        setAndGetEnv(t, "B")
    })
}

func setAndGetEnv(t *testing.T, value string) {
    t.Setenv("key", value)
    time.Sleep(time.Duration(rnd.Intn(1000)))
    got, _ := os.LookupEnv("key")
    if got != value {
        t.Errorf("unexpected value: want=%s, got=%s", value, got)
    }
}

なお余談ですが、T.Parallelは必ずしもテストの先頭で呼ぶ必要はなく呼び出し以降のコードが並列で実行されます。ですがそれを考慮すると説明が複雑になるため本記事内では常に先頭で呼び出されるものと仮定して話しています。他にも非並列テストの実行順序が図の通りになることが保証されるかも重要ではないので確認していません。

修正内容

ここまでの説明で修正が必要な理由はご理解いただけたかと思います（そうだと嬉しい）。 そして修正方法も察しがつくと思いますが至極単純で以下のようにT.Setenvを呼び出した際にテストツリーを遡って先祖に並列テストがいればpanicするという内容です。 これでT.Runでテストを階層化しても他のテストと並列に実行されうるテストではT.Setenvを呼び出せなくなりました。

func (t *T) Setenv(key, value string) {
    isParallel := false
    for c := &t.common; c != nil; c = c.parent {
        if c.isParallel {
            isParallel = true
            break
        }
    }
    if isParallel {
        panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
    }
    // 以下略
}

https://github.com/golang/go/blob/6a72514076f6e2be54af267959e3098693e02980/src/testing/testing.go#L1401-L1421

Goへのコントリビュート方法

コミット内容の説明は以上なので、あとはGoへのコントリビュート手順を紹介していきます。

Contribution Guide - The Go Programming Language

詳細な手順は上記のガイドに載っているので説明しませんが大雑把にいうと以下のような手順になります。

1. 任意のGoogleアカウントでGerrit（コードレビューツール、GitHubのようなもの）に登録
2. GitHubでIssueを作成して問題点や修正の方向性などを相談
3. あとはコミットを作成してレビューに応じて修正

少し通常のGitと異なる点として、コミットの作成とプッシュにはセットアップ手順を踏むと利用できるようになるgit codereviewコマンドを利用する必要があります。

コマンド	説明
git codereview change	変更を常に単一のコミットにまとめつつ必要な情報をコミットメッセージに追加してくれる（git commitのような役割）
git codereview mail	変更をGerritに送信してくれる（ git pushのような役割）

git codereview mailコマンドの初回実行時、以下のようなページが作成されるのでそこでレビューを受けつつApproveがもらえるまで修正を行うという流れになります。 （2つあるのはドキュメンテーションコメントの変更を分けるように指示されたためです）

• https://go-review.googlesource.com/c/go/+/431101
• https://go-review.googlesource.com/c/go/+/434115

なお、このPR（Pull Request）のようなものをCL（Change List）と呼ぶそうです。コミットは常に1つになるので「List？」と思いましたが以下のようにレビュアーに直接お伺いしたので正しいはずです。

I would like to know the meaning of "CL", is it an abbreviation for "Change List"? Does this mean creating a new commit? (Is it like PR?)

訳: 「CL」の意味を知りたいのですが、それは「Change List」の略でしょうか？新しいコミットを作成するという意味ですか？（PRのように？）

※直前にドキュメンテーションコメント変更用のCLを別に作ってくださいという旨の発言をされた上での質問です

Yes, “CL” is “change list”. (The term comes from the Perforce version control system, which was used for early development on the Go project.) (Somewhat confusingly, a Gerrit CL gets merged as only one commit per CL in the Go repo.)

訳: そうです、「CL」は「Change List」です。（この用語はGoプロジェクトの開発初期に使用されていたバージョン管理システムのPerforceから来ています）（ちょっと紛らわしいですが、GerritのCLはGoリポジトリでは単一のコミットとしてマージされます）

そしてレビュアー次第かもしれませんが、レスポンスは非常に早かった印象です。ただ、修正要求に対応できたら解決済みにチェックをつけなければいけなかったのですが、それを知らなかったためか一度だけ数日レスポンスがない期間があったので今後機会がある方は忘れないようにしましょう。

レビューで特にありがたかったのはコメントなどの英文が怪しいと具体的な修正版のテキストを提示してもらえたことで英語が苦手な自分としては非常に助かりました。

ちなみにGoのリリースサイクルは半年おきで前半の開発期間と後半のコードフリーズ期間に分けられており、開発期間にのみ変更を受け入れているのでコードフリーズ期間には不具合修正とドキュメント更新以外は受け付けられていません。私のコミットが取り込まれたのは今年の9月なのでリリース版に含まれるのは来年の2月になる予定かと思います。

Go Release Cycle · golang/go Wiki · GitHub

ちょっとした罠

今回の修正中に一つだけハマった点があるので共有しておきます。

Goのリポジトリには全てのテストを実行するためのall.bashというスクリプトが含まれています。 ただ、こちらのスクリプトはもちろん実行に時間がかかるため関連するユニットテストだけを確認するためにgo testコマンドを用いたのですが、変更が反映されている気配がなく困惑しました。これはgoコマンドが参照する標準ライブラリが$GOROOT配下のものになっているためで、クローンしたリポジトリ内のソースコードをいくら書き換えても参照されていないことが原因でした。$GOROOTをクローンしたリポジトリに変更しようかとも考えましたが、バイナリとソースコードのバージョンに差異があると問題が起きそうな気がしたので別の方法を探した結果、クローンしたリポジトリのbinディレクトリ内に生成されていたgoコマンドを利用すればよいことが分かりました。

全てが解決してから気づいたのですが、このことはガイドの「Quickly testing your changes」というセクションに明記されておりドキュメントはちゃんと読み込まないといけないというよい教訓になりました。

In this section, we'll call the directory into which you cloned the Go repository $GOROOT. The go tool built by $GOROOT/src/make.bash will be installed in $GOROOT/bin/go and you can invoke it to test your code. For instance, if you have modified the compiler and you want to test how it affects the test suite of your own project, just run go test using it:

訳: このセクションでは、あなたがGoリポジトリをクローンしたディレクトリを$GOROOTと呼ぶことにします。$GOROOT/src/make.bashによりビルドされたGoツールが$GOROOT/bin/goにインストールされてコードをテストするために呼び出せるようになります。例えば、コンパイラを修正してそれが自分のプロジェクトのテストスイートにどのように影響するかをテストしたいのなら、それを使用してgo testを実行するだけです。

並列化について

上記で述べたようにテストはパッケージ単位に個別のプロセスで実行されるため、ある程度は特に意識することなく並列実行できるようになっています。しかしながら何もしないと同一パッケージ内では非並列にテストが実行されるため、ミラティブのバックエンドではT.Parallelの呼び出しを原則必須化することでテスト環境のCPUコア数を上げた時にさらに効率よくCPUリソースを使い切れるようにしています。

また、T.Parallelの呼び出し漏れを検知してくれるリンターを内製しているため意図しない漏れが生じることはありません。バックエンドチームではこのように規約などをリンター化することで形骸化させることなく強制力を高める取り組みを積極的に行っており、入社当時からこれはとてもよい方針だなと感じています（ドキュメントに書かれているだけの規約を組織単位で守るというのが如何に困難なことか分かっていただける方は多いのではないでしょうか）。

関連して、テストの並列数を指定する方法も説明しておきます。基本的にはgo testコマンドのオプションとして指定するのですが、関連するオプションは-pと-parallelの2種類あります。go helpの説明も載せますが長いので訳はなしで要約して説明すると以下の通りです。

• -pは-parallelの短縮形ではなく別のオプション
• どちらもデフォルトはGOMAXPROCS（通常は利用可能なCPUの数）
• -pはプロセス単位の並列数（最大いくつのパッケージを並列に実行するか）
• -parallelはテスト単位の並列数（最大いくつの並列テストを実行するか）

-p n
    the number of programs, such as build commands or
    test binaries, that can be run in parallel.
    The default is GOMAXPROCS, normally the number of CPUs available.

（go help buildより抜粋、go buildのオプションはgo testにも共有される）

-parallel n
    Allow parallel execution of test functions that call t.Parallel, and
    fuzz targets that call t.Parallel when running the seed corpus.
    The value of this flag is the maximum number of tests to run
    simultaneously.
    While fuzzing, the value of this flag is the maximum number of
    subprocesses that may call the fuzz function simultaneously, regardless of
    whether T.Parallel is called.
    By default, -parallel is set to the value of GOMAXPROCS.
    Setting -parallel to values higher than GOMAXPROCS may cause degraded
    performance due to CPU contention, especially when fuzzing.
    Note that -parallel only applies within a single test binary.
    The 'go test' command may run tests for different packages
    in parallel as well, according to the setting of the -p flag
    (see 'go help build').

（go help testflagより抜粋）

上記のように並列数はデフォルトでよしなな値になっているはずなので、何らかの事情がない限り明示的に指定する必要はなさそうです。むしろ利用可能なCPUの数を上回ると疑似的な並列実行となりパフォーマンスが低下する可能性も考えられます。

まとめ

これまでOSSへのコントリビュートはドキュメント系しかしたことがなかったのですが今回初めて機能関連のコミットを行うことができました。 しかもGoという有名なOSSのコントリビュータになれたのはとても嬉しいです。ただ、Goコミッターを名乗るとコンパイラにコミットしたと想像する人もそれなりにいるんじゃないかなと思い、考えすぎかもしれませんが誇大表現にならないようなタイトルにしました。

それでもFlakyなテストというのは原因が分かりにくい上にテストにかかる時間が長ければ長いほど再実行に時間がかかりエンジニアの時間を無駄に奪ってしまうものなのでその穴を塞げたというのは価値のあることだと信じています。

少し長い記事になってしまいましたが、ここまでのご拝読に感謝します。 憎いFlakyテストの根本にはコミットチャンスというお宝が眠っているかもしれない、そんなお話でした！

さいごに

ミラティブではバックエンドエンジニアを含め複数のポジションで力を貸してくださる方々を募集しています！

www.mirrativ.co.jp

mirrativ.notion.site