Quic: add unittests for reduced PING Timeout

Quic: add unittests for reduced PING Timeout

BUG=

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

[Zhongyi Shi, 2016-09-02 21:11:48]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2016-09-02 21:26:52]

lgtm, mod a few nits.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
File net/quic/core/quic_connection.h (right):

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection.h:486: // Used in Chromium, but not internally.
I'd just remove this comment.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection.h:487: // Called only before ping_alarm_ is set.
nit: // Must only be called before ping_alarm_ is set.

(To make it clear that this is the contract, and not just a general description
of callers)

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
File net/quic/core/quic_connection_test.cc (right):

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection_test.cc:2916: // of the 1st ping timer alarm
granularity.
this really mean 1 second, not first.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection_test.cc:2941:
connection_.set_ping_timeout(QuicTime::Delta::FromSeconds(10));
To confirm, this test fails if you comment out this line, right?

[Zhongyi Shi, 2016-09-02 21:44:45]

The tests fails if the setter is commented :)

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
File net/quic/core/quic_connection.h (right):

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection.h:486: // Used in Chromium, but not internally.
On 2016/09/02 21:26:52, Ryan Hamilton wrote:
> I'd just remove this comment.

This line is intentionally put there to note that this is not used internally,
and to keep the comments also in sync, it's needed here.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection.h:487: // Called only before ping_alarm_ is set.
On 2016/09/02 21:26:52, Ryan Hamilton wrote:
> nit: // Must only be called before ping_alarm_ is set.
> 
> (To make it clear that this is the contract, and not just a general
description
> of callers)

Done.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
File net/quic/core/quic_connection_test.cc (right):

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection_test.cc:2916: // of the 1st ping timer alarm
granularity.
On 2016/09/02 21:26:52, Ryan Hamilton wrote:
> this really mean 1 second, not first.

Sorry for misread the code.

https://codereview.chromium.org/2307933003/diff/1/net/quic/core/quic_connecti...
net/quic/core/quic_connection_test.cc:2941:
connection_.set_ping_timeout(QuicTime::Delta::FromSeconds(10));
On 2016/09/02 21:26:52, Ryan Hamilton wrote:
> To confirm, this test fails if you comment out this line, right?

Yes! I just ran the test with this setter commented and it fails.

[Ryan Hamilton, 2016-09-02 22:39:58]

lgtm

[Zhongyi Shi, 2016-09-02 22:54:59]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-09-02 22:55:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 00:03:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-03 00:05:19]

Description was changed from

==========
Quic: add unittests for reduced PING Timeout

BUG=
==========

to

==========
Quic: add unittests for reduced PING Timeout

BUG=

Committed: https://crrev.com/b429bfb95e3cf4a3947b660ee0f44d07833722d5
Cr-Commit-Position: refs/heads/master@{#416400}
==========

[commit-bot: I haz the power, 2016-09-03 00:05:20]

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

[commit-bot: I haz the power, 2016-09-03 00:08:35]

Description was changed from

==========
Quic: add unittests for reduced PING Timeout

BUG=

Committed: https://crrev.com/b429bfb95e3cf4a3947b660ee0f44d07833722d5
Cr-Commit-Position: refs/heads/master@{#416400}
==========

to

==========
Quic: add unittests for reduced PING Timeout

BUG=

Committed: https://crrev.com/df9cb098cf816fb589e0ad6627012b524c78e9eb
Cr-Commit-Position: refs/heads/master@{#416401}
==========

[commit-bot: I haz the power, 2016-09-03 00:08:36]

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

[agrieve, 2016-09-03 00:25:47]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2296393005/ by agrieve@chromium.org.

The reason for reverting is: Broke cronet compile:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Lol...

../../net/quic/core/quic_connection_test.cc:2980:8: error: redefinition of
'class net::test::{anonymous}::QuicConnectionTest_ReducedPingTimeout_Test'
 TEST_P(QuicConnectionTest, ReducedPingTimeout) {
        ^
../../testing/gtest/include/gtest/internal/gtest-internal.h:1211:3: note: in
definition of macro 'GTEST_TEST_CLASS_NAME_'
   test_case_name##_##test_name##_Test
   ^
../../net/quic/core/quic_connection_test.cc:2980:1: note: in expansion of macro
'TEST_P'
 TEST_P(QuicConnectionTest, ReducedPingTimeout) {
 ^
../../net/quic/core/quic_connection_test.cc:2935:8: error: previous definition
of 'class net::test::{anonymous}::QuicConnectionTest_ReducedPingTimeout_Test'
 TEST_P(QuicConnectionTest, ReducedPingTimeout) {
        ^
../../testing/gtest/include/gtest/internal/gtest-internal.h:1211:3: note: in
definition of macro 'GTEST_TEST_CLASS_NAME_'
   test_case_name##_##test_name##_Test
   ^
../../net/quic/core/quic_connection_test.cc:2935:1: note: in expansion of macro
'TEST_P'
 TEST_P(QuicConnectionTest, ReducedPingTimeout) {
 ^.

[findit-for-me, 2016-09-03 00:33:16]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 416401 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...