fix: report all coverage when no filters are specified

[felangel]

• When no coverage filters are specified, coverage is reported for all packages to maintain backward compatibility.
• Fixes Test Coverage is Empty when Files are not in lib/ directory #2581
• Tagging @liamappelbe since this change was originally introduced in Add --coverage-path and --branch-coverage options #2517

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

[kevmoo]

No tests?

[felangel]

No tests?

I didn’t see any tests in pkg:test_core but happy to write some. Let me know how you want me to proceed.

[kevmoo]

it's a workspace / monorepo – guessing the tests would be in test_api or just test

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

[felangel]

it's a workspace / monorepo – guessing the tests would be in test_api or just test

Okay I’ll take a look tomorrow 👍

[kevmoo]

it's a workspace / monorepo – guessing the tests would be in test_api or just test

Okay I’ll take a look tomorrow 👍

EXACTLY! 😉

[felangel]

it's a workspace / monorepo – guessing the tests would be in test_api or just test

Okay I’ll take a look tomorrow 👍

EXACTLY! 😉

Added tests and updated the changelog ✅

[kevmoo]

@natebosch is (likely?) the right person to review here...

[liamappelbe]

If the goal is to change the default behaviour to include coverage for all packages, including transitive deps, then this approach works. But for most users that's going to mean that the default behaviour includes a lot of coverage data that they don't care about. It also means package:coverage is going to waste time gathering all that coverage.

We do need to support the use case in #2581 somehow, but it's an obscure use case. So I think it would be reasonable to ask those users to change the flags they pass, if this makes the common use case simpler.

Before we make a UX decision like that though, I have a few questions:

1. @felangel Am I right that this changes the default behaviour to include coverage data for all the transitive deps?
2. @stefanhk31, if you pass --coverage-package=.* to package:test, does that fix your bug?
3. What about --coverage-package= with no value?

I think if either of those flags work, then we don't need to change anything in package:test. I suspect they won't work though. I think --coverage-package=.* won't work because the files missing in #2581 are not part of a package path at all, and I'm guessing --coverage-package= won't work because package:args expects a value.

So I think the best option would be to make --coverage-package= or --coverage-package=.* have this behaviour of zero filters. I suppose another (more hacky) option would be to default to no filter if outputting JSON, and keep the current default filter if outputting LCOV.

Ofc, if @natebosch says maintaining backwards compatibility is more important, then this PR LGTM.

[felangel]

If the goal is to change the default behaviour to include coverage for all packages, including transitive deps, then this approach works. But for most users that's going to mean that the default behaviour includes a lot of coverage data that they don't care about. It also means package:coverage is going to waste time gathering all that coverage.

We do need to support the use case in #2581 somehow, but it's an obscure use case. So I think it would be reasonable to ask those users to change the flags they pass, if this makes the common use case simpler.

Before we make a UX decision like that though, I have a few questions:

1. @felangel Am I right that this changes the default behaviour to include coverage data for all the transitive deps?
2. @stefanhk31, if you pass --coverage-package=.* to package:test, does that fix your bug?
3. What about --coverage-package= with no value?

I think if either of those flags work, then we don't need to change anything in package:test. I suspect they won't work though. I think --coverage-package=.* won't work because the files missing in #2581 are not part of a package path at all, and I'm guessing --coverage-package= won't work because package:args expects a value.

So I think the best option would be to make --coverage-package= or --coverage-package=.* have this behaviour of zero filters. I suppose another (more hacky) option would be to default to no filter if outputting JSON, and keep the current default filter if outputting LCOV.

Ofc, if @natebosch says maintaining backwards compatibility is more important, then this PR LGTM.

