GN: Tighten up test target visibility + refactorings

GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

The mock files and targets should probably be moved into webrtc/test in
the future, but that's out of the scope of this CL.

BUG=webrtc:5716
NOTRY=True

Review-Url: https://codereview.webrtc.org/2828793003
Cr-Commit-Position: refs/heads/master@{#17863}
Committed: https://chromium.googlesource.com/external/webrtc/+/e0629c045e699a896682edc1c0c67dab0f26428d

[kjellander_webrtc, 2017-04-20 16:34:35]

Description was changed from

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved:

New targets:
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
==========

to

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
==========

[kjellander_webrtc, 2017-04-20 16:34:35]

kjellander@webrtc.org changed reviewers:
+ stefan@webrtc.org

[kjellander_webrtc, 2017-04-20 18:06:16]

Description was changed from

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
==========

to

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
==========

[kjellander_webrtc, 2017-04-20 19:06:35]

Check PS#3 for majority of bots. PS#4 only contains win fix.

[kjellander_webrtc, 2017-04-21 09:13:44]

On 2017/04/20 19:06:35, kjellander_webrtc wrote:
> Check PS#3 for majority of bots. PS#4 only contains win fix.

(the linux_android_rel_ng trybot failure in PS#3 is just a flaky test).

[kjellander_webrtc, 2017-04-21 09:13:56]

Description was changed from

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
==========

to

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
NOTRY=True
==========

[kjellander_webrtc, 2017-04-21 12:10:55]

Description was changed from

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

BUG=webrtc:5716
NOTRY=True
==========

to

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

The mock files and targets should probably be moved into webrtc/test in
the future, but that's out of the scope of this CL.

BUG=webrtc:5716
NOTRY=True
==========

[kjellander_webrtc, 2017-04-21 12:13:59]

Clarified some decisions with comments

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/BUILD.gn
File webrtc/BUILD.gn (left):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/BUILD.gn#oldcode285
webrtc/BUILD.gn:285: "api:rtc_api_unittests",
The removed targets in here are rtc_source_set targets that are already included
in rtc_test targets that are listed here, so they're redundant.

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:233: if (!is_android && !is_ios) {  # Generated targets
makes this check break.
What I mean with this is that when building rtc_unittests for Android and iOS,
there's a bunch of extra targets generated by the templates, which has names
that aren't matched by this.
Covering all those would look spammy and ugly here, so I decided it's easier to
just exclude those platforms, since the main benefit of this is to make it clear
where the test is used and to avoid accidental dependencies.

[stefan-webrtc, 2017-04-21 12:28:28]

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:233: if (!is_android && !is_ios) {  # Generated targets
makes this check break.
On 2017/04/21 12:13:59, kjellander_webrtc wrote:
> What I mean with this is that when building rtc_unittests for Android and iOS,
> there's a bunch of extra targets generated by the templates, which has names
> that aren't matched by this.
> Covering all those would look spammy and ugly here, so I decided it's easier
to
> just exclude those platforms, since the main benefit of this is to make it
clear
> where the test is used and to avoid accidental dependencies.

I think it would be good to describe this in a bit more detail in the comment.

Does it mean these tests won't run on android and ios, or does it just mean that
there will be no visibility restrictions at all?

s/makes/make

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:2025: if (!is_android && !is_ios) {  #
Generated targets makes this check break.
Why twice? :)

[kjellander_webrtc, 2017-04-21 12:48:21]

PTAL

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:233: if (!is_android && !is_ios) {  # Generated targets
makes this check break.
On 2017/04/21 12:28:27, stefan-webrtc wrote:
> On 2017/04/21 12:13:59, kjellander_webrtc wrote:
> > What I mean with this is that when building rtc_unittests for Android and
iOS,
> > there's a bunch of extra targets generated by the templates, which has names
> > that aren't matched by this.
> > Covering all those would look spammy and ugly here, so I decided it's easier
> to
> > just exclude those platforms, since the main benefit of this is to make it
> clear
> > where the test is used and to avoid accidental dependencies.
> 
> I think it would be good to describe this in a bit more detail in the comment.
> 
> Does it mean these tests won't run on android and ios, or does it just mean
that
> there will be no visibility restrictions at all?
> 
> s/makes/make

Done.

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2828793003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:2025: if (!is_android && !is_ios) {  #
Generated targets makes this check break.
On 2017/04/21 12:28:28, stefan-webrtc wrote:
> Why twice? :)

Search-replace error. Fixed :)

[stefan-webrtc, 2017-04-25 08:31:57]

lgtm

[kjellander_webrtc, 2017-04-25 11:02:11]

The CQ bit was checked by kjellander@webrtc.org

[kjellander_webrtc, 2017-04-25 11:02:11]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2828793003/#ps160001 (title:
"Move out fec_test_helper from rtc_include_tests")

[commit-bot: I haz the power, 2017-04-25 11:02:16]

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-04-25 11:04:51]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493118131201630,
"parent_rev": "b8a654c218f1bb6859f2eabf103b09454e4803b2", "commit_rev":
"e0629c045e699a896682edc1c0c67dab0f26428d"}

[commit-bot: I haz the power, 2017-04-25 11:04:55]

Description was changed from

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

The mock files and targets should probably be moved into webrtc/test in
the future, but that's out of the scope of this CL.

BUG=webrtc:5716
NOTRY=True
==========

to

==========
GN: Tighten up test target visibility + refactorings

Make all rtc_source_test target that contains tests that
are included in a test executable only be visible to the
rtc_test target. Doing this exposed a couple of errors and
dependency problems that were resolved. Having this could
have prevented duplicated execution of tests like the case that
was recently fixed by deadbeef@ in
https://codereview.webrtc.org/2820263004

New targets:
* //webrtc/modules/rtp_rtcp:fec_test_helper
* //webrtc/modules/rtp_rtcp:mock_rtp_rtcp
* //webrtc/modules/remote_bitrate_estimator:mock_remote_bitrate_observer

The mock files and targets should probably be moved into webrtc/test in
the future, but that's out of the scope of this CL.

BUG=webrtc:5716
NOTRY=True

Review-Url: https://codereview.webrtc.org/2828793003
Cr-Commit-Position: refs/heads/master@{#17863}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e0629c045e699a896682edc1c...
==========

[commit-bot: I haz the power, 2017-04-25 11:04:56]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/e0629c045e699a896682edc1c...