Remove possibly outdated suppression

Remove possibly outdated suppression

(The bug has been fallow since 2011 and I couldn't reproduce it with --gtest_repeat=100 in my checkout.)

BUG=88228
R=rsleevi@chromium.org

Committed: https://crrev.com/cb31f0b610d1cf96479db8a32fb5f08e582c1b0d
Cr-Commit-Position: refs/heads/master@{#332281}

[Randy Smith (Not in Mondays), 2015-06-01 20:35:28]

Ryan: PTAL?

I've stripped the suppression and run with --gtest_repeat=100 under valgrind,
and not managed to reproduce the problem.  In the absence of that, I think my
options for pursuing this are to land logging to try to understand what's going
on when it fails on the bots, and strip the suppression out while doing so,
which is what I've done.

I'll note that this bug is old enough that I think it's possible the problem
doesn't exist anymore, in which case this will end up being the fix CL.  For
that reason, I'd like to hear any problems that you have with this code being
permanently in the codebase--it may still land, but if you don't want it
permanently, I'll revert it when I'm comfortable the problem is gone.

[Ryan Sleevi, 2015-06-01 20:50:35]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-06-01 20:50:35]

Redirecting to David because I'm out today/tomorrow.

My gut is that I would prefer the Dump code not be landed. In the same way that
people forget to remove suppressions, or forget to re-examine them, I fear the
Dump code will be the same. Except this time, there won't be a forcing function
(Fixit bugs) to force us to revaluate whether the code is needed, and the code
will be obviously "not dead" (since it's reachable in the test)

Preferred approach is:
1) Remove the suppression, nothing more
2) If the suppression fails, one of the memory sherriffs will add a suppression
back and yell.
3) If that happens, keep the suppression, and add this code
4) Find instances on the bots where the suppression is hit. Investigate.
5) Fix code, remove suppression, remove diagnostic code.

If you're wrong, the memory sheriff is just going to have to re-add a
suppression either way. So you can defer adding the logging code until then.

If you're right, and it's resolved, there's no forcing function to ever
re-evaluate this code. Hence my preference to not add it.

[davidben, 2015-06-01 20:58:57]

LogContainsEndEvent returns a testing::AssertionResult. It's possible to staple
messages to those, right? How about we do that instead:

https://code.google.com/p/chromium/codesearch#chromium/src/net/log/test_net_l...

There, in the "Type does not match" bits and whatnot, stick the type. Maybe more
info if we really feel like it, but the type is probably decent enough.

It gets swallowed for this failure because of the wrapper that turns it to bool,
but https://codereview.chromium.org/1162323002/ will fix that.

[Randy Smith (Not in Mondays), 2015-06-01 21:04:56]

I interpret Ryan's comment as about goals and David's as about implementation,
so focussing on Ryan's for the moment.

Ryan: I'm tempted to just go with your preference, since I don't care that much,
but I don't understand it, and I like to understand things :-}.  What's wrong
with the dump code being permanent?  It only triggers on a test failure, and
what comes out is useful information for debugging that particularly type of
test failure.

[davidben, 2015-06-01 21:15:16]

On 2015/06/01 21:04:56, rdsmith wrote:
> I interpret Ryan's comment as about goals and David's as about implementation,
> so focussing on Ryan's for the moment.
> 
> Ryan: I'm tempted to just go with your preference, since I don't care that
much,
> but I don't understand it, and I like to understand things :-}.  What's wrong
> with the dump code being permanent?  It only triggers on a test failure, and
> what comes out is useful information for debugging that particularly type of
> test failure.

I think there's a slight difference: Peppering individual tests with this
logging information is pretty silly, and I'm not very thrilled about that. But
having utility assertions like LogContains* emit obviously relevant things like
"I wanted type A and got type B" seems pretty sound. EXPECT_EQ(a, b) will
generate a useful printout, etc.

[Randy Smith (Not in Mondays), 2015-06-01 21:18:52]

On 2015/06/01 21:15:16, David Benjamin wrote:
> On 2015/06/01 21:04:56, rdsmith wrote:
> > I interpret Ryan's comment as about goals and David's as about
implementation,
> > so focussing on Ryan's for the moment.
> > 
> > Ryan: I'm tempted to just go with your preference, since I don't care that
> much,
> > but I don't understand it, and I like to understand things :-}.  What's
wrong
> > with the dump code being permanent?  It only triggers on a test failure, and
> > what comes out is useful information for debugging that particularly type of
> > test failure.
> 
> I think there's a slight difference: Peppering individual tests with this
> logging information is pretty silly, and I'm not very thrilled about that. But
> having utility assertions like LogContains* emit obviously relevant things
like
> "I wanted type A and got type B" seems pretty sound. EXPECT_EQ(a, b) will
> generate a useful printout, etc.

Ah!  Good point, I agree.  Ryan, WDYT?  Still opposed if it's a general feature
to dump the log when an assertion fails?

[Ryan Sleevi, 2015-06-01 21:25:49]

On 2015/06/01 21:18:52, rdsmith wrote:
> Ah!  Good point, I agree.  Ryan, WDYT?  Still opposed if it's a general
feature
> to dump the log when an assertion fails?

In general, yes :)

It's code that has to be maintained, adding cost, for a situation that should
never, ever happen, and when it does, should have a developer attending to it
debugging it, who can just get the information from the debugger (common case)
or, in the worst case, temporarily add debugging that gets removed shortly
thereafter.

[In general, I've been repeatedly burned by libraries that have test code that
handles 'impossible' / 'exceptional' cases, and ends up taking longer to migrate
or is just flat out "wrong" or buggy, compared to fixing the 'production'
callers]

[Randy Smith (Not in Mondays), 2015-06-01 21:27:49]

On 2015/06/01 21:25:49, Ryan Sleevi wrote:
> On 2015/06/01 21:18:52, rdsmith wrote:
> > Ah!  Good point, I agree.  Ryan, WDYT?  Still opposed if it's a general
> feature
> > to dump the log when an assertion fails?
> 
> In general, yes :)
> 
> It's code that has to be maintained, adding cost, for a situation that should
> never, ever happen, and when it does, should have a developer attending to it
> debugging it, who can just get the information from the debugger (common case)
> or, in the worst case, temporarily add debugging that gets removed shortly
> thereafter.
> 
> [In general, I've been repeatedly burned by libraries that have test code that
> handles 'impossible' / 'exceptional' cases, and ends up taking longer to
migrate
> or is just flat out "wrong" or buggy, compared to fixing the 'production'
> callers]

Cool; thanks for the explanation.  I'll execute on the path proposed in c#3
(unless David objects :-}).

[davidben, 2015-06-01 21:31:15]

On 2015/06/01 21:25:49, Ryan Sleevi wrote:
> On 2015/06/01 21:18:52, rdsmith wrote:
> > Ah!  Good point, I agree.  Ryan, WDYT?  Still opposed if it's a general
> feature
> > to dump the log when an assertion fails?
> 
> In general, yes :)
> 
> It's code that has to be maintained, adding cost, for a situation that should
> never, ever happen, and when it does, should have a developer attending to it
> debugging it, who can just get the information from the debugger (common case)
> or, in the worst case, temporarily add debugging that gets removed shortly
> thereafter.
> 
> [In general, I've been repeatedly burned by libraries that have test code that
> handles 'impossible' / 'exceptional' cases, and ends up taking longer to
migrate
> or is just flat out "wrong" or buggy, compared to fixing the 'production'
> callers]

Hah! Actually, I was looking at the wrong function all this time. We *already*
have the basic logging I was thinking of!

https://code.google.com/p/chromium/codesearch#chromium/src/net/log/test_net_l...

It just gets suppressed because it turns into a bool. Which is getting fixed.

[Randy Smith (Not in Mondays), 2015-06-01 21:36:14]

I'm going to go with "Great!  You both win!" and completely ignore the
underlying philosophical dispute that isn't being resolved.  At least, not by
me, and not in this CL :-}.

David, willing to stamp the suppression removal?

[davidben, 2015-06-01 21:37:46]

lgtm! :-)

[Randy Smith (Not in Mondays), 2015-06-01 21:38:38]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-06-01 22:56:32]

The CQ bit was unchecked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-06-01 22:56:40]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-06-01 22:59:47]

The CQ bit was unchecked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-06-01 22:59:59]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2015-06-01 23:36:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159183005/40001

[commit-bot: I haz the power, 2015-06-02 00:52:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-02 00:53:34]

Patchset 3 (id:??) landed as
https://crrev.com/cb31f0b610d1cf96479db8a32fb5f08e582c1b0d
Cr-Commit-Position: refs/heads/master@{#332281}