Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for coverage config file (.coveragerc) #10289

Merged
merged 4 commits into from Jul 9, 2020

Conversation

asherf
Copy link
Member

@asherf asherf commented Jul 8, 2020

Problem

We currently hard code the coverage configuration file (.coveragerc) so users can't set their own config.

Solution

Allow users to specify a .coveragerc config file and add the inject the parameters we need to that file before passing it to the coverage tool.

Closes #10289.

@asherf asherf marked this pull request as draft July 8, 2020 01:26
@asherf
Copy link
Member Author

asherf commented Jul 8, 2020

still working on adding more tests

@asherf asherf force-pushed the msp branch 3 times, most recently from bd22f99 to b67a4bd Compare July 8, 2020 22:05
@asherf asherf changed the title WIP Coverage config Add support for converage config file (.coveragerc) Jul 8, 2020
@asherf asherf marked this pull request as ready for review July 8, 2020 22:36
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff... just nits. Thanks!

type=file_option,
default=None,
advanced=True,
help="Path to `.coveragerc` or alternative coverage config file",
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"alternative coverage config file"

I'm not sure what this means... does it mean something like "or other file in the .coveragerc format"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wording comes from the other subsystems. It's meant to say that it could for example be pyproject.toml. See all the options they have at https://coverage.readthedocs.io/en/coverage-5.2/config.html.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/python/pants/backend/python/rules/coverage.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Thanks Asher!

type=file_option,
default=None,
advanced=True,
help="Path to `.coveragerc` or alternative coverage config file",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wording comes from the other subsystems. It's meant to say that it could for example be pyproject.toml. See all the options they have at https://coverage.readthedocs.io/en/coverage-5.2/config.html.

def _validate_and_update_config(coverage_config: configparser.ConfigParser) -> None:
if not coverage_config.has_section("run"):
coverage_config.add_section("run")
coverage_config.set("run", "branch", "True")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we still want this turned on by us? I personally don't like when Pants makes decisions like this - who are we to say that you want branch data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just trying to preserve existing behavior....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. To clarify, I'm proposing breaking with existing behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this kind of change should be part of its own PR.... especially since this change breaks existing integration tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/python/pants/backend/python/rules/coverage_test.py Outdated Show resolved Hide resolved
@stuhood stuhood changed the title Add support for converage config file (.coveragerc) Add support for coverage config file (.coveragerc) Jul 9, 2020
@stuhood stuhood merged commit 93e0b48 into pantsbuild:master Jul 9, 2020
asherf added a commit to asherf/pants that referenced this pull request Jul 16, 2020
asherf added a commit to asherf/pants that referenced this pull request Jul 16, 2020
asherf added a commit to asherf/pants that referenced this pull request Jul 16, 2020
Eric-Arellano pushed a commit that referenced this pull request Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants