Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: migrate test generation features to gopls #1594

Open
emil14 opened this issue Jun 28, 2021 · 18 comments
Open

tools: migrate test generation features to gopls #1594

emil14 opened this issue Jun 28, 2021 · 18 comments

Comments

@emil14
Copy link

emil14 commented Jun 28, 2021

Is your feature request related to a problem? Please describe.
I'm always frustrated when I click on "Generate test for function" and see "No tests were generated for ...".

Describe the solution you'd like
I would like to see a reason of why there was no tests were generated. For example - "test for ... already exist".

Additional context
It would be perfect to see a location of generated tests if there is already existing one. Or after successful generation.

@gopherbot gopherbot added this to the Untriaged milestone Jun 28, 2021
@stamblerre stamblerre modified the milestones: Untriaged, Backlog Jun 28, 2021
@stamblerre stamblerre changed the title Add info to "No tests generated for" message Add feedback if test generation fails Jun 28, 2021
@stamblerre stamblerre added the HelpWanted Issues that are not prioritized by the maintainers. Help is requested from community contributors. label Jun 28, 2021
@n1lesh
Copy link
Contributor

n1lesh commented Jul 13, 2021

@stamblerre I would like to work on this if no one's working.

@stamblerre
Copy link
Contributor

Sounds good! /cc @hyangah to confirm that this would be a good thing to work on

@n1lesh
Copy link
Contributor

n1lesh commented Jul 21, 2021

Please correct me if I'm wrong, to generate tests vscode-go calls gotests lib internally as a child process.

However, gotests doesn't return any error if test(s) don't get generated other than "No tests were generated for...". The generateTest method (from gotests) ignores the func if test is already generated for it, and returns no error.

Probably modifying gotests to return an error for existing tests would be a better idea?

@stamblerre
Copy link
Contributor

@n1lesh: Thanks for digging into the underlying cause! An alternative option may actually be to move the test generation logic into gopls itself, so that we can adjust the behavior more easily. Would you be willing to work on a gopls change that adds a command to generate tests instead? I know that's a bigger ask than this fix, but I'd be happy to offer pointers if you're interested.

@n1lesh
Copy link
Contributor

n1lesh commented Jul 23, 2021

@stamblerre I can work on the gopls change.

@hyangah hyangah changed the title Add feedback if test generation fails tools: migrate test generation features to gopls Jul 28, 2021
@suzmue suzmue self-assigned this Jan 11, 2022
@hyangah hyangah removed the HelpWanted Issues that are not prioritized by the maintainers. Help is requested from community contributors. label Feb 8, 2022
@suzmue suzmue removed their assignment Feb 8, 2022
@h9jiang
Copy link
Member

h9jiang commented Oct 7, 2024

The Gopls will handle the add unit tests request following steps below:

  • Gopls respond to code action request and add three new buttons including add unit test for selected functions, functions in this file and functions in this package. The implementation of those three buttons are another LSP rpc call add_unit_test to gopls.
image
  • The add unit test for functions button will only show up when the select range overlap with a top level (file level not inline) function/method declaration.
image
  • When user click on the button and request for unit test generation, the add_unit_test executeCommand rpc will be invoked and gopls will return WorkspaceEdit as response.

  • As documented, WorkspaceEdit allows us to return TextDocumentEdit | CreateFile. Gopls will return CreateFile only if the test file does not exist already and will follow the right sequence (create file, then edit the text).

NOTE:

  • The screenshot is only for demonstration purposes (is my local experiment).
  • The test file name will follow the convention FILE.go -> FILE_test.go.
  • The test name will follow the same convention as gotest. See method TestName defined in gotest repo for detail implementation.
  • For un-exported function, the test function name is Test_function(t *testing.T).
  • For exported function, the test function name is TestFunction(t *testing.T)
  • For un-exported struct's method. Test_foo_function(t *testing.T) and Test_foo_Function(t *testing.T)
  • For exported struct's method. TestFoo_function(t *testing.T) and TestFoo_Function(t *testing.T)
  • The receiver's type (pointer or struct) does not make any difference to the test function name.

@hyangah @adonovan @findleyr

@findleyr
Copy link
Contributor

findleyr commented Oct 7, 2024

Cool! A few notes:

Gopls respond to code action request and add three new buttons including add unit test

Let's just say "Add unit test for X". No need to prefix the code action with "Source:", since it is in the "Source Action" section.

When user click on the button and request for unit test generation, the add_unit_test grpc will be invoked and gopls will return WorkspaceEdit as response.

s/grpc/rpc. Also, the command does not return a workspace edit. Rather it calls the workspace/applyEdit server->client rpc.

Test function naming LGTM.

It may be simpler to start with just "Add unit test for function".

@adonovan
Copy link
Member

adonovan commented Oct 7, 2024

Excited to use this! A couple of suggestions:

  • The wording should be "Add" (since there may already be one), should avoid "unit", and should name the symbol. No "Source:" prefix is needed. So: Add test for function F or Add test for method T.M.
  • It should only pop up if the symbol is exported. We shouldn't encourage writing tests of internals.

@h9jiang
Copy link
Member

h9jiang commented Oct 18, 2024

