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

Enforce minimum coverage in all test types #305

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vereis
Copy link

@vereis vereis commented Mar 16, 2023

Ahoy again!

This PR implements a new flag to excoveralls: --enforce-minimum-coverage. When this is provided, we always run Stats.ensure_minimum_coverage/1 regardless of what testing type you're running.

For example:

mix coveralls.json --enforce-minimum-coverage
mix coveralls.github --enforce-minimum-coverage
...

I've refactored the existing type implementations that call Stats.ensure_minimum_coverage/1 by default and removed these calls. Instead, when running Mix.Tasks.Coveralls.do_run/2 I check to see if the type requested was either html, local, or cobertuna and if so, I automatically set --enforce-minimum-coverage to retain backwards behavioural compatibility.

Closes #297

@vereis
Copy link
Author

vereis commented Jun 5, 2023

@parroty ahoy! Any thoughts on this PR? Happy to keep iterating on it if you think there is anything missing?

@tielur
Copy link

tielur commented Jun 29, 2023

Following up on this as well. Ran into this at work and didn't realize that coveralls.json didn't support the minimum coverage. Would love to see it work for that

@nathany-copia
Copy link

Likewise for coveralls.github -- kind've expected that to be the default behaviour, to be honest.

@@ -68,37 +68,4 @@ defmodule ExCoveralls.HtmlTest do
assert(size == @file_size)
end

test_with_mock "Exit status code is 1 when actual coverage does not reach the minimum",
Copy link
Owner

Choose a reason for hiding this comment

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

@vereis Very sorry not being responsive. Could help me understand these removed portion of test codes? The intention is the relevant conditions are covered in other test cases? (just wanted to confirm the exiting behaviors can be maintained if the new options is not specified).

Copy link

Choose a reason for hiding this comment

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

@vereis it sounds like we might actually want these removed tests? Possibly adding them to the other coverage types as well?

Copy link
Author

Choose a reason for hiding this comment

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

Hi sorry for the delays... not sure how I missed all these emails.

Yes so these tests were removed as the coverage checking is no longer coupled to a given coverage type.

It would be a good idea to test this at a higher level rather than in each type... but yeah in lieu of that we could add these tests to each type.. that just sounds more painful than necessary haha

Happy to circle back around to this in the near future as I've got some time off 😆

@gtrias
Copy link

gtrias commented Dec 6, 2023

Hey! I'm very interested on these changes. Any plans to merge it anytime soon?

@gitneko
Copy link
Contributor

gitneko commented Jan 6, 2024

Hello! What is the status of this PR? Is there anything blocking this from merging?

@parroty
Copy link
Owner

parroty commented Apr 1, 2024

I'm sorry being not responsive. I appreciate any help resolving conflicts.

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.

don't ignore min_coverage in mix coveralls.json
6 participants