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: Update tests workflow #199

Merged
merged 4 commits into from
Sep 28, 2022
Merged

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Sep 21, 2022

  • Add workflow dispatch option to run on demand.
  • Test against Python 3.8 which is the version of Python the ROOT binary was compiled against given the output of root-config --python-version.
  • Update all GitHub Actions.
  • Avoid deprecated direct calls to python setup.py and use local pip installs.
  • Use python -m pytest to add current working directory to sys.path as src/ dir layout not used.
  • Run on push events only if to master to let coverage from PRs report properly.
  • Use Codecov for reporting coverage.
    • Update badge in README to use Codecov Coverage Status

@matthewfeickert
Copy link
Member Author

@clelange this is ready for review, but it covers quite a bit in its updates. If you'd prefer to just have the GHA updates and few other things I can do git surgery to leave the rest alone.

@clelange
Copy link
Collaborator

Thank you very much! CI really needs a revamp...

I think is looks all good, I'm just wondering why test coverage doesn't work anymore. Any idea?

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 22, 2022

I think is looks all good, I'm just wondering why test coverage doesn't work anymore. Any idea?

Hm. I had assumed at first because this was coming from a fork that the coverage reporting wasn't working, but that's not the case given that the logs show

$ python -m pip install coveralls==2.2
$ coveralls --service=github

Collecting coveralls==2.2
  Downloading coveralls-2.2.0-py2.py3-none-any.whl (13 kB)
