matthewfeickert commented 1 year ago

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

matthewfeickert commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@clelange I had forgotten what a pain coveralls is if you want to use pytest. c.f. https://github.com/coverallsapp/github-action/issues/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 commented 1 year ago

Sure, whatever is easier!

codecov-commenter commented 1 year ago

Codecov Report

:exclamation: 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 commented 1 year ago

@@           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 commented 1 year ago

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.

matthewfeickert commented 1 year ago

@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 https://github.com/HEPData/hepdata_lib/pull/199#issuecomment-1255366232 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 commented 1 year ago

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

HEPData / hepdata_lib

ci: Update tests workflow #199

Codecov Report