fredrikaverpil / neotest-golang

Reliable Neotest adapter for running Go tests in Neovim.
MIT License
54 stars 5 forks source link

Support running single test in a testify suite #25

Open jstensland opened 3 weeks ago

jstensland commented 3 weeks ago

When running the nearest test in a suite, I would expect the run command to only run the specified test, but instead it quietly misses completely.

Expected run spec command -run option

go test ./path/... -run '^TestSuiteName/TestName$'

Actual run spec command -run option

go test ./path/... -run '^TestName$'

The additional surprise here is that when then actual command is run, there is no notification that no tests were matched, the summary suggested it worked, but the coverage wasn't updated.

This is when running the nearest test, looking at the run spec produced by build_single_test_runspec

fredrikaverpil commented 3 weeks ago

Hm 🤔, I'm wondering what you are referring to when you say "suite".

Neotest has a notion to run all tests of a project, which I believe is what is called a "suite". But it kind of sounds like you mean something very specific here (and not just all test functions of a project)?

Exactly what is TestSuiteName on your end, if it is not a test function?

thelooter commented 3 weeks ago

I think this is related to testify-suites

https://pkg.go.dev/github.com/stretchr/testify/suite

jstensland commented 3 weeks ago

Yes, apologies, I mean a testify suite, or something similar.

fredrikaverpil commented 3 weeks ago

Ok, got it, thanks for this @jstensland and @thelooter !

I'm not sure, by just looking at the example below (from the testify docs), what could be the best way to attempt to initially support this.

// Basic imports
import (
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/suite"
)

// Define the suite, and absorb the built-in basic suite
// functionality from testify - including a T() method which
// returns the current testing context
type ExampleTestSuite struct {
    suite.Suite
    VariableThatShouldStartAtFive int
}

// Make sure that VariableThatShouldStartAtFive is set to five
// before each test
func (suite *ExampleTestSuite) SetupTest() {
    suite.VariableThatShouldStartAtFive = 5
}

// All methods that begin with "Test" are run as tests within a
// suite.
func (suite *ExampleTestSuite) TestExample() {
    assert.Equal(suite.T(), 5, suite.VariableThatShouldStartAtFive)
    suite.Equal(5, suite.VariableThatShouldStartAtFive)
}

// In order for 'go test' to run this suite, we need to create
// a normal test function and pass our suite to suite.Run
func TestExampleTestSuite(t *testing.T) {
    suite.Run(t, new(ExampleTestSuite))
}

I haven't used testify personally, but I would like to look into how it works by setting up a testify suite and play around with it. My gut feeling is though that this could lead to having to redesign the adapter into being able to handle multiple "modes" or similar, and the detection of said modes. Today we have testify, tomorrow we might have something different. Could get ugly if you mix them... I'm also wondering how far users are taking it with these testify suites. The more crazy you get with e.g. the receiver signature, the harder it could be to do the right thing and have bugs.

Anyway, I'm mostly thinking out loud here. Would love any of your input/insights into this if you have any 😄 so I can take better decisions here on trying to support testify.

@jstensland I'm away from my laptop until next week, but if I read this correctly, one solution would be to tweak the test detection and have all testify tests prefixed by its suite name. Meaning, instead of listing TestExample, you would see ExampleTestSuite/TestExample in the Neotest test summary window. I don't believe that Neotest has a notion of "groups" or "suites" in this sense that could be leveraged. Would this theory hold up and scale?

Also, right now, I believe TestExampleTestSuite will be picked up as a regular test, and would be run if you e.g. executed all tests in a file. This would result in that you first run all tests individually and finally this one would get run (which runs all the tests again). Not really sure how to reliably disable this test from when you e.g. run all tests of a file or all tests underneath a folder.

fredrikaverpil commented 3 weeks ago

I'm just leaving it here for now, but there are likely a lot of good insights posted on the neotest-go adapter:

But like I said, I would like to play around with this all and find a maintainable and robust solution.

jstensland commented 3 weeks ago

Starting with an example to try to make my statements more concrete. using that example testify suite with some slight modifications, say you add this to your test file testname_test.go

