Issues and PRs are always welcome!
NFIBrokerage shoots for 100% coverage (via
:excoveralls
), but we always allow
ignoring lines or blocks of code that would either be impractical to test
or would consume too much of your time to do so (your time is valuable :).
In those cases, we strongly prefer using comment-style ignores, like so:
# N.B.: to get coverage on this block, we'd have to brutally kill the HTTP
# server, which we need to be running for other tests to pass
# coveralls-ignore-start
def handle_event(%ServerDisconnect{}, state) do
{:stop, {:shutdown, :disconnected}, state}
end
# coveralls-ignore-stop
With a helpful comment explaining why this case is too tough to test.
Please do not use coveralls' skip_files feature
Coveralls also allows ignoring by adding file patterns to the coveralls.json
file:
{
"coverage_options": {
"minimum_coverage": 100,
"treat_no_relevant_lines_as_covered" : true
},
"skip_files": [
"^test",
"^deps",
"^lib/my_app/my_file.ex"
]
}
But in our experience, this only leads to confusion over why a file is not
being reported in the coverage detail. If an entire module needs to be ignored,
we prefer wrapping the entire defmodule/2
call in ignore comments.
Why we shoot for 100% coverage
Shooting for 100% code coverage is a bit controversial. Proponents sometimes claim: "if the line isn't covered why do you need it?" Critics often accuse the practice of being overly perfectionist and typically impractical, forcing developers to write tests simply for the sake of coverage instead of meaningfulness. Certainly there is a balance between the time spent writing a test and the time saved by that test catching a regression.
NFIBrokerage tries to take a practical and center-line approach. Truly,
we do not require 100% lines covered. Through usage of coveralls-ignore
comments, we artificially inflate our numbers to 100% so a change can be
immediately and naively validated as "not having changed coverage." If we
required a lower percentage, say 80%, adding new features or otherwise code
could change whether or not a project meets the requirement, even assuming
all new code is merged at 100% coverage, simply because the denominator of
covered_lines / total_lines
changes.
We use the Elixir formatter introduced in Elixir 1.6 in check-mode as a part
of CI. Some editors have supported formatting before the formatter was
introduced, and ignore configuration or differ in their methods of formatting.
In particular, we set our line-length option to 80 columns and make sparing
use of the locals_without_parens
option, which some formatting extensions
ignore. A simple run of mix format
on the command line before committing
is always wise.
We use two tools for linting: mix compile --warnings-as-errors
and
credo
. Either of these checks failing to
pass will fail the CI, even for test files.
Why so stringent about test files?
It may seem overzealous to care so much about the linting and formatting of test files. They're just tests, right?
We believe that more often than not, the testing suite of a library or service most clearly explains how that library or service works. Test suites are also integration-points between developers: a well-written suite attracts a community of fast-moving feature implementations and quick maintenance. Therefore we treat the readability and consistency of tests with paramount concern.
To pull all of these checks together for development, we publish a
bless
library which runs all commands
in serial and fails if any check fails. A successful run of mix bless
before committing usually translates into a green check mark from the CI.
If you cannot get a good bless run, submit the PR anyways! We'll be happy to
help you get that green check mark.