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

Make automatic codecov report less noisy #35056

Merged
merged 4 commits into from
Mar 26, 2023

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Feb 10, 2023

Fixes #35016. We turn off automatic codecov comment.

After this PR, someone needs to implement a github action that sends code coverage information as a quiet comment, like the current built-documentation comment.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Feb 10, 2023

The location was determined according to https://docs.codecov.com/docs/codecov-yaml#can-i-name-the-file-codecovyml

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Feb 10, 2023

Thanks for looking into this.

Since this is currently the only check we have on github that newly introduced code is covered by checks, it should be clearly visible from the PR. So simply disabling is not an option in my opinion. At least a GH check should be added as well.

You can also test this in your fork first (simply install the codecov app in your fork: https://github.com/marketplace/codecov)

@kwankyu
Copy link
Collaborator Author

kwankyu commented Feb 10, 2023

So simply disabling is not an option in my opinion. At least a GH check should be added as well.

I agree. Would you do that, adding a GH check?

@tobiasdiez
Copy link
Contributor

This is something that can be configured as well: https://docs.codecov.com/docs/github-checks

@kwankyu
Copy link
Collaborator Author

kwankyu commented Feb 10, 2023

I misunderstood the GH check as something like checks in "Checks" tab. I don't know whether the codecov's GH check you meant is something developers would like or not.

What I want is to make the code coverage information available to developers in less intrusive way. The issue of #35016 is that the current automatic codecov comments are distracting. Hence I think we should merge this PR now to stop adding codecov comments to issues.

Then someone, perhaps you, make a new PR either to turn on the codecov's GH check or to turn on the automatic codecov comment again by reconfiguring codecov.yml.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I misunderstood the GH check as something like checks in "Checks" tab. I don't know whether the codecov's GH check you meant is something developers would like or not.

So this https://docs.codecov.com/docs/commit-status ?

What I want is to make the code coverage information available to developers in less intrusive way.

As far as I understand it is sage's convention that changes should be covered by tests. The codecov report gives the reviewer a tool to check this convention. I agree that its somewhat annoying to get this comment as someone contributing a PR, but I would value the convenience/time of the reviewer higher.

I'm still of the opinion we should just test it for a bit and disable/change the codecov comment once everyone got used to the new github workflow a bit more.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Feb 10, 2023

I'm still of the opinion we should just test it for a bit and disable/change the codecov comment once everyone got used to the new github workflow a bit more.

We just have different opinions.

I don't plan to do (and I am not capable of doing) anything more in this PR. Others will decide on it.

Whether this PR is merged or not, as I said already, I hope that you make a new PR to provide less intrusively code coverage information to reviewers.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Feb 12, 2023

You can also test this in your fork first (simply install the codecov app in your fork: https://github.com/marketplace/codecov)

I did. It works well, that is, no more codecov report comments.

@codecov-commenter
Copy link

Codecov Report

Base: 88.59% // Head: 88.56% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (bcafbf4) compared to base (05329f6).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35056      +/-   ##
===========================================
- Coverage    88.59%   88.56%   -0.03%     
===========================================
  Files         2140     2140              
  Lines       396998   396998              
===========================================
- Hits        351740   351621     -119     
- Misses       45258    45377     +119     
Impacted Files Coverage Δ
src/sage/algebras/fusion_rings/fusion_ring.py 63.97% <0.00%> (-29.97%) ⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py 50.71% <0.00%> (-3.92%) ⬇️
src/sage/groups/additive_abelian/qmodnz.py 91.48% <0.00%> (-2.13%) ⬇️
src/sage/combinat/root_system/weyl_characters.py 93.26% <0.00%> (-1.39%) ⬇️
src/sage/groups/generic.py 88.34% <0.00%> (-0.68%) ⬇️
src/sage/schemes/elliptic_curves/hom.py 83.33% <0.00%> (-0.62%) ⬇️
src/sage/sets/integer_range.py 91.41% <0.00%> (-0.51%) ⬇️
src/sage/quadratic_forms/binary_qf.py 92.78% <0.00%> (-0.50%) ⬇️
src/sage/graphs/tutte_polynomial.py 93.57% <0.00%> (-0.46%) ⬇️
src/sage/modular/arithgroup/arithgroup_perm.py 92.57% <0.00%> (-0.38%) ⬇️
... and 19 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kwankyu kwankyu changed the title Add codecov.yml to control codecov report Turn off automatic codecov comment Feb 20, 2023
@mezzarobba
Copy link
Member

I don't understand the technical details of the issue nor what the available options are, but for what it's worth, I've almost immediately started ignoring the codecov comments as visual noise. I don't care at all about coverage statistics; the only thing that matters to me is that new code and especially conventions that affect multiple functions/files/... are covered by relevant tests, and I don't think codecov can help much with that.

@tobiasdiez
Copy link
Contributor

I don't think its a good idea to simply turn the comments off, but I agree that replacing them by the less noisy status checks is a good idea. The following should do the job but is untested:

# https://docs.codecov.com/docs/pull-request-comments#disable-comment
comment: false
# https://docs.codecov.com/docs/commit-status
coverage:
  status:
    project:
      default:
        target: auto
        threshold: 0%
        base: auto 
    patch:
      default:
        target: auto
        threshold: 0%
        base: auto

@kwankyu
Copy link
Collaborator Author

kwankyu commented Mar 18, 2023

Thanks. I am testing.

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 8c804c7

@kwankyu
Copy link
Collaborator Author

kwankyu commented Mar 19, 2023

It works perfect!

Screenshot 2023-03-19 at 11 42 39 AM

@kwankyu kwankyu changed the title Turn off automatic codecov comment Make automatic codecov report less noisy Mar 19, 2023
Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Nice, thanks for testing!

@tobiasdiez
Copy link
Contributor

@vbraun could this please be merged relatively quickly? It doesn't touch anything else then the codecov config, so it doesn't need to go through extensive testing.

@vbraun vbraun merged commit ce41bee into sagemath:develop Mar 26, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 26, 2023
@kwankyu kwankyu deleted the fix-codecov-yml branch April 7, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Codecov Report" automatic comment is extravagant
6 participants