Collecting docopt>=0.6.1
  Downloading docopt-0.6.2.tar.gz (25 kB)
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'
Requirement already satisfied: requests>=1.0.0 in /opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages (from coveralls==2.2) (2.28.1)
Collecting coverage<6.0,>=4.1
  Downloading coverage-5.5-cp38-cp38-manylinux2010_x86_64.whl (245 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 245.3/245.3 kB 5.4 MB/s eta 0:00:00
Requirement already satisfied: charset-normalizer<3,>=2 in /opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages (from requests>=1.0.0->coveralls==2.2) (2.1.1)
Requirement already satisfied: urllib3<1.27,>=1.21.1 in /opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages (from requests>=1.0.0->coveralls==2.2) (1.26.12)
Requirement already satisfied: certifi>=2017.4.17 in /opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages (from requests>=1.0.0->coveralls==2.2) (2022.9.[14](https://github.com/HEPData/hepdata_lib/actions/runs/3101546764/jobs/5023021281#step:12:15))
Requirement already satisfied: idna<4,>=2.5 in /opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages (from requests>=1.0.0->coveralls==2.2) (3.4)
Building wheels for collected packages: docopt
  Building wheel for docopt (setup.py): started
  Building wheel for docopt (setup.py): finished with status 'done'
  Created wheel for docopt: filename=docopt-0.6.2-py2.py3-none-any.whl size=13706 sha256=600a765776aad8afb53411c577e03ee681871a0f042ea0bfaa3447c0b9fe8a6d
  Stored in directory: /home/runner/.cache/pip/wheels/56/ea/58/ead137b087d9e326852a851351d1debf4ada529b6ac0ec4e8c
Successfully built docopt
Installing collected packages: docopt, coverage, coveralls
  Attempting uninstall: coverage
    Found existing installation: coverage 6.4.4
    Uninstalling coverage-6.4.4:
      Successfully uninstalled coverage-6.4.4
Successfully installed coverage-5.5 coveralls-2.2.0 docopt-0.6.2
Submitting coverage to coveralls.io...
Coverage submitted!
Job #3101546764.1
https://coveralls.io/jobs/106515848

https://coveralls.io/builds/52666161

I'll put this PR into draft mode for the time being and look into it later.

@matthewfeickert matthewfeickert marked this pull request as draft September 22, 2022 15:59
@matthewfeickert matthewfeickert force-pushed the ci/update-ci branch 4 times, most recently from 19b3885 to 34e751f Compare September 22, 2022 16:57
@matthewfeickert
Copy link
Member Author

@clelange I had forgotten what a pain coveralls is if you want to use pytest. c.f. coverallsapp/github-action#30. Any chance I could convince you to switch to using Codecov (https://about.codecov.io/)? We use that for basically all of the Scikit-HEP projects and it works super well with pytest.

@clelange
Copy link
Collaborator

Sure, whatever is easier!

Comment on lines 72 to 82
- name: Report contrib coverage with Codecov
uses: codecov/codecov-action@v3
with:
files: ./coverage.xml
flags: unittests-${{ matrix.python-version }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get coverage report comments on the PR you'll need to enable Codecov for the repo in the GitHub settings (similarly to how you did for Coveralls).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, should be done. Thanks again!
Even though it's probably not needed, I've added a variable CODECOV_TOKEN containing the token.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay let me do a force push to try to trigger.

@matthewfeickert matthewfeickert marked this pull request as ready for review September 22, 2022 17:26
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@4a65a17). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #199   +/-   ##
=========================================
  Coverage          ?   87.75%           
=========================================
  Files             ?        4           
  Lines             ?      964           
  Branches          ?      195           
=========================================
  Hits              ?      846           
  Misses            ?       86           
  Partials          ?       32           
Flag Coverage Δ
unittests-3.8 87.75% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matthewfeickert
Copy link
Member Author

@@           Coverage Diff            @@
##             master    #199   +/-   ##
========================================
  Coverage          ?   0.00%           
========================================
  Files             ?       4           
  Lines             ?     964           
  Branches          ?       0           
========================================
  Hits              ?       0           
  Misses            ?     964           
  Partials          ?       0           

Hm. That's certainly not right. I'll have to figure out why a bit later.

@matthewfeickert matthewfeickert marked this pull request as draft September 22, 2022 18:00
@matthewfeickert matthewfeickert force-pushed the ci/update-ci branch 4 times, most recently from 9419467 to d98dfd2 Compare September 22, 2022 18:33
@matthewfeickert matthewfeickert marked this pull request as ready for review September 22, 2022 18:38
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 22, 2022

Codecov seems slow in updating its comment, but the coverage is now being reported at 87%: https://app.codecov.io/github/HEPData/hepdata_lib/pull/199

That might be laggy though as the CI logs are reporting 90%:

---------- coverage: platform linux, python 3.8.13-final-0 -----------
Name                           Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------
hepdata_lib/__init__.py          306     22    120      9    92%   17-18, 30, 35, 262-266, 320-324, 358, 446, 505-508, 513-516, 564->566, 567, 599->exit, 603-604, 672
hepdata_lib/c_file_reader.py     312     18    114      6    94%   37->53, 40-43, 54, 200-201, 298-300, 308, 314-316, 359-360, 379, 390-391
hepdata_lib/helpers.py           134     21     67      8    81%   47, 82, 131-137, 153-160, 201-203, 250, 284, 305, 324, 339
hepdata_lib/root_utils.py        212     25     92      9    87%   39->48, 42, 49, 84->83, 88-93, 109-111, 118, 252-265, 298, 378, 432
--------------------------------------------------------------------------
TOTAL                            964     86    393     32    90%
Coverage HTML written to dir htmlcov
Coverage XML written to file coverage.xml

edit: Bumping again with a force push to see if that helps.

* Add workflow dispatch option to run on demand.
* Test against Python 3.8 which is the version of Python the ROOT binary
  was compiled against given the output of `root-config --python-version`.
* Update all GitHub Actions.
* Avoid deprecated direct calls to `python setup.py` and use local pip
  installs.
* Use 'python -m pytest' to add current working directory to sys.path as
  src/ dir layout not used.
   - c.f. https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html
* Run on push events only if to master to let coverage from PRs report properly.
* Use Codecov for reporting coverage.
   - Update badge in README to use Codecov
   - Add Codecov config file
@matthewfeickert
Copy link
Member Author

@clelange okay after some bumping to get https://app.codecov.io/github/HEPData/hepdata_lib/commits?branch=ci%2Fupdate-ci to recognize that there was a diff from the last commit that it should inspect, the Codecov comment #199 (comment) has been updated by the GHA to show the new coverage it is reporting:

@@            Coverage Diff            @@
##             master     #199   +/-   ##
=========================================
  Coverage          ?   87.75%           
=========================================
  Files             ?        4           
  Lines             ?      964           
  Branches          ?      195           
=========================================
  Hits              ?      846           
  Misses            ?       86           
  Partials          ?       32           

It is a bit lower than what coveralls was reporting because I added the --cov-branch option.

@matthewfeickert
Copy link
Member Author

@clelange this is ready for review again (but no rush).

Copy link
Collaborator

@clelange clelange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, this all looks good to me!

@clelange clelange merged commit 764d939 into HEPData:master Sep 28, 2022
@matthewfeickert matthewfeickert deleted the ci/update-ci branch September 28, 2022 13:25
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.

3 participants