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 :)