Test line/col/filenames are incorrect when using package:test_reflective_loader

[DanTup]

Usually when we run tests with the JSON reporter, we get the file/line/col of the test back. We can now also run tests by line number.

However, the file/line/col information is incorrect for tests using package:test_reflective_loader so if we try to run by line then things don't work right. Or if you try to "Go to Test" that goes to the wrong place. I believe the issue is that when the test() function is run, the actual location of the test method isn't on the stack at all, so what we end up with is the location of the call to defineReflectiveSuite:

danny@Dannys-MacBook-Pro analysis_server % dart test -r json test/lsp/call_hierarchy_test.dart --plain-name "PrepareCallHierarchyTest | test_method"

// snip ...

"line": 175,
"column": 28,
"url": "package:test_reflective_loader/test_reflective_loader.dart",
// This is the call to defineReflectiveSuite
"root_line": 12,
"root_column": 3,
"root_url": "file:///Users/danny/Dev/Google/dart-sdk/sdk/pkg/analysis_server/test/lsp/call_hierarchy_test.dart"

// snip ...

So, the request is to support this style of test in a way that the correct location can be reported. The code that's calling test() lives here:

https://github.com/dart-lang/test_reflective_loader/blob/40d61b16647cd61b02d806fea46362ef07e7c502/lib/test_reflective_loader.dart#L175

Looking at the code, it has a MethodMirror at the point where test is called, and that has a .location that looks like it's probably the values we want. However I can't see a way it could pass them to package:test. Would it be feasible to allow passing an overridden location to test() (or having some similar API that supported this)?

[jakemac53]

hmm, yeah the only way I could see this working is through some sort of special information passed through the test function. We would likely have a similar issue with a codegen/macro based package that was trying to do the same thing as this package, not sure if we would ever want to expose line/col info to macros though (it would hurt invalidation).

[jakemac53]

cc @natebosch wdyt about a way to override the line/col info?

[natebosch]

Would we also need to allow overriding the URI for where the caller wants to pretend the test call was made?

I think this would be OK to expose. We should consider whether to do something like require importing the definitions from test_core and having a copy in test that doesn't expose those arguments.

[DanTup]

Would we also need to allow overriding the URI

It's not necessary for my example above since the calls to defineReflectiveSuite are in the same file, but I don't think that's a requirement so I think it should be allowed to provide all three.

(Having a different function seems fine to me too - it could even be named differently if you want to avoid people accidentally selecting it from code completion over the pkg:test one)

[jakemac53]

I could see a different name being used, not coming up with one at the moment though :)

[DanTup]

I bumped into this again today because I've recently enabled the VS Code test runner for the SDK and most analyzer/analysis_server tests use this.

I think the changes in test_reflective_loader to provide this information would be fairly straight forward because they're available on the mirror here:

But I'm less sure how complex the changes here would be. Could they reasonably be contributed without a lot of hand-holding?

[jakemac53]

This should be pretty easy I think - assuming this bottoms out on a normal call to test somewhere.

Basically, add optional line/col fields to Test, update runner.dart to use those instead of looking at the trace if they are provided, and then plumb similar arguments from test and group all the way through to the concrete instances of the Test class, for example LocalTest.

I don't think you need to deal with the RunnerTest class but I could be wrong, but if so it would be just serializing this line/col info when sending test information back to the runner.

[jakemac53]

Actually you probably need the fields to be on GroupEntry so you can also run groups by line/col.

[DanTup]

This should be pretty easy I think - assuming this bottoms out on a normal call to test somewhere.

Yup, it does.

I'll try to take a look, thanks!

[DanTup]

I've started on a PR for this at #2481 (currently needs some feedback).