Rewrite PeerConnection integration tests using better testing practices.

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

[Taylor Brandstetter, 2017-03-17 03:52:05]

Description was changed from

==========
Rewrite PeerConnection integration tests using better testing practices.

The main problem with the previous tests was that the test assertions
occured 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.

This is currently a work in progress; at the time of typing this I
barely have things compiling.
==========

to

==========
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 with the previous tests 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.
==========

[Taylor Brandstetter, 2017-03-17 04:30:36]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2017-03-17 04:30:38]

Peter, I think this is ready for an initial round of review. Can you take a look
when you have time? Not high priority, but I believe this will allow us to much
more easily merge PeerConnection and WebRtcSession, which will be needed at some
point by the upcoming refactoring we're doing.

https://codereview.webrtc.org/2738353003/diff/130001/webrtc/pc/iceserverparsi...
File webrtc/pc/iceserverparsing.cc (right):

https://codereview.webrtc.org/2738353003/diff/130001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing.cc:1: /*
This code is all unchanged.

https://codereview.webrtc.org/2738353003/diff/130001/webrtc/pc/iceserverparsi...
File webrtc/pc/iceserverparsing_unittest.cc (right):

https://codereview.webrtc.org/2738353003/diff/130001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing_unittest.cc:1: /*
This code is unchanged too.

[pthatcher1, 2017-03-17 17:18:42]

The overall direction I think is great.  

Can you point out which specific parts or new tests that I should look at more
thoroughly?

[Taylor Brandstetter, 2017-03-17 18:00:12]

On 2017/03/17 17:18:42, pthatcher1 wrote:
> The overall direction I think is great.  
> 
> Can you point out which specific parts or new tests that I should look at more
> thoroughly?

Basically all of the tests themselves have been rewritten. Though
EndToEndConnectionTimeWithTurnTurnPair is mostly unchanged, and some of the
helper methods are unchanged.

[Taylor Brandstetter, 2017-03-17 23:39:09]

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

[commit-bot: I haz the power, 2017-03-17 23:39:19]

Dry run: 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, 2017-03-17 23:43:57]

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

[commit-bot: I haz the power, 2017-03-17 23:43:59]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17413)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)

[pthatcher1, 2017-03-20 18:23:17]

lgtm

These are all just suggestions for some improvements in readability.  As a
whole, this is great.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
File webrtc/pc/iceserverparsing_unittest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing_unittest.cc:59: TEST_F(IceServerParsingTest,
ParseStunPrefixes) {
ParsePrefixes?

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing_unittest.cc:63: stun_servers_.clear();
It's only test code, so it doesn't matter, but this clear business every time
seems a little clunky.  It seems like something like this would be better:

auto servers = ParseUrl("...");
EXPECT_TRUE(servers.ok());
EXPECT_EQ(1U, servers.stun().size());
EXPECT_EQ(0U, servers.turn().size());
// No clear needed

or maybe this:

Servers servers;
EXPECT_TRUE(ParseUrl("..."), &servers);
EXPECT_EQ(1U, servers.stun().size());
EXPECT_EQ(0U, servers.turn().size());


I think the ParseUrl method could be easily changed to be like either of these
and it would be somewhat nicer.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:86: static const int
kEndAudioFrameCount = 3;
Can you leave a comment explaining what these are?

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:107: class SignalingMessageReceiver
{
SignalingReceiverInterface might be a better name.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:209: return client;
This is repeated 4 times.  A simple Create method that takes all 7 parameters
and does the "delete if Init failed" part might make sense.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:217: void CreateSetAndSignalOffer()
{
I read that as "(Create set) and (Signal offer)".  It took me a while to realize
that it means "(create offer) and (set offer) and (signal offer)".

Maybe "CreateAndSetAndSignalOffer"?

Or maybe just change the call sites to SetLocalAndSignal(CreateOffer())?

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:348: bool
ReceivedVideoFramesForEachTrack(int number_of_frames) {
Would it give more information to failing tests to return the number of tracks
that received enough frames, and then compare that to the number of tracks? 
That way, you get "3 vs 4" or "0 vs 4" instead of just "false".

But maybe that doesn't matter.  Just an idea.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:673: return
CreateOfferOrAnswer(false);
It seems like this would be a little more clear as:

rtc::scoped_refptr<MockCreateSessionDescriptionObserver> observer(...));
pc()->CreateOffer(observer, offer_answer_options_);
return WaitForDescriptionFromObserver(observer);

And then move the common code into WaitForDescriptionFromObserver.

But it's just test code, so maybe it doesn't matter.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1066:
std::unique_ptr<PeerConnectionWrapper> callee_pc_wrapper_;
I think you ought to just call this caller_ and callee_.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1090: }),
This is repeated many times.  Does it make sense to make a method for it?  Then
you'd have 


EXPECT_TRUE_WAIT(ReceivedPackets(caller), kMaxWaitForFramesMs)
EXPECT_TRUE_WAIT(ReceivedPackets(callee), kMaxWaitForFramesMs)

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1142: PeerConnectionWrapper*
receiver) {
Could probably just call it "TestDtmf".  I'm not sure what "between" adds.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1455: kEndVideoFrameCount),
This seems like another duplicate that could be moved into a method, so you can
have something like:

EXPECT_TRUE_WAIT(ReceivedFrames(caller, callee), kMaxWaitForFramesMs);

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1498: // not just assumed.
Perhaps make constants for kUnrotatedAspectRatio and kRotatedAspectRatio and use
those here and move the comment about "why 4:3" up to where they are defined.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1800: });
Would it be worth giving this a named method? 
callee_pc_wrapper()->SetReceivedSdpMunger(MakeSpecCompliantMaxBundle) would look
nice.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:2736: //                  the first
of which should have arrived before the answer.
Wow.  That's thorough.

[Taylor Brandstetter, 2017-03-23 04:39:29]

Description was changed from

==========
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 with the previous tests 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.
==========

to

==========
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.
==========

[Taylor Brandstetter, 2017-03-23 04:46:26]

