Merge pull request #247 from umangachapagain/split-tests

Logcheck: Separate correct usage and migration tests

hack/tools/logcheck/main.go

@@ -17,6 +17,7 @@ limitations under the License.
 package main
 
 import (
+	"flag"
 	"fmt"
 	"go/ast"
 	"go/token"
@@ -27,21 +28,32 @@ import (
 	"golang.org/x/tools/go/analysis/singlechecker"
 )
 
-// Doc explaining the tool.
-const Doc = "Tool to check use of unstructured logging patterns."
-
-// Analyzer runs static analysis.
-var Analyzer = &analysis.Analyzer{
-	Name: "logcheck",
-	Doc:  Doc,
-	Run:  run,
+type config struct {
+	// When enabled, logcheck will ignore calls to unstructured klog methods (Info, Infof, Error, Errorf, Warningf, etc)
+	allowUnstructured bool
 }
 
 func main() {
-	singlechecker.Main(Analyzer)
+	singlechecker.Main(analyser())
+}
+
+func analyser() *analysis.Analyzer {
+	c := config{}
+	logcheckFlags := flag.NewFlagSet("", flag.ExitOnError)
+	logcheckFlags.BoolVar(&c.allowUnstructured, "allow-unstructured", c.allowUnstructured, `when enabled, logcheck will ignore calls to unstructured
+klog methods (Info, Infof, Error, Errorf, Warningf, etc)`)
+
+	return &analysis.Analyzer{
+		Name: "logcheck",
+		Doc:  "Tool to check use of unstructured logging patterns.",
+		Run: func(pass *analysis.Pass) (interface{}, error) {
+			return run(pass, &c)
+		},
+		Flags: *logcheckFlags,
+	}
 }
 
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass, c *config) (interface{}, error) {
 
 	for _, file := range pass.Files {
 
@@ -50,7 +62,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			// We are intrested in function calls, as we want to detect klog.* calls
 			// passing all function calls to checkForFunctionExpr
 			if fexpr, ok := n.(*ast.CallExpr); ok {
-				checkForFunctionExpr(fexpr, pass)
+				checkForFunctionExpr(fexpr, pass, c)
 			}
 
 			return true
@@ -60,7 +72,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 }
 
 // checkForFunctionExpr checks for unstructured logging function, prints error if found any.
-func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass) {
+func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
 
 	fun := fexpr.Fun
 	args := fexpr.Args
@@ -98,7 +110,7 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass) {
 				} else if fName == "ErrorS" {
 					isKeysValid(args[2:], fun, pass, fName)
 				}
-			} else {
+			} else if !c.allowUnstructured {
 				msg := fmt.Sprintf("unstructured logging function %q should not be used", fName)
 				pass.Report(analysis.Diagnostic{
 					Pos:     fun.Pos(),

hack/tools/logcheck/main_test.go

@@ -23,5 +23,27 @@ import (
 )
 
 func TestAnalyzer(t *testing.T) {
-	analysistest.Run(t, analysistest.TestData(), Analyzer)
+	tests := []struct {
+		name              string
+		allowUnstructured string
+		testPackage       string
+	}{
+		{
+			name:              "Allow unstructured logs",
+			allowUnstructured: "true",
+			testPackage:       "allowUnstructuredLogs",
+		},
+		{
+			name:              "Do not allow unstructured logs",
+			allowUnstructured: "false",
+			testPackage:       "doNotAllowUnstructuredLogs",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			analyzer := analyser()
+			analyzer.Flags.Set("allow-unstructured", tt.allowUnstructured)
+			analysistest.Run(t, analysistest.TestData(), analyzer, tt.testPackage)
+		})
+	}
 }

hack/tools/logcheck/testdata/src/allowUnstructuredLogs/allowUnstructuredLogs.go

@@ -0,0 +1,63 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// This fake package is created as golang.org/x/tools/go/analysis/analysistest
+// expects it to be here for loading. This package is used to test allow-unstructured
+// flag which suppresses errors when unstructured logging is used.
+// This is a test file for unstructured logging static check tool unit tests.
+
+package allowUnstructuredLogs
+
+import (
+	klog "k8s.io/klog/v2"
+)
+
+func allowUnstructuredLogs() {
+	// Structured logs
+	// Error is expected if structured logging pattern is not used correctly
+	klog.InfoS("test log")
+	klog.ErrorS(nil, "test log")
+	klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod")                // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.`
+	klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod")          // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.`
+	klog.InfoS("Starting container in a pod", "测试", "containerID")                                // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
+	klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID")                          // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
+	klog.InfoS("Starting container in a pod", 7, "containerID")                                   // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
+	klog.ErrorS(nil, "Starting container in a pod", 7, "containerID")                             // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
+	klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
+	testKey := "a"
+	klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
+	klog.InfoS("test: %s", "testname")                                      // want `structured logging function "InfoS" should not use format specifier "%s"`
+	klog.ErrorS(nil, "test no.: %d", 1)                                     // want `structured logging function "ErrorS" should not use format specifier "%d"`
+
+	// Unstructured logs
+	// Error is not expected as this package allows unstructured logging
+	klog.V(1).Infof("test log")
+	klog.Infof("test log")
+	klog.Info("test log")
+	klog.Infoln("test log")
+	klog.InfoDepth(1, "test log")
+	klog.Warning("test log")
+	klog.Warningf("test log")
+	klog.WarningDepth(1, "test log")
+	klog.Error("test log")
+	klog.Errorf("test log")
+	klog.Errorln("test log")
+	klog.ErrorDepth(1, "test log")
+	klog.Fatal("test log")
+	klog.Fatalf("test log")
+	klog.Fatalln("test log")
+	klog.FatalDepth(1, "test log")
+}

hack/tools/logcheck/testdata/data.go → hack/tools/logcheck/testdata/src/doNotAllowUnstructuredLogs/doNotAllowUnstructuredLogs.go

@@ -14,15 +14,36 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// test file for unstructured logging static check tool unit tests.
+// This fake package is created as golang.org/x/tools/go/analysis/analysistest
+// expects it to be here for loading. This package is used to test default
+// behavior which is to report errors when unstructured logging is used.
+// This is a test file for unstructured logging static check tool unit tests.
 
-package testdata
+package doNotAllowUnstructuredLogs
 
 import (
 	klog "k8s.io/klog/v2"
 )
 
-func printUnstructuredLog() {
+func doNotAllowUnstructuredLogs() {
+	// Structured logs
+	// Error is expected if structured logging pattern is not used correctly
+	klog.InfoS("test log")
+	klog.ErrorS(nil, "test log")
+	klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod")                // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.`
+	klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod")          // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.`
+	klog.InfoS("Starting container in a pod", "测试", "containerID")                                // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
+	klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID")                          // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
+	klog.InfoS("Starting container in a pod", 7, "containerID")                                   // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
+	klog.ErrorS(nil, "Starting container in a pod", 7, "containerID")                             // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
+	klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
+	testKey := "a"
+	klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
+	klog.InfoS("test: %s", "testname")                                      // want `structured logging function "InfoS" should not use format specifier "%s"`
+	klog.ErrorS(nil, "test no.: %d", 1)                                     // want `structured logging function "ErrorS" should not use format specifier "%d"`
+
+	// Unstructured logs
+	// Error is expected as this package does not allow unstructured logging
 	klog.V(1).Infof("test log")      // want `unstructured logging function "Infof" should not be used`
 	klog.Infof("test log")           // want `unstructured logging function "Infof" should not be used`
 	klog.Info("test log")            // want `unstructured logging function "Info" should not be used`
@@ -41,19 +62,3 @@ func printUnstructuredLog() {
 	klog.FatalDepth(1, "test log")   // want `unstructured logging function "FatalDepth" should not be used`
 
 }
-
-func printStructuredLog() {
-	klog.InfoS("test log")
-	klog.ErrorS(nil, "test log")
-	klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod")                // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.`
-	klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod")          // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.`
-	klog.InfoS("Starting container in a pod", "测试", "containerID")                                // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
-	klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID")                          // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.`
-	klog.InfoS("Starting container in a pod", 7, "containerID")                                   // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
-	klog.ErrorS(nil, "Starting container in a pod", 7, "containerID")                             // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value`
-	klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
-	testKey := "a"
-	klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. `
-	klog.InfoS("test: %s", "testname")                                      // want `structured logging function "InfoS" should not use format specifier "%s"`
-	klog.ErrorS(nil, "test no.: %d", 1)                                     // want `structured logging function "ErrorS" should not use format specifier "%d"`
-}