// Define the example testify suite
type exampleSuite struct {
    suite.Suite
    VariableThatShouldStartAtFive int
}

func (suite *exampleSuite) SetupTest() {
    suite.VariableThatShouldStartAtFive = 5
}

func (suite *exampleSuite) TestSuiteTest1() {
    suite.Equal(5, suite.VariableThatShouldStartAtFive)
}

func (suite *exampleSuite) TestSuiteTest2() {
    suite.Equal(5, suite.VariableThatShouldStartAtFive)
}

// Where go detects the test
func TestExampleSuite(t *testing.T) {
    suite.Run(t, new(exampleSuite))
}

Go test output is like this

â–¶ go test -v
=== RUN   TestAdd
--- PASS: TestAdd (0.00s)
=== RUN   TestNames
=== RUN   TestNames/Mixed_case_with_space
=== RUN   TestNames/Comma_,_and_'_are_ok_to_use
=== RUN   TestNames/Brackets_[1]_(2)_{3}_are_ok
--- PASS: TestNames (0.00s)
    --- PASS: TestNames/Mixed_case_with_space (0.00s)
    --- PASS: TestNames/Comma_,_and_'_are_ok_to_use (0.00s)
    --- PASS: TestNames/Brackets_[1]_(2)_{3}_are_ok (0.00s)
=== RUN   TestExampleSuite
=== RUN   TestExampleSuite/TestSuiteTest1
=== RUN   TestExampleSuite/TestSuiteTest2
--- PASS: TestExampleSuite (0.00s)
    --- PASS: TestExampleSuite/TestSuiteTest1 (0.00s)
    --- PASS: TestExampleSuite/TestSuiteTest2 (0.00s)
PASS
ok      github.com/fredrikaverpil/neotest-golang        0.304s

you can target a particular suite test with a -run regex like this

â–¶ go test ./... -run '^TestExampleSuite/TestSuiteTest2$' -v
=== RUN   TestExampleSuite
=== RUN   TestExampleSuite/TestSuiteTest2
--- PASS: TestExampleSuite (0.00s)
    --- PASS: TestExampleSuite/TestSuiteTest2 (0.00s)
PASS
ok      github.com/fredrikaverpil/neotest-golang        0.261s

or with flags to testify like this

â–¶ go test ./... -run TestExampleSuite -testify.m  TestSuiteTest2 -v 
=== RUN   TestExampleSuite
=== RUN   TestExampleSuite/TestSuiteTest2
--- PASS: TestExampleSuite (0.00s)
    --- PASS: TestExampleSuite/TestSuiteTest2 (0.00s)
PASS
ok      github.com/fredrikaverpil/neotest-golang        0.315s

The former approach seems more general to me, and is where I started

You're right that TestExampleTestSuite will be picked up as a regular test, and that's what testify depends on too. go test starts there.

This would result in that you first run all tests individually and finally this one would get run

This is only an issue because you're running the entire package test by test, right? Once packages are run all at once, you wouldn't have the double invocation. go test PATH/TO/PKG only runs things once.

Those are the most relevant threads I have found. I see you have this code as well, which is why they get detected but the name is only the method, and does not include the Suite the way the go test output does.

I don't believe that Neotest has a notion of "groups" or "suites" in this sense that could be leveraged.

This is where I stopped, since I don't know the neotest framework well. Can you not create a namespace for all the suite tests some how? I haven't figured out how the treesitter tags get interpreted yet, when discovering test positions. From a user point of view, I would expect the methods to be grouped under the suite, similar to test cases.

Other thoughts

uroborosq commented 1 week ago

Hello everyone,

I've investigated a bit this problem and want to share some results.

The branch is here https://github.com/uroborosq/neotest-golang/tree/feat/testify_suites

For now it's only extending treesitter query. I've add two like subqueries - first collect suite methods into namespace with name of standard go test function and second collects directly suite methods. These names adds to position id which lets tests and subtests runs with go test -run ...

So generally it works, I can run and debug suite method/subtests, but there is a list of limitations:

Mb there are some other possibilites, I am very new to lua and neovim)

fredrikaverpil commented 1 week ago

Thanks for having a look at this @uroborosq 😄 You queries looks quite advanced and I can't get them all to work together in the query editor (:EditQuery). I can get some of them to work one by one...

Hm, I wonder if this can work:

image

I didn't know namespaces existed in Neovim, but apparently they do: https://github.com/nvim-neotest/neotest/blob/26ed90509c377d10dbdebd25b7094a886323b32b/lua/neotest/types/init.lua#L3-L9

The args.tree:data().pos.type gives me the namespace name in the build_spec interface function, which is great! That gives me the option to potentially execute test suites differently from directories (or actually, packages), files and individual tests.

The position id for the individual test becomes something like this, which is also great:

{
  id = "/Users/fredrik/code/public/neotest-golang/tests/go/testify_test.go::ExampleTestSuite::TestExample",
  name = "TestExample",
  path = "/Users/fredrik/code/public/neotest-golang/tests/go/testify_test.go",
  range = { 26, 0, 29, 1 },
  type = "test"
}

Right now, the test command won't come out proper and the test won't get executed, but that can be fixed.

Also, there's a regex (Test|Example|Suite) to match with the namespace/receiver, but that can be removed. Neotest will only pick up on the test under the receiver if the method starts with ^Test anyway, which might be ok.

I haven't fully wrapped my head around what drawbacks/caveats there are here...

EDIT: I had a bran fart. I will have to prepend Test to the namespace in the scenario above to make it work. This means I'm dictating how struct naming must be done...

EDIT 2: The neotest-python adapter "auguments" the AST-detected test names with runtime-collected data. It's possible this technique would be helpful here...

EDIT 3: A potential solution would be to create a lookup table, which gets created right after test discovery. The known (potential) suite would be ExampleTestSuite and custom AST detection would map it to TestExampleTestSuite. The lookup could look something like { "ExampleTestSuite" : "TestExampleTestSuite" }. This custom AST detection could potentially be done as part of discovering positions, and it could potentially augument the test position data with the actual namespace name. Either way, it sounds like testify suites will defintively be an opt-in feature - if we can make this work.

jstensland commented 6 days ago

You you're determining the name TestExampleTestSuite by convention of the example?

func TestExampleTestSuite(t *testing.T) {
    suite.Run(t, new(ExampleTestSuite))
}

Taking a quick look through a few repos, I see the struct is often private exampleTestSuite, which might lead to TestexampleTestSuite, if I'm reading this right. That would not match anything.

The potential to be in a different file is a tricky edge case as well with how neotest is set up. Go considers all files in a package in scope. Does this mean given the neotest interface the adapter would need to parse the whole package for each file to have chance at a more deterministic route? As in, it would have to do a few queries to trace down the right Name space and suite runner test?

Feel free to just point me at some docs. All I have found is the readme overview pointing to here, and I haven't had the chance to read through neotest.

Perhaps another adapter could be an inspiration too. What other supported languages compile this way... 🤔

fredrikaverpil commented 6 days ago

@jstensland I added some edits to my previous comment and I did not realize this until it was too late, but I got the namespace from the receiver (ExampleTestSuite), not from the suite test function. You can see in my screenshot by the name of the suite that it is incorrect. And thus, the actual test TestExampleTestSuite/TestExample is not found and won't execute because it took the receiver as namespace.

I think that supporting test suites will be super difficult just in general, because I cannot run a go test or go list command which will give me back a solid single source of truth in terms of test names, from go runtime. We'll always have to rely on some sort of AST detection and/or code inspection shenanigans. Or perhaps we could solve it by requiring a manual test run to populate runtime data into neotest-golang, but I am not a big fan of that and doubt the complexity is going to be ever worth it.

