[Feature] Allow tests to be run in sequence, not in parallel

[akinsho]

When using neotest to test an entire file, these tests are run in parallel however in some cases, e.g. in a Golang test suite with multiple test cases each depending on creating and cleaning up a database, this can lead to race conditions causing bugs in the test behaviour.

This is partially a problem in userland that tests can't be run in parallel, but if this is not within your control, as is the case in my work (i.e. I can't refactor these tests) I'd prefer to just have each test run in sequence.

Is there an easy way to achieve this behaviour? i.e. a flag that can be passed to the run command to say run tests in this file sequentially.

[rcarriga]

Very valid use case that should be covered! I've added a concurrent config option for global defaults and also a concurrent argument to individual runs

[akinsho]

@rcarriga that's awesome, thanks ❤️, I'll give it a shot and report back

[akinsho]

@rcarriga I've been trying this out and have found the behaviour of using concurrent = false to be much slower than I'd have expected. I think this might be due to a peculiarity of how the go tests are structured. So go has the concept of table tests similar to what you get with jest in js
e.g.

package add
import "testing"
func TestAdd(t *testing.T) {
	// DB setup happens hear
    cases := []struct {
        desc     string
        a, b     int
        expected int
    }{
        {"TestBothZero", 0, 0, 0},
        {"TestBothPositive", 2, 5, 7},
        {"TestBothNegative", -2, -5, -7},
    }
    for _, tc := range cases {
        actual := add(tc.a, tc.b)
        if actual != tc.expected {
            t.Fatalf("%s: expected: %d got: %d for a: %d and b %d",
                tc.desc, actual, tc.expected, tc.a, tc.b)
        }
    }
}

We've current got things setup so that each one of the Test* items is detected as a test. This is how a lot of tests in go are written.

In my case the parent function does the DB setup and clean up runs inside each test case (the above is too simple, but hopefully the idea is clear), running that in parallel is what causes the issues since cleanup is happening whilst other tests are running.

With concurrent = false things are very, very slow/can block seemingly indefinitely, I think this is maybe because the test setup is being run as though the TestAdd was called but with only one of those items at a time. I'm not sure since I can't really inspect what is being run (without adding logs throughout neotest). In the first instance I was wondering if you had any ideas?

[rcarriga]

The concurrency that neotest core controls is the running of the commands that an adapter returns, so it's up to the adapter to return whatever command is suitable.

So in this case I'm guessing that neotest-go is returning a separate command for each TestBothZero, TestBothPositive and TestBothNegative which will mean that 3 commands will be run that'll each do the setup and tear down completely isolated. Without disabling concurrency these 3 still ran all setup/tear down, it was just concurrent so you didn't notice however now since they're run serially it's noticeable. The solution to this problem would be for neotest-go to return a command to run the TestAdd test.

[akinsho]

Yeah, this will be a quirk of our desire to support running individual table based test cases, but I guess when the request is for a parent test then we'd ideally want it to just test the file and run that command. I think it isn't in this case that we are explicitly constructing a command to run all tests, but each test case is being identified as a separate (parallel) test rather than because the parent is being run there is no reason to attempt to run the child tests since that is already being covered.

I don't know if neotest has the concept of parent and child relationships, as AFAIK all we are doing is sending the command to run a test file or nearest test for example, but if that test has subtests these are also being run separately. I mean, the there is no difference based on the fact that some positions are children of another.

Does that make sense? I think my explanation here was probably not ideal bit too tired to formulate this clearly

[rcarriga]

Yes I think I follow. The way neotest breaks apart the tree is very simple, as you said it doesn't account for parent/child relationships in the tree, it just breaks up positions based on type. You can see the logic here https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/client/runner.lua#L125-L132. I'd be open to allowing for more complex splitting of positions but not sure how.

Off the top of my head, neotest could allow adapters to return multiple specs and so they can split the tree however they like.
So currently if we have a tree like

{
  type = "file",
  id = "test_file",
  {
    { type = "file",
      id = "lone_test"
    },
    {
      type = "test",
      id = "parent_test",
      {
        {
          type = "test",
          id = "child_test_1",
        },
        {
          type = "test",
          id = "child_test_2",
        },
        {
          type = "test",
          id = "child_test_3",
        },
      },
    },
  },
}

and the adapter only returns a run spec for tests, we'll end up with 5 separate run specs for each test.

However instead the adapter could return multiple specs for the "file" position, and return specs for lone_test and
parent_test. That way neotest doesn't need to worry about more complex relationships between positons but the adapter
maintains control of exactly which positions are run.

[akinsho]

@rcarriga that makes sense if we knew all the positions in the adapter that were going to be returned for the file then we could filter then based on the parent and the strategy e.g. file etc to decide what to run inside the adapter. Haven't looked at neotest code or the adapter in a while but does the adapter have access to a tree similar to the example in your code snippets I know we have functions to help get the positions but can't remember if in run command function we can access the positions for a particular type of run command 🤔

[rcarriga]

Yep the build_spec function receives the tree to run so the adapter has full context of what is being run, which is where it would return multiple specs

[rcarriga]

OK the build_spec function can now return a list instead of just one RunSpec. Going to close this as the rest of the required changes are in neotest-go