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.

Sign up for GitHub

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

Jump to bottom

Allow per-file minimum coverage #304

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

Allow per-file minimum coverage #304

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b42538
Allow per-file minimum coverage
vereis Mar 15, 2023
3347b78
Minor refactors and unit tests for minimum coverage
vereis Mar 21, 2023
86f0e99
Expose minimum file coverage as a seperate configuration option
vereis Apr 19, 2023
605cca9
Update README.md
vereis Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Owner

Choose a reason for hiding this comment

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

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
    }
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Of course!

How do you feel about letting these strings also be globs? So that we could support "lib/my_app/important_context/*"?

Copy link
Owner

@parroty parroty Mar 17, 2023 •
edited

Choose a reason for hiding this comment

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

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).

Copy link
Author

@vereis vereis Mar 21, 2023 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry being late to respond. Yeah, minimum_coverage_exceptions type of the exception would make more sense.

Copy link
Owner

Choose a reason for hiding this comment

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

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?)

Copy link

Choose a reason for hiding this comment

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

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 issues means? (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)

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

@parroty I've also made a change to the README.md now if you'd like to take a look 🙏

- `html_filter_full_covered`
- A boolean, when `true` files with 100% coverage are not shown in the HTML report. Default to `false`.

Expand Down
36 changes: 31 additions & 5 deletions lib/excoveralls/stats.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,18 +226,44 @@ defmodule ExCoveralls.Stats do
Exit the process with a status of 1 if coverage is below the minimum.
"""
def ensure_minimum_coverage(stats) do
coverage_options = ExCoveralls.Settings.get_coverage_options
coverage_options = ExCoveralls.Settings.get_coverage_options()
minimum_coverage = coverage_options["minimum_coverage"] || 0
if minimum_coverage > 0, do: check_coverage_threshold(stats, minimum_coverage)

check_coverage_threshold(stats, minimum_coverage)
end

defp check_coverage_threshold(stats, minimum_coverages) when is_map(minimum_coverages) do
parroty marked this conversation as resolved.
Show resolved Hide resolved
file_results = stats |> source() |> Map.get(:files, []) |> Map.new(&{&1.filename, &1})

messages =
for {file, minimum_coverage} <- minimum_coverages,
%Source{coverage: file_coverage, filename: filename} = file_results[file],
minimum_coverage > 0 && file_coverage < minimum_coverage do
"FAILED: Expected minimum coverage of #{minimum_coverage}% for `#{filename}`, got #{file_coverage}%."
end

unless Enum.empty?(messages) do
IO.puts(IO.ANSI.format([:red, :bright, Enum.join(messages, "\n\n")]))
exit({:shutdown, 1})
end

:ok
end

defp check_coverage_threshold(stats, minimum_coverage) do
defp check_coverage_threshold(stats, minimum_coverage) when minimum_coverage > 0 do
result = source(stats)

if result.coverage < minimum_coverage do
message = "FAILED: Expected minimum coverage of #{minimum_coverage}%, got #{result.coverage}%."
IO.puts IO.ANSI.format([:red, :bright, message])
message =
"FAILED: Expected minimum coverage of #{minimum_coverage}%, got #{result.coverage}%."

IO.puts(IO.ANSI.format([:red, :bright, message]))
exit({:shutdown, 1})
end
end

defp check_coverage_threshold(_stats, _minimum_coverage) do
:ok
end

end