@liamappelbe yup this reverts to the previous behavior and the specific goal was to restore coverage reporting for Dart Frog which includes code in a top level routes directory (see the newly added test case). This used to be reported by default and I don’t think either of those command line options work unfortunately so coverage data that we were previously relying is no longer accessible without a change to pkg:test :(

[liamappelbe]

@liamappelbe yup this reverts to the previous behavior and the specific goal was to restore coverage reporting for Dart Frog which includes code in a top level routes directory (see the newly added test case). This used to be reported by default and I don’t think either of those command line options work unfortunately so coverage data that we were previously relying is no longer accessible without a change to pkg:test :(

Yeah, I didn't think they would work immediately. What I'm suggesting is that you modify this PR to make one of those options work. That we can keep the default behaviour fulfilling the use case of most users.

[stefanhk31]

If the goal is to change the default behaviour to include coverage for all packages, including transitive deps, then this approach works. But for most users that's going to mean that the default behaviour includes a lot of coverage data that they don't care about. It also means package:coverage is going to waste time gathering all that coverage.

We do need to support the use case in #2581 somehow, but it's an obscure use case. So I think it would be reasonable to ask those users to change the flags they pass, if this makes the common use case simpler.

Before we make a UX decision like that though, I have a few questions:

1. @felangel Am I right that this changes the default behaviour to include coverage data for all the transitive deps?
2. @stefanhk31, if you pass --coverage-package=.* to package:test, does that fix your bug?
3. What about --coverage-package= with no value?

I think if either of those flags work, then we don't need to change anything in package:test. I suspect they won't work though. I think --coverage-package=.* won't work because the files missing in #2581 are not part of a package path at all, and I'm guessing --coverage-package= won't work because package:args expects a value.

So I think the best option would be to make --coverage-package= or --coverage-package=.* have this behaviour of zero filters. I suppose another (more hacky) option would be to default to no filter if outputting JSON, and keep the current default filter if outputting LCOV.

Ofc, if @natebosch says maintaining backwards compatibility is more important, then this PR LGTM.

I just tested on my minimal repro sample, and neither --coverage-package=.* nor --coverage-package= gave coverage results in the lcov file.

[felangel]

@liamappelbe @natebosch let me know how you want me to proceed. I personally wouldn’t change the default behavior in a breaking way unless there are really good reasons to do that. fwiw I’ve been collecting coverage in dart for many years in fairly large projects and the speed of this particular step has never been an issue 🤷

I would highly recommend reverting to the previous behavior and if we want to make this change it should be made in a new major version imo (2.0.0)

[liamappelbe]

The excess coverage is mainly an issue in the new LCOV workflow. If you're using the old JSON workflow, then you are likely applying filters in the format_coverage step (and the performance benefits might not be noticeable depending on how many transitive deps you have). The LCOV workflow doesn't need a separate formatting step, so there's no opportunity to filter the coverage after it has been gathered (other than using 3rd party tools for dealing with LCOV files). That's why the default behaviour should do some filtering in the new workflow.

If maintaining backwards compatibility is very important, maybe we can go with the other option I mentioned, where the default behaviour in LCOV mode remains the same (only reporting coverage on the package under test), but we revert the behaviour for JSON mode (back to not filtering anything). @natebosch can decide.

[natebosch]

maybe we can go with the other option I mentioned, where the default behaviour in LCOV mode remains the same (only reporting coverage on the package under test), but we revert the behaviour for JSON mode (back to not filtering anything).

This sounds like a good choice to me. We can figure out a way to migrate away from the old JSON behavior later and for now use the nicer behavior only in LCOV mode.

[felangel]

maybe we can go with the other option I mentioned, where the default behaviour in LCOV mode remains the same (only reporting coverage on the package under test), but we revert the behaviour for JSON mode (back to not filtering anything).

This sounds like a good choice to me. We can figure out a way to migrate away from the old JSON behavior later and for now use the nicer behavior only in LCOV mode.

Sounds good I’ll update this PR shortly 👍

[felangel]

@kevmoo @liamappelbe @natebosch can you please take a look? I attempted to refactor the PR to only affect the old JSON workflow. Let me know if I missed anything, thanks!

[felangel]

@natebosch @kevmoo is there an ETA for when this will be published?

[kevmoo]

@felangel – good question for Monday morning 😉

[natebosch]

We need to get this package rolled through to the SDK and internally to google3 before we can publish and we hit a snag with a breaking change in one repo. https://dart-review.googlesource.com/c/sdk/+/482520

Should be able to publish in the next few days.