nvim-neotest / neotest

An extensible framework for interacting with tests within NeoVim.
MIT License
2.45k stars 123 forks source link

Indicate that a `position` should not have children #206

Closed thenbe closed 1 year ago

thenbe commented 1 year ago

Hi. I'm trying to create multiple position's for a test in build_position, but I ran into an issue.

If I don't include a range for the child positions, aka a rangeless position, I hit an error for trying to access range on a nil value, which happens here.

If I do include a range. The tests will become children of one another. i.e. instead of this:

test
├── test A
├── test B
├── test C

we get this:

test
├── test A
│   ├── test B
│   │   ├── test C

The way I've worked around this is by adding an is_sterile flag to Position. Setting a position as sterile signals to neotest that it should not have children. But I think a better permanent fix is something like #172 's solution, which uses Tree:closest_value_for("range") for range-less positions.

One thing I'm still confused about is why #172 deliberately left out positions.contains(). I'm not familiar with this codebase's history so I figured the reason might be outdated? Or do I have a bug in my code?

rcarriga commented 1 year ago

Hey awesome to see the work on a new adapter! Can you provide an example of the tests you're trying to create a tree from? It'll make figuring out the issue a lot easier.

One thing I'm still confused about is why https://github.com/nvim-neotest/neotest/pull/172 deliberately left out positions.contains() This was because positions.contains is only supposed to be called for positions with ranges or files/directories as that is how the logic works for determining if a position contains another, otherwise there is no way to know.

thenbe commented 1 year ago

Basically I'm trying to represent one test node for each project listed in the playwright.config.ts file. Also I wrote I posted a bit about it here earlier. If it's not clear, or if there is something specific that would help you let me know.

This is what the desired result looks like using the workaround I described above: image

rcarriga commented 1 year ago

Sorry I meant the actual list of positions that you're returning from your build_position function. I'd like the structure that you're passing to see where the issue is

thenbe commented 1 year ago

Sure. Here is an example:

What I return from build_position:

{
{
name = "test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
id = "a30a6eba6312f6b87ea5-93cb44d41cd8598ed709",
name = "[chromium] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
id = "a30a6eba6312f6b87ea5-ec27787a330f7ce50671",
name = "[firefox] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
id = "a30a6eba6312f6b87ea5-75e3e7555b70e5a1bf6c",
name = "[webkit] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
}

From the logfile:

-- ...te/pack/packer/start/neotest/lua/neotest/client/init.lua:306 | Found
{
{
id = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
name = "example.spec.ts",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 0, 0, 9, 0 },
type = "file",
},
{
{
id = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts::test 1",
name = "test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
{
id = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts::test 1::[chromium] - test 1",
name = "[chromium] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
{
id = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts::test 1::[chromium] - test 1::[firefox] - test 1",
name = "[firefox] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
{
{
id = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts::test 1::[chromium] - test 1::[firefox] - test 1::[webkit] - test 1",
name = "[webkit] - test 1",
path = "/home/username/projects/playground/pwdemo2/tests/example.spec.ts",
range = { 2, 4, 4, 2 },
type = "test",
},
},
},
},
},
}

The tree that is rendered

image


Side questions:

  1. You may notice that the id's I returned were overwritten. How can I prevent this? Should I be using position_id? For reference, the id's I'm using were generated by the playwright cli npx playwright test --list --reporter json. Those id's should when implementing stuff like this.

  2. Would you recommend not passing a range at all for the parameterized tests for my use case?

  3. If I don't pass a range, should I then keep nested_tests set to true in parse_positions?

rcarriga commented 1 year ago

Thanks for that! So the parsed tree is what is "expected" based on what you've provided. You're correct that this is a case for positions with no range attribute instead. I've added support for range-less children to the treesitter parsing so just remove the range attribute from the the browser specific tests and the structure should be as you want.

For setting the IDs, yes you should be using the position_id option, but you can set the attribute in build_position and just use this


{ 
  position_id = function (position) return position.id end,
  ...
}
thenbe commented 1 year ago

This works great. Thank you!