microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.48k stars 28.97k forks source link

Support for "related code" in tests #126932

Open connor4312 opened 3 years ago

connor4312 commented 3 years ago

@connor4312 Is there any consideration to how I would present one or more tests as being related to a code item like a function?

For instance, in Pester you can tag tests with anything. If I allowed a special tag that let you say a test was assigned to a certain function named Invoke-MyFunction, I'd want to expose a "play" button right next to the Invoke-MyFunction definition to run all tests "tagged" to that function. In this case though I don't want it to be tracked with a separate ID, I want it to invoke those individual tests as part of the tag "rollup" which might be scattered across different files. Also tricky is how you would present a failure to the user in this case.

Ideally this would work with AutoRun as well whenever I change the function.

At any rate this is definitely a "nice to have" as opposed to a "must have" and could come at a later iteration, just a thought that would make testing really simple for people and not have to lose the context of where they are directly working on the application. I was inspired by your markdown test adapter example.

With the existing implementation it could just be offered under a "Function Test Rollup" header or something and aggregate the results together somehow, but when run it wouldn't update the "referenced" tests necessarily (though I guess I could do this within the context of the extension) so its fragmented and not ideal.

Originally posted by @JustinGrote in https://github.com/microsoft/vscode/issues/107467#issuecomment-861177620

firelizzard18 commented 3 years ago

This is applicable to Go. go test recognizes three types of test functions: TestXxx, BenchmarkXxx, and ExampleXxx. Go has an established naming convention for associating examples with what they are an example of. If you follow this naming convention, pkg.go.dev/go doc/etc will associate each example with the thing it is an example of. This naming convention could be used to attach examples (which are a kind of test) to functions/etc.

As far as I know, that naming convention isn't used for test and benchmark functions, but that's probably because those aren't included in documentation. I think it would be reasonable to use the same convention to attach tests and benchmarks to functions/etc.

I am the author of the Go Test Explorer extension, and I may be submitting PRs to vscode-go to implement the testing API.

connor4312 commented 2 years ago

@jasonrudolph recently did some work on a signficant-other extension that implements something like this. Our friends on the Java team (@jdneo) released similar functionality in November, and @brettcannon wants this in Python too.

How would the API for this look like? Would it be something along the lines of related?: vscode.Location[] on the TestItem? Something more or less advanced?

JustinGrote commented 2 years ago

How would the API for this look like? Would it be something along the lines of related?: vscode.Location[] on the TestItem? Something more or less advanced?

I don't see why it would need to be more complicated than this, since the extension can also keep updating the location if the code/tests move.

An open question is how to handle "one to many" situations in both directions.

jdneo commented 2 years ago

An open question is how to handle "one to many" situations in both directions.

In Java, we leverage the reference-view to deal with the "one to many" case.

firelizzard18 commented 2 years ago

How would the API for this look like? Would it be something along the lines of related?: vscode.Location[] on the TestItem? Something more or less advanced?

@connor4312 I think Location[] is a good first step. I do think it would be valuable to have something like relatedSymbols?: vscode.DocumentSymbol[]. The most useful features that come to mind are:

  1. Auto-run a test whenever the user changes the referenced location. This can be done with Location.
  2. Allow the user to see what symbols are linked to a test, similar to (or incorporated into) find references. This would be much more straight forward if the test item can explicitly link to DocumentSymbols.
connor4312 commented 2 years ago

(2) is interesting, how would you envision the UI/UX for that?

JustinGrote commented 2 years ago

Linking to symbols would be useful, since you wouldn't have to constantly update the location for add-on commands, but it could always be a dropdown menu on right-click for the test (maybe even a tree expansion)

connor4312 commented 2 years ago

The other simple alternative is a pair of functions such as provideImplementationRanges(testItem: vscode.TestItem): ProviderResult<vscode.Range[]> / provideAssociatedTests(range: vscode.Range): ProviderResult<vscode.TestItem>, which VS Code can call on demand when the user asks to go between tests and implementation. This is likely more efficient for implementors.

brettcannon commented 2 years ago

/cc @kimadeline

firelizzard18 commented 2 years ago

(2) is interesting, how would you envision the UI/UX for that?

@connor4312 IMO what would be most valuable would be to show the test play button in the gutter by the related code. That way, if I write a test for a function, I can edit the function and run the test without changing context. If autorun is enabled, then I can configure it to run related tests automatically, so I can edit the function, save, and then see the test results immediately (after the test completes).

Also I was thinking of a context menu option for tests. For example, if the user right clicks on the test item or the play button in the gutter, the context menu has "Go to related code", which either jumps or opens a peek view like "Go to definition"; and "Find all related code", which opens a side bar view like "Find all references".

The other simple alternative is a pair of functions such as provideImplementationRanges(testItem: vscode.TestItem): ProviderResult<vscode.Range[]> / provideAssociatedTests(range: vscode.Range): ProviderResult<vscode.TestItem>, which VS Code can call on demand when the user asks to go between tests and implementation. This is likely more efficient for implementors.

That should work. I would prefer provideAssociatedTests(symbol: vscode.DocumentSymbol): ProviderResult<vscode.TestItem>. Go associates functions with tests based on symbol name, so it would be useful not to have to get the symbol myself.

brettcannon commented 2 years ago

what would be most valuable would be to show the test play button in the gutter by the related code. That way, if I write a test for a function, I can edit the function and run the test without changing context.

Would you expect a specific test to run, or just the test file that was found and believed to be associated with the code? While the latter would at least be possible in Python (based on file name matching), for instance, there's no way we would be able to associate code to a specific test.

firelizzard18 commented 2 years ago

@brettcannon Here's what I'm thinking:

It is up to the extension to determine how to associate TestItems with DocumentSymbols (or ranges). Assuming Python creates a TestItem for the file, it can associate feature_func with the file's TestItem, such that clicking ▷ next to feature_func runs the file tests.

I am the author of the test explorer implementation for vscode-go, so my interest is how this relates to Go.

// ExecuteFeature does something
func ExecuteFeature() { ... }

// ExampleExecuteFeature is an example of how to use ExecuteFeature
func ExampleExecuteFeature() { ... }

// TestExecuteFeature tests ExecuteFeature
func TestExecuteFeature(t *testing.T) { ... }

In Go, examples are treated as tests, and are linked to functions or methods by matching the name. Therefore, it is reasonable for vscode-go to assert that ExampleExecuteFeature is an example of ExecuteFeature and TestExecuteFeature is a test of ExecuteFeature and therefore when ExecuteFeature is modified, the example and test should be executed.

connor4312 commented 2 years ago

Thanks for the feedback, all.

From feedback, I think one approach we can take is, like we do on tests, something of a hybrid:

cc @jdneo / @kimadeline / @JustinGrote

firelizzard18 commented 2 years ago

@connor4312 I've been thinking about how I would actually implement this kind of functionality for Go:

So the two provide handlers seem like the best choice when I have to scan. But I do want to have a push mechanism (such as the property) so I can automatically connect tests to source ranges when both files have been opened. Additionally, I am considering adding a configuration flag (disabled by default) that will automatically scan the entire directory for tests and source symbols when any file in the directory is opened. This should have reasonable performance for small projects and it can be done in the background. This would also require a push mechanism.

jrieken commented 2 years ago

This should have reasonable performance for small projects and it can be done in the background. This would also require a push mechanism.

🤣 this is why we prefer pull instead of push. The workbench is usually better equipped in knowing when to ask for what. E.g we should know that files have been opened and we should be asking for info. It should be two steps: statically describe the files for which you can provide something (e.g document selector) and later answer our question. That also enables non obvious scenarios like LS where we might need to know about tests that aren't opened on the machine you are running in.

connor4312 commented 2 years ago

Perhaps we could actually go with a "pull-only" model here, and have a user settings to configure the "pull aggression." So by default, we would only pull related code when asked for via a command ("run tests for" or "implementations for"). But maybe there is a setting which does what you describe that pulls implementation ranges for tests of active editors.

I'm not super keen on this since there is complexity around dealing with invalidation and re-pulling, but the path is there.

firelizzard18 commented 2 years ago

@jrieken @connor4312 I see your points and I'm OK with letting VSCode/the user decide when to pull that info.

But in the specific case of both files being opened, I very much like to automatically connect the source ranges. If Sort is defined in sort.go and TestSort is defined in sort_test.go, VSCode will already be fetching document symbols for highlighting, so it should be almost free for me to ask for the document symbols for both files and connect TestSort to Sort. Having the source file and the test file open at the same time seems like it would be a pretty common scenario so I'd like to support it. And once we have the ability to autorun a test when a source range changes, it would be even more useful to support that scenario.

jdneo commented 2 years ago

So which approach are we going forward now?

From the conversation looks like option 2(handler) is preferred? But looks like the draft PR raised by @connor4312 is for the 'push' solution?

kimadeline commented 2 years ago

On the Python side there is no way to automatically or easily infer which tests are related to a specific piece of code (no naming conventions), so we would probably not use this feature as it stands right now.

From a general standpoint, I do agree with the handler approach, this way in the extension we might come up with a few simple rules that may help with basic inference.

jdneo commented 2 years ago

On the Python side there is no way to automatically or easily infer which tests are related to a specific piece of code (no naming conventions), so we would probably not use this feature as it stands right now.

From a general standpoint, I do agree with the handler approach, this way in the extension we might come up with a few simple rules that may help with basic inference.

Same in Java, lots of Java developers tends to append Test(s) at the end of the class name. But that's not part of the Java spec. The handler approach can let us do some 'guess' action to find the related test code.

firelizzard18 commented 2 years ago

