Open dgw opened 8 months ago
dgw
commented
8 months ago 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:
dgw
commented
8 months ago 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
commented
8 months ago 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
commented
8 months ago 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
commented
7 months ago The missing link here was generating an XML coverage report, instead of LCOV. https://github.com/coverallsapp/coverage-reporter/issues/109#issuecomment-1807646155
However, I do need to still track down a problem with the tree view after switching. The top-level sopel/ folder is missing now:
Compare to the previous PR build:
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".
dgw
commented
7 months ago 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
commented
2 months ago 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
commented
2 months ago @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
commented
2 months ago @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. :smile_cat:
afinetooth
commented
2 months ago 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:
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:
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.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).
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
make qa(runsmake lintandmake test)