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

ci: switch to Coveralls' first-party GitHub Action #2551

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 9, 2023

Description

This is a first step toward improving our Coveralls integration based on advice in lemurheavy/coveralls-public#1733. We should also keep an eye out for PR builds that display a message about being out of sync with the target branch; that might necessitate separate changes to how and when this CI workflow runs (explained in the issue and in upstream documentation).

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
    • It's impossible for a change not touching code to cause new issues in code. However, I still haven't checked this yet; need to make sure the modified CI job runs as expected in GHA first.
  • I have tested the functionality of the things this change touches
    • Ditto above: Need the PR to run in GHA for verification.

@dgw dgw added the Build label Nov 9, 2023
@dgw dgw marked this pull request as draft November 9, 2023 21:25
@dgw
Copy link
Member Author

dgw commented Nov 9, 2023

The Coveralls action is supposed to be more or less a drop-in replacement for the coveralls CLI command, or so I thought. Will have to dig deeper into how the action works and where it's looking for a .coverage file that it doesn't see but is clearly present on the runner's filesystem after tests finish:

ls -la debug output showing .coverage file
2023-11-09T21:50:44.3648952Z drwxr-xr-x 12 runner docker   4096 Nov  9 21:50 .
2023-11-09T21:50:44.3649649Z drwxr-xr-x  3 runner docker   4096 Nov  9 21:49 ..
2023-11-09T21:50:44.3650614Z -rw-r--r--  1 runner docker   1911 Nov  9 21:49 .codeclimate.yml
2023-11-09T21:50:44.3651544Z -rw-r--r--  1 runner docker 438272 Nov  9 21:50 .coverage
2023-11-09T21:50:44.3652687Z -rw-r--r--  1 runner docker    710 Nov  9 21:49 .coveragerc
2023-11-09T21:50:44.3653349Z drwxr-xr-x  8 runner docker   4096 Nov  9 21:49 .git
2023-11-09T21:50:44.3653830Z drwxr-xr-x  4 runner docker   4096 Nov  9 21:49 .github
2023-11-09T21:50:44.3654320Z -rw-r--r--  1 runner docker    285 Nov  9 21:49 .gitignore
2023-11-09T21:50:44.3654829Z drwxr-xr-x  3 runner docker   4096 Nov  9 21:50 .mypy_cache
2023-11-09T21:50:44.3655373Z drwxr-xr-x  3 runner docker   4096 Nov  9 21:50 .pytest_cache
2023-11-09T21:50:44.3655910Z -rw-r--r--  1 runner docker   9088 Nov  9 21:49 CONTRIBUTING.md
2023-11-09T21:50:44.3656415Z -rw-r--r--  1 runner docker   1022 Nov  9 21:49 COPYING
2023-11-09T21:50:44.3656893Z -rw-r--r--  1 runner docker   1761 Nov  9 21:49 CREDITS
2023-11-09T21:50:44.3657712Z -rw-r--r--  1 runner docker    152 Nov  9 21:49 MANIFEST.in
2023-11-09T21:50:44.3658218Z -rw-r--r--  1 runner docker   1113 Nov  9 21:49 Makefile
2023-11-09T21:50:44.3658710Z -rw-r--r--  1 runner docker 112103 Nov  9 21:49 NEWS
2023-11-09T21:50:44.3659435Z -rw-r--r--  1 runner docker   5191 Nov  9 21:49 NEWS.spec.md
2023-11-09T21:50:44.3659945Z -rw-r--r--  1 runner docker   4564 Nov  9 21:49 README.rst
2023-11-09T21:50:44.3660459Z drwxr-xr-x  2 runner docker   4096 Nov  9 21:50 __pycache__
2023-11-09T21:50:44.3660986Z -rw-r--r--  1 runner docker    998 Nov  9 21:49 conftest.py
2023-11-09T21:50:44.3661489Z drwxr-xr-x  3 runner docker   4096 Nov  9 21:49 contrib
2023-11-09T21:50:44.3662022Z -rw-r--r--  1 runner docker    875 Nov  9 21:49 dev-requirements.txt
2023-11-09T21:50:44.3662550Z drwxr-xr-x  3 runner docker   4096 Nov  9 21:49 docs
2023-11-09T21:50:44.3663056Z -rw-r--r--  1 runner docker   2553 Nov  9 21:49 pyproject.toml
2023-11-09T21:50:44.3663564Z -rw-r--r--  1 runner docker      3 Nov  9 21:49 runtime.txt
2023-11-09T21:50:44.3664067Z -rw-r--r--  1 runner docker   1032 Nov  9 21:49 setup.cfg
2023-11-09T21:50:44.3664548Z drwxr-xr-x 10 runner docker   4096 Nov  9 21:50 sopel
2023-11-09T21:50:44.3665049Z drwxr-xr-x  2 runner docker   4096 Nov  9 21:49 sopel.egg-info
2023-11-09T21:50:44.3665556Z drwxr-xr-x 12 runner docker   4096 Nov  9 21:50 test
Coveralls action failing to find/upload .coverage file
2023-11-09T21:50:45.0949460Z  
2023-11-09T21:50:45.0950070Z ⠀⠀⠀⠀⠀⠀⣿
2023-11-09T21:50:45.0950806Z ⠀⠀⠀⠀⠀⣼⣿⣧⠀⠀⠀⠀⠀⠀⠀ ⣠⣶⣾⣿⡇⢀⣴⣾⣿⣷⣆ ⣿⣿⠀⣰⣿⡟⢸⣿⣿⣿⡇ ⣿⣿⣿⣷⣦⠀⠀⢠⣿⣿⣿⠀⠀⣿⣿⠁⠀⣼⣿⡇⠀⢀⣴⣾⣿⡷
2023-11-09T21:50:45.0951871Z ⠶⣶⣶⣶⣾⣿⣿⣿⣷⣶⣶⣶⠶  ⣸⣿⡟ ⠀⢠⣿⣿⠃⠈⣿⣿⠀⣿⣿⢠⣿⡿⠀⣿⣿⣧⣤⠀⢸⣿⡇⣠⣿⡿⠀⢠⣿⡟⣿⣿⠀⢸⣿⡿⠀⠀⣿⣿⠃⠀⢸⣿⣧⣄
2023-11-09T21:50:45.0952921Z ⠀⠀⠙⢻⣿⣿⣿⣿⣿⡟⠋⠁⠀⠀ ⣿⣿⡇⠀ ⢸⣿⣿⠀⣸⣿⡟⠀⣿⣿⣾⡿⠁ ⣿⣿⠛⠛⠀⣿⣿⢿⣿⣏⠀⢀⣿⣿⣁⣿⣿⠀⣾⣿⡇⠀⢸⣿⡿⠀⠀⡀⠙⣿⣿⡆
2023-11-09T21:50:45.0953980Z ⠀⠀⢠⣿⣿⣿⠿⣿⣿⣿⡄⠀⠀⠀ ⠙⢿⣿⣿⠇⠈⠿⣿⣿⡿⠋⠀⠀⢿⣿⡿⠁⠀⢸⣿⣿⣿⡇⢸⣿⣿⠀⣿⣿⣄⣾⣿⠛⠛⣿⣿⢠⣿⣿⣿ ⣼⣿⣿⣿ ⣿⣿⡿⠋⠀
2023-11-09T21:50:45.0955021Z ⠀⢀⣾⠟⠋⠀⠀⠀⠙⠻⣷⡀⠀⠀
2023-11-09T21:50:45.0955405Z  
2023-11-09T21:50:45.0955737Z   v0.6.6
2023-11-09T21:50:45.0955948Z 
2023-11-09T21:50:45.1175309Z 🔍 Detected coverage file: .coverage
2023-11-09T21:50:45.1240242Z 🚨 Nothing to report