We're writing tools to improve the developer UX, not language specs, so I think it's fine to go with convention. Go doesn't have any spec for test names, but it does suggest naming examples ExampleFunction or ExampleType_Method. So it's logical to extend that to tests.

icetbr commented 2 years ago

I hacked away a simple extension for my needs, very crude, maybe it can help/inspire someone.

https://marketplace.visualstudio.com/items?itemName=icetbr.vscode-testing-nirvana

abhijit-chikane commented 5 months ago

I encountered an issue where I need to meet the 80% test coverage criteria. Unfortunately, I can't determine which test cases cover which parts of the code simply by looking at the coverage UI.

connor4312 commented 5 months ago

That is a little different, but tracked in https://github.com/microsoft/vscode/issues/212196

connor4312 commented 2 months ago

Hello, it's been a minute!

I've taken this up again and implemented a proposed API in https://github.com/microsoft/vscode/pull/222252. This is a simpler provider-based approach, where the provider is called on-demand. There's no passive gutters as in my previous implementation (perhaps an API point we can add in the future) but it should be a lot easier to implement.

In my initial PR this is surfaced in a few ways:

image image

The PR implements this in the selfhost provider we use to run tests in the VS Code repo, using a simple file-level import graph.

firelizzard18 commented 2 months ago

@connor4312 What kind of time frame are you looking for on feedback? Coincidentally I'm currently working on the Go test controller again, but I'm part way through a full rewrite so I'm currently focused on feature parity with the existing implementation. Side note, I implemented a TestItemResolver that allows me to expose Go tests via a TreeDataProvider-style interface, which feels much more natural to me than my previous resolver. Also, this new implementation is using the Go language server (gopls) to discover tests, so my previous statements about document symbols are not applicable to the new implementation.

For anyone else looking for the type definitions, here they are (from the PR):

export interface TestController {
    /**
     * A provider used for associating code location with tests.
     */
    relatedCodeProvider?: TestRelatedCodeProvider;
}

export interface TestRelatedCodeProvider {
    /**
     * Returns the tests related to the given code location. This may be called
     * by the user either explicitly via a "go to test" action, or implicitly
     * when running tests at a cursor position.
     *
     * @param document The document in which the code location is located.
     * @param position The position in the document.
     * @param token A cancellation token.
     * @returns A list of tests related to the position in the code.
     */
    provideRelatedTests?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<TestItem[]>;

    /**
     * Returns the code related to the given test case.
     *
     * @param test The test for which to provide related code.
     * @param token A cancellation token.
     * @returns A list of locations related to the test.
     */
    provideRelatedCode?(test: TestItem, token: CancellationToken): ProviderResult<Location[]>;
}
connor4312 commented 2 months ago

I would expect that this would not be finalized sooner than September, as we're at the end of the July iteration and will probably want to let it bake in proposed for at least one iteration.

firelizzard18 commented 2 months ago

@connor4312 Here's a bug for your brain that's kind of related. Go doesn't have explicit support for table-driven tests, but there are common patterns such as:

func TestMath(t *testing.T) {
    cases := []struct {
        Name   string
        Expr   string
        Result string
    }{
        {"add", "1+1", "2"},
        {"sub", "1-1", "0"},
        {"mul", "1*1", "1"},
        {"div", "1/1", "1"},
    }

    for _, c := range cases {
        // This creates a sub-test with the given name
        t.Run(c.Name, func(t *testing.T) {
            // Example sub-test implementation
            result, err := script.Eval(c.Expr)
            if err != nil {
                t.Fatal(err)
            }
            if result != c.Result {
                t.Errorf("want %s, got %s", c.Result, result)
            }
        })
    }
}

I plan to add static analysis to detect cases like this, but this presents a dilemma: what do I report as the range for the test item? It seems to me that the "Go to test" action should jump to body (func(t *testing.T)) since if I'm jumping to the test, I probably want to see the logic of the test. On the other hand, there are two issues (golang/vscode-go#1602, golang/vscode-go#2445) that request a way to run a specific case, which would be much easier if I reported the entry in the 'table' as the test item's range.

The best solution I can think of (that doesn't involve adding explicit table-drive test support to test items) is some kind of secondaryRanges: Range[] test item property that VSCode uses to add additional ▷ buttons. I could use provideRelatedCode but A) it sounds like that doesn't add ▷ buttons and B) it doesn't really fit the intent of related code as I understand it.

connor4312 commented 2 months ago

Hm, yea, we don't have a very good solution to that right now. I feel like related code might solve that approach but we'd want some more explicit "executed related tests" binding, such that a user could place their cursor in the case's struct and then run that. But it's also not very discoverable without its own 'play' buttons.

I think the way to go might be to actually declare the subtest's range as its case in the struct, rather than t.Run. That would mean no 'play' button in t.Run(c.Name, but if a user wanted to run all subtests at that point they could just run the parent test.