From 7cd5685e2e388165f477f7e51de77760e8325cd9 Mon Sep 17 00:00:00 2001 From: miagilepner Date: Tue, 7 Feb 2023 10:41:04 +0100 Subject: [PATCH] VAULT-13169 Require go docs for all new test functions (#18971) * example for checking go doc tests * add analyzer test and action * get metadata step * install revgrep * fix for ci * add revgrep to go.mod * clarify how analysistest works --- .github/workflows/godoc-test-checker.yml | 27 +++++++ .gitignore | 2 + Makefile | 20 ++++- go.mod | 1 + go.sum | 2 + tools/godoctests/main.go | 10 +++ tools/godoctests/pkg/analyzer/analyzer.go | 78 +++++++++++++++++++ .../godoctests/pkg/analyzer/analyzer_test.go | 20 +++++ .../godoctests/pkg/analyzer/testdata/funcs.go | 17 ++++ tools/tools.go | 3 + 10 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/godoc-test-checker.yml create mode 100644 tools/godoctests/main.go create mode 100644 tools/godoctests/pkg/analyzer/analyzer.go create mode 100644 tools/godoctests/pkg/analyzer/analyzer_test.go create mode 100644 tools/godoctests/pkg/analyzer/testdata/funcs.go diff --git a/.github/workflows/godoc-test-checker.yml b/.github/workflows/godoc-test-checker.yml new file mode 100644 index 0000000000..048042cf75 --- /dev/null +++ b/.github/workflows/godoc-test-checker.yml @@ -0,0 +1,27 @@ +name: Check Go Docs for tests + +on: + pull_request: + types: [opened, synchronize] + # Runs on PRs to main + branches: + - main + +jobs: + godoc-test-check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + - name: get metadata + id: get-metadata + run: echo "go-version=$(cat ./.go-version)" >> $GITHUB_OUTPUT + - name: Set Up Go + uses: actions/setup-go@v3 + with: + cache: true + go-version: ${{ steps.get-metadata.outputs.go-version }} + - name: Verify new tests have go docs + run: make ci-vet-godoctests \ No newline at end of file diff --git a/.gitignore b/.gitignore index 83d0d703c6..cdd542d606 100644 --- a/.gitignore +++ b/.gitignore @@ -126,3 +126,5 @@ website/components/node_modules .buildcache/ .releaser/ *.log + +tools/godoctests/.bin \ No newline at end of file diff --git a/Makefile b/Makefile index 9ae95a6c2f..ac68d55f07 100644 --- a/Makefile +++ b/Makefile @@ -8,13 +8,13 @@ EXTENDED_TEST_TIMEOUT=60m INTEG_TEST_TIMEOUT=120m VETARGS?=-asmdecl -atomic -bool -buildtags -copylocks -methods -nilfunc -printf -rangeloops -shift -structtags -unsafeptr EXTERNAL_TOOLS_CI=\ - golang.org/x/tools/cmd/goimports + golang.org/x/tools/cmd/goimports \ + github.com/golangci/revgrep/cmd/revgrep EXTERNAL_TOOLS=\ github.com/client9/misspell/cmd/misspell GOFMT_FILES?=$$(find . -name '*.go' | grep -v pb.go | grep -v vendor) SED?=$(shell command -v gsed || command -v sed) - GO_VERSION_MIN=$$(cat $(CURDIR)/.go-version) PROTOC_VERSION_MIN=3.21.12 GO_CMD?=go @@ -102,6 +102,20 @@ vet: echo "and fix them if necessary before submitting the code for reviewal."; \ fi +# tools/godoctests/.bin/godoctests builds the custom analyzer to check for godocs for tests +tools/godoctests/.bin/godoctests: + @cd tools/godoctests && $(GO_CMD) build -o .bin/godoctests . + +# vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep +# which will only return an error if a new function is missing a godoc +vet-godoctests: bootstrap tools/godoctests/.bin/godoctests + @$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep + +# ci-vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep +# which will only return an error if a new function that is not on main is missing a godoc +ci-vet-godoctests: ci-bootstrap tools/godoctests/.bin/godoctests + @$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep origin/main + # lint runs vet plus a number of other checkers, it is more comprehensive, but louder lint: @$(GO_CMD) list -f '{{.Dir}}' ./... | grep -v /vendor/ \ @@ -250,7 +264,7 @@ ci-config: ci-verify: @$(MAKE) -C .circleci ci-verify -.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci +.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci vet-godoctests ci-vet-godoctests .NOTPARALLEL: ember-dist ember-dist-dev diff --git a/go.mod b/go.mod index 2fbb0ac994..de1b91e17f 100644 --- a/go.mod +++ b/go.mod @@ -314,6 +314,7 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/mock v1.6.0 // indirect github.com/golang/snappy v0.0.4 // indirect + github.com/golangci/revgrep v0.0.0-20220804021717-745bb2f7c2e6 // indirect github.com/google/flatbuffers v2.0.0+incompatible // indirect github.com/google/gnostic v0.5.7-v3refs // indirect github.com/google/go-querystring v1.1.0 // indirect diff --git a/go.sum b/go.sum index 75131d6e3b..434247095a 100644 --- a/go.sum +++ b/go.sum @@ -844,6 +844,8 @@ github.com/golang/snappy v0.0.2/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEW github.com/golang/snappy v0.0.3/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= github.com/golang/snappy v0.0.4 h1:yAGX7huGHXlcLOEtBnF4w7FQwA26wojNCwOYAEhLjQM= github.com/golang/snappy v0.0.4/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= +github.com/golangci/revgrep v0.0.0-20220804021717-745bb2f7c2e6 h1:DIPQnGy2Gv2FSA4B/hh8Q7xx3B7AIDk3DAMeHclH1vQ= +github.com/golangci/revgrep v0.0.0-20220804021717-745bb2f7c2e6/go.mod h1:0AKcRCkMoKvUvlf89F6O7H2LYdhr1zBh736mBItOdRs= github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ= github.com/google/btree v1.0.0/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ= github.com/google/btree v1.0.1 h1:gK4Kx5IaGY9CD5sPJ36FHiBJ6ZXl0kilRiiCj+jdYp4= diff --git a/tools/godoctests/main.go b/tools/godoctests/main.go new file mode 100644 index 0000000000..3eba556704 --- /dev/null +++ b/tools/godoctests/main.go @@ -0,0 +1,10 @@ +package main + +import ( + "github.com/hashicorp/vault/tools/godoctests/pkg/analyzer" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { + singlechecker.Main(analyzer.Analyzer) +} diff --git a/tools/godoctests/pkg/analyzer/analyzer.go b/tools/godoctests/pkg/analyzer/analyzer.go new file mode 100644 index 0000000000..8903f5ea5f --- /dev/null +++ b/tools/godoctests/pkg/analyzer/analyzer.go @@ -0,0 +1,78 @@ +package analyzer + +import ( + "go/ast" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "godoctests", + Doc: "Verifies that every go test has a go doc", + Run: run, + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + } + + inspector.Preorder(nodeFilter, func(node ast.Node) { + funcDecl, ok := node.(*ast.FuncDecl) + if !ok { + return + } + + // starts with 'Test' + if !strings.HasPrefix(funcDecl.Name.Name, "Test") { + return + } + + // has one parameter + params := funcDecl.Type.Params.List + if len(params) != 1 { + return + } + + // parameter is a pointer + firstParamType, ok := params[0].Type.(*ast.StarExpr) + if !ok { + return + } + + selector, ok := firstParamType.X.(*ast.SelectorExpr) + if !ok { + return + } + + // the pointer comes from package 'testing' + selectorIdent, ok := selector.X.(*ast.Ident) + if !ok { + return + } + if selectorIdent.Name != "testing" { + return + } + + // the pointer has type 'T' + if selector.Sel == nil || selector.Sel.Name != "T" { + return + } + + // then there must be a godoc + if funcDecl.Doc == nil { + pass.Reportf(node.Pos(), "Test %s is missing a go doc", + funcDecl.Name.Name) + } else if !strings.HasPrefix(funcDecl.Doc.Text(), funcDecl.Name.Name) { + pass.Reportf(node.Pos(), "Test %s must have a go doc beginning with the function name", + funcDecl.Name.Name) + } + }) + return nil, nil +} diff --git a/tools/godoctests/pkg/analyzer/analyzer_test.go b/tools/godoctests/pkg/analyzer/analyzer_test.go new file mode 100644 index 0000000000..d7c80f2df7 --- /dev/null +++ b/tools/godoctests/pkg/analyzer/analyzer_test.go @@ -0,0 +1,20 @@ +package analyzer + +import ( + "os" + "path/filepath" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +// TestAnalyzer runs the analyzer on the test functions in testdata/funcs.go. The report from the analyzer is compared against +// the comments in funcs.go beginning with "want." If there is no comment beginning with "want", then the analyzer is expected +// not to report anything. +func TestAnalyzer(t *testing.T) { + f, err := os.Getwd() + if err != nil { + t.Fatal("failed to get working directory", err) + } + analysistest.Run(t, filepath.Join(f, "testdata"), Analyzer, ".") +} diff --git a/tools/godoctests/pkg/analyzer/testdata/funcs.go b/tools/godoctests/pkg/analyzer/testdata/funcs.go new file mode 100644 index 0000000000..e6f7deab17 --- /dev/null +++ b/tools/godoctests/pkg/analyzer/testdata/funcs.go @@ -0,0 +1,17 @@ +package testdata + +import "testing" + +// Test_GoDocOK is a test that has a go doc +func Test_GoDocOK(t *testing.T) {} + +func Test_NoGoDocFails(t *testing.T) {} // want "Test Test_NoGoDocFails is missing a go doc" + +// This test does not have a go doc beginning with the function name +func Test_BadGoDocFails(t *testing.T) {} // want "Test Test_BadGoDocFails must have a go doc beginning with the function name" + +func test_TestHelperNoGoDocOK(t *testing.T) {} + +func Test_DifferentSignatureNoGoDocOK() {} + +func Test_DifferentSignature2NoGoDocOK(t *testing.T, a int) {} diff --git a/tools/tools.go b/tools/tools.go index 7458113cec..9a4972b73c 100644 --- a/tools/tools.go +++ b/tools/tools.go @@ -16,6 +16,7 @@ package tools //go:generate go install google.golang.org/protobuf/cmd/protoc-gen-go //go:generate go install google.golang.org/grpc/cmd/protoc-gen-go-grpc //go:generate go install github.com/favadi/protoc-go-inject-tag +//go:generate go install github.com/golangci/revgrep/cmd/revgrep import ( _ "golang.org/x/tools/cmd/goimports" @@ -28,4 +29,6 @@ import ( _ "google.golang.org/grpc/cmd/protoc-gen-go-grpc" _ "github.com/favadi/protoc-go-inject-tag" + + _ "github.com/golangci/revgrep/cmd/revgrep" )