@dgw
Copy link
Member Author

dgw commented Nov 10, 2023

For fun, I tried to run coveralls report --debug --dry-run on my local .coverage file, after running coverage run -m pytest.

coveralls correctly detected the coverage format (python) and filename (.coverage), but no dice on results locally either. Running the SQL query myself (interactively with sqlite3 .coverage) also yielded no results. The line_bits table is empty.

The job now has a new step to generate a report in Lcov format (coverage lcov, a relatively new feature), which the Coveralls reporter can understand. I also dropped the old coveralls dev-requirement and directly required coverage~=7.0 instead, since we don't need the Python package version of coveralls any more (which also constrained us to an older coverage).

@dgw dgw marked this pull request as ready for review November 10, 2023 01:46
@dgw
Copy link
Member Author

dgw commented Nov 10, 2023

Hmm, this changed some things and I think it's considering some things covered that shouldn't be, hence the +0.6% bump. For example, some information about branches seems to be lost or corrupted.

Look at bot.reload_plugin(): With this patch, the conditional if not self.has_plugin(name): is considered fully executed, even though the code that should run if the condition isn't true never runs (the red, uncovered lines below that if block). Results from a recent merge (of #2525) correctly show that same if statement as partially covered, using the older coveralls uploader tool that we've been installing from PyPI.

