[ex_unit] Omit excluded tests from test count in CLI summary

[kieraneglin]

Discussed here: https://groups.google.com/g/elixir-lang-core/c/xh-hMsDi7c4

This PR omits the count of excluded tests from the overall test count in the CLI summary report

Example

Before: 27 tests, 3 failures, 14 excluded
After: 13 tests, 3 failures, 14 excluded

[ericmj]

Is it confusing that the failures are a subset of the number of tests but the excluded or not?

If the goal is to quickly figure out the number of passing tests I think printing that directly would be more hopeful:

27 tests, 10 passing, 3 failures, 14 excluded

[josevalim]

I am not sure that tests that have been excluded actually matter on the overall results. It is good to know that something was excluded but otherwise not relevant? So we could show that information between parens.

We could also show how many passed instead of how many tests ran (and you could always compute the sum if you need it for some reason).

[kieraneglin]

I'm open to whatever approach is preferred! Personally, I haven't seen much value in knowing the overall count of tests (regardless of how many were excluded) but every use case is different.

Just let me know what the desired output is and I'll update the PR (:

[josevalim]

Now that I think about it, it probably makes sense to not have "excluded" as part of tests because they have been excluded after all.

[kieraneglin]

I see what you mean and I do recognize that you're told when you're only testing a certain tag/line number, but for me it's still a small QOL improvement to indicate that not all tests were run in the final summary.

My preference is still 13 tests, 3 failures, 14 excluded. I think this contains all relevant data at a glance. I'm not a fan of 27 tests, 10 passing, 3 failures, 14 excluded but I can't articulate why - maybe it's because indicating how many pass could be considered redundant?

[josevalim]

I will go ahead and merge this now that v1.18 is out. We will have 6 months to give this a try and feel how it works in practice.

[kieraneglin]

@josevalim refactored!

Part of this was needing to add a default to test_counter of %{test: 0}. Without this, runs that had all tests excluded would have the output of 0 failures, 2 excluded instead of the previous output of 0 tests, 0 failures, 2 excluded. With this change, the side effect is now that test runs that are 100% doctests indicate that there are 0 tests now.

Do you have a preference for approach?

[josevalim]

So instead of adding tests at the beginning, we should check if the map is empty at the end, and then add tests with a value of zero. Otherwise if I only run doctests, it will say 234 doctests, 0 tests.

[josevalim]

Sorry, I have one last nitpick. In this large list, we have excluded, invalid, and skipped, and it is hard to know which ones are counted as tests. I think we should either move "excluded" as the last one or perhaps show it in parens, such as "1 skipped (1 excluded)". WDYT?

[kieraneglin]

Don't be sorry! I appreciate the patience (:

I tried both locally and I think moving it to the last of the list reads the best. I'll push that up shortly and see what you think!

[josevalim]

💚 💙 💜 💛 ❤️