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

Display simple coverage report to console #7664

Merged
merged 3 commits into from May 11, 2018

Conversation

Projects
None yet
10 participants
@hauleth
Contributor

hauleth commented May 9, 2018

This should reduce need for packages like excoveralls in some use cases
when all what is needed is to have total coverage percentage in any
form available for CI to provide coverage badge.

Reference: https://docs.gitlab.com/ee/user/project/pipelines/settings.html#test-coverage-parsing

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

Can you please include an example of the report? Does any other client besides gitlab parses coverage reports like that?

@@ -2,6 +2,8 @@ defmodule Mix.Tasks.Test do
defmodule Cover do
@moduledoc false
@threshold 90

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

This would need to be made configurable.

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

This is configurable via threshold value in test_coverage configuration option. I haven't documented it yet thought.

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

Beautiful. Maybe we should call this @default_threshold then?

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

No problem

format(percentage, 9),
"%",
:reset,
" | #{name}"

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

You need to inspect the module name so it shows only as "MyApp.Module" and not "Elixir.MyApp.Module". But you need to be careful to not inspect the string "Total".

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

That is the reason why I haven't done it, however I can make it happen.

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

Also please check the build and add some tests for those reports. Those changes can wait though as we are not sure if we should merge this.

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

Let's also provide an option that skips the summary if desired.

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

One last thing: gitlab default's regex expects the word "covered" in there. Do we need to add something similar? Wouldn't just printing percentages be a bit ambiguous? What regex are you using currently?

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

@josevalim about comments, from top to bottom:

  • I am not aware of any that works in such way, however I only skimmed through available options, so maybe there are ones that I am no aware of.
  • I need to update tests.
  • I was thinking about something other: sort solutions based on their coverage percentage and then display only "bottom N" of options. Then such option could be set to false to disable CLI coverage report at all. What do you think? Or rather add additional option that would disable output and separate that would print sorted results?
  • Default regex yes, but this can be parsed using \d+(\.\d+)?%\s*\|\s*Total.
@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

Example result:

Cover results
Percentage | Module
-----------|--------------------------
      0.0% | Elixir.Cover
      0.0% | Elixir.ImagerWeb.Instrumenter
     50.0% | Elixir.Imager.Application
      0.0% | Elixir.Imager.Convert
      0.0% | Elixir.ImagerWeb.ChannelCase
      0.0% | Elixir.ImagerWeb.UserSocket
      0.0% | Elixir.ImagerWeb.WebhookController
     6.45% | Elixir.ImagerWeb.Router.Helpers
    15.79% | Elixir.ImagerWeb.ErrorView
    45.45% | Elixir.ImagerWeb.ConnCase
     5.26% | Elixir.ImagerWeb.Endpoint
    23.81% | Elixir.Imager.Stats
     5.71% | Elixir.Imager
      0.0% | Elixir.ImagerWeb.Router
      0.0% | Elixir.ImagerWeb
-----------|--------------------------
     7.88% | Total
@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

I was thinking about something other: sort solutions based on their coverage percentage and then display only "bottom N" of options. Then such option could be set to false to disable CLI coverage report at all.

To me it is OK to sort by percentage but is there a reason to allow a cut off?

@michalmuskala

This comment has been minimized.

Member

michalmuskala commented May 9, 2018

Should we print the module names with elixir syntax?

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

@michalmuskala @josevalim how about such output:

Coverage results:

Percentage | Module
-----------|--------------------------
      0.0% | Imager.Convert
      0.0% | Cover
      0.0% | ImagerWeb.Instrumenter
      0.0% | ImagerWeb
      0.0% | ImagerWeb.ChannelCase
      0.0% | ImagerWeb.UserSocket
      0.0% | ImagerWeb.WebhookController
      0.0% | ImagerWeb.Router
     5.26% | ImagerWeb.Endpoint
     5.71% | Imager
     6.45% | ImagerWeb.Router.Helpers
    15.79% | ImagerWeb.ErrorView
    23.81% | Imager.Stats
    45.45% | ImagerWeb.ConnCase
     50.0% | Imager.Application
-----------|--------------------------
     7.73% | Total

Generating coverage results in 'cover' directory
@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

I am thinking about changing the last line to Generated HTML coverage results in 'cover' directory.

@ancient-alchemist

This comment has been minimized.

ancient-alchemist commented May 9, 2018

@hauleth could you show us what an example of the HTML page would look like?

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

@webdeb HTML result is left as is. For more information check Erlang's cover module.

@ancient-alchemist

This comment has been minimized.

ancient-alchemist commented May 9, 2018

You coverage logs are fine. Not sure if would be possible to have some additional info like lines, relevant or missed but I think that would be great to have. Just an opinion though and thanks for this pull request.
i.e.

COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/module_name.ex                              9        0        0
Enum.each(:cover.modules(), fn mod ->
{:ok, _} = :cover.analyse_to_file(mod, '#{output}/#{mod}.html', [:html])
end)
Mix.shell().info("\nCover results")

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

We can keep this line as it was before because it will also work if folks choose to skip the summary.

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

How about keeping this line and adding additional output after HTML result was generated?

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

Yes this is perfectly possible, however I want to keep output format as simple as possible without any additional clutter that can be provided by external tools.

@josevalim @michalmuskala any opinions about output format?

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

@hauleth I like your suggested output last. I would just keep the first line of the output and the last line the same as it is today. Let's minimiza changes and make sure the summary is optional.

@webofbits let's start simple for now we can always add more information later.

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

@josevalim @webofbits

Generating cover results ...

Coverage results:

Percentage | Module
-----------|--------------------------
      0.0% | Imager.Convert
      0.0% | ImagerWeb.ChannelCase
      0.0% | ImagerWeb.UserSocket
      0.0% | ImagerWeb
      0.0% | ImagerWeb.WebhookController
      0.0% | Cover
      0.0% | ImagerWeb.Instrumenter
      0.0% | ImagerWeb.Router
     5.26% | ImagerWeb.Endpoint
     5.71% | Imager
     6.45% | ImagerWeb.Router.Helpers
    15.79% | ImagerWeb.ErrorView
    23.81% | Imager.Stats
    45.45% | ImagerWeb.ConnCase
     50.0% | Imager.Application
-----------|--------------------------
     7.65% | Total

Generated HTML coverage results in 'cover' directory
@ancient-alchemist

This comment has been minimized.

ancient-alchemist commented May 9, 2018

Sounds good to me 👍

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

@hauleth "Coverage results:" is unnecessary. From the previous line and context we know they are results.

@default_formatters [:console, :html]
@default_threshold 90
@moduledoc """

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

Let's please keep this module still private. I don't think we should expose it. Let's just have a section in the mix test docs that says the configurations for the default cover handling.

@@ -14,17 +30,91 @@ defmodule Mix.Tasks.Test do
Mix.raise("Failed to cover compile directory: " <> compile_path)
end
output = opts[:output]
formatters = Keyword.get(opts, :formatters, @default_formatters)

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

:console is not really a formatter because it doesn't print it for every module. It is just a summary. So I would rather have a summary: true | false option.

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

According to Erlang cover:analyse/2 docs it will print data for all cover compiled modules. However I can misunderstanding your comment.

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

Do you mean that we have also added an option to print all of the results to the terminal? If so, that's a different feature than the one we were debating. Let's focus on the summary feature first. Other additions should be considered separately.

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

That is the exact result of what this PR produces #7664 (comment). It display all modules and their respective coverage percentage. You meant something else? Like we should display only overall coverage like:

Generating cover results ...

Coverage: 72.32%

Generated HTML coverage results in 'cover' directory

?

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

Ok. Sorry for the misunderstanding. The current output is fine. However, I wouldn't call it a formatter because what we print to the console and what we write to html files are two very distinct things. My proposal is to remove the idea of formatters and always generate HTML as before. Let's call the new reporting that you have added as part of this PR "Summary" and it can be disabled via the summary: true | false option.

This comment has been minimized.

@hauleth

hauleth May 9, 2018

Contributor

Actually from viewpoint of the :cover module these two are reports but on different level of granularity: per-line or per-module (at least up to my understanding). However I have updated naming accordingly to your suggestion.

fn ->
{:result, ok, _fail} = :cover.analyse(:coverage, :module)
Mix.shell().info("\nGenerating cover results ...")

This comment has been minimized.

@josevalim

josevalim May 9, 2018

Member

Let's print this before we call analyze, otherwise the user may wait for something and they don't know what it is.

@whatyouhide

This comment has been minimized.

Member

whatyouhide commented May 9, 2018

I might be missing something obvious, but why have the percentage first (left) and the module name after (right) instead of the other way around?

@OvermindDL1

This comment has been minimized.

OvermindDL1 commented May 9, 2018

@whatyouhide The percentage is pretty bounded in how many characters it can take up, where a module name is not (in comparison), thus I think it is easier to keep both readable instead of the module name occasionally pushing the percentages off the side with long module names. :-)

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

Exactly what @OvermindDL1 said. It is easier to display it in that way instead of looping through module names twice and building list based on the longest module name.

@whatyouhide

This comment has been minimized.

Member

whatyouhide commented May 9, 2018

Yeah I get that it's easier to build but is that an argument for the user of this or for the implementer? I think the difference in implementation complexity is so small that it justifies having what IMO feels more natural to the user, that is, modules first and percentage after. Navigating the list of modules twice shouldn't be a performance issue since these lists are supposed to be on the "very small" side.

@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

I think the most important information here is the percentage and given that we will be ordering by %, I vote to keep it first too. The goal here is not to list modules.

@OvermindDL1

This comment has been minimized.

OvermindDL1 commented May 9, 2018

As the user I find the percentages first significantly more preferable as well as I want to see what they are before I care about module names.

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

Only open question is whether sort in increasing or decreasing order. I do not think it should be configureable, but there should be defined order.

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

@whatyouhide another reason why it is better to leave percentage in front is that it is much easier to crop lines at the end in the terminal window rather than crop them in the middle or at the beginning to have visible results in case of very long module names.

@OvermindDL1

This comment has been minimized.

OvermindDL1 commented May 9, 2018

Only open question is whether sort in increasing or decreasing order. I do not think it should be configureable, but there should be defined order.

Honestly I'd prefer decreasing so that the 0%'ers are at the bottom as that is the most immediate thing I will see when running commands on the terminal.

@AndrewDryga

This comment has been minimized.

Contributor

AndrewDryga commented May 9, 2018

@OvermindDL1 I think it's much better to see them in increasing (ascending) order. I feel like it's better to have issues to be on top of the list (first things should be addressed first), not on the bottom of it.

@OvermindDL1

This comment has been minimized.

OvermindDL1 commented May 9, 2018

@AndrewDryga Except with them being at the top then the issues have scrolled off the screen and I can't see them without doing <Ctrl>+<B>, <[> and then scroll around with the movement keys (up/down/left/right/pgup/pgdn/etc...), that's a pain to see the information that I want to see most, where the last printed information is the information that is immediately viewable and accessible.

### Default coverage module options
* `:threshold` - summary output colours resulting percentage red/green
depending on weather given value is below or over given threshold,

This comment has been minimized.

@fertapric

fertapric May 9, 2018

Member

s/weather/whether/

@@ -162,6 +232,14 @@ defmodule Mix.Tasks.Test do
It must return either `nil` or an anonymous function of zero arity that will
be run after the test suite is done.
### Default coverage module options
* `:threshold` - summary output colours resulting percentage red/green

This comment has been minimized.

@fertapric

fertapric May 9, 2018

Member

The :threshold option seems to only make sense when the :summary option is enabled. In order to reflect that hierarchy, what do you think about?

test_coverage: [summary: true] # Enables the summary with the default threshold
test_coverage: [summary: [threshold: 70]] # Enables the summary and sets the threshold to 70

If in the future the summary adds more configuration variables, they won't be mixed with the global coverage options.

@@ -162,6 +232,14 @@ defmodule Mix.Tasks.Test do
It must return either `nil` or an anonymous function of zero arity that will
be run after the test suite is done.
### Default coverage module options

This comment has been minimized.

@fertapric

fertapric May 9, 2018

Member

Should these options be listed in the "Coverage" section above?

Coverage

The :test_coverage configuration accepts the following options:

  • :output - the output for cover results, defaults to "cover"
  • :tool - the coverage tool
  • :summary - whether reporter should output coverage data summary to the console, defaults to true
@josevalim

This comment has been minimized.

Member

josevalim commented May 9, 2018

Let's go with decreasing order (100%) comes first. Let's also adopt @fertapric's suggestion.

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 9, 2018

Everything is updated to what @fertapric suggested.

@lexmag

This comment has been minimized.

Member

lexmag commented May 10, 2018

Does it make sense to drop % sign given that we explicitly state it is "Percentage"?

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 10, 2018

@lexmag when the module list is long then header can be outside of the screen view. I think that this is small price (1 character) for better readability. Especially if we want to add other metrics in the future, like calls per line ratio or calls per function ratio.

@@ -149,8 +225,14 @@ defmodule Mix.Tasks.Test do
The `:test_coverage` configuration accepts the following options:
* `:output` - the output for cover results, defaults to `"cover"`
* `:tool` - the coverage tool
* `:output` - the output for cover results, defaults to `"cover"`

This comment has been minimized.

@lexmag

lexmag May 10, 2018

Member

We don't align - anymore due to noise it generates in commits.

end
defp colour(percentage, true), do: colour(percentage, @default_threshold)

This comment has been minimized.

@lexmag

lexmag May 10, 2018

Member

Except "behaviour" we should use American spelling.

Mix.shell().info("")
end
defp console(_, _), do: nil

This comment has been minimized.

@lexmag

lexmag May 10, 2018

Member

Maybe match on [false, nil] to make summary: 123 fail?

@lexmag

This comment has been minimized.

Member

lexmag commented May 10, 2018

@hauleth in my opinion this one character does not improve readability (rather brings visual noise).
I think if we want to add other metrics in the future we'll reconsider the current output quite a bit anyway.

@lexmag

This comment has been minimized.

Member

lexmag commented May 10, 2018

@hauleth just checked the common agreement on this with the rest of the folks in the team: let's keep the sign. 👍

|> Float.round(2)
|> Float.to_string()
|> String.pad_leading(len)
end

This comment has been minimized.

@michalmuskala

michalmuskala May 10, 2018

Member

Could we use :io_lib.format("~#{len}.2f", [num]) for formatting here? This will always give us 2 fields of precision and nicely align the results.

This comment has been minimized.

@hauleth

hauleth May 10, 2018

Contributor

I will change integer version as well.

@hauleth

This comment has been minimized.

Contributor

hauleth commented May 10, 2018

@lexmag we could add space between percentage sign and number if you find this easier to read.

Display simple coverage report to console
This should reduce need for packages like excoveralls in some use cases
when all what is needed is to have total coverage percentage in any
form available for CI to provide coverage badge.

Reference: https://docs.gitlab.com/ee/user/project/pipelines/settings.html#test-coverage-parsing
defp percentage({0, 0}), do: 100
defp percentage({cov, not_cov}), do: cov / (cov + not_cov) * 100
defp format(num, len) when is_integer(num),

This comment has been minimized.

@eksperimental

eksperimental May 10, 2018

Member

Generally we use full name in arguments, so it reads better number and length

This comment has been minimized.

@hauleth

hauleth May 10, 2018

Contributor

@eksperimental my problem with length is that vim-elixir colours it as a keyword which I find distracting. Also I find len and num popular enough that I didn't even thought about using full name.

This comment has been minimized.

@OvermindDL1

OvermindDL1 May 10, 2018

my problem with length is that vim-elixir colours it as a keyword which I find distracting

Wait what why? It's not a keyword! o.O

This comment has been minimized.

@hauleth
end
defp percentage({0, 0}), do: 100
defp percentage({cov, not_cov}), do: cov / (cov + not_cov) * 100

This comment has been minimized.

@eksperimental

eksperimental May 10, 2018

Member

It reads better full name arg. covered and not_covered

|> Stream.each(&display(&1, opts))
|> Enum.reduce({0, 0}, fn {_, {covered, not_covered}},
{total_covered, total_not_covered} ->
{total_cov + cov, total_not_cov + not_cov}

This comment has been minimized.

@michalmuskala

michalmuskala May 10, 2018

Member

This line should be:

{total_covered + covered, total_not_covered + not_covered}
@michalmuskala

After the line causing test failures is fixed, I think this will be good to go.

@josevalim

This comment has been minimized.

Member

josevalim commented May 11, 2018

I have pushed a commit that fixes the build, so as soon as it passes we are good to go!

@josevalim josevalim merged commit 73201ca into elixir-lang:master May 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@josevalim

This comment has been minimized.

Member

josevalim commented May 11, 2018

❤️ 💚 💙 💛 💜

@hauleth hauleth deleted the hauleth:feat/display-coverage-results-to-console branch May 11, 2018

eksperimental added a commit to eksperimental/elixir that referenced this pull request May 12, 2018

lexmag added a commit that referenced this pull request May 12, 2018

Spelling corrections (#7679)
* Minor grammar correction in Exception.blame/3

* Correct British spelling colouring -> coloring

Introduced in #7664.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment