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

@@ -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

.gitignore

@@ -126,3 +126,5 @@ website/components/node_modules
 .buildcache/
 .releaser/
 *.log
+
+tools/godoctests/.bin

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
 

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

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=

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)
+}

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
+}

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, ".")
+}

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) {}

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"
 )