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

add codecov token, fixes #2363 #2366

Merged
merged 4 commits into from
Oct 15, 2024
Merged

add codecov token, fixes #2363 #2366

merged 4 commits into from
Oct 15, 2024

Conversation

Corvince
Copy link
Contributor

@Corvince Corvince commented Oct 15, 2024

Lets see if it works now

Closes #2363.

@quaquel
Copy link
Member

quaquel commented Oct 15, 2024

seems to be working again!

@quaquel quaquel self-requested a review October 15, 2024 15:08
@Corvince
Copy link
Contributor Author

Yes! But we still miss the 90% target, so I lowered it temporarily to 80%. This is mostly caused by the deprecated time.py file, which is untested. Were the tests already removed or did they never exist? Anyways it would be unfair to have every PR fail because our project is currently undercovered, thats why i lowered it to 80%.

I also enabled a "fail_ci_if_error" flag, so hopefully we catch errors with codecov earlier next time. Or maybe it will cause too many errors, in which we can later disable it again. Lets see

@quaquel
Copy link
Member

quaquel commented Oct 15, 2024

Yes! But we still miss the 90% target, so I lowered it temporarily to 80%. This is mostly caused by the deprecated time.py file, which is untested. Were the tests already removed or did they never exist? Anyways it would be unfair to have every PR fail because our project is currently undercovered, thats why i lowered it to 80%.

I agree. Not perse for this pr, but could we not exclude time.py somehow?

@Corvince
Copy link
Contributor Author

Not sure how I would feel about this. Even when deprecated, I think its good to highlight gaps in our code coverage. Of course no one is going to write tests for time.py now, but since the 80% or 90% mark is arbitrary anyways I feel like including it is less cheating. But I don't have a strong opinion about that, so if someone else wants to open a PR to exclude it I wont reject it.

@Corvince Corvince merged commit b42e272 into main Oct 15, 2024
12 checks passed
@Corvince Corvince added the bug Release notes label label Oct 15, 2024
@quaquel
Copy link
Member

quaquel commented Oct 15, 2024

There was a test_time.py that I think got removed when we deprecated it. This was unfortunate because although deprecated it is still being used in placed. Today I fixed #2359 which would not have been needed if test_time.py had not been removed. I'll try to remember adding it back in asap.

@EwoutH
Copy link
Member

EwoutH commented Oct 15, 2024

There was a test_time.py that I think got removed when we deprecated it.

Yeah sorry that's a mistake from my side, dully noted for future reference.

Thanks a lot for picking this up so quickly!

@EwoutH EwoutH added ci Release notes label maintenance Release notes label and removed bug Release notes label labels Oct 16, 2024
@EwoutH EwoutH deleted the Corvince-patch-1 branch October 26, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Release notes label maintenance Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codecov upload broken - needs token from someone with admin access
3 participants