Allow specifying coverage flags via a yaml file

[liamappelbe]

Spin off of #510

It's a common pattern for Dart tools to read the default values for all their command line flags from yaml. This lets developers specify flag values once per project, and not have to remember/retype them each time. We should do this for package:coverage's flags.

There are a couple of open questions:

1. Should we read the config from a field in pubspec.yaml, or define a new coverage.yaml. Ffigen supports both, the analyzer and dart test only support the latter. I'd prefer to have our own yaml config.
2. We have multiple tools (collect_coverage, format_coverage, run_and_collect, test_with_coverage, and more to come: Add a tool to find uncovered files #529). Do any of their flags have the same name? If not we can just mash them all together into one big config. If there are collisions we might need to group the flags by tool. We'll need to audit all the tool's command line flags and figure out which ones should be exposed in this config, and if any collide.

[khushi-hura]

Hey @liamappelbe I can work on this issue
Please assign this to me!

[liamappelbe]

@khushi-hura The first step is to collect all our tool's command line flags and figure out if any collide, if any are deprecated or redundant, and figure out how we want to structure them in a yaml file. Why don't you look into it and post your findings here, and if your answers looks good I'll assign the bug to you and you can start implementing it. That'll be easier than getting my feedback on the design after you've already implemented it.

[Dhruv-Maradiya]

Hi @liamappelbe

I have made some progress on this issue and have compiled all the details in Notion Document. Here's a quick summary:

1. Complete List of Flags and Options:
   • The document includes all options and flags for collect_coverage, format_coverage, run_and_collect, and test_with_coverage.
   • Common options and flags have been identified and grouped.
2. Deprecated Flags:
   • Flags that are deprecated have been marked in the document.
3. Proposed YAML Structure:
   • I’ve drafted a coverage_options.yaml file that organizes defaults under tool-specific keys while handling shared options efficiently.

I’d appreciate it if you could review the document and share your feedback. If everything looks good, kindly assign this task to me.
Looking forward to your response!

[liamappelbe]

Hi @Dhruv-Maradiya

Sorry for the slow response, I was away for Christmas. Looks like I'm 1 day too late to review your design before you sent out the PR, so there might be some churn, sorry 😅. Let's nail down the yaml format before I review the PR.

Some high level comments on your design (I'll use "flags" to refer to both flags and options, since the distinction only matters in the ArgParser):

• I think that the yaml format should combine all the flags into a single set, rather than splitting out the flags for each tool
• The yaml format doesn't need to include deprecated flags. The only reason those flags still exist is so we don't break old users who are still using them, so when we're defining a new format we shouldn't include them.
• I think we can conceptually divide all the flags into two groups:
  • Package flags - stuff that is specific to the whole package, and how it uses package:coverage, and won't change often
  • Run time flags - stuff that could vary between runs of the tool, or between host machines
• The config yaml should only include package flags. It's going to be checked in to people's github repos, so anything that varies between computers, or between runs of the tool, shouldn't be in there.
• All paths should be resolved relative to the config file itself. We can't expect users to specify absolute paths, since those will change between computers.
• The goal is to make the common use cases easier. We don't need to include uncommon flags. We can always add missing flags later if users want them.

With that in mind, I reviewed your design and thought about which flags the yaml file should include. After I did that, I realized I'd basically ended up with the list of flags that test_with_coverage.dart supports (which makes sense, since that's the script that is designed to support the common use cases), but the other tools should also look at this yaml file for their flags. I didn't include port though, since that seems like more of a run time flag to me (I'm not 100% sure about that though, what do you think?)

So I think the yaml structure should look like this:

package: 'path/to/package'  # Defaults to '.'
package-name: 'package'  # Deduced from package directory by default
out: 'path/to/output/directory'  # Defaults to '$packageDirectory/coverage'
test: 'path/to/test.dart'  # Defaults to test
function-coverage: false  # Defaults to false
branch-coverage: false  # Defaults to false
scope-output:  # A list that defaults to containing only the name of the package
  - 'package'
  - 'package2'

Also, we might want to use package:cli_config for this, since this is what it's designed for. I haven't used that package before, but their flags look like -Dsome_key=some_value, so we can't fully replace the existing CLI with it if we want existing user's commands to continue working (users often bake coverage commands into github workflows, so it's important we don't break them). But we could have a hybrid solution that keeps our existing flags, while using cli_config for the yaml etc. Not sure how this package handles bool flags and list flags though.

[Dhruv-Maradiya]

Hi @liamappelbe,

Thanks for getting back to me, and no worries about the delay! I really appreciate the detailed feedback. 😊

Here’s what I think about the points you mentioned:

• I agree that runtime flags, like port, are machine-specific and can vary between runs. To handle this, I think we should allow all flags to be specified in the YAML file but let CLI flags override their values. This way, the YAML remains portable for common use cases, but users can still adjust machine-dependent settings as needed.
• About combining all the flags into a single set: I was wondering how we’d handle cases like the out flag, which is used in both collect_coverage and format_coverage and also a very common flag. The value for each would likely be different, so how would we distinguish between them?
• I’ll also look into package:cli_config and see how it works for this use case. I didn’t realize it existed—thanks for pointing it out!

Let me know what you think about these points, and based on your feedback, I’ll update the PR to align with the decided approach.

[liamappelbe]

• I agree that runtime flags, like port, are machine-specific and can vary between runs. To handle this, I think we should allow all flags to be specified in the YAML file but let CLI flags override their values. This way, the YAML remains portable for common use cases, but users can still adjust machine-dependent settings as needed.

I don't see a good reason why the config should contain those flags. For flags that vary between runs, it's actually less convenient for the user to put them in a config than to specify them on the command line. Why would anyone want to do it?

It's very easy to add more flags to the config in future, but hard to remove them (without breaking users). So we should be careful about the flags we add. We should only add flags we think are going to actually be useful. We can add more of the flags later if users ask for them.

• About combining all the flags into a single set: I was wondering how we’d handle cases like the out flag, which is used in both collect_coverage and format_coverage and also a very common flag. The value for each would likely be different, so how would we distinguish between them?

out is a directory. collect_coverage would write to "$out/coverage.json", format_coverage would read from "$out/coverage.json" and write to "$out/lcov.info". So this is replacing the out and in flags for all the tools.

[Dhruv-Maradiya]

Hi @liamappelbe,

Thanks for clarifying! That totally makes sense to me now.

I'll update the PR to incorporate these changes. Let me know if there's anything else you'd like me to tweak. 😊

[Dhruv-Maradiya]

Hi @liamappelbe
I’ve committed my changes to the PR based on our earlier discussion.

I’ve removed all device-specific flags. However, I’ve currently included the following flags, which I believe are uncommon. If we only need to support common flags, we can remove these:

• wait-paused: false
• resume-isolates: false
• include-dart: false
• sdk-root: null
• bazel-workspace: ''
• bazel: false
• pretty-print-branch: false
• pretty-print-func: false
• check-ignore: false

Let me know your thoughts on whether these should be retained or removed.
Thanks!

[liamappelbe]

Yeah, those should all be removed from the yaml.

• Pretty much all users will want wait-paused, resume-isolates, and check-ignore to be true, and include-dart to be false.
• sdk-root is only needed when include-dart is true, and is a machine specific flag anyway.
• The bazel flags are only needed by bazel users, which I think is just a google internal thing.
• The pretty print flags are only relevant for format coverage, and are designed for human readability. This yaml workflow is mainly about supporting automated workflows, which are only going to be using the json and lcov formats.

One issue I just thought of is that some of the default values we want here are different to the default flag values in the tools. wait-paused, resume-isolates, and check-ignore should all default to true when using the yaml workflow, but in collect_coverage and format_coverage they default to false (for historical reasons). In test_with_coverage they default to true (in fact they're not even settable flags). My first thought was to default the flags to true if the user is using the yaml workflow, but that would be really confusing (adding a particular file in a particular location changes the default values of certain flags?). It also doesn't really make sense to include them in the yaml because they wouldn't have any effect in test_with_coverage. Another option would be to only check the yaml file in test_with_coverage, but that also seems a bit confusing to me. So let's just leave those flags out for now, and I'll decide what to do about them later.

[Dhruv-Maradiya]

Thanks for your detailed explanation! I’ve removed all the mentioned flags for now and pushed the updated code.

[liamappelbe]

@Dhruv-Maradiya when your PR is ready for review, add me as a reviewer and I'll take a look. Remember to update the PR description with the new design.

[Dhruv-Maradiya]

Hi @liamappelbe, it seems I don't have the permissions to assign you as a reviewer. The PR is ready for your review.