Allow per-file minimum coverage #304
Conversation
|
Ended up making #305 anyway |
vereis
force-pushed
the
master
branch
2 times, most recently
from
c8e099a to
2aa39f9
Compare
README.md
Outdated
| @@ -427,6 +427,7 @@ to `false`: | |||
| - A custom path for html reports. This defaults to the htmlcov report in the excoveralls lib. | |||
| - `minimum_coverage` | |||
| - When set to a number greater than 0, this setting causes the `mix coveralls` and `mix coveralls.html` tasks to exit with a status code of 1 if test coverage falls below the specified threshold (defaults to 0). This is useful to interrupt CI pipelines with strict code coverage rules. Should be expressed as a number between 0 and 100 signifying the minimum percentage of lines covered. | |||
| - Can also be set to a nested object of filepath and minimum coverage key-value pairs. If this is the case, then `mix coveralls` and `mix coveralls.html` tasks will exit with a status code of 1 if test coverage falls below the specified threshold for any of the configured files. | |||
There was a problem hiding this comment.
How do you think about adding example json definition in addition to this description? Maybe something like,
Example configuration file with nested minimum_coverage option:
{
"coverage_options": {
"minimum_coverage": {
"lib/my_app/example.ex": 100
}
}
}
There was a problem hiding this comment.
Of course!
How do you feel about letting these strings also be globs? So that we could support "lib/my_app/important_context/*"?
There was a problem hiding this comment.
I see. Maybe, listing individual file might be too tedious, but supporting glob can become complex (especially when a certain file matches with multiple conditions).
One option could be, keep the original default value (global), and make this per-file value as additional exception parameter (for allowing certain files which cannot reach the globally expected coverage).
There was a problem hiding this comment.
I see. Maybe, listing individual file might be too tedious, but supporting glob can become complex (especially when a certain file matches with multiple conditions).
Hmm yeah you're correct -- that is a very obvious edge case I didn't think about! Maybe not worth doing immediately then thanks 👍
One option could be, keep the original default value (global), and make this per-file value as additional exception parameter (for allowing certain files which cannot reach the globally expected coverage).
So instead of:
{
"coverage_options": {
"minimum_coverage": {
"some_file.ex": 100
}
}
}We could do:
{
"coverage_options": {
"minimum_coverage": 75,
"minimum_coverage_exceptions": {
"some_file.ex": 100
}
}
}Is this what you were suggesting?
There was a problem hiding this comment.
Sorry being late to respond. Yeah, minimum_coverage_exceptions type of the exception would make more sense.
There was a problem hiding this comment.
Thank you for the comment. I think I was misunderstanding the intention of this change (and also original #130 issue). As you pointed out, having both values minimum_coverage_exceptions and minimum_file_coverage may not make sense.
Hmm, I am getting less confident about how to define more granular threshold values (I just wondered listing out individual files would be tedious if there're many files).
Will it have the same issues as the previous proposal using globs?
Is it possible for you to elaborate what same issues means? (or do you have insights around how the parameters should be?)
There was a problem hiding this comment.
Hmm, I am getting less confident about how to define more granular threshold values (I just wondered listing out individual files would be tedious if there're many files).
To be honest, I would just keep it simple and add a global threshold definition per file and keep having a more granular configuration out of the scope for now. If there are files that don't need to be tested, most probably they are added to the skip_files configuration so having a general threshold might make sense at first.
Will it have the same issues as the previous proposal using globs?
Is it possible for you to elaborate what
same issuesmeans? (or do you have insights around how the parameters should be?)
Nevermind, my question was related to your previous comment:
(...) but supporting glob can become complex (especially when a certain file matches with multiple conditions)
There was a problem hiding this comment.
If there are files that don't need to be tested, most probably they are added to the skip_files configuration
Thank you, this part makes sense (relatively simple one, though it might not be perfect).
Having granular control might resolve certain scenario, but I am getting less confident whether it can kept as simple, hmm.
There was a problem hiding this comment.
Agreed; I'm totally onboard with keeping it simple and adding thresholds per file as an additional thing; ignoring globbing and other more granular considerations for now.
I'll update the PR to the following:
{
"coverage_options": {
"minimum_coverage": 75,
"minimum_file_coverage": {
"some_file.ex": 100
}
}
}Perhaps if we go live with something like this, we can revisit whether or not we need more granular control in the future?
There was a problem hiding this comment.
@parroty I've also made a change to the README.md now if you'd like to take a look 🙏
|
@parroty ahoy! Any remaining thoughts on this PR? Happy to keep iterating on it if you think there is anything missing? |
Ahoy!!
This PR allows you to have per-file minimum coverage when you have coverage options that looks like:
{ "coverage_options": { "minimum_coverage": 80, "minimum_file_coverage": { "lib/my_app/example.ex": 100 } } }If any files don't meet the configured threshold, then you'll get an error as follows:
This should also close #130
WDYT @parroty?