I also noticed that if I take branch = True out of .coveragerc, the new coveralls uploader is able to parse .coverage without converting it to Lcov first—but obviously that means we lose branch coverage tracking entirely.

Guess this is going back to draft status again.

@dgw dgw marked this pull request as draft November 10, 2023 02:14
@dgw
Copy link
Member Author

dgw commented Nov 10, 2023

Filed coverallsapp/coverage-reporter#109 for the --branch coverage weirdness, and we'll see where that goes before I spend more time on this.

@dgw dgw marked this pull request as ready for review November 13, 2023 18:33
@dgw dgw requested a review from a team November 13, 2023 18:34
@dgw
Copy link
Member Author

dgw commented Nov 13, 2023

The missing link here was generating an XML coverage report, instead of LCOV. coverallsapp/coverage-reporter#109 (comment)

However, I do need to still track down a problem with the tree view after switching. The top-level sopel/ folder is missing now:

image

Compare to the previous PR build:

image

The files are still tracked and available in the "List" tab, but it's not ideal to show only part of the tree under "Tree".

Dropped `coveralls` dev-requirement in favor of using the `coverage`
package directly, and upgraded it to the latest release series (7.x).

The `line_bits` table in .coverage (SQLite DB of results, generated by
`coverage`) isn't populated. coverallsapp/coverage-reporter tries an SQL
query against the database based on this empty table, and fails because
there aren't any coverage results to upload.

We work around this by having `coverage xml` generate a file format that
the reporter can understand.

In `.coveragerc`, switched from setting the `source` option to using the
`include` option instead; this fixes incorrect relative paths being
reported to Coveralls (which breaks both source-fetching and the tree).
@dgw
Copy link
Member Author

dgw commented Nov 13, 2023

OK! The good news is, fixing the screwed-up tree/source views was pretty simple. Bad news is that display of branch coverage is still partially broken on Coveralls with this new pipeline—uploading the XML report turns yellow (partially covered) branch points green, which is a direct step backwards in terms of report readability.

I'm going to continue the existing conversation in coverallsapp/coverage-reporter#109 and/or keep an eye on work toward getting direct parsing of .coverage out of beta. Fortunately I don't think the new coveralls binary is technically in full release yet, and we can still upload reports from the old coveralls package on PyPI. (It does prevent updating to coverage 7.x, though.)

@dgw dgw added the Long-term Planning Things that need to happen at some point in the future, but need to NOT happen soon. label Nov 13, 2023
@dgw dgw removed the request for review from a team November 13, 2023 19:29
@dgw dgw marked this pull request as draft December 11, 2023 23:33
dgw added 2 commits April 18, 2024 14:29
New release today is supposed to fix the issues with Python coverage
parsing. We will see!
@dgw
Copy link
Member Author

dgw commented Apr 18, 2024

Hm, still does not seem to report branch = True results correctly using the .coverage file report type. 🤔

There is also a "Drifted build" warning, but that should be easier to solve later after the main coverage-reporting issue is settled.

@afinetooth
Copy link

afinetooth commented Apr 23, 2024

@dgw dropping by to address just the relatively new changes and recommendations re: "drifted builds."

A "drifted build" is a pull_request build whose base commit is no longer the HEAD of your target branch.

In these instances, since GitHub pull_request builds are based on a prospective merge of the HEADs of both PR and target branches, coverage reports for these types of build may include changes from outside the PR.

Therefore, we make some, again, relatively new recommendations around Configuring your CI service for accurate PR coverage reports, and document Ideal and Minimum CI configurations.

The Ideal configuration (build all push commits on all branches) will avoid "drifted builds" altogether and always guarantee accurate, so-called three-dot diff comparisons.

The Minimum configuration (build push commits on default branch and all pull_request commits on PR branches) will still result in occasional drifted builds. But there are actions you can take to mitigate their effect. We have also added a new repo setting (under status updates) to go ahead and show (potentially accurate) coverage report details with drifted build warnings, which is useful for cases when you know that new commits on the target branch would not affect coverage.

Please let me know if you have any thoughts or suggestions.

@dgw
Copy link
Member Author

dgw commented Apr 23, 2024

@afinetooth Thanks for the rundown! We aren't so attached to the coverage diff of each patch being perfectly accurate, and I suspect we will keep the current CI setup (build all pushes to base branch + all pull_requests) with the new option to show coverage anyway. What matters to the project is overall coverage, and seeing what the coverage looks like after a prospective merge is exactly what we want.

It hasn't caused a problem for us in the past, so there's no reason to increase our load on GHA infra (building more pushes than we do now) unless we later see an impact from the "potentially correct" coverage data misleading us. 😸

@afinetooth
Copy link

afinetooth commented Apr 24, 2024

Hi @dgw.

[...] We aren't so attached to the coverage diff of each patch being perfectly accurate [...] What matters to the project is overall coverage, and seeing what the coverage looks like after a prospective merge is exactly what we want.

Makes complete sense.

In case it's helpful, one way to increase your sense that coverage is moving in the right direction is to use patch status updates, which give you the coverage for each patch itself (as opposed to the coverage diff). When you see those updates in the positive, it means new code is coming in covered and you generally know that coverage is increasing for the PR and for the project.

You can enable that in your Repo Settings:
https://coveralls.io/github/sopel-irc/sopel/settings

Under STATUS UPDATES:

Screenshot 2024-04-23 at 6 07 11 PM

It hasn't caused a problem for us in the past, so there's no reason to increase our load on GHA infra (building more pushes than we do now) unless we later see an impact from the "potentially correct" coverage data misleading us. 😸

That's good and also totally makes sense.

At the risk of sounding like I'm trying to sell you on the "ideal" config, I did just want to explain two more things, which aren't necessarily very clear:

  • Following that "ideal" recommendation---building all pushes on PR branches, but not all pull_requests as well---shouldn't increase the total number of builds since there's already an associated pull_request build for every push build on a PR. On the other hand, if you need the pull_request builds for other reasons, you will be be roughly doubling the number of builds which obviously does increase your load, time and costs.
  • When you switch to the "ideal" config (building only push builds), you don't lose coverage reports for pull_request builds. That's because we check each push build to see if it belongs to a PR, and, if it does, we render the incoming coverage report, as if it were a pull_request build. In addition, we send a push style status update to the commit in the PR commit history, and a pull_request style status update to the PR (in the checks area).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Long-term Planning Things that need to happen at some point in the future, but need to NOT happen soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants