Collecting RTCIceCandidatePairStats.transport_id and improved unittests

Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
Committed: https://crrev.com/0583b286e44b6f6d4d5c5185ed94b60f37aad598
Cr-Commit-Position: refs/heads/master@{#15316}

[hbos, 2016-11-24 11:11:22]

Description was changed from

==========
RTCTransportStats and related RTCStatsCollector fixes.

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

to

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, RTCStats::set_timestamp_us was added.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

[hbos, 2016-11-24 11:11:53]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 11:12:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2016-11-24 11:14:45]

hbos@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, hta@webrtc.org

[hbos, 2016-11-24 11:14:46]

Please take a look, hta and deadbeef.

[hbos, 2016-11-24 11:20:11]

This TODO was removed when removing ExpectReportContainsTransportStats, do you
think I should re-add it at the transport stats unittest or try to resolve it
before landing this CL?

// TODO(hbos): Instead of testing how the ID looks, test that the 
// corresponding pair's IP addresses are equal to the IP addresses of 
// the |best_connection_info| data. crbug.com/653873

[hbos, 2016-11-24 11:20:38]

On 2016/11/24 11:20:11, hbos wrote:
> This TODO was removed when removing ExpectReportContainsTransportStats, do you
> think I should re-add it at the transport stats unittest or try to resolve it
> before landing this CL?
> 
> // TODO(hbos): Instead of testing how the ID looks, test that the 
> // corresponding pair's IP addresses are equal to the IP addresses of 
> // the |best_connection_info| data. crbug.com/653873

I want to close the referenced bug :)

[commit-bot: I haz the power, 2016-11-24 11:32:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 11:32:50]

Dry run: This issue passed the CQ dry run.

[hta-webrtc, 2016-11-28 08:00:20]

On 2016/11/24 11:20:38, hbos wrote:
> On 2016/11/24 11:20:11, hbos wrote:
> > This TODO was removed when removing ExpectReportContainsTransportStats, do
you
> > think I should re-add it at the transport stats unittest or try to resolve
it
> > before landing this CL?
> > 
> > // TODO(hbos): Instead of testing how the ID looks, test that the 
> > // corresponding pair's IP addresses are equal to the IP addresses of 
> > // the |best_connection_info| data. crbug.com/653873
> 
> I want to close the referenced bug :)

Just remove the bug reference. Or file a new one and mark it P3 (cleanup).

[hta-webrtc, 2016-11-28 08:14:16]

lgtm

apart from the setting-timestamps thing.

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1682:
expected_rtp_transport.set_timestamp_us(report->timestamp_us());
Given that you also send report->timestamp_us() to the constructor for
expected_rtp_transport above, why does this have any effect?

(repeat question for all uses of set_timestamp_us in this file)

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1693: RTCTransportStats>());
<painful-looking code>
Is leaving the < on the previous line really what git cl format tells you to do?

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/stats/rtcstats.h
File webrtc/api/stats/rtcstats.h (right):

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/stats/rtcstats.h#n...
webrtc/api/stats/rtcstats.h:63: void set_timestamp_us(int64_t timestamp_us) {
timestamp_us_ = timestamp_us; }
Who calls this new function? If it's just for testing, add a comment saying so.
(Or rename to SetTimestampUsForTesting).

[hbos, 2016-11-28 11:53:18]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 11:53:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2016-11-28 11:53:51]

Please take a look, deadbeef.

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1682:
expected_rtp_transport.set_timestamp_us(report->timestamp_us());
On 2016/11/28 08:14:16, hta-webrtc wrote:
> Given that you also send report->timestamp_us() to the constructor for
> expected_rtp_transport above, why does this have any effect?
> 
> (repeat question for all uses of set_timestamp_us in this file)
> 

This one was a mistake, but in other places I needed because report's timestamp
is updated every new GetStatsReport (and its using a non-mockable timestamp for
this).

Instead of renaming to SetTimestampUsForTesting, I removed set_timestamp_us in
favor of making the comparison operator ignore the timestamp - not much use in
that comparison. operator=='s comment and unittest updated.

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1693: RTCTransportStats>());
On 2016/11/28 08:14:16, hta-webrtc wrote:
> <painful-looking code>
> Is leaving the < on the previous line really what git cl format tells you to
do?
> 

No, updated with git cl format.

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/stats/rtcstats.h
File webrtc/api/stats/rtcstats.h (right):

https://codereview.webrtc.org/2527113002/diff/1/webrtc/api/stats/rtcstats.h#n...
webrtc/api/stats/rtcstats.h:63: void set_timestamp_us(int64_t timestamp_us) {
timestamp_us_ = timestamp_us; }
On 2016/11/28 08:14:16, hta-webrtc wrote:
> Who calls this new function? If it's just for testing, add a comment saying
so.
> (Or rename to SetTimestampUsForTesting).
> 
> 

(Function removed in favor of making operator== not care about timestamps.)

[commit-bot: I haz the power, 2016-11-28 12:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 12:18:22]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2016-11-29 16:48:39]

lgtm

[hbos, 2016-11-29 17:05:41]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-11-29 17:05:41]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2527113002/#ps40001 (title:
"git cl format")

[commit-bot: I haz the power, 2016-11-29 17:05:50]

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

[commit-bot: I haz the power, 2016-11-29 17:11:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:11:02]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/15404)

[hbos, 2016-11-29 17:12:19]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-11-29 17:12:22]

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

[commit-bot: I haz the power, 2016-11-29 17:14:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:14:53]

Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20575)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/20562)

[hbos, 2016-11-30 08:45:26]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-11-30 08:45:27]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2527113002/#ps60001 (title:
"Rebase with master and update rtcstats_integrationtest.cc to include
transport_id")

[commit-bot: I haz the power, 2016-11-30 08:45:37]

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

[hbos, 2016-11-30 08:47:31]

Description was changed from

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, RTCStats::set_timestamp_us was added.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

to

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

[commit-bot: I haz the power, 2016-11-30 08:58:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 08:58:47]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/15436)

[hbos, 2016-11-30 09:38:49]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-11-30 09:39:01]

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

[commit-bot: I haz the power, 2016-11-30 09:50:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480498729449740,
"parent_rev": "08c538215b7304c6944620c9b099064704b6672f", "commit_rev":
"ceeb37dfef7985da2e49ba28c8f20e5b4ce641c8"}

[commit-bot: I haz the power, 2016-11-30 09:50:17]

Description was changed from

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

to

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

[commit-bot: I haz the power, 2016-11-30 09:50:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-30 09:50:39]

Description was changed from

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755
==========

to

==========
Collecting RTCIceCandidatePairStats.transport_id and improved unittests.

RTCIceCandidatePairStats.transport_id is set to the related
RTCTransportStats' id.

Unittest for RTCIceCandidatePairStats is updated to do EXPECT_EQ
between actual and an expected hardcoded dictionary. The previous way of
testing, ExpectReportContainsCandidatePair, is removed.

(ExpectReportContainsCandidate still exist, we might want to replace
this by EXPECT_EQ testing in a follow up.)

Unittest for RTCTransportStats is similarly updated and
ExpectReportContainsTransportStats is removed. A bug was uncovered where
the "rtcp_connection_info.best_connection = true" case was not tested
(a copy of rtcp_connection_info was used in the test, modifying that had
no affect on the test) - fixed.

rtcstats_integrationtest.cc updated to take transport_id into account.

In order to reuse an updated version of expected_rt[c]p_transport in the
unittest, timestamps are ignored by RTCStats::operator==.

BUG=chromium:627816, chromium:653873, chromium:653873, webrtc:6755

Committed: https://crrev.com/0583b286e44b6f6d4d5c5185ed94b60f37aad598
Cr-Commit-Position: refs/heads/master@{#15316}
==========

[commit-bot: I haz the power, 2016-11-30 09:50:40]

Patchset 4 (id:??) landed as
https://crrev.com/0583b286e44b6f6d4d5c5185ed94b60f37aad598
Cr-Commit-Position: refs/heads/master@{#15316}