search

pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
561 stars 304 forks source link

Try to implement coverage action instead of using codecov. #1756

Closed Carreau closed 2 months ago

Carreau commented 3 months ago

See #1753

Carreau commented 3 months ago

cc @trallard

See https://github.com/Carreau/pydata-sphinx-theme/pull/1 for the type of comments it posts.

trallard commented 3 months ago

Thanks @Carreau I will check later today Aside I have a CI refactor (you can see a run here https://github.com/trallard/pydata-sphinx-theme/actions/runs/8559269974) that uploads the coverage so we can sync on how to proceed

12rambau commented 2 months ago

Small question here, is this tool still generating a dashboard ? to me that's the main functionality of codecov, the interactivity is great and it's super easy to spot what is wrong. If we really want to drop codecov (which seems to be the consensus) I'm using this for creating dashboard artifact in Azure pipelines I guess that could be used here as well: https://github.com/danielpalme/ReportGenerator

trallard commented 2 months ago

No this does not cover the report this only adds the pr comment. The report generation and codecov replacement is to be covered in a separate pr.

12rambau commented 2 months ago

Any specific reason why you want to add this information in PRs then ? because the only instruction we are giving to codecov currently is to not send report to PR: https://github.com/pydata/pydata-sphinx-theme/blob/main/codecov.yml

drammock commented 2 months ago

for me at least, seeing coverage info in the PR comment tells me whether or not I should pester the contributor to add/modify a test (or add one myself). So it's useful.

Carreau commented 2 months ago

I can look into not having a comment but having a status check that is red only of coverage decreases. Maybe that would lower the notification noise, but without removing the notification of needing more coverage ?

trallard commented 2 months ago

I am +1 with @drammock having coverage info is useful on PRs.

I am not sure if one of the reasons why we do not have codecov PR messages right now has to do with the flakiness we are experiencing, but also I am not too worried about this PR adding too much noise as is.

12rambau commented 2 months ago

I just wanted to point out the current status, if you are fine receiving notification I'm fine as well (I never silence them myself)

trallard commented 2 months ago

Since we all seem to be +1 on trying this codec implementation, the PR has been reviewed. I suggest we merge, see how this goes and make changes/tweaks as needed.