x/tools/gopls: issue an event when tests change

[firelizzard18]

In my work to integrate #59445 into vscode-go, I am relying on file-change notifications to keep the test tree up to date as files change. @hyangah pointed out that the ideal solution would be to use notifications that tests were changed instead. I'm opening this issue to discuss whether it is feasible for gopls to send notifications to the editor when test discovery results change, and if so how that may be achieved.

As a followup, it would be powerful to extend change detection to arbitrary symbols and to follow the reference graph to affected tests, so that changing a function called by a test (and more generally, a symbol within the test's reference graph) would trigger an event and (in an editor with an active continuous test run) re-execute the test. I think the reference graph tracing should be reasonable to do within a package. I would be happy to submit a separate proposal for this.

CC @adonovan @findleyr

[gabyhelp]

Related Issues and Documentation

• x/tools/gopls: integrate test discovery #59445 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[firelizzard18]

I'm currently implementing logic in the extension that I suspect is functionally similar or possibly identical to the logic gopls would use. It's turning out to be one of those "looks obvious but has lots of hidden corners" problems. Given that the editor (vscode) is displaying "run this test" widgets, the position of a test is relevant even if nothing meaningfully changed (such as adding a newline). So there are various types of events, which I will describe as "A test has been ...":

• Added
• Removed
• Modified (change in contents)
• Moved (change in position)

A single event may involve a move, a modification, or both. The extension must differentiate between moves and modifications: moving a test (without modifying it) should not invalidate the test results nor trigger a run when in continuous run mode, whereas modifying a test should always do both. If gopls does implement test events, it may choose not to differentiate these event types. However I recommend against that, as the extension is required to do so to provide a good user experience, so without gopls providing that differentiation, the extension would have to do that itself which would somewhat defeat the point.

To be clear, by "move" I mean an edit that results in the position of a test changing, such as adding a line of comments above the test. I am not suggesting detecting copy and pasting a test to a different location or file (though that would be cool). And by "modify" I mean any change to the contents (or name or signature) of a test. I am not suggesting detecting whether the change does or does not modify the emitted binary, though that would also be cool.

[findleyr]

@firelizzard18 would it suffice to send a notification whenever the package is recomputed (in gopls terms: whenever we run storePackageResults, which happens when gopls detects that it needs to update its indexes).

Gopls already has its own rather complicated logic for invalidating package data. I suggest we start by using that heuristic, rather than trying to do something more fine-grained.

[firelizzard18]

@findleyr Possibly. This idea started when Hana was reviewing my work on gopls-based test discovery and saw the fairly complex code I have for change detection, driven by vscode's file change notification. Having a notification for "package X changed" may eliminate some of that, but I'm not sure and it wouldn't be a lot.

• Currently, vscode says "file X changed" with a list of ranges and the extension:
  • Ignores events where the list of changes is empty (I don't know where these come from),
  • Ignores events for non-files (we get notifications for git:// URIs) and for non-Go files,
  • Locates the workspace folder and ignores files that are not within any workspace,
  • Calls gopls.packages, updates test definitions, and detects what and how tests have changed.

Using a "package change" notification should eliminate the first two items, but those are single-line tests; the vast majority of the complexity is in the last point. And I still need to know if the contents of a test are modified - unless the notification included which ranges of which files were changed, I'd need to continue using my current solution. Otherwise I'd need to store the contents of the test to compare, and I'd have to compare every single test in the package in case the contents of a test was changed without changing its range.

[hyangah]

@findleyr Possibly. This idea started when Hana was reviewing my work on gopls-based test discovery and saw the fairly complex code I have for change detection, driven by vscode's file change notification. Having a notification for "package X changed" may eliminate some of that, but I'm not sure and it wouldn't be a lot.

• Currently, vscode says "file X changed" with a list of ranges and the extension:

  • Ignores events where the list of changes is empty (I don't know where these come from),
  • Ignores events for non-files (we get notifications for git:// URIs) and for non-Go files,
  • Locates the workspace folder and ignores files that are not within any workspace,

I think the "(test variant) package change" notification can help these three cases.

And, if we let the notification being sent when the test dependency is updated, that will also help the client to know approximately when to invalidate the previous test results and rerun invalidated tests if requested.

• Calls gopls.packages, updates test definitions, and detects what and how tests have changed.

Using a "package change" notification should eliminate the first two items, but those are single-line tests; the vast majority of the complexity is in the last point. And I still need to know if the contents of a test are modified - unless the notification included which ranges of which files were changed, I'd need to continue using my current solution.

I naively thought the last step would be

• calls gopls.packages for updated test.
• generates a new test item trees based on the new response, and computes diff of the response to detect changed tests. (the response has all the ranges )

If that is the majority of the complexity in the current implementation (and it was just me overwhelmed by TS/JS :-() I think we can stay with your current implementation.

Otherwise I'd need to store the contents of the test to compare, and I'd have to compare every single test in the package in case the contents of a test was changed without changing its range.

I am afraid that will be challenging for gopls to correctly recognize (e.g. a function called in the test function was refactored and renamed, no test function range was changed; do we want to detect it as an test change event).

[firelizzard18]

Given foo_test.go:

package foo

import "testing"

func TestFoo(t *testing.T) {
	t.Log("foo")
}

func TestBar(t *testing.T) {
	t.Log("bar")
}

The response from gopls.packages will be something like:

Response (JSON)

{
    "Packages": [
        {
            "Path": "example.com/foo",
            "ModulePath": "example.com",
            "ForTest": "example.com/foo",
            "TestFiles": [
                {
                    "URI": "file:///src/example.com/foo/foo_test.go",
                    "Tests": [
                        {
                            "Name": "TestFoo",
                            "Loc": {
                                "uri": "file:///src/example.com/foo/foo_test.go",
                                "range": {
                                    "start": {
                                        "line": 3,
                                        "character": 0
                                    },
                                    "end": {
                                        "line": 5,
                                        "character": 1
                                    }
                                }
                            }
                        },
                        {
                            "Name": "TestBar",
                            "Loc": {
                                "uri": "file:///src/example.com/foo/foo_test.go",
                                "range": {
                                    "start": {
                                        "line": 7,
                                        "character": 0
                                    },
                                    "end": {
                                        "line": 9,
                                        "character": 1
                                    }
                                }
                            }
                        }
                    ]
                }
            ]
        },
        {
            "Path": "example.com/foo",
            "ModulePath": "example.com",
            "ForTest": "",
            "TestFiles": null
        }
    ],
    "Module": {
        "example.com": {
            "Path": "example.com",
            "Version": "",
            "GoMod": "file:///src/example.com/go.mod"
        }
    }
}

The problem: I can't detect changes that don't change the range without more information. Scenarios:

1. I add a second line to TestBar
   • This will work fine. The range will change, the extension will detect that TestBar (and only TestBar) changed.
2. In TestFoo I change "foo" to "foo2"
   • There is no change to gopls.packages. The response is identical; all of the ranges, paths, names, etc are unchanged.
3. I add a second line to TestFoo
   • The range of both tests change even though TestBar was not modified.

In scenarios 2 and 3, I have no way of correctly identifying which tests have meaningfully changed without more information (or doing the whole copy-the-source-range-and-compare thing).

I naively thought the last step would be [...]

Me too. I implemented it that way and that might be the version of the code I showed you. But I discovered bugs when I tested the scenarios above.

Otherwise I'd need to store the contents of the test to compare, and I'd have to compare every single test in the package in case the contents of a test was changed without changing its range.

I am afraid that will be challenging for gopls to correctly recognize (e.g. a function called in the test function was refactored and renamed, no test function range was changed; do we want to detect it as an test change event).

I'm not actually suggesting store-and-compare as a real solution, that was a rhetorical comment. Granted, doing AST-level analysis to omit irrelevant changes would be super cool, but that's besides the point/not what I want to talk about now. A list of changes expressed as (file, modified range) is sufficient. After a refactoring I did yesterday, the update process now distinguishes between changes to the range and changes to the contents. When the range changes, I update the TestItem so VSCode's testing support works correctly. However, to determine whether the test should be invalidated/rerun I look at which ranges were modified. My logic is simply: "If one of the modification ranges overlaps the test range, invalidate/rerun it." If gopls's package change notification included Changes []protocol.Location that would be enough, though it wouldn't remove all the complexity because I'd still need to figure out which tests were affected by that list.