nvim-neotest / neotest-go

MIT License
124 stars 43 forks source link

[Feature request] Support for Testify framework #6

Open lourenci opened 2 years ago

lourenci commented 2 years ago

Hi, thank you for the plugin.

When using the frameworks' suite feature, this plugin fails to run individual tests. Since the usage of the Testify in Go projects has been increasing daily, it would be awesome to have this plugin supporting it.

akinsho commented 2 years ago

@lourenci features are driven by people in the community contributing what changes they need for their work, see https://github.com/nvim-neotest/neotest-go#contributing

Although tbh I'm not sure why this isn't already working, I don't use the suite feature myself, but it's just a function call inside a TestXxx function all of which should be picked up as tests already and should run granted the command to run them is still go test <blah>

akinsho commented 2 years ago

As far as I can see

// 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))
}

is still wrapped by a regular go test func so this should run like anything else 🤷🏿

lourenci commented 2 years ago

features are driven by people in the community

Sorry. I didn't mean to urge for a feature. I know OSS is community-driven, but, unfortunately, I don't have enough knowledge in Lua to properly do that by myself. 😕 Please, leave it open for someone willing to help.

Yes. It's weird. In the attached print, after running all the tests (lua require("neotest").run.run(vim.fn.expand("%"))), you see that only the suite setup is marked as run, though the "fail" is in another spec right below.

image
akinsho commented 2 years ago

Oh, that's sort of a different issue, it isn't so much that testify isn't working since from what I can see the test does run, the issue you are reporting is that the test isn't detected in the correct place? Is that right?

That's makes more sense now, so the reason that happens is that this adapter needs to be told exactly "what" constitutes a test for a given language, which it does with treesitter queries. Currently, the queries identify t.Run as a test and TestXxxx, but in this case you'd want to add func (blah) Testxxx() as a test, i.e. presumably any method starting with Test in a test file.

It shouldn't be too much work and tbh I think if there are things you are going to want to tweak it's worth looking into lua, it's really not a complex language I'd say easier than go. Although in this case what you probably want to do is add a new treesitter query to https://github.com/nvim-neotest/neotest-go/blob/e67dd4caada39f4795c29b1d65ebf95139ea91ad/lua/neotest-go/init.lua#L177-L193

I'd also suggest pasting a test here so that someone picking this up can add it to the tests in the sample go project in this repo so anyone developing can test against it

akinsho commented 2 years ago

@lourenci I added this in b3b8cb8e6d3fd3aa436dfbd96696a6b8ae8eceaa, but there are some limitations to this which are limitations in go test i.e. you cannot run methods as test using go test -run <NAME-OF-TESTS> so running test nearest will fail and this seems to be an unavoidable limitation.

@rcarriga there's another issue, go test returns the result of the subtest as ParentTestFunc/childFunc since I don't capture the ParentTestFunc as a namespace it doesn't associate it with the right results. We've discussed this before, but it boils down to go allowing a test to both be a test and namespace which there are a bunch of subtests within, e.g.

func TestFunc() { // This is just a test
  assert.True(...)
}

func TestFunc2() { // This is namespace with one test
 t.Run("one", func() {...})
}

I tried tweaking the query to capture the test func as both test definition and namespace definition, but that doesn't work for some reason it fails silently in neotest somewhere and no tests are identified.

lourenci commented 2 years ago

Hey, @akinsho. Thank you so much for going into this.

In Testify, we can run individual tests like this https://github.com/stretchr/testify/issues/460#issuecomment-334397027.

I'll try to look into this if no one gets this before.

akinsho commented 2 years ago

@lourenci that's a great find and good to know. I think it might be a good use case for adding the first test runner to the plugin since running that command is dependent on using testify, so there should probably be a runner like there is for https://github.com/nvim-neotest/neotest-python.

e.g.

require('neotest-go')({
    runner = 'testify',
})

And then if running the test nearest command with the runner set to testify then it should run the command you found

rcarriga commented 2 years ago

@rcarriga there's another issue, go test returns the result of the subtest as ParentTestFunc/childFunc since I don't capture the ParentTestFunc as a namespace it doesn't associate it with the right results. We've discussed this before, but it boils down to go allowing a test to both be a test and namespace which there are a bunch of subtests within

Yep I'm not surprised it fails, neotest doesn't expect overlapping positions without a parent/child link.

since I don't capture the ParentTestFunc as a namespace it doesn't associate it with the right results

I'm not really sure on why the IDs can't be linked as is, could you clarify what prevents this?

akinsho commented 2 years ago

@rcarriga so the current way that ids are generated in this plugin is that the namespaces are prefixed with the test name which is transformed to match what go test expects e.g. trimming white space etc so the average test will have an id like /path/to/file::TestOne at this point when creating the ids there is no way to know that the parent test should be part of the prefix for the subtest so, you end up with a bunch of tests with the id in the structure I described but one of those tests could be a parent but there is no relationship.

The output from go test though looks like ParentTest/TestOne I can't just check the suffix to see if it matches because you can have duplicate names since the names can be based on the descriptions which could be something like happy path so will be conflicted.

Essentially the issue is that there is no way when generating ids for me to namespace a subtest underneath the parent. So I can't correctly map the nested tests to the correct result later on.

Hopefully that makes sense

rcarriga commented 2 years ago

I think I understand the issue with IDs being linked, my gap in understanding is how setting the parent test to be a namespace would solve this issue. Or am I am misunderstanding the solution too? :sweat_smile:

akinsho commented 2 years ago

Ah right so currently when generating the IDs there is no namespace returned by the ID generation function, so my thinking is that if the parent test is correctly identified as a namespace via a TS query then it will be returned as part of the namespaces in the id gen function and so I can prefix tests at the id gen stage.

local function generate_position_id(position, namespaces) <--- these don't have the parent in them since it is not a namespace
  local prefix = {}
  for _, namespace in ipairs(namespaces) do
    if namespace.type ~= 'file' then
      table.insert(prefix, namespace.name)
    end
  end
  local name = transform_test_name(position.name)
  return table.concat(vim.tbl_flatten({ position.path, prefix, name }), '::')
end
rcarriga commented 2 years ago

Ah OK now I see what the issue is! So you should be seeing the parent tests in the namespaces arg (should really be called parents to be clearer). If you're not that would be a bug, but I'd ask to double check that because there is a test checking that it works. Note that right now the parsing assumes that parent tests/namespaces are defined around their children, so if they don't overlap it won't detect the relationship at all.

akinsho commented 2 years ago

@rcarriga that's part of the problem here I think because in go especially in this case they might not overlap because the testify library allows specifying a test as a method which is defined outside the parent test function

akinsho commented 2 years ago

func (suite *ExampleTestSuite) TestExampleFailure() {
    assert.Equal(suite.T(), 5, 3)
}

// 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))
}

This is one such example where TestExampleFailure is a child test but is defined as a method of suite so is outside of the TestExampleTestSuite function which would be it's parent

rcarriga commented 2 years ago

OK so the actual issue then is detecting children not within the range of the parent. The logic to determine if a position is a child is the contains call here https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/lib/treesitter/init.lua#L88. We could make that configurable, though what info would you need to have to determine the relationship in the above example?

akinsho commented 2 years ago

🤔 this might be a little tough since a user could theoretically alias the name of the suite variable to anything, but I guess if I had access to all matches to the test definition queries with some metadata, then in this adapter you could do something like check if the receiver of a method was a suite.

Tbh I don't use the suite function of testify in this way personally, so I'd say I've sort of run out of juice on this one personally. Maybe someone who uses it this way can try and carry on with implementing this.

rcarriga commented 2 years ago

If anyone does wish to pick this up, I'd be happy to assist :+1:

Davincible commented 1 year ago

I think a good way to begin solving this issue would be with t.Run. Any test can invoke a subtest like this, resulting in TestSuite/Test{One,Two,Three}

func TestSuite(t *testing.T) {
    t.Run("TestOne", func(t *testing.T){})
    t.Run("TestTwo", func(t *testing.T){})
    t.Run("TestThree", func(t *testing.T){})
}

EDIT: nevermind, this actually works; image

mats852 commented 1 year ago

They do it in VSCode pretty well, I'll try to look into it.

https://github.com/microsoft/vscode-go/blob/9ee1f173b05bb74ee64e4906f832603cd7380687/src/testUtils.ts#L174-L179

rstcruzo commented 1 year ago

@rcarriga I'd suggest adding two new captures, something like: @namespace.match and @test.match.

If any namespace N match capture is the same as a test T match capture, neotest would automatically consider namespace N to be a parent of T.

Example:

type ExampleSuite struct {
    suite.Suite
}

func (suite *ExampleSuite) TestExample() {
    assert.Equal(suite.T(), 4, 4)
}

func TestFuncExample(t *testing.T) {
    suite.Run(t, new(ExampleSuite))
}

If the query captures the following:

namespace.name == TestFuncExample
namespace.match == ExampleSuite
test.name == TestExample
test.match == ExampleSuite

Neotest will consider TestFuncExample a parent of TestExample. (This would require a sequential iteration over all namespaces and all tests. I wonder if there is some performance implications about that.)

This would probably be best implemented in neotest itself, not in the adapters, the adapters would just add the captures to the treesitter query. Let me know if you like the idea and I can find some time to implement it.

rcarriga commented 1 year ago

This would be a pretty large departure from how the treesitter parsing currently works. Not at all opposed to seeing a PR, but I'm not sure if this would fit nicely within the existing code or would require a separate parsing function in which case it might be best to live in this adapter for now since no other adapter that I know of requires this and it'd be easier to make changes since it wouldn't be public API.

kayn1 commented 10 months ago

Did anyone find a good workaround for that?

roytouw7 commented 7 months ago

Did anyone find a good workaround for that?

Nope, tried tweaking the code a bit to make it run but I don't understand the code enough to make it work. I tried making the adapter run using -testify.m instead of -run and the individual tests are now actually ran, but the matching of the test state messes up now.

For now this is a blocker for me to use neotest as the codebase I work on will not drop testify.

I have somewhat of a workaround, I made a script which will get the method name under my cursor(or the closest) and run go test -v -testify.m method-name in a new terminal. It is not as pretty as neotest sadly but at least it works, it's similar to how running a testify unit test in Goland works.

roveo commented 6 months ago

I hacked together a solution that works reasonably well, see #67