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 <neil.con...@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 <jor...@gmail.com> wrote: > > > >> On May 1, 2017, at 4:28 PM, Benjamin Mahler <bmah...@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 <jor...@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 > > >