Added in-build coverage collection and checking in CI

[altendky]

No description provided.

[altendky]

A passing build: https://github.com/Parquery/pylddwrap/pull/20/checks?check_run_id=2318888705
A project percent failure: https://github.com/Parquery/pylddwrap/pull/20/checks?check_run_id=2318963566
A diff percent failure: https://github.com/Parquery/pylddwrap/pull/20/checks?check_run_id=2319049716

[altendky]

Coveralls doesn't have great integration for GitHub Actions. It can only manage comments in the PR for in-repo PRs, not PRs from forks (I think we could do something, wouldn't be great). To report coverage to them you have to publicly post your Coveralls repository token. I think that codecov handles this by providing a GitHub application that you give some level of access to your repository, but Coveralls doesn't seem to have this yet.

As to posting the Coveralls token publicly, both Trio and Twisted have posted such tokens to deal with PRs from forks. They concluded that the associated risk was acceptable since, at least for the first order, it is just a hazard of uploading bogus coverage reports.

https://github.com/altendky/pylddwrap/blob/65e90cbb62ccb8a050519af72535f47ced1813c7/.github/workflows/ci.yml#L246-L252

    - name: Publish to Coveralls
      if: always()
      continue-on-error: true
      run: |
        python -m coveralls -v
      env:
        COVERALLS_REPO_TOKEN: 'fill in here'

As is, the project is using coverage itself to assert >90% project coverage and diff-cover to assert 100% coverage of the changes.

Let me know where we should go from here, or how any questions you have on the topic.

[altendky]

@mristin ping about ^ just to help with notifications maybe. :]

[mristin]

Hi @altendky ,
LGTM! The coverage of the pull requests doesn't have to go to coveralls. I think your solution with the artifacts is great for the code reviews. The only thing that has to go to coveralls is the coverage on merges to main branch, which run within the repository, so no tokens need to be leaked etc.

[altendky]

Alrighty, I'll update in hopes the coverage upload works on merge...

[altendky]

@mristin, welp, I don't know if the error is the right one or not...

Could not submit coverage: 422 Client Error: Unprocessable Entity for url: coveralls.io/api/v1/jobs

But, it is trying to upload anyways. We could try this in-repo if you push the branch and PR if you want to avoid trying to fix this on master.

[mristin]

But, it is trying to upload anyways. We could try this in-repo if you push the branch and PR if you want to avoid trying to fix this on master.

@altendky Let's just fix it on master -- I think the coveralls will just work once it is merged. Could you please ping me when you'd like me to review the pull request? (Or is it already review-ready?)

[altendky]

@mristin I think it's ready for review, yeah.

[altendky]

No particular rush on this end, but just a gentle bump in case this got lost.

[altendky]

@mristin (bah, forgot the pingy bit)

[mristin]

LGTM! Please let me know when it is ready to be merged in!

[altendky]

I'm good with it, thanks.

[mristin]

Great, thanks! It's in :).