Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(486)

Issue 2738353003: Rewrite PeerConnection integration tests using better testing practices. (Closed)

Created:
3 years, 9 months ago by Taylor Brandstetter
Modified:
3 years, 8 months ago
Reviewers:
pthatcher1
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Rewrite PeerConnection integration tests using better testing practices. Also renames "peerconnection_unittests" to "peerconnection_integrationtests", and moves the ICE URL parsing code to separate files. The main problem previously was that the test assertions occurred in various places in the main test class, and this shared test code was overly complex and stateful. As a result, it was difficult to tell what a test even does, let alone what assertions it's meant to be making. And writing a new test that does what you want can be a frustrating ordeal. The new code still uses helper methods, but they have intuitive names and a smaller role; all of the important parts of the test's logic are in the test case itself. We're planning on merging PeerConnection and WebRtcSession at some point soon, so it seemed valuable to do this, so that the WebRtcSession tests can be rewritten as PeerConnection tests using better patterns. BUG=None Review-Url: https://codereview.webrtc.org/2738353003 Cr-Commit-Position: refs/heads/master@{#17458} Committed: https://chromium.googlesource.com/external/webrtc/+/1dcb16409a3e82518ef852ec5614f87dff04f248

Patch Set 1 #

Patch Set 2 : Fix race between ICE and SDP messages. 33/50 tests passing. #

Patch Set 3 : Fixing a few tests; 39/50 passing. #

Patch Set 4 : Fixed remaining tests. Now just need to address TODOs and possible gaps in coverage. #

Patch Set 5 : Addressing TODO about ICE renomination test. #

Patch Set 6 : Move ICE server parsing code and tests to separate files #

Patch Set 7 : Rename peerconnection_unittest to peerconnection_integrationtest. #

Patch Set 8 : Add the missing tests for CreateOffer/CreateAnswer with constraints. #

Total comments: 2

Patch Set 9 : Cleaning up some minor things, moving tests around. #

Patch Set 10 : Minor improvements in various places. #

Patch Set 11 : More minor cleanup; just comments and "sanity check" code at this point. #

Patch Set 12 : Merge with master. #

Total comments: 30

Patch Set 13 : Address some of pthatcher's comments #

Patch Set 14 : Merge with master #

Patch Set 15 : Fixing issues caught by trybots. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3415 lines, -3162 lines) Patch
M tools-webrtc/valgrind/gtest_exclude/peerconnection_unittests.gtest-memcheck.txt View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M webrtc/api/peerconnectioninterface.h View 1 1 chunk +4 lines, -2 lines 0 comments Download
M webrtc/media/base/fakevideorenderer.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M webrtc/pc/BUILD.gn View 1 2 3 4 5 6 2 chunks +4 lines, -1 line 0 comments Download
A webrtc/pc/iceserverparsing.h View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
A webrtc/pc/iceserverparsing.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +294 lines, -0 lines 0 comments Download
A webrtc/pc/iceserverparsing_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +231 lines, -0 lines 0 comments Download
M webrtc/pc/peerconnection.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +1 line, -8 lines 0 comments Download
M webrtc/pc/peerconnection.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +0 lines, -277 lines 0 comments Download
A webrtc/pc/peerconnection_integrationtest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2706 lines, -0 lines 0 comments Download
M webrtc/pc/peerconnection_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -2869 lines 0 comments Download
M webrtc/pc/peerconnectioninterface_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +137 lines, -1 line 0 comments Download
M webrtc/pc/test/mockpeerconnectionobservers.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 31 (20 generated)
Taylor Brandstetter
Peter, I think this is ready for an initial round of review. Can you take ...
3 years, 9 months ago (2017-03-17 04:30:38 UTC) #3
pthatcher1
The overall direction I think is great. Can you point out which specific parts or ...
3 years, 9 months ago (2017-03-17 17:18:42 UTC) #4
Taylor Brandstetter
On 2017/03/17 17:18:42, pthatcher1 wrote: > The overall direction I think is great. > > ...
3 years, 9 months ago (2017-03-17 18:00:12 UTC) #5
pthatcher1
lgtm These are all just suggestions for some improvements in readability. As a whole, this ...
3 years, 9 months ago (2017-03-20 18:23:17 UTC) #10
Taylor Brandstetter
Want to take a quick look at the last patchset? The main non-stylistic change is ...
3 years, 9 months ago (2017-03-23 04:46:26 UTC) #12
pthatcher1
lgtm Very Nice https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection_integrationtest.cc File webrtc/pc/peerconnection_integrationtest.cc (right): https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection_integrationtest.cc#newcode209 webrtc/pc/peerconnection_integrationtest.cc:209: return client; On 2017/03/23 04:46:25, Taylor ...
3 years, 9 months ago (2017-03-25 03:38:34 UTC) #13
Taylor Brandstetter
https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection_integrationtest.cc File webrtc/pc/peerconnection_integrationtest.cc (right): https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection_integrationtest.cc#newcode209 webrtc/pc/peerconnection_integrationtest.cc:209: return client; On 2017/03/25 03:38:34, pthatcher1 wrote: > On ...
3 years, 9 months ago (2017-03-27 17:12:16 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2738353003/270001
3 years, 8 months ago (2017-03-29 23:40:38 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/3064)
3 years, 8 months ago (2017-03-29 23:47:48 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2738353003/270001
3 years, 8 months ago (2017-03-30 04:00:55 UTC) #28
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 04:08:21 UTC) #31
Message was sent while issue was closed.
Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/external/webrtc/+/1dcb16409a3e82518ef852ec5...

Powered by Google App Engine
This is Rietveld 408576698