自動テストは優れものですが、書き手の認知(≒どこまで考慮できるか)に依存するものでもある気がしてます。
また、多くの場合テストはアプリケーション(≒仕様)を起点に記述されるため、非機能要件に関する部分だったり、その言語のプラクティスを守ってるどうかついてはカバーしにくいものでもあります。
私もレビューの時に、コネクションやディスクリプタの Close 忘れ、closure への変数束縛の実装ミス、共有インスタンス(singleton など)の初期化ミスなどは何度か指摘したことがありますが、この手のミスを自動テストでカバーするのは難しいなぁ、と感じることは多いです。
ので、私の場合は、レビューでこの手のミスを見つけると、そのまま linter を作っちゃうことが多いです。
最近では「キャッシュを map としてパッケージ変数に載せておく、sync.Once で lazy に初期化する」という実装をしてたためにデータに変更があったのにキャッシュが書き変わらないというバグに遭遇しました。
1回しか実行されない分には問題ないので、unit test は通っていました。
singleton が最善手である場合は、こうした実装が必要になる場面もあるかもしれないですが、大体の場合はデザインを見直した方が良い気がします。
というわけで、パッケージ変数としての sync.Once を singleton 風のデザインが使われていると考えてこれを見つける形で vet plugin を入れました。
(vet plugin や静的解析については tenten さんがたくさん解説してる ので割愛)
package main
import (
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/analysis/singlechecker"
)
const doc = `find-pkg-sync-once
パッケージ変数として sync.Once をしてるコードを検出します。
`
func main() {
singlechecker.Main(Analyzer)
}
var Analyzer = &analysis.Analyzer{
Name: "find_pkg_sync_once",
Doc: doc,
URL: "https://github.com/...",
Run: run,
RunDespiteErrors: true,
Requires: []*analysis.Analyzer{inspect.Analyzer},
}
func run(pass *analysis.Pass) (interface{}, error) {
root := pass.Pkg.Scope()
targets := map[string]bool{
"sync.Once": true,
"*sync.Once": true,
}
for _, v := range root.Names() {
if obj := root.Lookup(v); targets[obj.Type().String()] {
pass.Report(analysis.Diagnostic{
Pos: obj.Pos(),
Message: "do not use package level 'sync.Once'",
})
}
}
return nil, nil
}
たったこれだけ。
テストはこんな感じ。
package main
import (
"testing"
"golang.org/x/tools/go/analysis/analysistest"
)
var filemap = map[string]string{
"sample/a.go": `
package sample
import (
"sync"
)
var (
a = sync.Once{} // want "do not use package level 'sync.Once'"
b = &sync.Once{} // want "do not use package level 'sync.Once'"
c = "sync.Once{}"
)
var d = sync.Once{} // want "do not use package level 'sync.Once'"
var e = new(sync.Once) // want "do not use package level 'sync.Once'"
type t struct {
s sync.Once
ss *sync.Once
}
func foo() {
f := sync.Once{}
}
`,
}
func TestAnalyzer(t *testing.T) {
dir, clean, err := analysistest.WriteFiles(filemap)
if err != nil {
t.Fatal(err)
}
defer clean()
analysistest.Run(t, dir, Analyzer, "./...")
}
プロジェクトでは、これを circleci に組み込んで、 review-dog で PR comment として書き込ませてます。