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

feat: use --run flag when executing single test ( run single test instead of all of them ) #81

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

encero
Copy link

@encero encero commented Feb 20, 2024

Run the go test command with --run flag when executing single test.

Support for neotest type=test was removed in #72

This is unfortunate but i believe in wisdom of the maintainers. Anyway i really need this functionality. Otherwise the plugin is unusable on projects with e2e/integration tests. I can't execute the whole suite each time, that is job for the CI.


The solution is untested for all the different nested, map/slice based tests. This works for me right now. This PR is mostly for visibility only. I hope when someone encounter the same roadblock they can use this a inspiration how to solve it.

Plus multiple people are trying to merge stuff to the same piece of code and i don't want to fight over it right now.

Way of avoiding this all together would be providing extension point inside the build_spec function for users to change the outcome themselves. Something in line of

require("neotest-go")({
     test_command_hook = function(command, position) 
         table.insert(command, "--run")
         table.insert(command, position.name)
     end,
      args = { "-count=1", "-timeout=60s" }
    })

sergii4 and others added 6 commits February 11, 2024 19:43
Run the go test command with --run flag when executing single test.

Support for neotest `type=test` was removed in nvim-neotest#72

This is unfortunate but i believe in wisdom of the maintainers. Anyway i
really need this functionality. Otherwise the plugin is unusable on
projects with e2e/integration tests. I can't execute the whole suite
each time, that is job for the CI.
feat: use --run flag when executing single test
@fredrikaverpil
Copy link

I just added an issue about this here without realizing there was a PR up for this already: #83

@fredrikaverpil fredrikaverpil mentioned this pull request Feb 24, 2024
fredrikaverpil

This comment was marked as outdated.

@fredrikaverpil
Copy link

fredrikaverpil commented Feb 24, 2024

@encero I think the regex looks a bit wrong for subtests (nested t.Run()).

Let's say you have this and run "Level 3":

func Test_Level_1(t *testing.T) {
	t.Run("Level 2", func(t *testing.T) {
		t.Run("Level 3", func(t *testing.T) {  // I had my cursor here when I ran "nearest test"
			assert.Equal(t, 1, 1)
		})
	})
}

The neotest-go command then gets generated like this on my end:

{ "cd", "/Users/fredrik/code/public/go-playground/bugs/neotest-go", "&&", "go", "test", "-v", "-json", "", "--run", '\\^"Level 2"/"Level 3"\\$', "./" }

Here, the regex does not look right: \\^"Level 2"/"Level 3"\\$. The test's actual name is Test_Level_1/Level_2/Level_3.


local run_flag = {}
if position.type == "test" then
run_flag = { "--run", "\\^" .. utils.get_prefix(args.tree, position.name) .. "\\$" }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other arguments use just one dash. Not sure what the difference would be here (if any) but perhaps keep it consistent?

Also, why the backslashes?

Suggested change
run_flag = { "--run", "\\^" .. utils.get_prefix(args.tree, position.name) .. "\\$" }
run_flag = { "-run", "^" .. utils.get_prefix(args.tree, position.name) .. "$" }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those slashes are from here, without it works also fine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what @sergii4 says about them, but I think we don't need those backslashes. And I'd vote for -run (one dash) to keep the arguments consistently using one dash.

@fredrikaverpil
Copy link

fredrikaverpil commented Feb 24, 2024

I took a quick stab at trying to get the regex cleaner (as pointed out here). But I have no idea how brittle my approach is.

commit 41a50d2feb9320e830bf9e37f40606b04bb6f81e
Author: Fredrik Averpil <[email protected]>
Date:   Sat Feb 24 16:39:21 2024 +0100

    fix: run nearest tests

diff --git a/lua/neotest-go/init.lua b/lua/neotest-go/init.lua
index f2b0ab7..92b35ea 100644
--- a/lua/neotest-go/init.lua
+++ b/lua/neotest-go/init.lua
@@ -147,6 +147,14 @@ function adapter.discover_positions(path)
   })
 end
 
+function cleanString(str)
+  str = str:match(".*%.go::(.*)") -- Remove path up to and including '.go::'
+  str = str:gsub("[\"']", "") -- Remove quotes
+  str = str:gsub("%s", "_") -- Replace spaces with underscores
+  str = str:gsub("::", "/") -- Replace double colons with slashes
+  return str
+end
+
 ---@async
 ---@param args neotest.RunArgs
 ---@return neotest.RunSpec
@@ -161,6 +169,14 @@ function adapter.build_spec(args)
   if fn.isdirectory(position.path) ~= 1 then
     location = fn.fnamemodify(position.path, ":h")
   end
+
+  local run_flag = {}
+  if position.type == "test" then
+    run_flag = { "-run", "^" .. cleanString(position.id) .. "$" }
+  end
+
   local command = vim.tbl_flatten({
     "cd",
     location,
@@ -171,8 +187,12 @@ function adapter.build_spec(args)
     "-json",
     utils.get_build_tags(),
     vim.list_extend(get_args(), args.extra_args or {}),
+    run_flag,
     dir,
   })
+
   return {
     command = table.concat(command, " "),
     context = {

This will yield the regex ^Test_Level_1/Level_2/Level_3$

Unfortunately, I'm not sure how to figure out dynamically named tests, like this one:

type Skipper struct {
	name string
	skip bool
}

func Test_Level_1(t *testing.T) {
	for _, c := range []Skipper{{name: "Level 2a", skip: false}, {name: "Level 2b", skip: true}} {
		c := c
		t.Run(c.name, func(t *testing.T) {
			if c.skip {
				t.Skip()
			}
			t.Run("Level 3", func(t *testing.T) {
				if c.skip {
					t.Skip()
				}
			})
		})
	}
}

@encero
Copy link
Author

encero commented Feb 24, 2024

Unfortunately, I'm not sure how to figure out dynamically named tests, like this one:

I don't think you can do that without a go "interpreter".

I would expect those kind of test to shop up in the summary window. I they would then a workaround will be to run the whole test func and then select a sub test in the summary window. But that is not the case, the test doesn't show up in the summary 🤔.


  str = str:match(".*%.go::(.*)") -- Remove path up to and including '.go::'

You can maybe avoid doing regex magic by walking the tree?

There is already "partial" walk in the utils.get_prefix. I believe you can walk the whole tree and build the test name bit by bit that way.

function utils.get_prefix(tree, name)
  local parent_tree = tree:parent()
  if not parent_tree or parent_tree:data().type == "file" then
    return name
  end
  local parent_name = parent_tree:data().name
  return parent_name .. "/" .. name
end

Thank you @fredrikaverpil for spending time on this.

@fredrikaverpil
Copy link

fredrikaverpil commented Feb 25, 2024

Dynamically generated sub-tests

I don't think you can do that without a go "interpreter".

@encero you are right, after some investigation it doesn't seem that we can figure out what the names are of dynamically runtime-generated sub-tests here.

I would expect those kind of test to shop up in the summary window.

The test summary doesn't show the full picture, and you cannot run the "Level 3" test unless you "run nearest" at Level 1 or Level 2.

image

So putting that dream to rest, maybe let's focus on landing this PR as it is super useful and I'm also in the same integration test scenario as you, where I cannot have all tests run.

And just in general, we shouldn't run all tests when you choose to "run nearest".

Regex

You can maybe avoid doing regex magic by walking the tree?

Maybe. Not sure if it's worth it though, since we get the data we need from the position, it just needs to be reformatted, like I did in the regex.

What do you think @sergii4 (see my diff here)?
It does the job IMHO.

@encero encero force-pushed the reintroduce-run-flag branch from a63bdd3 to 70a38ab Compare February 27, 2024 19:18
},
{
{
id = vim.loop.cwd() .. "/neotest_go/cases_test.go::TestAdd::\"test three\"::test_four",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something wonky with the treesitter query, those quotes are unexpected to say the least

@encero
Copy link
Author

encero commented Feb 27, 2024

Found few minutes to spent on this

  • got rid of the double dash in run flag
  • renamed utils.get_prefix to more accurate name
  • more spec tests around this feature
  • implemented the "walk tree to get test name" approach

Maybe. Not sure if it's worth it though, since we get the data we need from the position, it just needs to be reformatted, like I did in the regex.

I personally hate string manipulation, and regexes in general. Sometimes they are unavoidable sure.

you are right, after some investigation it doesn't seem that we can figure out what the names are of dynamically runtime-generated sub-tests here.

You could unleash an ast on it, i sure don't have a time for that though.


Still need to find a will power to test the approach for all the different supported test types ( tables, spec, etc... )

@fredrikaverpil
Copy link

@encero @sergii4 what are your opinions on the best approach here?

I would really like us to try and move forward here with any kind of implementation that avoids having to maintain a local fork of neotest-go, which is currently what I have to do.

@sergii4
Copy link
Collaborator

sergii4 commented Mar 13, 2024

@fredrikaverpil what's the urgency? Why do you have to maintain a local fork ?

@sergii4
Copy link
Collaborator

sergii4 commented Mar 13, 2024

@encero could you please fix formatting?

@fredrikaverpil
Copy link

@fredrikaverpil what's the urgency? Why do you have to maintain a local fork ?

I am relying on neotest-go at work and I am dependent on being able to run just one single test.

local result = plugin.build_spec(args)
assert.are.same(expected_command, result.command)
assert.are.same(path, result.context.file)
end)
-- This test is overwriting plugin global state, keep it at end of the file or face the consequences ¯\_(ツ)_/¯
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it overwrite plugin global state?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know enough about lua module imports, but... i think you can't set recursive_run to false, after it was set to something

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, if you try to move the test anywhere in the spec file. The test will fail

---@return string
function utils.get_prefix(tree, name)
local parent_tree = tree:parent()
if not parent_tree or parent_tree:data().type == "file" then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't need that check anymore, do we?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which check do you mean?

@encero
Copy link
Author

encero commented Apr 29, 2024

@encero could you please fix formatting?

@sergii4 the formatting should be fixed now, at least is green in my fork.

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 this pull request may close these issues.

3 participants