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

Quality of Life: Make codecov less meticulous #1966

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Conversation

Corvince
Copy link
Contributor

If you currently look at the open PRs there is not a single PR that passes all our checks. For most PRs I found that at least Codecov is failing. I think the current settings are a bit too agressive. For example take #1964 . In this This PR restructures some code. Everything is still covered, but because its a single line of code more the overall % of covered lines decreased by 0.07% and the check failed.

In this PR I set the target value for the project to 80% with a 1% threshold. We are currently just above 79% so if this decreases significantly the checks will fail.

I also set it to ignore experimental folders (jupyterviz)

And finally I disabled comments on PR. This might be debatable, but I found myself just ignoring those comments after having seen too many. For me its very similar for checks. I don't notice failed checks sometimes because I am so used to the red cross on Mesa. I hope this will get better with this PR. Additionally it would be great if we could setup pre-commit autoupdating PRs, but this would be in contrast to the ideas proposed in #1963

@Corvince
Copy link
Contributor Author

In this PR I set the target value for the project to 80% with a 1% threshold. We are currently just above 79% so if this decreases significantly the checks will fail.

Maybe I should change this. With experimental ignored we are actually at 90%

@quaquel
Copy link
Contributor

quaquel commented Jan 15, 2024

I would like you to include experimental stuff in the code coverage to encourage proper quality assurance. I also find it often helpful to understand better what a given piece of code is trying to achieve. Beyond that, I fully agree with your motivation for making the threshold less strict.

I am all in favor of using pre-commit for stuff like a ruff. It is a simple quality-of-life improvement that makes committing and creating PRs just that bit easier. I am still gathering my thoughts on #1963 and will reply there later this week.

@EwoutH EwoutH added the ci Release notes label label Jan 16, 2024
Copy link
Contributor

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Agreed on the sentiment and direction, indifferent on the exact implementation.

The experimental space is a bit of a free for all, so I think we don't need such strict standards, but experimental features outside the experimental space are still required coverage. So I think this PR is good as is, but I also wouldn't object against it including the experimental space.

@Corvince
Copy link
Contributor Author

I would like you to include experimental stuff in the code coverage to encourage proper quality assurance. I also find it often helpful to understand better what a given piece of code is trying to achieve. Beyond that, I fully agree with your motivation for making the threshold less strict.

Thought about this the last days. I think this is again a matter of how you define experimental. Currently only the JupyterViz is in the ignored folder. I decided to keep this PR for two reasons

  • The API there is very likely to change so writing tests at this point is very tedious
  • Writing frontend unit tests isn't super useful. Yes, you can test some things, but for most things End-to-End tests make much more sense - but they are unrelated to code coverage.

@Corvince Corvince merged commit 0bd481d into main Jan 18, 2024
12 of 13 checks passed
@quaquel
Copy link
Contributor

quaquel commented Jan 19, 2024

Fair enough.

@rht rht deleted the calm-down-codecov branch January 26, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants