Skip to content

Conversation

tobiasmcnulty
Copy link
Member

Ensures test coverage does not fall below 80% (currently 81%) via make ci. This limit can be increased as test coverage is added.

@ulgens
Copy link
Member

ulgens commented Sep 16, 2025

My understanding is this was an easy & quick & dirty alternative for failing Coveralls integration. Now that Coveralls is working again, I don't think this is needed. We can, of course, merge it but I don't think the result will be meaningful.
Coverage percentage, as a single numeric value, is as misleading as the number of lines of code. It doesn't give information about the history or context. I'm okay with us having 75% test coverage, or even 50%, but it's more important to understand an individual PR's effect on that value - which is one of the major benefits of tools like Coveralls.

@tobiasmcnulty
Copy link
Member Author

tobiasmcnulty commented Sep 18, 2025

@ulgens To close the loop on this one, do you object to enforcing no decreases in test coverage via CI? Or do your comments and downvote stem from preferring to keep Coveralls or a different tool? As mentioned here, this was the solution preferred by the Website WG during its last meeting, not an "easy & quick & dirty alternative" when Coveralls went down. This may go without saying, but I also invite you to propose some positive changes with the coverage solution you would like to see. I don't think these two ideas are mutually exclusive. 🙂

Copy link
Member

@adamzap adamzap left a comment

Choose a reason for hiding this comment

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

👍 This looks good to me since I hope any new code would include tests.

@ulgens
Copy link
Member

ulgens commented Sep 18, 2025

@tobiasmcnulty

enforcing no decreases in test coverage via CI

I don't agree that this change does what you describe here.

Regardless of the coveralls situation, I do think that this is not a good metric to follow up and enforce. As I mentioned earlier, the thing that provides value to the development process is the understanding of what is covered and how it changes, rather than an arbitrary numeric value about how much is covered.

As a general take on the topic, I don't think this discussion is going healthy. There was a weird push and rush to remove the integration without proper reasoning, and things were being framed as "everyone wants to remove it!" when it's not the case. Regardless of the final result, I believe we can and should have more respectful discussions instead of acting like one party has the complete authority and the other one is wrong by default.

@tobiasmcnulty
Copy link
Member Author

Hi @ulgens @adamzap, I get the sense we should wait to decide on this PR until we can discuss it at the next website working group meeting. How does that sound as a next step?

@adamzap
Copy link
Member

adamzap commented Sep 19, 2025

@tobiasmcnulty Sounds good, thanks!

@tobiasmcnulty tobiasmcnulty changed the title Enforced test coverage vi CI Enforced test coverage via CI Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants