ixc / python-edtf

MIT License
52 stars 19 forks source link

Code Coverage #54

Closed ColeDCrawford closed 3 months ago

ColeDCrawford commented 3 months ago

The ci.yml updates add a commit to PRs. I tested this locally using act as best I could, but ran into an issue that I think will resolve when running on the real runner (undefined head).

The new workflow adds a badge to the readme based on coverage for Python 3.12.

This PR also removes some unnecessary files.

Closes #53

ColeDCrawford commented 3 months ago

@aweakley I might not have enough permissions for this to run (need to be able to modify comments on the PR)

aweakley commented 3 months ago

I've made some changes on https://github.com/ixc/python-edtf/tree/53_coverage_ci_check which amounts to this. I think if we want to include those other items in the printed report we need to add junit-xml, but the comments I'm getting on the commits (eg. f262199f120b49d13c48e4110c4d56929b9d99fb ) work well (especially by email).

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index fb06083..d5416ed 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -1,10 +1,12 @@
 name: CI

 on:
-    workflow_dispatch:
     pull_request:
+    push:
+    workflow_dispatch:

 permissions:
+  checks: write
   contents: write
   pull-requests: write

@@ -58,6 +60,7 @@ jobs:
                 coverage xml -o coverage_combined.xml --omit="edtf_django_tests/*"

             - name: Pytest coverage comment
+              id: coverageComment
               uses: MishaKav/pytest-coverage-comment@main
               with:
                 pytest-xml-coverage-path: ./coverage_combined.xml
@@ -68,12 +71,4 @@ jobs:
               run: |
                 echo "Coverage Percentage - ${{ steps.coverageComment.outputs.coverage }}"
                 echo "Coverage Color - ${{ steps.coverageComment.outputs.color }}"
-                echo "Coverage Html - ${{ steps.coverageComment.outputs.coverageHtml }}"
-                echo "Summary Report - ${{ steps.coverageComment.outputs.summaryReport }}"
                 echo "Coverage Warnings - ${{ steps.coverageComment.outputs.warnings }}"
-                echo "Coverage Errors - ${{ steps.coverageComment.outputs.errors }}"
-                echo "Coverage Failures - ${{ steps.coverageComment.outputs.failures }}"
-                echo "Coverage Skipped - ${{ steps.coverageComment.outputs.skipped }}"
-                echo "Coverage Tests - ${{ steps.coverageComment.outputs.tests }}"
-                echo "Coverage Time - ${{ steps.coverageComment.outputs.time }}"
-                echo "Not Success Test Info - ${{ steps.coverageComment.outputs.notSuccessTestInfo }}"
ColeDCrawford commented 3 months ago

@aweakley I rebased in your changes, added JUnit XML outputs, and tried adding back the JUnit stats. I was getting an error because the comments were too long and modified the coverage report to use the --cov-report=term-missing:skip-covered flag.

I think this is still failing because it is a fork: see https://github.com/MishaKav/pytest-coverage-comment/issues/68. Not sure if you can tweak these permissions: https://github.com/MishaKav/pytest-coverage-comment/issues/68#issuecomment-1469626721. Otherwise you could try another branch off of this to see if the tests pass and then merge either that branch or this PR - I think the fork aspect is making this a bit of a headache.

aweakley commented 3 months ago

It's got that permission already: image

I've made another branch, but I'm getting the same-looking error, although I can see the output above now:

https://github.com/ixc/python-edtf/actions/runs/9217035942/job/25358362828#step:12:25

image

aweakley commented 3 months ago

Ah 7e15e8909b528b5e5979a52f47c9ba692d041030 fixes it. I'll merge 53a_coverage_with_junit

aweakley commented 3 months ago

Thank you.