Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature request] Support for Testify framework #6

Open
lourenci opened this issue Jun 19, 2022 · 24 comments · May be fixed by #67
Open

[Feature request] Support for Testify framework #6

lourenci opened this issue Jun 19, 2022 · 24 comments · May be fixed by #67

Comments

@lourenci
Copy link

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
Copy link
Collaborator

akinsho commented Jun 19, 2022

@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
Copy link
Collaborator

akinsho commented Jun 19, 2022

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
Copy link
Author

lourenci commented Jun 19, 2022

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
Copy link
Collaborator

akinsho commented Jun 19, 2022

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

local query = [[
((function_declaration
name: (identifier) @test.name)
(#match? @test.name "^Test"))
@test.definition
(call_expression
function: (selector_expression
field: (field_identifier) @test.method)
(#match? @test.method "^Run$")
arguments: (argument_list . (interpreted_string_literal) @test.name))
@test.definition
(package_clause
(package_identifier) @namespace.name)
@namespace.definition
]]

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
Copy link
Collaborator

akinsho commented Jun 19, 2022

@lourenci I added this in b3b8cb8, 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
Copy link
Author

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

In Testify, we can run individual tests like this stretchr/testify#460 (comment).

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

@akinsho
Copy link
Collaborator

akinsho commented Jun 20, 2022

@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
Copy link
Contributor

@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
Copy link
Collaborator

akinsho commented Jun 21, 2022

@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
Copy link
Contributor

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? 😅

@akinsho
Copy link
Collaborator

akinsho commented Jun 21, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

akinsho commented Jun 21, 2022

@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
Copy link
Collaborator

akinsho commented Jun 21, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

akinsho commented Jun 22, 2022

🤔 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
Copy link
Contributor

If anyone does wish to pick this up, I'd be happy to assist 👍

@Davincible
Copy link
Contributor

Davincible commented Oct 26, 2022

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
Copy link

mats852 commented Jan 14, 2023

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
Copy link

rstcruzo commented Apr 4, 2023

@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
Copy link
Contributor

rcarriga commented Apr 9, 2023

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
Copy link

kayn1 commented Aug 16, 2023

Did anyone find a good workaround for that?

@roytouw7
Copy link

roytouw7 commented Nov 19, 2023

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
Copy link

roveo commented Dec 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants