https://github.com/user-attachments/assets/cf57b295-ab3c-4888-aca1-3dd309a521b7

Renaming a function also renames references to it. If there is a correspondingly named test function, it'd make sense to also rename it (TestHello to TestFoo when Hello renamed to Foo). Keeping the names in sync, and reducing work burden.

Comment From: findleyr

Sure, that would be cool, and seems like a useful feature (we already rename symbols in comments)! Marking this as help wanted.

Comment From: findleyr

@h9jiang points out that the 'AddTests' code action already looks for an existing test following the standard naming schema, so there is precedent for us honoring this convention.

Comment From: AlexsanderHamir

Hi, I’d love to help contribute this feature if that's okay, I’ve been reviewing the existing refactor/rename package and have started outlining a possible technical approach.

Comment From: h9jiang

Hi AlexsanderHamir@

Thanks! I think the most important bit is how to correctly find the test function based on the function you are observing. And the other way around, how to find the function based on the test function you are observing. E.g. (From function Foo to function TestFoo, from function TestFoo to function Foo).

The name determination can be found this testName function. You should be able to do a reverse engineering to find the original function by looking at the test function name.

But one more thing to notice is, following the naming convention might not be enough. I think to make sure we have a higher chance of locating the right function, we want to make sure the function or method is actually being called within the test function. 

-- foo_test.go --

import (
  renamed "..../foo"
)
func TestFoo() {
    // ....
    renamed .Foo()
    // ....
}

-- foo.go --
package foo

func Foo() {
    // ....
}

To verify the function Foo is actually being called inside of the test function, we need to make sure both the package and the function are correct. In the example above, (probably the most complicated case), the package foo get renamed in the test file as renamed. But this is still considered as a valid case. The qualifier function maybe helpful for understanding.

I think the rename is pretty straightforward. I hope.

Comment From: findleyr

@AlexsanderHamir I've assigned this to you. Please let us know if you have any difficulty implementing it or want advice.

If you find you can't implement it, just let us know or unassign yourself.

Thanks!

Comment From: AlexsanderHamir

Thank you for your comment @h9jiang, it really help me organize myself to get started on the task !

Comment From: AlexsanderHamir

Thank you @findleyr,I really appreciate it! I’ll make sure to communicate as needed while I progress with the implementation.

Comment From: AlexsanderHamir

Hey @h9jiang, thanks for pointing me to those parts of the codebase, I was able to follow through and understand the implementation. I do have a question though, and I’m not sure if I might have missed something: should this new feature be integrated into an existing command handled by the Dispatch function, or should it be defined as its own command?

My third guess was that the implementation should be under textDocument/rename operation

Comment From: h9jiang

Hi @AlexsanderHamir

The code I'm pointing you to is probably not the place related to rename. Instead the previous pointer is to help you understand how to link a function to the corresponding test functions.

The actually place for rename is gopls/internal/server/rename.go and gopls/internal/golang/rename.go. This is the place where gopls calculate the edits that should be applied when the client ask for renaming something. 

func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp protocol.Position, newName string) (map[protocol.DocumentURI][]protocol.TextEdit, bool, error)

The function signature above include, snapshot a snapshot of the workspace, Handle which file is this rename is happening , position the line and column number to the symbol you are renaming and newName the name you want to give it.

This function returns text edits. The current implementation is, returning all the reference of this symbol. Imagine you want to rename a function called foo defined in foo.go and this function foo is being used in bar.go & baz.go & foo_test.go 

// Imagine this is the text edits
map[protocol.DocumentURI][]protocol.TextEdit{
   foo.go: "change text between line 10 column 1 and line 10 column 4 to bar" // definition of foo
   bar.go: "change text between line 10 column 1 and line 10 column 4 to bar" // caller of foo
   baz.go: "change text between line 10 column 1 and line 10 column 4 to bar" // caller of foo
   foo_test.go: "change text between line 10 column 1 and line 10 column 4 to bar" // test of foo
}

What this new feature require you to do is adding one more text edits:

// Imagine this is the text edits
map[protocol.DocumentURI][]protocol.TextEdit{
   foo.go: "change text between line 10 column 1 and line 10 column 4 to bar"
   bar.go: "change text between line 10 column 1 and line 10 column 4 to bar"
   baz.go: "change text between line 10 column 1 and line 10 column 4 to bar"
   foo_test.go: "change text between line 10 column 1 and line 10 column 4 to bar"
   foo_test.go: "change text between line 5 column 1 and line 5 column 8 to TestBar" // rename the test function name!
}

Make sure to detect the symbol that is being renamed is actually a function or methods and exit early. (We don't need to worry about test function rename if this is a var or const)

Comment From: h9jiang

Prepare Rename is a operation that happens before the actual rename is being calculated. It is not so obvious by just looking at the lsp method name.

You don't need to worry about the part related to prepare rename. For this feature specifically, we should focus on the actual rename function that I mentioned above.

After typing the comment above, I think it make more clear that the existing logic of rename already find the all the references of function foo. So you just need to locate any references that is in a test function body and the test function name is following the naming convention mentioned above.

In case there is already a TestFoo test function, we should abandon this rename and send user an error message.

Let me know if you need more info! Looking forward to see this happening!

Comment From: AlexsanderHamir

Thank you very much @h9jiang, that clarified everything for me!!