[Feature] Support child tests which cannot be focused

[OddBloke]

In nvim-neotest/neotest-python#36, I'm working on introducing support for pytest parameterization, which produces multiple test instances for each test definition. These test instances do not have a unique location in the codebase: they can be generated by the cross-product of multiple different parameter declarations (which do not necessarily even have to be in the same test file). It follows that they should never be selected/focused by location: only their parent test should be.

I need some support from neotest core for this use case: without any changes, the last test instance discovered for a given test will be focused in the summary view, and only that test will be run by neotest.run.run. The correct UX in these cases is for the parent test to be focused, and the parent test (and so all instances) to be run.

[OddBloke]

My first thought for a proper implementation here was to indicate that a test can't be focused by setting range to nil (as opposed to setting it to the same as the parent). Code within neotest core would switch from reading range directly to finding the closest node in the tree with non-nil range and using its value. I've pushed up a rough partial PoC of this idea: 3df6db7

However, I realised that it's only cases where we're mapping from a location to a test that we need to change the current behaviour: if we're annotating lines with diagnostics or statuses, or jumping to them, then we can use either the child or parent's range, and the outcome will be the same. Instead of having to traverse the tree every time we encounter a child test (albeit just one node up, generally), we can instead have tests indicate whether or not they can be selected when mapping a location:

--- a/lua/neotest/client/init.lua
+++ b/lua/neotest/client/init.lua
@@ -122,10 +122,12 @@ function NeotestClient:get_nearest(file_path, row, args)
   local nearest
   for _, pos in positions:iter_nodes() do
     local data = pos:data()
-    if data.range and data.range[1] <= row then
-      nearest = pos
-    else
-      return nearest, adapter_id
+    if data.can_focus then
+      if data.range and data.range[1] <= row then
+        nearest = pos
+      else
+        return nearest, adapter_id
+      end
     end
   end
   return nearest, adapter_id
diff --git a/lua/neotest/lib/treesitter/init.lua b/lua/neotest/lib/treesitter/init.lua
index 11ddc80..c80ee72 100644
--- a/lua/neotest/lib/treesitter/init.lua
+++ b/lua/neotest/lib/treesitter/init.lua
@@ -30,6 +30,7 @@ local function build_position(file_path, source, captured_nodes)
       path = file_path,
       name = name,
       range = { definition:range() },
+      can_focus = true,
     }
   end
 end
@@ -49,6 +50,7 @@ local function collect(file_path, query, source, root, opts)
       path = file_path,
       name = path_elems[#path_elems],
       range = { root:range() },
+      can_focus = true,
     },
   }
   for _, match in query:iter_matches(root, source) do

This is, at the very least, a simpler implementation.

What do you think?

[rcarriga]

If we use a can_focus attribute there are two scenarios for the range:

1. We set it to the parent test's range which makes usage more simple but semantically incorrect for the sake of relatively minor convenience.
2. We don't set the range, which IMO is "correct" but also means that now the can_focus attribute is redundant as for every position, position.can_focus == (position.range ~= nil)

I think the original proposal of a nil range is a more elegant and semantic API decision. I also like it because it means that neotest can move towards detaching the test tree structure from other structures such as the filesystem, or test files. Some languages don't use the same relationships of files and modules (e.g. go) and so allowing for the tree to be more abstract opens up interesting directions for the future 😄

[OddBloke]

OK, I've fleshed that PoC out:

--- a/lua/neotest/client/init.lua
+++ b/lua/neotest/client/init.lua
@@ -121,8 +121,8 @@ function NeotestClient:get_nearest(file_path, row, args)
   end
   local nearest
   for _, pos in positions:iter_nodes() do
-    local data = pos:data()
-    if data.range and data.range[1] <= row then
+    local range = pos:closest_value_for("range")
+    if range and range[1] <= row then
       nearest = pos
     else
       return nearest, adapter_id
diff --git a/lua/neotest/consumers/diagnostic.lua b/lua/neotest/consumers/diagnostic.lua
index a20a113..081deaa 100644
--- a/lua/neotest/consumers/diagnostic.lua
+++ b/lua/neotest/consumers/diagnostic.lua
@@ -77,7 +77,7 @@ local function init(client)
       local result = results[pos_id]
       if position.type == "test" and result and result.errors and #result.errors > 0 then
         local placed = self.tracking_marks[pos_id]
-          or self:init_mark(pos_id, result.errors, positions:get_key(pos_id):data().range[1])
+          or self:init_mark(pos_id, result.errors, positions:get_key(pos_id):closest_value_for("range")[1])
         if placed then
           for error_i, error in pairs(result.errors or {}) do
             local mark = api.nvim_buf_get_extmark_by_id(
diff --git a/lua/neotest/consumers/jump.lua b/lua/neotest/consumers/jump.lua
index 43f75e6..35cf588 100644
--- a/lua/neotest/consumers/jump.lua
+++ b/lua/neotest/consumers/jump.lua
@@ -30,7 +30,7 @@ local get_nearest = function()
 end
 
 local function jump_to(node)
-  local range = node:data().range
+  local range = node:closest_value_for("range")
   async.api.nvim_win_set_cursor(0, { range[1] + 1, range[2] })
 end
 
@@ -47,7 +47,7 @@ local jump_to_prev = function(pos, predicate)
   if pos:data().type == "file" then
     return false
   end
-  if async.api.nvim_win_get_cursor(0)[1] - 1 > pos:data().range[1] then
+  if async.api.nvim_win_get_cursor(0)[1] - 1 > pos:closest_value_for("range")[1] then
     jump_to(pos)
     return true
   end
diff --git a/lua/neotest/consumers/output.lua b/lua/neotest/consumers/output.lua
index cf2717b..f23f256 100644
--- a/lua/neotest/consumers/output.lua
+++ b/lua/neotest/consumers/output.lua
@@ -121,13 +121,15 @@ local init = function()
       if not positions then
         return
       end
-      for _, pos in positions:iter() do
+      for _, node in positions:iter_nodes() do
+        local pos = node:data()
+        local range = node:closest_value_for("range")
         if
           pos.type == "test"
           and results[pos.id]
           and results[pos.id].status == "failed"
-          and pos.range[1] <= line
-          and pos.range[3] >= line
+          and range[1] <= line
+          and range[3] >= line
         then
           open_output(
             results[pos.id],
diff --git a/lua/neotest/consumers/status.lua b/lua/neotest/consumers/status.lua
index 18fe96f..275ca6e 100644
--- a/lua/neotest/consumers/status.lua
+++ b/lua/neotest/consumers/status.lua
@@ -15,7 +15,7 @@ local function init(client)
 
   local namespace = async.api.nvim_create_namespace(sign_group)
 
-  local function place_sign(buf, pos, adapter_id, results)
+  local function place_sign(buf, pos, range, adapter_id, results)
     local status
     if results[pos.id] then
       local result = results[pos.id]
@@ -28,12 +28,12 @@ local function init(client)
     end
     if config.status.signs then
       async.fn.sign_place(0, sign_group, "neotest_" .. status, pos.path, {
-        lnum = pos.range[1] + 1,
+        lnum = range[1] + 1,
         priority = 1000,
       })
     end
     if config.status.virtual_text then
-      async.api.nvim_buf_set_extmark(buf, namespace, pos.range[1], 0, {
+      async.api.nvim_buf_set_extmark(buf, namespace, range[1], 0, {
         virt_text = {
           { statuses[status].text .. " ", statuses[status].texthl },
         },
@@ -51,9 +51,11 @@ local function init(client)
         if not tree then
           return
         end
-        for _, pos in tree:iter() do
+        for _, node in tree:iter_nodes() do
+          local pos = node:data()
+          local range = node:closest_value_for("range")
           if pos.type ~= "file" then
-            place_sign(async.fn.bufnr(file_path), pos, adapter_id, results)
+            place_sign(async.fn.bufnr(file_path), pos, range, adapter_id, results)
           end
         end
       end
diff --git a/lua/neotest/consumers/summary/component.lua b/lua/neotest/consumers/summary/component.lua
index 00e64a5..2b20c6a 100644
--- a/lua/neotest/consumers/summary/component.lua
+++ b/lua/neotest/consumers/summary/component.lua
@@ -167,7 +167,8 @@ function SummaryComponent:_render(canvas, tree, expanded, focused, indent)
         if position.type == "file" then
           lib.ui.open_buf(buf)
         else
-          lib.ui.open_buf(buf, position.range[1], position.range[2])
+          local range = node:closest_value_for("range")
+          lib.ui.open_buf(buf, range[1], range[2])
         end
       end)
     end
diff --git a/lua/neotest/types/tree.lua b/lua/neotest/types/tree.lua
index b0ae7af..406ccdf 100644
--- a/lua/neotest/types/tree.lua
+++ b/lua/neotest/types/tree.lua
@@ -144,6 +144,19 @@ function Tree:iter_parents()
   end
 end
 
+function Tree:closest_value_for(data_attr)
+  local our_value = self:data()[data_attr]
+  if our_value ~= nil then
+    return our_value
+  end
+  for parent in self:iter_parents() do
+    local parent_value = parent:data()[data_attr]
+    if parent_value ~= nil then
+      return parent_value
+    end
+  end
+end
+
 ---@return neotest.Tree
 function Tree:root()
   local node = self

The one remaining use of .range is in .contains:

neotest/lua/neotest/lib/positions/init.lua

Line 39 in 36aa842

return parent.range[1] <= child.range[1] and parent.range[3] >= child.range[3]

If parent.range == nil then it should return false, but if child.range is nil then it's less clear what the correct behaviour is: a child without position might be contained by parent, if parent is one of its parents within the tree. Do I need to start passing the nodes themselves in here, so I can check for this case? (I don't fully understand how .contains is used, so I'm not sure what impact that might have!)

[rcarriga]

Nice!

I think we should leave the contains function as is. I'm happy to leave that as a purely filesystem-based inference. There's no way for the function to know unless the nodes are from the same tree which means it can already be checked by just going up the tree.

The only places to use it (had a quick check of the adapters and don't think any of them use it) is the merge function in the same module which only uses it for file or directory trees so that's covered, and

neotest/lua/neotest/client/runner.lua

Line 233 in 22dbe53

if not lib.positions.contains(root, parent_pos) then

which we can replace with walking up the tree