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

Simplify pytest-cov configuration. #3391

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Simplify pytest-cov configuration. #3391

merged 4 commits into from
Feb 20, 2024

Conversation

jdangerx
Copy link
Member

@jdangerx jdangerx commented Feb 12, 2024

Overview

I was investigating #3381 when I ran into a slight weirdness in our pytest-cov configuration in pyproject.toml.

It sort of seems like this would fix it? @zaneselvans I know you were the one who was messing with this in the first place, did you already try this and have it not work?

See docs for v4.1.0 at
https://github.com/pytest-dev/pytest-cov/blob/2c9f2170d8575b21bafb6402eb30ca7de31e20b9/docs/config.rst

  • particularly:

If you use the --cov=something option (with a value) then coverage's
source option will also get overridden. If you have multiple sources it
might be easier to set those in .coveragerc and always use --cov
(without a value) instead of having a long command line with --cov=pkg1
--cov=pkg2 --cov=pkg3 ....

Testing

How did you make sure this worked? How can a reviewer verify this?

I removed .coverage, then ran make pytest-coverage and saw that the coverage denominator was still 13450 statements.

To-do list

Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

I think you're probably right about swapping in a --cov here.

There are a couple of other places where we're calling pytest that we might want to check to see if we're repeatedly specifying the same args unnecessarily or in conflict: the nightly build script, the Makefile, and the pre-commit configuration. (the pytest workflow uses make)

pyproject.toml Outdated
# (though it *does* pick up the omit parameters!). This means we need to specify the
# source directories we want to collect coverage twice, and keep the two in sync.
addopts = "--verbose --pdbcls=IPython.terminal.debugger:TerminalPdb --cov-append --cov-config=pyproject.toml --cov-report=xml --cov=src/pudl --cov=test/integration --cov=test/unit"
addopts = "--verbose --pdbcls=IPython.terminal.debugger:TerminalPdb --cov-append --cov-config=pyproject.toml --cov-report=xml --cov -n auto"
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember exactly what was happening here, but it seemed to have to do with coverage behaving differently when run directly, vs. run through pytest. But maybe I just didn't realize that a blanket --cov would default to whatever sources were listed in the configuration file. Seems likely.

We also need to deal with -n auto crashing your machine when you try to run the validation tests since we end up with potentially several copies of our largest dataframes getting read into memory simultaneously which is too much. But this problem isn't limited to this line.

Did you look at the Makefile, .pre-commit-config.yaml and docker/gcp_pudl_etl.sh to see if the pytest / coverage commands being used in there need to be updated to work with these addopts and/or avoid duplicative specifications of the same arguments in multiple places?

@jdangerx jdangerx force-pushed the tweak-coverage-settings branch 2 times, most recently from 57b70b9 to 189528c Compare February 16, 2024 22:07
@jdangerx
Copy link
Member Author

@zaneselvans - went through the places you mentioned (Makefile, gcp_pudl_etl.sh, and pre-commit) and found that we were respecifying the gcs cache path everywhere. So I pulled that into the pyproject.toml.

Other than that, I removed the -n auto because of the memory issue, and everything else seems to be fine / non-duplicative.

@zaneselvans zaneselvans added the testing Writing tests, creating test data, automating testing, etc. label Feb 16, 2024
@jdangerx jdangerx added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@zaneselvans
Copy link
Member

If there's a safe number of CPUs we could use by default (like 4?) it would nice to keep some amount of parallelism. It does speed things up. Maybe we can turn it back on after we kill PudlTabl and get all the validations into the ETL / Pandera setup.

Make pytest-cov pick up coverage sources from tool.coverage.run

See docs for v4.1.0 at
https://github.com/pytest-dev/pytest-cov/blob/2c9f2170d8575b21bafb6402eb30ca7de31e20b9/docs/config.rst
- particularly:

> If you use the --cov=something option (with a value) then coverage's
> source option will also get overridden. If you have multiple sources it
> might be easier to set those in .coveragerc and always use --cov
> (without a value) instead of having a long command line with --cov=pkg1
> --cov=pkg2 --cov=pkg3 ....
@jdangerx jdangerx added this pull request to the merge queue Feb 20, 2024
Merged via the queue into main with commit 1dc05fd Feb 20, 2024
12 checks passed
@jdangerx jdangerx deleted the tweak-coverage-settings branch February 20, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Writing tests, creating test data, automating testing, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants