Should we test for error messages and debug traits?

[sffc]

This came up in the review for #61.

Should we write test code that exercises failure cases such that we test all Error types?

When our types implement the Debug trait, should we assert that the messages are a particular format? If not, should we at least call println! in the test file to ensure that the code is covered?

My personal preference would be to be Yes on testing all failure cases, and No on testing debug traits. I haven't formed an opinion on whether we should call println! to cause the code to be covered.

CC @zbraniecki

[zbraniecki]

I don't have a lot of experience with consequences of either approach, so I operate on instinct here.

Using println to get coverage feels like cheating to me - you say "this line is covered, we verify that it works as intended", when it fact you don't.
I'd prefer to keep it either uncovered, or cover with a test, but it's 0.5 on apache scale. I'm ok with going in other direction :)

[sffc]

Using println to get coverage feels like cheating to me - you say "this line is covered, we verify that it works as intended", when it fact you don't.

It's marginally better than not running the code, because at least you assert that the code doesn't panic when you run that code path.

[Manishearth]

Yeah typically debug impls aren't tested, given that they're mostly used while logging and debugging.

It's typical to test that an error occurs. I don't think we have to test all cases (in particular, some errors may end up being opaque objects depending on the use case) but it's worth to try and test them.

[filmil]

2 cents: once you exercise the code paths relevant in execution, incremental coverage is more useful than absolute code coverage.

Or; tl;dr: once you have ~85% test coverage, what we usually want is to have all new code exercised.

[zbraniecki]

In #112 I'm going to add documentation on what we agreed on (with examples) wrt. Errors.

[zbraniecki]

I ended up not including this information in my PR for #112. We should write a new PR once that one lands.