But if we limit the scope to at least initially only support testify, it should be possible to do support this using tree-sitter AST detection;

  1. We could to look for suite.Run(t, new(xxx) and store xxx for later, as the key in a lookup table: { "xxx": nil }
  2. We then need to fetch the function holding this t.Run. That function's name which would be our namespace. We'll place this function name in the lookup: { "xxx": "TestSuiteFunction" }. This lookup is required because the test function and the actual test might not be located in the same file.
  3. Then to find candidates for potential suite tests, we need to detect tests defined on the receiver xxx, like func (suite *xxx) TestMe { ... }.
  4. I'm not entirely sure what's the best path forward, but finally we'd like to have the Neotest tree show the actual namespace, not the receiver name. And we also want to correct test to be executed (again, not with the receiver as namespace). I haven't looked into mutating the position tree returned from the treesitter lib in the adapter's discover_positions function, but I think it can be done easily in the discover_positions function before returning. It'll slow the adapter down though because of this constant parsing, especially in a large code base - hence this having to be a opt-in configuration. It wouldn't be far-fetched to investigate if this can be offloaded to Go and if that would be faster...

Taking a quick look through a few repos, I see the struct is often private exampleTestSuite, which might lead to TestexampleTestSuite, if I'm reading this right. That would not match anything.

I don't quite follow. You mean like this?

func TestSuite(t *testing.T) {
    suite.Run(t, new(privateStruct))
}

The above should be totally fine.

Feel free to just point me at some docs.

Sorry, I don't think there are any. You have to install lazydev.nvim and dive into Neotest's codebase. You can take inspiration from neotest-python, neotest-zig, neotest-rust and other fairly recently developed adapters too.

But anyway, have a look at ast.lua https://github.com/fredrikaverpil/neotest-golang/pull/58/files#diff-2bea1cce72f6f14447e94556ded816a451e8362ee57ecf237d8212296d1cf2b9 and the newly added contribution info on tree-sitter/AST detection Neovim commands in this project's README.

You can parse code using any logic you want here. Could even drop to Go and run AST detection with Go to do certain things where the Neotest position tree structure isn't important, such as when creating a receiver/namespace lookup.

uroborosq commented 5 days ago

From my side, here are two main problems:

  1. How to match suite method with ordinary test function (parent), which contains suite.Run(...)
  2. How to translate to Neotest our tests structure.

For first one, we have two options:

  1. Do several queries, use some map SuiteName (aka receiver) : test function. It must be easier way, and I think, that overhead of running a few queries won't be noticeable. Possibly, it will be simpler to use this method to solve the problem, when suite method and parent function are in different files. Want to notice, that overriding build_position function in the opts for parse_positions function can be useful - https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/lib/treesitter/init.lua#L95. In this function it's possible to get access to all query captures.
  2. Try to do this with one query, as I tried to do. It is achievable to do this comparing receiver of test method == type parameter in suite.Run. My queries detects it (at least for my tests :) ). The drawback of this solution is that neotest.lib.treesitter.parse_positions applies to only one file. We may don't use it, but of course it's really not desirable. And also I haven't found out yet how to parse several files with treesitter query from lua api. Although, it looks like possible in general - http://tree-sitter.github.io/tree-sitter/creating-parsers#command-parse. And I don't know whether query can detect parent test function in one file and suite method in another.

So, generally first point looks doable. I think, that it might be OK to accept this limitation for different files for the initial support.

But second one looks harder. As I understand, Neotest tree design has very restricted structure:

  1. Namespace or test must have one uninterrupted range (begin and end of node in the source code).

  2. As I can see, dir contains files, files contains namespaces, namespaces contains tests and tests can also contain tests. For example, this line indirectly proves this - https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/lib/positions/init.lua#L57. So file can't have any parent, but in our case namespace of whole suite should contain files. It is again about problem when parent test function and suite method are in different files.

  3. Even for one single file I has a problem that namespace (or test) as neotest.Position has field range which is uninterrupted. As I can see, this range is used by neotest to determine the nearest test. But methods of suite can be mixed with other functions (and of course be in other files (again)). I has such example:

    
    func TestProductService(t *testing.T) {
    suite.Run(t, new(ProductServiceSuite))
    }

type ProductServiceSuite struct { suite.Suite }

func (s *ProductServiceSuite) TestKEK() { cases := []struct { name string err error }{ { name: "kek", err: assert.AnError, }, { name: "lol", }, }

for _, test := range cases {
    s.Run(test.name, func() {
        if test.err != nil {
            s.Fail(test.err.Error())
        }
    })
}

}

func TestSmth(t testing.T) { t.Run("noname", func(t testing.T) { t.Fail() })

t.Run("newname", func(t *testing.T) {
})

}

func (s *ProductServiceSuite) TestLOL() { s.Run("first", func() { const a = 4 if 1 != 2 || a == 4 { s.Fail("oh no") } })

s.Run("kek", func() {
    if 2 == 3 {
        s.Fail("yes")
    }
})

}


There is one suite with two methods, which separated with one standard test function.

With my queries, it produces the following neotest tree:

{ { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", name = "service_test.go", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 0, 0, 63, 0 }, type = "file" }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService", name = "TestProductService", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 9, 0, 11, 1 }, type = "test" } }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService", name = "TestProductService", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 17, 0, 38, 1 }, type = "namespace" }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestKEK", name = "TestKEK", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 17, 0, 38, 1 }, type = "test" }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestKEK::"kek"', name = '"kek"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 22, 2, 25, 3 }, type = "test" } }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestKEK::"lol"', name = '"lol"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 26, 2, 28, 3 }, type = "test" } } } }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestSmth", name = "TestSmth", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 40, 0, 47, 1 }, type = "test" }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestSmth::"noname"', name = '"noname"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 41, 1, 43, 3 }, type = "test" } }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestSmth::"newname"', name = '"newname"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 45, 1, 46, 3 }, type = "test" } } }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService", name = "TestProductService", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 49, 0, 62, 1 }, type = "namespace" }, { { id = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestLOL", name = "TestLOL", path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 49, 0, 62, 1 }, type = "test" }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestLOL::"first"', name = '"first"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 50, 1, 55, 3 }, type = "test" } }, { { id = '/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go::TestProductService::TestLOL::"kek"', name = '"kek"', path = "/home/uroborosq/desktop/single_projects/shoplanner/go-backend/internal/product/service_test.go", range = { 57, 1, 61, 3 }, type = "test" } } } } }



As you can see, there are two TestProductService namespaces, which are almost the same, except range. This produce the weird output, which I've posted later.
So it looks like some changes in neotest are required or some compromises decisions on the adapter side. 
fredrikaverpil commented 5 days ago

@uroborosq I think there is a fairly simple solution to support multiple test files, by leveraging a receiver-testfunction lookup like I explained above/earlier. Do you see a caveat or drawback with this?

For the second problem, I would have to tinker a bit with postprocessing of the positions returned by the Neotest/treesitter lib currently used. Either we can postprocess the position nodetree directly or create a supporting data structure. The latter could be less coupled, less disruptive and less problematic with future changes to Neotest but could also be noticeably slower.

I am pretty confident I can solve this, but with the limitation of only supporting Testify (as this seems to follow a suite.Run pattern) and not custom-built suites. But I will very likely have to do custom AST-parsing, not leveraging the Neotest-provided lib. I would even consider doing the AST parsing in Go. In short, a considerable amount of dev time.

Having all this said, and as a side note, I would personally like to focus on getting some basic support in for running all tests in a file using one go test command, detect more table test syntax variations and fix some visual status related bugs before attempting to tackle this. But you are of course welcome to also take a stab at this any time. But it could be that some learning (on my end) as I develop aforementioned features will lead to increased understanding and the possibilities of having new avenues opened of seeing potential solutions.

Supporting test suites will be tricky and I want to carefully decide where to add the complexity required. Quite frankly, this increased insight/understanding while exploring this topic so far has given me a reason not to ever want to use test suites (not limited to testify) in my own projects.

I really wish go test or go list will one day be capable of give a reliable list of all tests (including namespaces and nested/table tests) by relying on runtime inference which would be super reliable.

jstensland commented 5 days ago

Thanks for the responses. I am away for a bit but will come back and look more deeply later next week, although you're miles ahead 🙂. A few thoughts in case they can be helpful.

It looks to me like testify uses the reflect package to find it's methods and build out test for testing.T. Accordingly, I think the go tools aren't ever going to help the way you want. Although, perhaps if it's an optional configuration anyways, could gopls close the gap?

Suite detection in VSCode go extension. Regex rather than tree sitter and I'm sure the internals of recording the tests are different, to the larger problems discussed. At least an example of cases they are trying to cover between the comment and regex