Filter getSourceReport by package name

[liamappelbe]

All the major users of getSourceReport (package:coverage, flutter, google) do something like this:

• Get a list of all the scripts in the isolate (getScripts)
• Check the script URI vs a filter or set of filters
• Get a source report for each script allowed by the filter
• Combine all the source reports

The number of RPCs this involves leads to performance issues. Coverage can also be gathered for the entire isolate at once, but this leads to a trade off between sending many RPCs vs sending one RPC that returns data we don't care about.

Fortunately, all the filtering that users are doing is of the form:

• If the script URI scheme is not 'package', ignore it
• If the first element of the script path is not in an allow list, ignore it
• Otherwise include it

See the usage of scopedOutput in package:coverage for an example.

Flutter's test framework is slightly more permissive, and just checks if the script URI contains the project name. But I think in practice this is equivalent to the filter algorithm above. Eg hello_world project has URIs like this: package:hello_world/main.dart, which would pass the same filter.

So if we add a list of strings to getSourceReport, and do this filtering in the VM, we can significantly reduce the number of RPCs clients have to do.

See also:
flutter/flutter#100751
flutter/flutter#101383

EDIT: To simplify the implementation, and generalize the use cases, I've changed the semantics to just do essentially urlString.startsWith(filter), using the URL of the package/library, not the individual source files. It's still the case that only one of the filters needs to match for the library to be accepted.

[mraleph]

Great idea!

There is another possible optimization which we should consider including into this API. IIRC when requesting coverage we also sometimes pass a flag to force compilation of all code to be able to figure out possible coverage positions even for the code which was not executed. Compiling all functions takes time and in reality only needs to happen once for any script.

So we could consider making this more granular - make it possible to request compilation for specific scripts only. If it's the first time we encounter the script - we request all coverage positions. if we already know all coverage positions (seen this script before) we don't need to request full compilation.

[liamappelbe]

@jonahwilliams The new approach I'm proposing is to add a packageFilters param to getSourceReport, which does this:

if (packageFilters != null) {
  final uri = Uri.parse(script.uri!);
  if (uri.scheme != 'package') continue;  // Ignore this script.
  if (!packageFilters.any((filter) => uri.path.startsWith(filter))) continue;  // Ignore this script.
}

For Flutter's case I'll just pass the project name as a filter. Will this work for Flutter? It's a stricter check than Flutter tester is doing at the moment (just a string.contains). In particular, I think this might exclude users who import their testee script as a relative import, import "../lib/foo.dart";. Is that going to be an issue?

[jonahwilliams]

Is there a way we could normalize things to package imports? While we do recommend that folks use package imports in their tests it would be incredibly confusing if that import style changed coverage info

[liamappelbe]

Actually, looks like this is already a bug. I just tried running a test with coverage, and switched from a package import to a relative one, and my coverage file is now empty. string.contains(projectName) doesn't work either, if the testee file doesn't have the same name as the project: import "../lib/bar.dart"

[jonahwilliams]

:(

[jonahwilliams]

guess it will be no worse than today!

[lrhn]

@liamappelbe You have a relative path containing /lib/ there. The general rule is to never do that. If a file outside the lib/ directory (so outside the package: URI domain) refers to one inside the lib/ directory, use a full package:myPackageName/... URI instead. It's not just that we recommend using a package: URI, we're saying that it's almost certainly a bug if you don't, and you must do so.
Otherwise the import URI is not a package: URI and you risk double-importing the same library, once with a file: URI and once with a package: URI, and it then counts as two different libraries. Which is bad.

That said, what you want for this issue is perhaps to get all the files belonging to the package, even those outside of lib/. That is, the files that are under the root of the package_config.json file's entry for the package, not just its package-root.
There should be ways to figure that out as well, at least if you're running from source. (I don't know if a kernel or dill file remembers which package code belongs to if it was loaded from a file: URI.)

[liamappelbe]

package:coverage change: dart-archive/coverage#378
Flutter change: flutter/flutter#104122