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

Coveralls promoted to green, expand on what coverage reporting *is*. #288

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

samcunliffe
Copy link
Member

@samcunliffe samcunliffe commented Jan 24, 2024

@samcunliffe samcunliffe added the documentation Improvements or additions to documentation label Jan 24, 2024
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I disagree about moving codecov to amber - although it can be a bit flaky sometimes, unless we can recommend a better tool I think it should stay green.

For reference our guidance on amber is:

🟠 We don’t discourage using this, but it may duplicate functionality of a green tool.

@paddyroddy
Copy link
Member

paddyroddy commented Jan 24, 2024

I disagree about moving codecov to amber - although it can be a bit flaky sometimes, unless we can recommend a better tool I think it should stay green.

Without being a nag, we discussed various points in person yesterday. Would have been good if you were able to join these, consider it already "decided".

🫐

@dstansby
Copy link
Member

And FWIW I think codecov should be green above coveralls, because you can use codecov with private repos for free (see their pricing), but can't use coveralls with private repos for free (see their pricing)

@dstansby
Copy link
Member

I disagree about moving codecov to amber - although it can be a bit flaky sometimes, unless we can recommend a better tool I think it should stay green.

Without being a nag, we discussed various points in person yesterday. Would have been good if you were able to join these, consider it already "decided".

This is totally not fair - stuff shouldn't be discussed in person, that discussion not documented, and others not able to challenge or question it afterwards just because they weren't in the room.

@samcunliffe
Copy link
Member Author

samcunliffe commented Jan 24, 2024

Why did we decide it should go 🟠 ? Was it just because slow and a little flakey?

Perhaps it should stay green because it's widely used (by us).

In answer to a question (maybe mine) from yesterday...
We generate coverage reports in the template, but we don't upload anywhere.

@samcunliffe
Copy link
Member Author

And FWIW I think codecov should be green above coveralls, because you can use codecov with private repos for free (see their pricing), but can't use coveralls with private repos for free (see their pricing)

This would also convince me. Not all research software projects are open.

@paddyroddy
Copy link
Member

Why did we decide it should go 🟠 ? Was it just because slow and a little flakey?

I think it was because we were claiming we can only have 🟢 and the tools were considered as good as each other. Personally, I've had fewer issues with coveralls over codecov - in terms of flaky-ness.

@paddyroddy
Copy link
Member

This is totally not fair - stuff shouldn't be discussed in person, that discussion not documented, and others not able to challenge or question it afterwards just because they weren't in the room.

I see your point, but we wouldn't achieve much if literally everything discussed had to be documented. Unless we had someone taking minutes.

@dstansby
Copy link
Member

I see your point, but we wouldn't achieve much if literally everything discussed had to be documented. Unless we had someone taking minutes.

I think in the interests of being an open and collaborative project reasons for decisions should at least be written down somewhere.

@dstansby
Copy link
Member

I think it was because we were claiming we can only have 🟢 and the tools were considered as good as each other. Personally, I've had fewer issues with coveralls over codecov - in terms of flaky-ness.

I haven't used coveralls in ages, so could believe it's less flaky.

Another point in favour of codecov is that it's more widely used by big OSS scientific Python projects (AFAIK)?

@samcunliffe
Copy link
Member Author

I'm convinced to put these comments into the page. I've not got a strong feeling about 🟢 or 🟠 .

@matt-graham
Copy link
Collaborator

I'm convinced to put these comments into the page. I've not got a strong feeling about 🟢 or 🟠 .

I also have no opinion on 🟢 versus 🟠 but I strongly agree with @samcunliffe's suggestion to put the details discussed here comparing Codecov / Coveralls in the table / on the page, particularly @dstansby points about being able to use Codecov with private repositories, and that Codecov appears to be more widely used by major Scientific Python packages (from a quick scan of repositories, all of NumPy, Matplotlib and pandas appear to use Codecov).

@matt-graham
Copy link
Collaborator

This is totally not fair - stuff shouldn't be discussed in person, that discussion not documented, and others not able to challenge or question it afterwards just because they weren't in the room.

I think in the interests of being an open and collaborative project reasons for decisions should at least be written down somewhere.

I also think @dstansby points here are very valid and given we've been explicitly discussing how we might encourage getting outside contributors, making sure we have processes which make sure everyone can equally contribute to discussions is important.

@paddyroddy
Copy link
Member

I also think @dstansby points here are very valid and given we've been explicitly discussing how we might encourage getting outside contributors, making sure we have processes which make sure everyone can equally contribute to discussions is important.

I agree, but it is very easy to say without actually doing anything. The only way for that to actually happen is someone to spend the whole time taking minutes, or recording the whole thing. I would not volunteer for either of those roles.

@matt-graham
Copy link
Collaborator

I agree, but it is very easy to say without actually doing anything. The only way for that to actually happen is someone to spend the whole time taking minutes, or recording the whole thing. I would not volunteer for either of those roles.

I don't think we need minutes, we just need to make sure if we have an off-GitHub discussion (be it in-person or on Slack) and this leads to an issue being raised around some decision point about a change to be made we record the key points of the discussion and why the decision was made.

@paddyroddy
Copy link
Member

I don't think we need minutes, we just need to make sure if we have an off-GitHub discussion (be it in-person or on Slack) and this leads to an issue being raised around some decision point about a change to be made we record the key points of the discussion and why the decision was made.

Yes, but to remember those points...

@matt-graham
Copy link
Collaborator

Yes, but to remember those points...

If we are recording the points in the initial issue comment as we create an issue I don't think there should be a problem remembering them.

For example if we're creating issues in a discussion / planning session like we had in the first part of yesterday morning, I think the extra overhead of having someone (probably distinct from the person leading the planning) note down any key points discussed would be minimal. And this would also have the additional benefit of making it easier to understand why decisions were made previously if we subsequently review and decided to change.

@paddyroddy
Copy link
Member

If we are recording the points in the initial issue comment as we create an issue I don't think there should be a problem remembering them.

Sounds like a plan.

@matt-graham
Copy link
Collaborator

Sorry realise this went quite off-topic but I though it was a useful discussion to have. I'll create a new issue to remind us to somehow agree on and document a process around this.

@dstansby
Copy link
Member

dstansby commented Jan 24, 2024

Yeah, to be clear not looking for full discussion notes or anything, just a brief summary of why a decision has been made so it's clear to anyone who wasn't part of the discussion. For this PR, a summary in the body of #254 were what I was expecting.

@samcunliffe samcunliffe requested a review from dstansby January 25, 2024 10:06
@samcunliffe samcunliffe changed the title Codecov demoted to amber, expand on what coverage reporting _is_. Codecov demoted to amber, expand on what coverage reporting *is*. Jan 25, 2024
@samcunliffe
Copy link
Member Author

Still not a strong opinion, but I think two 🟠 is probably an accurate representation of the situation. We don't have a consensus about which is better. And we don't have either implemented in our template.

@dstansby
Copy link
Member

In that case I'd say have two 🟢 ? I think it's better to have at least one green per category, otherwise we run the risk of people not choosing anything if there's just 🟠 ?

@dstansby
Copy link
Member

But also, as I've said above codecov 1) can be used for private repos 2) is more widely used across scipy ecosystem, and I still think that's enough to edge out coveralls unless anyone has any other arguments for coveralls above codecov?

@samcunliffe
Copy link
Member Author

But also, as I've said above codecov 1) can be used for private repos 2) is more widely used across scipy ecosystem, and I still think that's enough to edge out coveralls unless anyone has any other arguments for coveralls above codecov?

Should we go all in and add it to the template's CI? (+ a badge, obvs.)

@paddyroddy
Copy link
Member

But also, as I've said above codecov 1) can be used for private repos 2) is more widely used across scipy ecosystem, and I still think that's enough to edge out coveralls unless anyone has any other arguments for coveralls above codecov?

Flakiness, from my own experience

@paddyroddy
Copy link
Member

Should we go all in and add it to the template's CI? (+ a badge, obvs.)

That would be good, but it's another thing they would need to set up #205

@dstansby
Copy link
Member

Flakiness, from my own experience

Ah fair enough, sorry for forgetting/not reading properly. I'm still pro both as green? Won't block this PR if others think both amber though

@dstansby
Copy link
Member

dstansby commented Feb 6, 2024

I'd be happy to merge this if the amber > green change is made

Co-authored-by: David Stansby <[email protected]>
@samcunliffe samcunliffe enabled auto-merge (squash) February 6, 2024 10:57
@samcunliffe samcunliffe disabled auto-merge February 6, 2024 10:57
@samcunliffe samcunliffe enabled auto-merge (squash) February 6, 2024 10:58
@samcunliffe samcunliffe changed the title Codecov demoted to amber, expand on what coverage reporting *is*. Coveralls promoted to green, expand on what coverage reporting *is*. Feb 6, 2024
@samcunliffe
Copy link
Member Author

I've updated the PR title and commit message to reflect our decision.

@samcunliffe samcunliffe merged commit fe5d2eb into main Feb 6, 2024
15 checks passed
@samcunliffe samcunliffe deleted the sc/254-codecov-vs-coveralls-plus-expand-the-words branch February 6, 2024 11:05
@paddyroddy
Copy link
Member

@samcunliffe
Copy link
Member Author

For those who can access it, we did a very lightweight solution over in INFORMus.
https://github.com/inform-us/INFORMus/blob/dev/.github/workflows/api.yml#L49-L75

And it's working well so far.

Using

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codecov should go amber, and expand on the descriptions in the table.
4 participants