gopls/internal/golang: improve test package name selection for new file

This commit refines the logic for determining the package name of new
test files in gopls.

Previously, when creating a new test file, gopls blindly choose x_test
package. This commit expands the logic to consider the target function's
signature.

For golang/vscode-go#1594

Change-Id: Ia78003bf007479e48861ce643c8c7c366ff960a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/629978
Auto-Submit: Hongxiang Jiang <hxjiang@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: h9jiang

gopls/internal/golang/addtest.go

@@ -97,7 +97,7 @@ func {{.TestFuncName}}(t *{{.TestingPackageName}}.T) {
       {{- $last := last .Receiver.Constructor.Results}}
       {{- if eq $last.Type "error"}}
       if err != nil {
-        t.Fatalf("could not contruct receiver type: %v", err)
+        t.Fatalf("could not construct receiver type: %v", err)
       }
       {{- end}}
       {{- else}}
@@ -309,13 +309,30 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.
 		// edits contains all the text edits to be applied to the test file.
 		edits []protocol.TextEdit
 		// xtest indicates whether the test file use package x or x_test.
-		// TODO(hxjiang): For now, we try to interpret the user's intention by
-		// reading the foo_test.go's package name. Instead, we can discuss the option
-		// to interpret the user's intention by which function they are selecting.
-		// Have one file for x_test package testing, one file for x package testing.
+		// TODO(hxjiang): We can discuss the option to interpret the user's
+		// intention by which function they are selecting. Have one file for
+		// x_test package testing, one file for x package testing.
 		xtest = true
 	)
 
+	start, end, err := pgf.RangePos(loc.Range)
+	if err != nil {
+		return nil, err
+	}
+
+	path, _ := astutil.PathEnclosingInterval(pgf.File, start, end)
+	if len(path) < 2 {
+		return nil, fmt.Errorf("no enclosing function")
+	}
+
+	decl, ok := path[len(path)-2].(*ast.FuncDecl)
+	if !ok {
+		return nil, fmt.Errorf("no enclosing function")
+	}
+
+	fn := pkg.TypesInfo().Defs[decl.Name].(*types.Func)
+	sig := fn.Signature()
+
 	testPGF, err := snapshot.ParseGo(ctx, testFH, parsego.Header)
 	if err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
@@ -352,7 +369,53 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.
 			header.WriteString("\n\n")
 		}
 
-		fmt.Fprintf(&header, "package %s_test\n", pkg.Types().Name())
+		// Determine if a new test file should use in-package test (package x)
+		// or external test (package x_test). If any of the function parameters
+		// reference an unexported object, we cannot write out test cases from
+		// an x_test package.
+		externalTestOK := func() bool {
+			if !fn.Exported() {
+				return false
+			}
+			if fn.Signature().Recv() != nil {
+				if _, ident, _ := goplsastutil.UnpackRecv(decl.Recv.List[0].Type); ident == nil || !ident.IsExported() {
+					return false
+				}
+			}
+			refsUnexported := false
+			ast.Inspect(decl, func(n ast.Node) bool {
+				// The original function refs to an unexported object from the
+				// same package, so further inspection is unnecessary.
+				if refsUnexported {
+					return false
+				}
+				switch t := n.(type) {
+				case *ast.BlockStmt:
+					// Avoid inspect the function body.
+					return false
+				case *ast.Ident:
+					// Use test variant (package foo) if the function signature
+					// references any unexported objects (like types or
+					// constants) from the same package.
+					// Note: types.PkgName is excluded from this check as it's
+					// always defined in the same package.
+					if obj, ok := pkg.TypesInfo().Uses[t]; ok && !obj.Exported() && obj.Pkg() == pkg.Types() && !is[*types.PkgName](obj) {
+						refsUnexported = true
+					}
+					return false
+				default:
+					return true
+				}
+			})
+			return !refsUnexported
+		}
+
+		xtest = externalTestOK()
+		if xtest {
+			fmt.Fprintf(&header, "package %s_test\n", pkg.Types().Name())
+		} else {
+			fmt.Fprintf(&header, "package %s\n", pkg.Types().Name())
+		}
 
 		// Write the copyright and package decl to the beginning of the file.
 		edits = append(edits, protocol.TextEdit{
@@ -412,24 +475,6 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.
 		return p.Name()
 	}
 
-	start, end, err := pgf.RangePos(loc.Range)
-	if err != nil {
-		return nil, err
-	}
-
-	path, _ := astutil.PathEnclosingInterval(pgf.File, start, end)
-	if len(path) < 2 {
-		return nil, fmt.Errorf("no enclosing function")
-	}
-
-	decl, ok := path[len(path)-2].(*ast.FuncDecl)
-	if !ok {
-		return nil, fmt.Errorf("no enclosing function")
-	}
-
-	fn := pkg.TypesInfo().Defs[decl.Name].(*types.Func)
-	sig := fn.Signature()
-
 	if xtest {
 		// Reject if function/method is unexported.
 		if !fn.Exported() {
@@ -438,7 +483,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.
 
 		// Reject if receiver is unexported.
 		if sig.Recv() != nil {
-			if _, ident, _ := goplsastutil.UnpackRecv(decl.Recv.List[0].Type); !ident.IsExported() {
+			if _, ident, _ := goplsastutil.UnpackRecv(decl.Recv.List[0].Type); ident == nil || !ident.IsExported() {
 				return nil, fmt.Errorf("cannot add external test for method %s.%s as receiver type is not exported", ident.Name, decl.Name)
 			}
 		}

gopls/internal/test/marker/testdata/codeaction/addtest.txt

@@ -100,12 +100,22 @@ func Foo(in string) string {return in} //@codeaction("Foo", "source.addTest", ed
 -- missingtestfile/missingtestfile.go --
 package main
 
+type Bar struct {}
+
+type foo struct {}
+
 func ExportedFunction(in string) string {return in} //@codeaction("ExportedFunction", "source.addTest", edit=missing_test_file_exported_function)
 
-type Bar struct {}
+func UnexportedInputParam(in string, f foo) string {return in} //@codeaction("UnexportedInputParam", "source.addTest", edit=missing_test_file_function_unexported_input)
+
+func unexportedFunction(in string) string {return in} //@codeaction("unexportedFunction", "source.addTest", edit=missing_test_file_unexported_function)
 
 func (*Bar) ExportedMethod(in string) string {return in} //@codeaction("ExportedMethod", "source.addTest", edit=missing_test_file_exported_recv_exported_method)
 
+func (*Bar) UnexportedInputParam(in string, f foo) string {return in} //@codeaction("UnexportedInputParam", "source.addTest", edit=missing_test_file_method_unexported_input)
+
+func (*foo) ExportedMethod(in string) string {return in} //@codeaction("ExportedMethod", "source.addTest", edit=missing_test_file_unexported_recv)
+
 -- @missing_test_file_exported_function/missingtestfile/missingtestfile_test.go --
 @@ -0,0 +1,26 @@
 +package main_test
@@ -164,6 +174,112 @@ func (*Bar) ExportedMethod(in string) string {return in} //@codeaction("Exported
 +    })
 +  }
 +}
