dart-lang/test

--coverage should generate a lcov.info file

rrousselGit opened this issue · 16 comments

rrousselGit commented

Currently when running pub run test --coverage or dart test --coverage, only proprietary JSON files are generated.

Most tools use an lcov.info file instead. It would be great if --coverage could generate that file too.

grouma commented

package:coverage can consume these files and output into the lcov format. I believe the command is format_coverage. See the readme: https://github.com/dart-lang/coverage

rrousselGit commented

But why do we need an extra dependency for it when the flag is built-in?

Flutter doesn't require people to install package:coverage.

grouma commented

What's the workflow like with Flutter? Note that Flutter Tools does depend on package:coverage so it is pulled in implicitly. Note also you don't need package:coverage as a dev dependency, it can be used as a global dependency.

rrousselGit commented

What's the workflow like with Flutter

flutter test --coverage generates a lcov.info file directly instead of all of these json files.

Sure people could use coverage. But why should they have to?
The generated json files are useless to most users, so that shouldn't be the default behavior.

Considering that as part of Dart 2.9, we now have an official dart test command, the current behavior looks sketchy.
It'd be logical to have dart test --coverage and flutter test --coverage behave the same

  • 👍5
grouma commented

That's a fair argument. I likely won't be able to get to this in the near future but I'm happy to review PRs. We'll likely have to model this as a breaking change.

  • 👍1
mit-mit commented

cc @jwren, and a +1 for supporting lcov in dart test

natebosch commented

The flutter test --coverage and pub run test --coverage flags are already quite different. Outputting to a different format by default won't normalize their behavior.

pub run test:

    --coverage=<directory>            Gathers coverage and outputs it to the specified directory.
                                      Implies --debug.

flutter test:

    --coverage                        Whether to collect coverage information.
    --merge-coverage                  Whether to merge coverage data with "coverage/lcov.base.info".
                                      Implies collecting coverage data. (Requires lcov)
    --coverage-path                   Where to store coverage information (if coverage is enabled).
                                      (defaults to "coverage/lcov.info")

I'd be fine with changing package:test options to match flutter test with --coverage and --coverage-path on the next breaking change release. I don't know what --merge-coverage does here, but if it is useful we could add it too.

I'm not sure that any other changes before then move the needle much in terms of making things feel consistent. I also do not think it is a good idea to try to do argument manipulation in dart test to try to normalize things with some hack in package:test.

rrousselGit commented

Would it make sense to make dart test do some extra logic on the top of package:test?
Such that dart pub run test --coverage would be untouched – but dart test --coverage would match Flutter?

This should make the change less breaking (if breaking at all as 2.9 is not stable yet) and not require hacking on package:test.

dart test could internally run the logic to combine both package:test and package:coverage together.

natebosch commented

Would it make sense to make dart test do some extra logic on the top of package:test?

This is what I was referring to when I said "I also do not think it is a good idea to try to do argument manipulation in dart test to try to normalize things with some hack in package:test."

The argument differences in webdev from pub run build_runner already cause us some headaches and I'm leaning towards that being a bad idea. Having 2 commands that mostly do the same thing, and is documented as "uses package:test to run tests" should behave as close as possible. @mit-mit and @jwren might have different opinions on that.

  • 👍1
natebosch commented

If we do think its best to try to normalize the flags we should check whether the 3 flags that flutter test have chosen make sense. It is strange that --merge-coverage implicitly enables --coverage, but --coverage-path doesn't.

mit-mit commented

cc @jonahwilliams @zanderso can you have some background on the flutter test flags?

jonahwilliams commented

I've never used merge-coverage or coverage-path. coverage-path might just be for the fuchsia tooling, and merge-coverage requires having the lcov tool available which requires a bit of hacking on windows. IMO it's not really clear how useful it is.

natebosch commented

@christopherfujino do you know if we have any analytics for the usage of coverage test runs in the flutter tool?

@bkonyi have we started collecting usage metrics from the dart dev tool?

If we can validate what set of flags and behavior makes the most sense and that usage is low enough to not break a bunch of folks then we could look at these changes.

bkonyi commented

@natebosch we do have usage metrics, but I don't believe we collect anything other than which commands are being used. I'm guessing we care about coverage flags provided to dart test in this case?

christopherfujino commented

we have analytics of flutter test, but not flutter test --coverage

rrousselGit commented

Gentle bump on this~

Being unable to run dart test --coverage and generate a lcov file is a significant user experience issue.
Even as an experienced Dart user, I always have to google how to convert the generated JSON in the industry-standard lcov file.

If we don't want a breaking change, what about a --coverage-format=lcov at least?
That would still be more intuitive than having to do it by hand

  • 👍7