Want to take a quick look at the last patchset? The main non-stylistic change is
the addition of the "ExpectNewFramesReceivedWithWait" helper method.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
File webrtc/pc/iceserverparsing_unittest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing_unittest.cc:59: TEST_F(IceServerParsingTest,
ParseStunPrefixes) {
On 2017/03/20 18:23:17, pthatcher1 wrote:
> ParsePrefixes?

Well a TURN server is a type of STUN server so technically this is accurate. And
I think it's more descriptive.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/iceserverparsi...
webrtc/pc/iceserverparsing_unittest.cc:63: stun_servers_.clear();
On 2017/03/20 18:23:16, pthatcher1 wrote:
> It's only test code, so it doesn't matter, but this clear business every time
> seems a little clunky.  It seems like something like this would be better:
> 
> auto servers = ParseUrl("...");
> EXPECT_TRUE(servers.ok());
> EXPECT_EQ(1U, servers.stun().size());
> EXPECT_EQ(0U, servers.turn().size());
> // No clear needed
> 
> or maybe this:
> 
> Servers servers;
> EXPECT_TRUE(ParseUrl("..."), &servers);
> EXPECT_EQ(1U, servers.stun().size());
> EXPECT_EQ(0U, servers.turn().size());
> 
> 
> I think the ParseUrl method could be easily changed to be like either of these
> and it would be somewhat nicer.

I have a simpler idea: Do the clear in ParseUrl. I think that's sufficient.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:86: static const int
kEndAudioFrameCount = 3;
On 2017/03/20 18:23:17, pthatcher1 wrote:
> Can you leave a comment explaining what these are?

Done.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:107: class SignalingMessageReceiver
{
On 2017/03/20 18:23:17, pthatcher1 wrote:
> SignalingReceiverInterface might be a better name.

I prefer the current name personally.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:209: return client;
On 2017/03/20 18:23:17, pthatcher1 wrote:
> This is repeated 4 times.  A simple Create method that takes all 7 parameters
> and does the "delete if Init failed" part might make sense.

That's what we had before which I intentionally changed because it results in
test code that looks like:

CreatePeerConnection(something, nullptr, nullptr, something, nullptr, nullptr,
nullptr);

My goal was prioritizing the readability of the tests, rather than reducing code
duplication. Though this could be cleaned up further by using the whole
"builder" pattern. I'll leave a todo.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:217: void CreateSetAndSignalOffer()
{
On 2017/03/20 18:23:17, pthatcher1 wrote:
> I read that as "(Create set) and (Signal offer)".  It took me a while to
realize
> that it means "(create offer) and (set offer) and (signal offer)".
> 
> Maybe "CreateAndSetAndSignalOffer"?
> 
> Or maybe just change the call sites to SetLocalAndSignal(CreateOffer())?

Went with first suggestion.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:348: bool
ReceivedVideoFramesForEachTrack(int number_of_frames) {
On 2017/03/20 18:23:17, pthatcher1 wrote:
> Would it give more information to failing tests to return the number of tracks
> that received enough frames, and then compare that to the number of tracks? 
> That way, you get "3 vs 4" or "0 vs 4" instead of just "false".
> 
> But maybe that doesn't matter.  Just an idea.

I'll make a helper function that handles this. It's the main point of
unpleasantness currently.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:673: return
CreateOfferOrAnswer(false);
On 2017/03/20 18:23:17, pthatcher1 wrote:
> It seems like this would be a little more clear as:
> 
> rtc::scoped_refptr<MockCreateSessionDescriptionObserver> observer(...));
> pc()->CreateOffer(observer, offer_answer_options_);
> return WaitForDescriptionFromObserver(observer);
> 
> And then move the common code into WaitForDescriptionFromObserver.
> 
> But it's just test code, so maybe it doesn't matter.

Done.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1066:
std::unique_ptr<PeerConnectionWrapper> callee_pc_wrapper_;
On 2017/03/20 18:23:17, pthatcher1 wrote:
> I think you ought to just call this caller_ and callee_.

Done.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1090: }),
On 2017/03/20 18:23:17, pthatcher1 wrote:
> This is repeated many times.  Does it make sense to make a method for it? 
Then
> you'd have 
> 
> 
> EXPECT_TRUE_WAIT(ReceivedPackets(caller), kMaxWaitForFramesMs)
> EXPECT_TRUE_WAIT(ReceivedPackets(callee), kMaxWaitForFramesMs)

I did something similar, see new patchset.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1142: PeerConnectionWrapper*
receiver) {
On 2017/03/20 18:23:17, pthatcher1 wrote:
> Could probably just call it "TestDtmf".  I'm not sure what "between" adds.

I'll call it TestDtmfFromSenderToReceiver to hint at the meaning of the
arguments.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1498: // not just assumed.
On 2017/03/20 18:23:17, pthatcher1 wrote:
> Perhaps make constants for kUnrotatedAspectRatio and kRotatedAspectRatio and
use
> those here and move the comment about "why 4:3" up to where they are defined.

I don't think that helps, since ultimately the constant should be removed and
the test should contain the 4:3 value explicitly, both when setting up the video
track and for the assertion.

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:1800: });
On 2017/03/20 18:23:17, pthatcher1 wrote:
> Would it be worth giving this a named method? 
> callee_pc_wrapper()->SetReceivedSdpMunger(MakeSpecCompliantMaxBundle) would
look
> nice.

Done.

[pthatcher1, 2017-03-25 03:38:34]

lgtm

