Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521663002:
RTCStatsIntegrationTest added. (Closed)
DescriptionRTCStatsIntegrationTest added.
This is an integration test using peerconnectiontestwrapper.h to set up
and end to end test using a real PeerConnection implementation. These
tests will complement rtcstatscollector_unittest.cc which collects all
stats using mocks.
The integration test is set up so that all stats types are returned by
GetStats and verifies that expected dictionary members are defined. The
test could in the future be updated to include sanity checks for the
values of members. There is a sanity check that references to other
stats dictionaries yield existing stats of the appropriate type, but
other than that members are only tested for if they are defined not.
StatsCallback of rtcstatscollector_unittest.cc is moved so that it can
be reused and renamed to RTCStatsObtainer.
TODO: Audio stream track stats members are missing in the test. Find out
if this is because of a real problem or because of testing without real
devices. Do this before closing crbug.com/627816.
BUG=chromium:627816
Committed: https://crrev.com/db346a7cbe511641e11307dc4cbbc8fc0c323845
Cr-Commit-Position: refs/heads/master@{#15287}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed deadbeef's comments #Patch Set 3 : Data channels included. Successfully separate audio from video track. TODO for missing audio stats. #
Total comments: 4
Patch Set 4 : Rebase with master #Patch Set 5 : Addressed nits #Patch Set 6 : Rebase /w master and taking RTCCodecStats into account #
Messages
Total messages: 44 (29 generated)
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test will be set up so that all stats types are returned and that expected dictionary members are defined. The test could be updated to include sanity tests for the values of members. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test will be set up so that all stats types are returned and that expected dictionary members are defined. The test could be updated to include sanity tests for the values of members. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. BUG=chromium:627816 ==========
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test will be set up so that all stats types are returned and that expected dictionary members are defined. The test could be updated to include sanity tests for the values of members. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. BUG=chromium:627816 ==========
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look deadbeef and hta. I'm a little bit worried about the testability of this, but I think this is pretty good. The CL makes sure we get all stats without much sanity checking. The idea is that rtcstatscollector_unittest.cc should already cover this pretty good with mocks, and the integration test should just ensure that something doesn't break when using non-mocks, that stats aren't missing, etc. Basic sanity checking could be added in the future, like sensible value ranges and such. I'm hoping to avoid testing every member of every dictionary type for specific values. But if you think that is necessary we should probably do a series of CLs where we test one dictionary type at a time, similar to rtcstatscollector_unittest.cc, with specific test scenarios designed for specific outcomes. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... File webrtc/api/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:251: // integration test to include data channels instead. Will address before landing. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:365: // |TestMemberIsDefined| or not? Currently optional: Will look into before landing.
(Note: The "TestMemberIs[Defined/Undefined]" tests are based on the current implementation where a lot of stats are not collected by RTCStatsCollector. As TODOs in rtcstats_objects.h are fixed, the integration test will need to be updated.)
Looks really good, I like how you went about this. The only question I have is how "end-to-end" this test is intended to go. Is it meant to use real sockets and a real STUN server, or would virtual sockets be sufficient for this level of testing? Also, small sidenote: is there an established way of naming integration tests? It's really confusing how "peerconnectioninterface_unittest", "peerconnection_unittest" and "peerconnectionendtoend_unittest" are named; I'd be happy if we renamed them. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... File webrtc/api/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:29: const int64_t kGetStatsTimeoutMs = 1000; I'd suggest using a larger timeout such as 10 seconds. With all the thread hops GetStats uses, some slow buildbots could feasibly take more than a second. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:33: RTCStatsIntegrationTest() { Should this test use real network resources, or use VirtualSocketServer as peerconnection_unittest (which is also an integration test in reality) uses? https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:46: ice_server.uri = "stun:stun.l.google.com:19302"; Does this test need a STUN server? If not I'd omit it, or use one under the test's control. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:99: void MemberTested( nit: I'd prefer the name MarkMemberTested or something similar. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:491: rtc::scoped_refptr<const RTCStatsReport> report = CallerGetStats(); Do you plan to add callee stats too?
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL deadbeef and hta. On 2016/11/22 10:02:32, Taylor Brandstetter wrote: > Looks really good, I like how you went about this. The only question I have is > how "end-to-end" this test is intended to go. Is it meant to use real sockets > and a real STUN server, or would virtual sockets be sufficient for this level of > testing? Virtual sockets and fake STUN server it is! > Also, small sidenote: is there an established way of naming integration tests? > It's really confusing how "peerconnectioninterface_unittest", > "peerconnection_unittest" and "peerconnectionendtoend_unittest" are named; I'd > be happy if we renamed them. Uhm, looks like the only file called "_integrationtest" is videoprocessor_integrationtest.cc. There's also a file called screen_capturer_integration_test.cc. Not sure if to rename our test to rtcstatsintegration_unittest.cc or similar, leave it as-is, or leave it + rename peerconnection unittests. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... File webrtc/api/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:29: const int64_t kGetStatsTimeoutMs = 1000; On 2016/11/22 10:02:31, Taylor Brandstetter wrote: > I'd suggest using a larger timeout such as 10 seconds. With all the thread hops > GetStats uses, some slow buildbots could feasibly take more than a second. Done. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:33: RTCStatsIntegrationTest() { On 2016/11/22 10:02:31, Taylor Brandstetter wrote: > Should this test use real network resources, or use VirtualSocketServer as > peerconnection_unittest (which is also an integration test in reality) uses? Updated to use VirtualSocketServer. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:46: ice_server.uri = "stun:stun.l.google.com:19302"; On 2016/11/22 10:02:32, Taylor Brandstetter wrote: > Does this test need a STUN server? If not I'd omit it, or use one under the > test's control. Done. After some confusion I was able to wire it up with a TestStunServer. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:99: void MemberTested( On 2016/11/22 10:02:32, Taylor Brandstetter wrote: > nit: I'd prefer the name MarkMemberTested or something similar. Done. https://codereview.webrtc.org/2521663002/diff/1/webrtc/api/rtcstats_integrati... webrtc/api/rtcstats_integrationtest.cc:491: rtc::scoped_refptr<const RTCStatsReport> report = CallerGetStats(); On 2016/11/22 10:02:31, Taylor Brandstetter wrote: > Do you plan to add callee stats too? Sure! Done.
Note: I have yet to make the test include RTCDataChannelStats or investigates what's up with tracks that are missing width and height stats. Not forgotten.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL. CL updated, no blocking TODOs IMO. It turns out the audio stream track had all of the audio-only members undefined, which made the test think audio track stats was video track stats. By looking at a video-only member instead audio and video are distinguished. I don't know if the missing audio track stats is a real bug or an artifact of testing without real devices, but I'm guessing that is the case. I made the audio track stats member optional and added a TODO, I don't think this should block this CL from landing.
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are as missing in test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ==========
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are as missing in test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are missing in the test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, the usual flurry of nits. https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... File webrtc/api/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... webrtc/api/rtcstats_integrationtest.cc:90: rtc::scoped_refptr<const RTCStatsReport> CallerGetStats() { Nit/preference: I would prefer to call these GetStatsFromCaller and vv. for Callee. Names for tuff that does things ought to start with the verb. https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... webrtc/api/rtcstats_integrationtest.cc:115: std::unique_ptr<cricket::TestStunServer> stun_server_; Do you really need to have the stun server here? I don't see it being passed to anything, so it may be removable. Shorter test = better test.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL beefy https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... File webrtc/api/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... webrtc/api/rtcstats_integrationtest.cc:90: rtc::scoped_refptr<const RTCStatsReport> CallerGetStats() { On 2016/11/23 07:52:11, hta-webrtc wrote: > Nit/preference: I would prefer to call these GetStatsFromCaller and vv. for > Callee. Names for tuff that does things ought to start with the verb. > Done. https://codereview.webrtc.org/2521663002/diff/40001/webrtc/api/rtcstats_integ... webrtc/api/rtcstats_integrationtest.cc:115: std::unique_ptr<cricket::TestStunServer> stun_server_; On 2016/11/23 07:52:11, hta-webrtc wrote: > Do you really need to have the stun server here? I don't see it being passed to > anything, so it may be removable. Shorter test = better test. Done, removed. Oh... I thought it was working because it hooked itself up to the network thread's socket server or something... But this is not necessary, and it doesn't seem to matter what ice server URI I use as long as it is a valid format. Not sure why this works but it does, hazaa!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
lgtm
As for naming the test, I prefer this naming scheme ("X_integrationtest") to our
existing one ("Xendtoend_unittest"). I made a bug to remind me to fix them
later: https://bugs.chromium.org/p/webrtc/issues/detail?id=6787
The CQ bit was checked by hbos@webrtc.org
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/2521663002/#ps80001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19795)
The CQ bit was checked by hbos@webrtc.org
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/2521663002/#ps100001 (title: "Rebase /w master and taking RTCCodecStats into account")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480411659540610,
"parent_rev": "01b33f67f50cffd5a44d57f12596a67908b7dd63", "commit_rev":
"732e87fc9b8b6e60d38b1cf5bbc4cc4f8c1efc28"}
Message was sent while issue was closed.
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are missing in the test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are missing in the test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are missing in the test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 ========== to ========== RTCStatsIntegrationTest added. This is an integration test using peerconnectiontestwrapper.h to set up and end to end test using a real PeerConnection implementation. These tests will complement rtcstatscollector_unittest.cc which collects all stats using mocks. The integration test is set up so that all stats types are returned by GetStats and verifies that expected dictionary members are defined. The test could in the future be updated to include sanity checks for the values of members. There is a sanity check that references to other stats dictionaries yield existing stats of the appropriate type, but other than that members are only tested for if they are defined not. StatsCallback of rtcstatscollector_unittest.cc is moved so that it can be reused and renamed to RTCStatsObtainer. TODO: Audio stream track stats members are missing in the test. Find out if this is because of a real problem or because of testing without real devices. Do this before closing crbug.com/627816. BUG=chromium:627816 Committed: https://crrev.com/db346a7cbe511641e11307dc4cbbc8fc0c323845 Cr-Commit-Position: refs/heads/master@{#15287} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/db346a7cbe511641e11307dc4cbbc8fc0c323845 Cr-Commit-Position: refs/heads/master@{#15287}
Message was sent while issue was closed.
On 2016/11/29 09:57:17, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/db346a7cbe511641e11307dc4cbbc8fc0c323845 > Cr-Commit-Position: refs/heads/master@{#15287} It seems this test is continuously failing on the linux memcheck bot. I've created a bug for it here: https://bugs.chromium.org/p/webrtc/issues/detail?id=6794 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