+-- @missing_test_file_function_unexported_input/missingtestfile/missingtestfile_test.go --
+@@ -0,0 +1,24 @@
++package main
++
++import "testing"
++
++func TestUnexportedInputParam(t *testing.T) {
++  tests := []struct {
++    name string // description of this test case
++    // Named input parameters for target function.
++    in string
++    f foo
++    want string
++  }{
++    // TODO: Add test cases.
++  }
++  for _, tt := range tests {
++    t.Run(tt.name, func(t *testing.T) {
++      got := UnexportedInputParam(tt.in, tt.f)
++      // TODO: update the condition below to compare got with tt.want.
++      if true {
++        t.Errorf("UnexportedInputParam() = %v, want %v", got, tt.want)
++      }
++    })
++  }
++}
+-- @missing_test_file_method_unexported_input/missingtestfile/missingtestfile_test.go --
+@@ -0,0 +1,26 @@
++package main
++
++import "testing"
++
++func TestBar_UnexportedInputParam(t *testing.T) {
++  tests := []struct {
++    name string // description of this test case
++    // Named input parameters for target function.
++    in string
++    f foo
++    want string
++  }{
++    // TODO: Add test cases.
++  }
++  for _, tt := range tests {
++    t.Run(tt.name, func(t *testing.T) {
++      // TODO: construct the receiver type.
++      var b Bar
++      got := b.UnexportedInputParam(tt.in, tt.f)
++      // TODO: update the condition below to compare got with tt.want.
++      if true {
++        t.Errorf("UnexportedInputParam() = %v, want %v", got, tt.want)
++      }
++    })
++  }
++}
+-- @missing_test_file_unexported_function/missingtestfile/missingtestfile_test.go --
+@@ -0,0 +1,23 @@
++package main
++
++import "testing"
++
++func Test_unexportedFunction(t *testing.T) {
++  tests := []struct {
++    name string // description of this test case
++    // Named input parameters for target function.
++    in string
++    want string
++  }{
++    // TODO: Add test cases.
++  }
++  for _, tt := range tests {
++    t.Run(tt.name, func(t *testing.T) {
++      got := unexportedFunction(tt.in)
++      // TODO: update the condition below to compare got with tt.want.
++      if true {
++        t.Errorf("unexportedFunction() = %v, want %v", got, tt.want)
++      }
++    })
++  }
++}
+-- @missing_test_file_unexported_recv/missingtestfile/missingtestfile_test.go --
+@@ -0,0 +1,25 @@
++package main
++
++import "testing"
++
++func Test_foo_ExportedMethod(t *testing.T) {
++  tests := []struct {
++    name string // description of this test case
++    // Named input parameters for target function.
++    in string
++    want string
++  }{
++    // TODO: Add test cases.
++  }
++  for _, tt := range tests {
++    t.Run(tt.name, func(t *testing.T) {
++      // TODO: construct the receiver type.
++      var f foo
++      got := f.ExportedMethod(tt.in)
++      // TODO: update the condition below to compare got with tt.want.
++      if true {
++        t.Errorf("ExportedMethod() = %v, want %v", got, tt.want)
++      }
++    })
++  }
++}
 -- xpackagetestfile/xpackagetestfile.go --
 package main
 
@@ -519,7 +635,7 @@ package main
 +    t.Run(tt.name, func(t *testing.T) {
 +      q, err := newQux1()
 +      if err != nil {
-+        t.Fatalf("could not contruct receiver type: %v", err)
++        t.Fatalf("could not construct receiver type: %v", err)
 +      }
 +      got := q.method(tt.in)
 +      // TODO: update the condition below to compare got with tt.want.
@@ -877,7 +993,7 @@ func (*ReturnPtrError) Method(in string) string {return in} //@codeaction("Metho
 +    t.Run(tt.name, func(t *testing.T) {
 +      r, err := main.NewReturnTypeError()
 +      if err != nil {
-+        t.Fatalf("could not contruct receiver type: %v", err)
++        t.Fatalf("could not construct receiver type: %v", err)
 +      }
 +      got := r.Method(tt.in)
 +      // TODO: update the condition below to compare got with tt.want.
@@ -938,7 +1054,7 @@ func (*ReturnPtrError) Method(in string) string {return in} //@codeaction("Metho
 +    t.Run(tt.name, func(t *testing.T) {
 +      r, err := main.NewReturnPtrError()
 +      if err != nil {
-+        t.Fatalf("could not contruct receiver type: %v", err)
++        t.Fatalf("could not construct receiver type: %v", err)
 +      }
 +      got := r.Method(tt.in)
 +      // TODO: update the condition below to compare got with tt.want.
@@ -1018,7 +1134,7 @@ func (*Bar) Method(in string) string {return in} //@codeaction("Method", "source
 +    t.Run(tt.name, func(t *testing.T) {
 +      b, err := main.ABar()
 +      if err != nil {
-+        t.Fatalf("could not contruct receiver type: %v", err)
++        t.Fatalf("could not construct receiver type: %v", err)
 +      }
 +      got := b.Method(tt.in)
 +      // TODO: update the condition below to compare got with tt.want.
@@ -1403,7 +1519,7 @@ var local renamedctx.Context
 +    t.Run(tt.name, func(t *testing.T) {
 +      f, err := main.NewFoo(renamedctx.Background())
 +      if err != nil {
-+        t.Fatalf("could not contruct receiver type: %v", err)
++        t.Fatalf("could not construct receiver type: %v", err)
 +      }
 +      got, got2, got3 := f.Method(renamedctx.Background(), "", "")
 +      // TODO: update the condition below to compare got with tt.want.