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

Bugfix/coveralls GitHub action updated #1772

Merged
merged 2 commits into from
Aug 13, 2021
Merged

Bugfix/coveralls GitHub action updated #1772

merged 2 commits into from
Aug 13, 2021

Conversation

vikramaditya91
Copy link
Contributor

The coverallsapp in GitHub actions was pinned to a particular commit from a year ago. It was done here #1402
This branch pins itself to the master branch of coverallsapp to avoid technical debt.

Also, for the coveralls command, it uses the https://coveralls-python.readthedocs.io/en
instead of https://github.com/bboe/coveralls-python/archive/github_actions.zip
But the coveralls-python seems to be maintained and up to date.
I am not too convinced about this.

FYI: Ideally, we would not be using the coveralls command in section: Submit to coveralls. But because of this issue coverallsapp/github-action#30 this seems to be the simpler choice

Vikramaditya Gaonkar added 2 commits July 30, 2021 02:51
@LilSpazJoekp
Copy link
Member

LilSpazJoekp commented Aug 13, 2021

FYI: Ideally, we would not be using the coveralls command in section: Submit to coveralls. But because of this issue coverallsapp/github-action#30 this seems to be the simpler choice

Could you elaborate on this? What we are currently doing works for Coveralls and your change does too.

@vikramaditya91
Copy link
Contributor Author

vikramaditya91 commented Aug 13, 2021

@LilSpazJoekp
The official coverallsapp's documentation suggests running the coverage through this pattern

    - name: Coveralls Parallel
      uses: coverallsapp/github-action@master
      with:
        github-token: ${{ secrets.GITHUB_TOKEN }}
        flag-name: run-${{ matrix.python-version }}
        parallel: true

The advantage of this method is that it removes dependency on the python package coveralls.

https://github.com/vikramaditya91/praw/commit/fb6a0dcc565bbcfb9ac2b484047c4fe2c93bd272#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f

However, as you can see in the GH actions of that above commit, it crashes with

 Using lcov file: ./coverage/lcov.info
Error: Lcov file not found.

This is because pytest coverage does not generate a lcov file as also stated in this issue coverallsapp/github-action#30

That is the reason, I had to still use the coveralls to push the generated .coverage file to coveralls.

If one day, they allow using the results from the coverage results in the line

coverage run --source praw --module pytest

then we can fully ditch the coveralls package which would be unnecessary here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants