Actions workflow to auto-remove unnecessary entries from .coveragerc #113914

davet2001 · 2024-03-20T23:02:16Z

Proposed change

.coveragerc omits certain files from the pytest code coverage report, but some of these omissions are unnecessary because the omitted file reports as having 100% coverage.

This PR includes a script to check .coveragerc for any lines that can be removed,
~~Also in the PR is the leaner .coveragerc it generated.~~ The generated leaner .coveragerc will be in a separate PR

............
Edit 29/3/2024:
The PR now includes a workflow set to run at 4AM each Monday morning, to automatically do the check and create a PR if an improvement is possible. At present I'm not sure if that type of automation is desirable on the HA project, so feedback welcome.

It's also set up so that it can be triggered manually in GH actions.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

edenhaus

Would it be possible to extend the CI to automatically check for it on both (partial and full) runs?
~~As we already create the coverage reports in the CI, I think we only need to parse them at the end~~

script/check_coveragerc.py

joostlek · 2024-03-21T09:05:14Z

Wouldn't checking in the CI making it cause the tests to run again? (Since these files are ignored in coverage calculation). Or you'd need another way to filter out these files from the generated coverage report

edenhaus · 2024-03-21T09:27:33Z

Wouldn't checking in the CI making it cause the tests to run again? (Since these files are ignored in coverage calculation). Or you'd need another way to filter out these files from the generated coverage report

True that... completely forgotten about it...

Not sure if it works but could we disable the .coveragerc on the ci runs and before uploading them to codecov, we filter ignored files out?
So we run the test only ones and can do both things

davet2001 · 2024-03-21T22:58:33Z

Ok, so I had originally envisaged this being something that someone would do once in a while e.g. every 6 months, then just create a PR

But I do agree that the coverage.xml produced by the CI is tantalisingly close to implementing coveragerc checking/trimming per build.

Firstly, we should be sure of where we are heading. Are we saying we'd fail a build if something was in .coveragerc that didn't need to be? If not, then what would the actionable output be and who would know to look at it?

In any case, a possible route for that would be:

Modify the ci.yaml script to do the following
- Generate a duplicate .coveragerc in a temporary dir with the entire omit section removed.
- Run pytests using the custom .coveragerc file specified as an argument
- examine the xml output
- Identify every file where all file lines got at least one hit
- Search for those files in .coveragerc
- If there are >0 files, display them and fail the test
- Modify the coverage.xml file to remove file entries that were in the orginal coveragerc before it was modified
- Upload the manipulated coverage.xml to codecov to make it behave like the removed lines were omitted from the test as per normal.

Some things complicate this approach though:

What about if we are testing more than one python version? Presumably a line would need to be found to be 100% tested on all versions before we removed it.
I see line-rate="x.xxx" entries in coverage.xml, which probably means we can't just delete tags and expect the top level data to remain accurate.

Overall, I am not convinced there is enough to gain by doing all this.

epenet · 2024-03-22T07:43:01Z

script/check_coveragerc.py

+        out_list = []
+        for filename, file_data in data["files"].items():
+            percent = file_data["summary"]["percent_covered"]
+            if percent == 100.0:


When I did a similar thing in the past, we used a lower threshold.
I think we used 90% or maybe even 80%

The idea behind that was that if it was already at say 80%, then we should leverage coverage to warn on coverage reductions.

Actually it was 97% threshold in #86466
See also #60349

joostlek · 2024-03-22T09:50:21Z

I think it makes sense to remove them every once in a while. I think it's also a task for the reviewer to keep an eye out on what's added and what's removed. For example, a const.py never needs any coverage (unless they aren't used, but then they should not exist in the first place).

One thing that I would think is easy to forget is say, I add tests for a sensor.py (test_sensor.py), but my sensor.py is in the .coveragerc file. Since the .coveragerc isn't touched as a reviewer I probably forget it's there.

joostlek · 2024-03-22T09:51:35Z

And maybe this can just be a GitHub action that runs once every week at Sunday night or something like that

epenet · 2024-03-29T07:06:47Z

I'm being picky here...

~~Can the entries be sorted?~~ Ensure coverage entries are sorted #114424
~~Can we prevent whole folders, and expand them instead (danfoss_air/*, dweet/*, etc.)~~ Prevent wilcard coverage ignore if tests exist #86455

Edit:
or maybe that is out of scope and should be adjusted in /dev/script/hassfest/coverage.py instead

davet2001 · 2024-03-29T10:11:01Z

Thanks for the reviews. As you may see I've got an auto-pruning workflow running but currently having some trouble with it failing the tests that normally pass.

I've been testing a cut down proof of concept here:
https://github.com/davet2001/coveragerc_test

And trying to run the full thing on my fork here:
https://github.com/davet2001/home-assistant

The full test suite is a beast! So will use [no ci] in future commits to this PR to avoid taking up too many of HA's CI minutes. Also will add the timeouts as a precaution.

davet2001 · 2024-03-29T16:14:56Z

Still lots of problems with the tests - seems like it's failing every other test. If anyone has ideas why this is happening I would value their ideas. I may try turning off parallel testing.

davet2001 · 2024-03-29T23:35:11Z

Ok, I think this is ready now. The pytest problems went away when I switched off parallel testing. I have tested on my fork and it's working ok including generating a PR.

Feedback welcome - a key question is whether workflows that generate PRs are allowed.

edenhaus · 2024-04-04T07:15:03Z

.github/workflows/codecov.yml

+          git fetch origin dev
+          git add .coveragerc
+          git commit -m "Remove unnecessary .coveragerc entries"
+          git push --force --set-upstream origin ${{ env.BRANCH_NAME }}


Do we really need to make a force push here?

Effectively this is aiming to delete the existing prune_coveragerc_1 branch and replace it with a newly generated one. I didn't want to deal with any merge conflicts. If there were any, I expect the best action would be to fetch, reset, re-run and force push anyway. By not deleting the branch it means any existing PR is retained.

TBH I'm not fully sure what happens with github if you delete a PR's branch then create a new branch with the exact same name.

.github/workflows/codecov.yml

edenhaus · 2024-04-04T07:18:07Z

.github/workflows/ci.yaml

@@ -31,6 +31,12 @@ on:
        description: "Only run mypy"
        default: false
        type: boolean
+  workflow_call:


Will the workflow_call run the whole CI or only the base job?

workflow_call here is just to allow codecov.yml to use parts of ci.yaml.
Actually it seems to be pretty intelligent and only runs the dependencies (in this case just 'base').
Example run:
https://github.com/davet2001/home-assistant/actions/runs/8503746102

Scratch that. I think you are right - it looks like it calls everything in the CI. So there are some extras we don't really need e.g. pytest-postgres, lint etc. I can't see a way of stopping this, although the big things seem to be the pytest which I do want. I can't see a way of selectively calling only part of a workflow_call, whilst having a custom trigger.

I suppose I could duplicate only the base job into the new .yml file but that seems not ideal.

If you can see a better approach, please let me know!

I would duplicate what you need from the ci pipeline. By duplicating, we decouple both.
Another idea would be to create a separate file for the base job and use it in both files. But we need to remember that ci.yml is the main ci and should run smoothly. If creating a separate file for the base job introduces delays or other problems to the ci.yml, we should duplicate it instead

Ok, done. Now duplicated and fully separated.

edenhaus · 2024-04-08T07:23:59Z

.github/workflows/codecov.yml

+  # - 10.11.2 is the version currently shipped with Synology (as of 11 Oct 2023)
+  # mysql 8.0.32 does not always behave the same as MariaDB
+  # and some queries that work on MariaDB do not work on MySQL
+  MARIADB_VERSIONS: "['mariadb:10.3.32','mariadb:10.6.10','mariadb:10.10.3','mariadb:10.11.2','mysql:8.0.32']"


Please check if all env variables are required, and if not remove the unused ones.
I expect the at least the db variables can be removed

home-assistant bot added cla-signed code-quality small-pr PRs with less than 30 lines. labels Mar 20, 2024

MartinHjelmare changed the title ~~Remove unnecesary entries from .coveragerc~~ Remove unnecessary entries from .coveragerc Mar 21, 2024

frenck added the smash Indicator this PR is close to finish for merging or closing label Mar 21, 2024

edenhaus reviewed Mar 21, 2024

View reviewed changes

script/check_coveragerc.py Outdated Show resolved Hide resolved

script/check_coveragerc.py Outdated Show resolved Hide resolved

script/check_coveragerc.py Outdated Show resolved Hide resolved

script/check_coveragerc.py Outdated Show resolved Hide resolved

epenet reviewed Mar 22, 2024

View reviewed changes

davet2001 marked this pull request as draft March 28, 2024 23:57

davet2001 force-pushed the check_coveragerc2 branch from 5ace68a to 43088f3 Compare March 28, 2024 23:58

epenet mentioned this pull request Mar 29, 2024

Ensure coverage entries are sorted #114424

Merged

20 tasks

davet2001 marked this pull request as ready for review March 29, 2024 21:39

davet2001 requested a review from a team as a code owner March 29, 2024 21:39

edenhaus reviewed Apr 4, 2024

View reviewed changes

davet2001 force-pushed the check_coveragerc2 branch from 5367ee9 to 3bbcb2e Compare April 4, 2024 22:10

davet2001 added 7 commits April 6, 2024 12:19

Add script to tighten .coveragerc

6f4a9d8

Updates following review

76f370f

Remove more by checking for >97%

2e998d7

Move test threshold to a constant

e5c57d9

Add .coveragerc automatic pruning

2c8bd2a

rename reference from ci.yml to ci.yaml

7eeef54

Allow ci workflow calls, output python ver and cache key

d5d697b

davet2001 added 11 commits April 6, 2024 12:19

Revert accidental indent

1fd42a6

[no ci] Add title and align pytest settings to ci.yaml

7a10597

[no ci] Allow manual workflow despatch

328d957

[no ci] Try turning off parallel pytest

9599312

[no ci] Update condition logic

2cc6af6

[no ci] Clean up generated PR body

f2beff5

Fix logic

fa52355

[no ci] Fix conditional logic

41b4f48

[no ci] Fix logic

e6f9e88

Update docs

1640d1a

Correct comment

0ce890f

davet2001 force-pushed the check_coveragerc2 branch from c038e00 to 0ce890f Compare April 6, 2024 13:54

davet2001 marked this pull request as draft April 6, 2024 13:57

davet2001 added 6 commits April 6, 2024 14:08

[no ci] Decouple codecov.yml from ci.yaml

d864e9b

[no ci] Match all ENV values from ci.yaml

d568837

[no ci] Correct variables

f96a2b0

[no ci] Correct variables

f548c67

[no ci] Fix missing dependency

9bf9cca

[no ci] Use full python version

1a8c67b

davet2001 changed the title ~~Remove unnecessary entries from .coveragerc~~ Actions worfklow to auto-remove unnecessary entries from .coveragerc Apr 6, 2024

[no ci] Revert unnecessary edit to ci.yaml

447d744

davet2001 marked this pull request as ready for review April 6, 2024 15:10

edenhaus reviewed Apr 8, 2024

View reviewed changes

[no ci] remove unused env vars

6436ca2

MartinHjelmare changed the title ~~Actions worfklow to auto-remove unnecessary entries from .coveragerc~~ Actions workflow to auto-remove unnecessary entries from .coveragerc Apr 12, 2024

davet2001 added 2 commits May 13, 2024 18:01

Merge branch 'dev' into check_coveragerc2

2016d93

Fix missing env var

157a7c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actions workflow to auto-remove unnecessary entries from .coveragerc #113914

Actions workflow to auto-remove unnecessary entries from .coveragerc #113914

davet2001 commented Mar 20, 2024 •

edited

edenhaus left a comment •

edited

joostlek commented Mar 21, 2024

edenhaus commented Mar 21, 2024

davet2001 commented Mar 21, 2024 •

edited

epenet Mar 22, 2024

epenet Mar 22, 2024 •

edited

joostlek commented Mar 22, 2024

joostlek commented Mar 22, 2024

epenet commented Mar 29, 2024 •

edited

davet2001 commented Mar 29, 2024

davet2001 commented Mar 29, 2024

davet2001 commented Mar 29, 2024

edenhaus Apr 4, 2024

davet2001 Apr 4, 2024

edenhaus Apr 4, 2024

davet2001 Apr 4, 2024

davet2001 Apr 4, 2024

edenhaus Apr 5, 2024

davet2001 Apr 6, 2024

edenhaus Apr 8, 2024

davet2001 Apr 9, 2024

Actions workflow to auto-remove unnecessary entries from .coveragerc #113914

Are you sure you want to change the base?

Actions workflow to auto-remove unnecessary entries from .coveragerc #113914

Conversation

davet2001 commented Mar 20, 2024 • edited

Proposed change

Type of change

Additional information

Checklist

edenhaus left a comment • edited

Choose a reason for hiding this comment

joostlek commented Mar 21, 2024

edenhaus commented Mar 21, 2024

davet2001 commented Mar 21, 2024 • edited

Choose a reason for hiding this comment

epenet Mar 22, 2024 • edited

Choose a reason for hiding this comment

joostlek commented Mar 22, 2024

joostlek commented Mar 22, 2024

epenet commented Mar 29, 2024 • edited

davet2001 commented Mar 29, 2024

davet2001 commented Mar 29, 2024

davet2001 commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davet2001 commented Mar 20, 2024 •

edited

edenhaus left a comment •

edited

davet2001 commented Mar 21, 2024 •

edited

epenet Mar 22, 2024 •

edited

epenet commented Mar 29, 2024 •

edited