documenting test expactations

[James Peach <jo...@gmail.com>]

Hi all,

In a couple of reviews, I've been asked to avoid emitting explanatory messages from the EXPECT() macro. The rationale for this is that tests usually use comments. However, I think that emitting the reason for a failed expectation into the test log is pretty helpful and we should do it more often.

What do people think about explicitly allowing (or even encouraging) this? ie. EXPECT(...) << "some explanation goes here"

J

Re: documenting test expactations

[Yan Xu <ya...@jxu.me>]

My take (in general I am in favor of comments):

   - In my experience, expectations almost always only tell you what's
   wrong but not why, i.e., how the test arrives at the condition. Therefore I
   almost always need to read the test itself.
   - Unlike CHECKs in the main code, tests are sequential, (reasonably)
   short, and should "tell a story", so we should strive to make the test very
   readable.
   - If there are important additional variables that could be printed to
   shed some light on the "why", I think we should allow this kind of EXPECT
   messages.


On Mon, May 8, 2017 at 9:09 AM, Neil Conway <ne...@gmail.com> wrote:

> My two cents:
>
> (1) I almost always need to read the test to understand/debug a test
> failure.
>
> (2) As long as the intent of the test is clear, I'm not picky about
> whether clarifying comments take the form of C++ comments or
> explanatory EXPECT messages.
>
> One thing I would be opposed to is adding explanatory EXPECT messages
> in an indiscriminate way; IMO that just adds redundancy and violates
> DRY (most test expectations are pretty self-explanatory).
>


This is in fact my main concern, comments/messages shouldn't replace
self-explanatory code but people have tendency to be verbose.

Comments help with conciseness and expressiveness in:

   - In many cases perhaps one simple comment explains a block of code
   which includes multiple expectations, then we don't need to repeat the same
   thing for each EXPECT.
   - If the explanation is multi-lined, comments give you more flexibility
   in terms the use of spaces and bullets to summarize the idea. EXPECT
   messages, being code, are subject to more styling rules and limitations
   which can make it harder to write and read.
   - Comments are usually mentally easier to gloss over when the reader is
   already familiar with the idea of the test. Some editors even use lighter
   colors for them or fold them automatically so you only need to look at them
   when necessary.

When it comes to very simple cases like the one James cited. I think the
two are equivalent so I would just prefer to stick with the convention of
using comments. It would look ugly and inconsistent if two tests or two
code blocks in the same test use two styles for basically identical
context.

Lastly, there is an exception to every rule of course and I am not against
special cases that make common sense. :)


> Neil
>
> On Mon, May 8, 2017 at 8:24 AM, James Peach <jo...@gmail.com> wrote:
> >
> >> On May 1, 2017, at 4:28 PM, Benjamin Mahler <bm...@apache.org> wrote:
> >>
> >> Do you have some examples?
> >
> > I think that this:
> >
> > EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())
> >     << "a partial block should round up";
> >
> > is a strict superset of this:
> >
> > // A partial block should round up.
> > EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())
> >
> > The former is preferable since the person triaging test failures gets
> the immediate context of what the expectation is doing. This is valuable
> even if you might also find you need to check the source.
> >
> >>
> >> Thinking through my own experience debugging tests, I tend to only get
> >> value out of EXPECT messages when they are providing information that I
> >> can't get access to from the line number / actual vs expected printing.
> >> (e.g. the value of a variable). If the EXPECT message is simply
> explaining
> >> what the test is doing, then I tend to ignore it and read the test
> instead,
> >> so it would be helpful to discuss some examples to get a better sense.
> :)
> >>
> >> On Sat, Apr 29, 2017 at 10:02 AM, James Peach <jo...@gmail.com> wrote:
> >>
> >>> Hi all,
> >>>
> >>> In a couple of reviews, I've been asked to avoid emitting explanatory
> >>> messages from the EXPECT() macro. The rationale for this is that tests
> >>> usually use comments. However, I think that emitting the reason for a
> >>> failed expectation into the test log is pretty helpful and we should
> do it
> >>> more often.
> >>>
> >>> What do people think about explicitly allowing (or even encouraging)
> this?
> >>> ie. EXPECT(...) << "some explanation goes here"
> >>>
> >>> J
> >
>

Re: documenting test expactations

[Neil Conway <ne...@gmail.com>]

My two cents:

(1) I almost always need to read the test to understand/debug a test failure.

(2) As long as the intent of the test is clear, I'm not picky about
whether clarifying comments take the form of C++ comments or
explanatory EXPECT messages.

One thing I would be opposed to is adding explanatory EXPECT messages
in an indiscriminate way; IMO that just adds redundancy and violates
DRY (most test expectations are pretty self-explanatory).

Neil

On Mon, May 8, 2017 at 8:24 AM, James Peach <jo...@gmail.com> wrote:
>
>> On May 1, 2017, at 4:28 PM, Benjamin Mahler <bm...@apache.org> wrote:
>>
>> Do you have some examples?
>
> I think that this:
>
> EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())
>     << "a partial block should round up";
>
> is a strict superset of this:
>
> // A partial block should round up.
> EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())
>
> The former is preferable since the person triaging test failures gets the immediate context of what the expectation is doing. This is valuable even if you might also find you need to check the source.
>
>>
>> Thinking through my own experience debugging tests, I tend to only get
>> value out of EXPECT messages when they are providing information that I
>> can't get access to from the line number / actual vs expected printing.
>> (e.g. the value of a variable). If the EXPECT message is simply explaining
>> what the test is doing, then I tend to ignore it and read the test instead,
>> so it would be helpful to discuss some examples to get a better sense. :)
>>
>> On Sat, Apr 29, 2017 at 10:02 AM, James Peach <jo...@gmail.com> wrote:
>>
>>> Hi all,
>>>
>>> In a couple of reviews, I've been asked to avoid emitting explanatory
>>> messages from the EXPECT() macro. The rationale for this is that tests
>>> usually use comments. However, I think that emitting the reason for a
>>> failed expectation into the test log is pretty helpful and we should do it
>>> more often.
>>>
>>> What do people think about explicitly allowing (or even encouraging) this?
>>> ie. EXPECT(...) << "some explanation goes here"
>>>
>>> J
>

Re: documenting test expactations

[James Peach <jo...@gmail.com>]

> On May 1, 2017, at 4:28 PM, Benjamin Mahler <bm...@apache.org> wrote:
> 
> Do you have some examples?

I think that this:

EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())
    << "a partial block should round up";

is a strict superset of this:

// A partial block should round up.
EXPECT_EQ(Bytes(512u), BasicBlocks(Bytes(128)).bytes())

The former is preferable since the person triaging test failures gets the immediate context of what the expectation is doing. This is valuable even if you might also find you need to check the source.

> 
> Thinking through my own experience debugging tests, I tend to only get
> value out of EXPECT messages when they are providing information that I
> can't get access to from the line number / actual vs expected printing.
> (e.g. the value of a variable). If the EXPECT message is simply explaining
> what the test is doing, then I tend to ignore it and read the test instead,
> so it would be helpful to discuss some examples to get a better sense. :)
> 
> On Sat, Apr 29, 2017 at 10:02 AM, James Peach <jo...@gmail.com> wrote:
> 
>> Hi all,
>> 
>> In a couple of reviews, I've been asked to avoid emitting explanatory
>> messages from the EXPECT() macro. The rationale for this is that tests
>> usually use comments. However, I think that emitting the reason for a
>> failed expectation into the test log is pretty helpful and we should do it
>> more often.
>> 
>> What do people think about explicitly allowing (or even encouraging) this?
>> ie. EXPECT(...) << "some explanation goes here"
>> 
>> J

Re: documenting test expactations

[Benjamin Mahler <bm...@apache.org>]

Do you have some examples?

Thinking through my own experience debugging tests, I tend to only get
value out of EXPECT messages when they are providing information that I
can't get access to from the line number / actual vs expected printing.
(e.g. the value of a variable). If the EXPECT message is simply explaining
what the test is doing, then I tend to ignore it and read the test instead,
so it would be helpful to discuss some examples to get a better sense. :)

On Sat, Apr 29, 2017 at 10:02 AM, James Peach <jo...@gmail.com> wrote:

> Hi all,
>
> In a couple of reviews, I've been asked to avoid emitting explanatory
> messages from the EXPECT() macro. The rationale for this is that tests
> usually use comments. However, I think that emitting the reason for a
> failed expectation into the test log is pretty helpful and we should do it
> more often.
>
> What do people think about explicitly allowing (or even encouraging) this?
> ie. EXPECT(...) << "some explanation goes here"
>
> J