Add a source code action for single function. if the selected range is enclosed by a function decl or a method decl, we will provide this source code action to Add test for FUNCTION_NAME

Right now, the result of this action is returning a fake error.

image image

@findleyr
Copy link
Contributor

Cool! Should probably be "Add a test for Bar", but we can discuss that on the CL. This will be great.

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/621056 mentions this issue: gopls/internal/golang: add source code action for add test

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/621096 mentions this issue: gopls/internal/golang: create test file if not exist

@h9jiang
Copy link
Member

h9jiang commented Oct 21, 2024

Next CL: Following the existing implementation from gotest. When adding for a method, first declare a struct have all the fields as the receiver type.

For embedding type, we will choose the type name as fields name. E.g. Field Node for type ast.Node since struct bar does not provide a name for field which have type ast.Node.

func Test_foo_String(t *testing.T) {
	type fields struct {                  // <----- fields declaration.                
		Node       ast.Node 
		barinside  barinside
		inside     int
		insidecopy int
	}              
        ...
...
type bar struct {
	ast.Node
	barinside
	inside     int
	insidecopy int
}

type foo bar

func (*foo) String() string {return ""}

The end goal of the test looks below. (This is generate by the gotest)

func Test_foo_String(t *testing.T) {
	type fields struct {                          
		Node       ast.Node
		barinside  barinside
		inside     int
		insidecopy int
	}
	tests := []struct {
		name   string
		fields fields
		want   string
	}{
		// TODO: Add test cases.
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			f := &foo{
				Node:       tt.fields.Node,
				barinside:  tt.fields.barinside,
				inside:     tt.fields.inside,
				insidecopy: tt.fields.insidecopy,
			}
			if got := f.String(); got != tt.want {
				t.Errorf("foo.String() = %v, want %v", got, tt.want)
			}
		})
	}
}

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/621675 mentions this issue: gopls/internal/golang: add type decl including receiver's type fields

@h9jiang
Copy link
Member

h9jiang commented Oct 22, 2024

To update this issue with some decisions:

  1. If the FILE_test.go does not exist, we will generate FILE_test.go with package PACKAGE_test. This follows the best practices where we want to test only the public APIs.
  2. If the FILE_test.go does exist, we will generate test based on the specified package. If the package name is PACKAGE, we will allow generating un-exported functions or methods. If the package name is PACKAGE_test, we will generate only exported functions or methods. If the package name is not PACKAGE or PACKAGE_test, we will not generate any test for any functions/methods.
  3. When adding test for a method, we will try to figure out the constructor instead of composite literals. For T's method, we will try to find any function that match one of the following signature (only allow if the constructor returns at most two values)
func NewT(....) T // returns T
func NewT(....) (T, any) // returns T, any 
func NewT(....) *T // returns *T
func NewT(....) (*T, any) // returns *T, any
  1. If there are multiple return values from the constructor, we can try our best to handle if the second return value is error t, err := NewT(...) otherwise, drop using t, _ := NewT(...)
  2. When adding test for PACKAGE, we will allow constructor newT.
  3. After getting the return value from the functions/methods, we will leave the comparison to the user. The comparison is entirely determined by the context, we can never find an ideal comparison function between reflect.DeepEqual, ==, cmp.Diff. We will leave a comment and ask the user to write the comparison function. (except for the error return type, will explain in the next point)
      // TODO: update the condition below to compare with test.want.
      if true {
        t.Errorf("Foo(%d) = %s, want %s", test.in, got, test.want)
      }
  1. If the function/method returns an error, there are some special handling. The test struct will have a field wantErr bool and correspondingly, a comparison block will be created to make sure the wantErr behavior matches with the returned error.
  got, got1, gotErr := Foo()
  if (err != nil) != test.wantErr {
    t.Errorf("Foo() got err %v, want error: %t", gotErr, test.wantErr)
  }

@hyangah @findleyr @adonovan

@adonovan
Copy link
Member

adonovan commented Oct 22, 2024

All good, though in item 7 I suggest splitting the two cases as it leads to better error messages. (In Google readability reviews people sometimes say "but it's longer", but this is generated code so that's excuse won't fly. ;-)

  got, err := Foo(args)
  if err != nil {
        if !test.wantErr {
            t.Errorf("Foo(%v) failed: %v", args, err)
        }
        return
  }
  if test.wantErr {
      t.Fatalf("Foo(%v) succeeded unexpectedly", args)
  }
  ... compare got and test.want ...

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/621057 mentions this issue: gopls/internal/golang: generate test for function

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/622320 mentions this issue: gopls/internal/golang: create imports map honor x_test.go over x.go

gopherbot pushed a commit to golang/tools that referenced this issue Oct 29, 2024
This CL is some glue code which build the connection between the
LSP "code action request" with second call which compute the actual
DocumentChange.

AddTest source code action will create a test file if not already
exist and insert a random function at the end of the test file.

For testing, an internal boolean option "addTestSourceCodeAction"
is created and only effective if set explicitly in marker test.

For golang/vscode-go#1594

Change-Id: Ie3d9279ea2858805254181608a0d5103afd3a4c6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/621056
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants