Simple tests for Call::SetBitrateConfig.

Simple tests for Call::SetBitrateConfig.

This will enable safer refactoring of SetBitrateConfig when we add methods to control BWE from PeerConnection (https://codereview.chromium.org/2838233002/).

BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2870383003
Cr-Commit-Position: refs/heads/master@{#18187}
Committed: https://chromium.googlesource.com/external/webrtc/+/8c96a148a871a9d0be5cdcf2e548b2f9658f2542

[Zach Stein, 2017-05-10 20:57:13]

Description was changed from

==========
Simple tests for Call::SetBitrateConfig.

BUG=webrtc:7395
==========

to

==========
Simple tests for Call::SetBitrateConfig.

This will enable safer refactoring of SetBitrateConfig when we add methods to
control BWE from PeerConnection (https://codereview.chromium.org/2838233002/).

BUG=webrtc:7395
==========

[Zach Stein, 2017-05-10 20:57:49]

zstein@webrtc.org changed reviewers:
+ ossu@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[Zach Stein, 2017-05-10 20:57:49]

PTAL

[ossu-google, 2017-05-11 15:20:03]

ossu@google.com changed reviewers:
+ ossu@google.com

[ossu-google, 2017-05-11 15:20:03]

lgtm

[ossu, 2017-05-11 15:21:35]

ossu@webrtc.org changed reviewers:
- ossu@google.com

[ossu, 2017-05-11 15:21:59]

Ah, jeez. Multiple accounts...

lgtm still :)

[stefan-webrtc, 2017-05-16 07:46:36]

lgtm

[the sun, 2017-05-16 21:30:11]

https://codereview.webrtc.org/2870383003/diff/1/webrtc/call/call_unittest.cc
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/2870383003/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:312: class CallBitrateTest : public testing::Test {
I don't really see the point of using a fixture here since you nevertheless need
to call CreateCall() from each test case. IMO the fixture and inheritance only
muddles configuration of the test.

You could instead create a little utility class/struct, without SetUp() - do
everything in the ctor, and just put that on the stack in each test case:

class BitrateTestHelper {
  BitrateTestHelper() {
    Clock* clock = Clock::GetRealTimeClock();
    ...
    call_.reset(Call::Create(config, std::move(fake_controller)));
  }

...

TEST_F(CallBitrateTest, SetBitrateConfigWithValidConfigCallsSetBweBitrates) {
  BitrateTestHelper helper;
  ...
  helper.call()->SetBitrateConfig(bitrate_config);
}

etc

[Zach Stein, 2017-05-17 18:00:12]

https://codereview.webrtc.org/2870383003/diff/1/webrtc/call/call_unittest.cc
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/2870383003/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:312: class CallBitrateTest : public testing::Test {
On 2017/05/16 21:30:11, the sun wrote:
> I don't really see the point of using a fixture here since you nevertheless
need
> to call CreateCall() from each test case. IMO the fixture and inheritance only
> muddles configuration of the test.
> 
> You could instead create a little utility class/struct, without SetUp() - do
> everything in the ctor, and just put that on the stack in each test case:
> 
> class BitrateTestHelper {
>   BitrateTestHelper() {
>     Clock* clock = Clock::GetRealTimeClock();
>     ...
>     call_.reset(Call::Create(config, std::move(fake_controller)));
>   }
> 
> ...
> 
> TEST_F(CallBitrateTest, SetBitrateConfigWithValidConfigCallsSetBweBitrates) {
>   BitrateTestHelper helper;
>   ...
>   helper.call()->SetBitrateConfig(bitrate_config);
> }
> 
> etc

The utility struct looks cleaner to me too - thanks for the suggestion!

[Zach Stein, 2017-05-17 18:00:19]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-05-17 18:00:19]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@google.com,
ossu@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2870383003/#ps20001 (title:
"Replace test fixture with utility struct.")

[commit-bot: I haz the power, 2017-05-17 18:00:24]

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-05-17 18:49:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495044019360740,
"parent_rev": "12fa8f444c0e7dec42c37caee457b2f02b0e95aa", "commit_rev":
"8c96a148a871a9d0be5cdcf2e548b2f9658f2542"}

[commit-bot: I haz the power, 2017-05-17 18:49:16]

Description was changed from

==========
Simple tests for Call::SetBitrateConfig.

This will enable safer refactoring of SetBitrateConfig when we add methods to
control BWE from PeerConnection (https://codereview.chromium.org/2838233002/).

BUG=webrtc:7395
==========

to

==========
Simple tests for Call::SetBitrateConfig.

This will enable safer refactoring of SetBitrateConfig when we add methods to
control BWE from PeerConnection (https://codereview.chromium.org/2838233002/).

BUG=webrtc:7395

Review-Url: https://codereview.webrtc.org/2870383003
Cr-Commit-Position: refs/heads/master@{#18187}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8c96a148a871a9d0be5cdcf2e...
==========

[commit-bot: I haz the power, 2017-05-17 18:49:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/8c96a148a871a9d0be5cdcf2e...

[the sun, 2017-05-17 21:54:33]

lgtm