Why does testrunner duplicate errors?

[jens1205]

Hi,

while adding errors to the neotest-go adapter (nvim-neotest/neotest-go#14), I stumbled upon following feature of neotest:

The adapter returns the following result, which looks fine IMHO:

{
  ["/Users/a1167272/privat/go/scratch/cmd/scratch/main_test.go::TestAdd"] = {
    errors = { {
        line = 12,
        message = "ErrorF\nnext line Errorf\n"
      }, {
        line = 15,
        message = "\tError Trace:\tmain_test.go:16\n\tError:      \tShould be true\n\tTest:       \tTestAdd\n\tMessages:   \tassert true\n"
      } },
    output = "/var/folders/mt/_y14clh90ljd4lvp2w5m8gz40000gn/T/nvim32rW8I/3",
    short = "=== RUN   TestAdd\n    main_test.go:11: good morning\n    main_test.go:13: ErrorF\n        next line Errorf\n    main_test.go:16: \n        \tError Trace:\tmain_test.go:16\n        \tError:      \tShould be true\n        \tTest:       \tTestAdd\n        \tMessages:   \tassert true\n=== RUN   TestAdd/subtest\n    main_test.go:19: in subtest\n    main_test.go:20: \n        \tError Trace:\tmain_test.go:20\n        \tError:      \tShould be false\n        \tTest:       \tTestAdd/subtest\n        \tMessages:   \tsubtest error\n=== CONT  TestAdd\n    testutil.go:6: helper failed\n\27[31m--- FAIL: TestAdd (0.00s)\n\27[0m\27[31m    --- FAIL: TestAdd/subtest (0.00s)\n\27[0m",
    status = "failed"
  },
  ["/Users/a1167272/privat/go/scratch/cmd/scratch/main_test.go::TestAdd::subtest"] = {
    errors = { {
        line = 19,
        message = "\tError Trace:\tmain_test.go:20\n\tError:      \tShould be false\n\tTest:       \tTestAdd/subtest\n\tMessages:   \tsubtest error\n"
      } },
    output = "/var/folders/mt/_y14clh90ljd4lvp2w5m8gz40000gn/T/nvim32rW8I/4",
    short = "=== RUN   TestAdd/subtest\n    main_test.go:19: in subtest\n    main_test.go:20: \n        \tError Trace:\tmain_test.go:20\n        \tError:      \tShould be false\n        \tTest:       \tTestAdd/subtest\n        \tMessages:   \tsubtest error\n\27[31m    --- FAIL: TestAdd/subtest (0.00s)\n\27[0m",
    status = "failed"
  }
}

But neotest testrunner adds here

neotest/lua/neotest/client/runner.lua

Line 31 in 8f4e4e3

fill_results(self:_missing_results(root, results, not output_path))

the error of the subtest to the main test:

  ["/Users/a1167272/privat/go/scratch/cmd/scratch/main_test.go::TestAdd"] = {
    errors = { {
        line = 12,
        message = "ErrorF\nnext line Errorf\n"
      }, {
        line = 15,
        message = "\tError Trace:\tmain_test.go:16\n\tError:      \tShould be true\n\tTest:       \tTestAdd\n\tMessages:   \tassert true\n"
      }, <1>{
        line = 19,
        message = "\tError Trace:\tmain_test.go:20\n\tError:      \tShould be false\n\tTest:       \tTestAdd/subtest\n\tMessages:   \tsubtest error\n"
      } },
    output = "/var/folders/mt/_y14clh90ljd4lvp2w5m8gz40000gn/T/nvim32rW8I/3",
    short = "=== RUN   TestAdd\n    main_test.go:11: good morning\n    main_test.go:13: ErrorF\n        next line Errorf\n    main_test.go:16: \n        \tError Trace:\tmain_test.go:16\n        \tError:      \tShould be true\n        \tTest:       \tTestAdd\n        \tMessages:   \tassert true\n=== RUN   TestAdd/subtest\n    main_test.go:19: in subtest\n    main_test.go:20: \n        \tError Trace:\tmain_test.go:20\n        \tError:      \tShould be false\n        \tTest:       \tTestAdd/subtest\n        \tMessages:   \tsubtest error\n=== CONT  TestAdd\n    testutil.go:6: helper failed\n\27[31m--- FAIL: TestAdd (0.00s)\n\27[0m\27[31m    --- FAIL: TestAdd/subtest (0.00s)\n\27[0m",
    status = "failed"
  },
  ["/Users/a1167272/privat/go/scratch/cmd/scratch/main_test.go::TestAdd::subtest"] = {
    errors = { <table 1> },
    output = "/var/folders/mt/_y14clh90ljd4lvp2w5m8gz40000gn/T/nvim32rW8I/4",
    short = "=== RUN   TestAdd/subtest\n    main_test.go:19: in subtest\n    main_test.go:20: \n        \tError Trace:\tmain_test.go:20\n        \tError:      \tShould be false\n        \tTest:       \tTestAdd/subtest\n        \tMessages:   \tsubtest error\n\27[31m    --- FAIL: TestAdd/subtest (0.00s)\n\27[0m",
    status = "failed"
  }
}

This leads to the error of line 19 been added 3 times to the diagnostics:

DEBUG | 2022-08-28T06:13:48Z+0200 | ...acker/start/neotest/lua/neotest/consumers/diagnostic.lua:57 | Setting diagnostics for /Users/a1167272/privat/go/scratch/cmd/scratch/main_test.go { {
    col = 0,
    lnum = 12,
    message = "ErrorF\nnext line Errorf\n",
    source = "neotest"
  }, {
    col = 0,
    lnum = 15,
    message = "\tError Trace:\tmain_test.go:16\n\tError:      \tShould be true\n\tTest:       \tTestAdd\n\tMessages:   \tassert true\n",
    source = "neotest"
  }, {
    col = 0,
    lnum = 19,
    message = "\tError Trace:\tmain_test.go:20\n\tError:      \tShould be false\n\tTest:       \tTestAdd/subtest\n\tMessages:   \tsubtest error\n",
    source = "neotest"
  }, {
    col = 0,
    lnum = 19,
    message = "\tError Trace:\tmain_test.go:20\n\tError:      \tShould be false\n\tTest:       \tTestAdd/subtest\n\tMessages:   \tsubtest error\n",
    source = "neotest"
  }, {
    col = 0,
    lnum = 19,
    message = "\tError Trace:\tmain_test.go:20\n\tError:      \tShould be false\n\tTest:       \tTestAdd/subtest\n\tMessages:   \tsubtest error\n",
    source = "neotest"
  } }

There must be some reason why neotest is doing this - but I don't understand this. Is there something in the original adapter result wrong? Or is this a bug in neotest?

Thanks and regards
Jens

[rcarriga]

It's not a bug, more just poor intended behaviour. There wasn't much reasoning to adding child errors to the parent result, we could remove that functionality from neotest and leave the error messages completely up to adapters. I think it makes more sense that way than assuming that errors should be collected by parents.

I'll have to double check for any side effects of that change but you can test yourself by just removing this line https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/client/runner.lua#L219

[jens1205]

https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/client/runner.lua#L219 is an empty line. So which line should I remove? I thought the whole _missing_results method might be superfluos? Or is there only something inside which I should comment out?

[rcarriga]

Sorry file changed after I linked. https://github.com/nvim-neotest/neotest/blob/master/lua/neotest/client/runner.lua#L231 is the line to remove

      if pos_result.errors then
        parent_result.errors = vim.list_extend(parent_result.errors or {}, pos_result.errors) -- Remove this
      end

[rcarriga]

_missing_results is needed for filling results for directories, files and namespaces so it is needed

[jens1205]

I can confirm that after removing the mentioned lines, the duplication of errors is fixed.

In my limited tests I couldn't see any sideeffects - but I've been only testing with neotest-go.

[rcarriga]

OK I've removed this from neotest core 👍