Very Nice

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:209: return client;
On 2017/03/23 04:46:25, Taylor Brandstetter wrote:
> On 2017/03/20 18:23:17, pthatcher1 wrote:
> > This is repeated 4 times.  A simple Create method that takes all 7
parameters
> > and does the "delete if Init failed" part might make sense.
> 
> That's what we had before which I intentionally changed because it results in
> test code that looks like:
> 
> CreatePeerConnection(something, nullptr, nullptr, something, nullptr, nullptr,
> nullptr);
> 
> My goal was prioritizing the readability of the tests, rather than reducing
code
> duplication. Though this could be cleaned up further by using the whole
> "builder" pattern. I'll leave a todo.

What I mean was to have the 7-parameter version and then call
CreatePeerConnection(something, nullptr, nullptr, something, nullptr, nullptr,
nullptr); in the specialized methods.  You would have the exact same test code,
but just less duplication in the helper methods.   The builder pattern is fine
too.

[Taylor Brandstetter, 2017-03-27 17:12:16]

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_integrationtest.cc (right):

https://codereview.webrtc.org/2738353003/diff/210001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_integrationtest.cc:209: return client;
On 2017/03/25 03:38:34, pthatcher1 wrote:
> On 2017/03/23 04:46:25, Taylor Brandstetter wrote:
> > On 2017/03/20 18:23:17, pthatcher1 wrote:
> > > This is repeated 4 times.  A simple Create method that takes all 7
> parameters
> > > and does the "delete if Init failed" part might make sense.
> > 
> > That's what we had before which I intentionally changed because it results
in
> > test code that looks like:
> > 
> > CreatePeerConnection(something, nullptr, nullptr, something, nullptr,
nullptr,
> > nullptr);
> > 
> > My goal was prioritizing the readability of the tests, rather than reducing
> code
> > duplication. Though this could be cleaned up further by using the whole
> > "builder" pattern. I'll leave a todo.
> 
> What I mean was to have the 7-parameter version and then call
> CreatePeerConnection(something, nullptr, nullptr, something, nullptr, nullptr,
> nullptr); in the specialized methods.  You would have the exact same test
code,
> but just less duplication in the helper methods.   The builder pattern is fine
> too.

Oh; that makes more sense. Sorry I misunderstood.

[Taylor Brandstetter, 2017-03-29 21:45:41]

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

[commit-bot: I haz the power, 2017-03-29 21:45:52]

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

[Taylor Brandstetter, 2017-03-29 21:46:00]

Description was changed from

==========
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.
==========

to

==========
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.

TBR=stefan@webrtc.org
==========

[Taylor Brandstetter, 2017-03-29 21:46:37]

Description was changed from

==========
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.

TBR=stefan@webrtc.org
==========

to

==========
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.
==========

[commit-bot: I haz the power, 2017-03-29 21:51:21]

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

[commit-bot: I haz the power, 2017-03-29 21:51:23]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15570)

[Taylor Brandstetter, 2017-03-29 23:38:48]

Description was changed from

==========
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.
==========

to

==========
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
==========

[Taylor Brandstetter, 2017-03-29 23:40:33]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-03-29 23:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2738353003/#ps270001 (title:
"Fixing issues caught by trybots.")

[commit-bot: I haz the power, 2017-03-29 23:40:38]

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, 2017-03-29 23:47:47]

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

[commit-bot: I haz the power, 2017-03-29 23:47:48]

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/buil...)

[Taylor Brandstetter, 2017-03-30 04:00:45]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-03-30 04:00:55]

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, 2017-03-30 04:08:17]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1490846445550600,
"parent_rev": "7735b1e932b439041238c1b68ae24d0529ba6828", "commit_rev":
"1dcb16409a3e82518ef852ec5614f87dff04f248"}

[commit-bot: I haz the power, 2017-03-30 04:08:20]

Description was changed from

==========
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
==========

to

==========
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/+/1dcb16409a3e82518ef852ec5...
==========

[commit-bot: I haz the power, 2017-03-30 04:08:21]

Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/external/webrtc/+/1dcb16409a3e82518ef852ec5...