Move iOS tests to XCTest from gtest.

Move iOS tests to XCTest from gtest.

This enables tighter integration with XCode tooling and is a prereq
for adding UI tests.

BUG=webrtc:7150
Review-Url: https://codereview.webrtc.org/2697603002
Cr-Commit-Position: refs/heads/master@{#16609}
Committed: https://chromium.googlesource.com/external/webrtc/+/4065a5762bdb2fd257586d85552206e2e776382f

[kthelgason, 2017-02-13 14:57:12]

Patchset #1 (id:1) has been deleted

[kthelgason, 2017-02-13 15:32:57]

kthelgason@webrtc.org changed reviewers:
+ denicija@webrtc.org, magjed@webrtc.org

[kthelgason, 2017-02-13 15:32:58]

kjellander: FYI

magjed, denicija: Please take a look. The tests are up and running on the bots
as well :)

[magjed_webrtc, 2017-02-13 15:57:36]

Great that you got this working! I only have one comment, see below.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (left):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:66: id
storeMock = [self setupMockStoreWithMediaConstraintString:@"960x480"];
What happened to this line? It's not included in the xctest.

[daniela-webrtc, 2017-02-13 16:08:22]

Some comments and nits that could be addressed if you see fit.
In general lgtm and great work!!! Finally proper XCTest support

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn
File webrtc/examples/BUILD.gn (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn#...
webrtc/examples/BUILD.gn:436: if (is_ios) {
This means that we'll no longer have Mac test target. Perhaps add a TODO with
bug tracking the target on that one?

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClient_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDAppClient_xctest.mm:231: #if
!TARGET_IPHONE_SIMULATOR // Expect to fail on simulator due to no camera support
How was this passing before? :O

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm:19: -
(void)testPreferVideoCodecH264;
nit: We could probably ditch the declaration of test methods. IMO they add
little value to the code, they are not required by the compiler and are tough to
maintain when adding new tests.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (left):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:66: id
storeMock = [self setupMockStoreWithMediaConstraintString:@"960x480"];
On 2017/02/13 15:57:36, magjed_webrtc wrote:
> What happened to this line? It's not included in the xctest.

+1

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:54:
XCTAssertEqual(string, @"640x480");
Use XCTAssertEqualObjects here as well.

[kjellander_webrtc, 2017-02-14 09:04:09]

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

[kjellander_webrtc, 2017-02-14 09:04:10]

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn
File webrtc/examples/BUILD.gn (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn#...
webrtc/examples/BUILD.gn:457: ios_xctest_test("apprtcmobile_tests") {
The rtc_test template in
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/webrtc.gni#232
gets the global configurations //webrtc:common_config and
//webrtc:common_inherited_config added to it, while this doesn't. I suspect that
could affect behavior depending on if code in this target actually uses any of
those defines etc, but going forward I think it's safest if we either:
* create our own rtc_ios_xctest_test template
* add those configurations to each instance of ios_xctest_test.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm:19: -
(void)testPreferVideoCodecH264;
On 2017/02/13 16:08:22, denicija-webrtc wrote:
> nit: We could probably ditch the declaration of test methods. IMO they add
> little value to the code, they are not required by the compiler and are tough
to
> maintain when adding new tests.

Fine with me. If they don't provide any value I don't see why we would have
them.

[kthelgason, 2017-02-14 09:48:46]

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn
File webrtc/examples/BUILD.gn (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn#...
webrtc/examples/BUILD.gn:436: if (is_ios) {
On 2017/02/13 16:08:21, denicija-webrtc wrote:
> This means that we'll no longer have Mac test target. Perhaps add a TODO with
> bug tracking the target on that one?

Good call, thanks. Done.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/BUILD.gn#...
webrtc/examples/BUILD.gn:457: ios_xctest_test("apprtcmobile_tests") {
On 2017/02/14 09:04:10, kjellander_webrtc wrote:
> The rtc_test template in
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/webrtc.gni#232
> gets the global configurations //webrtc:common_config and
> //webrtc:common_inherited_config added to it, while this doesn't. I suspect
that
> could affect behavior depending on if code in this target actually uses any of
> those defines etc, but going forward I think it's safest if we either:
> * create our own rtc_ios_xctest_test template
> * add those configurations to each instance of ios_xctest_test.

Done.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClient_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDAppClient_xctest.mm:231: #if
!TARGET_IPHONE_SIMULATOR // Expect to fail on simulator due to no camera support
On 2017/02/13 16:08:22, denicija-webrtc wrote:
> How was this passing before? :O

It wasn't, it was ifdef'd out in line 395 :)

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSDPUtils_xctest.mm:19: -
(void)testPreferVideoCodecH264;
On 2017/02/14 09:04:10, kjellander_webrtc wrote:
> On 2017/02/13 16:08:22, denicija-webrtc wrote:
> > nit: We could probably ditch the declaration of test methods. IMO they add
> > little value to the code, they are not required by the compiler and are
tough
> to
> > maintain when adding new tests.
> 
> Fine with me. If they don't provide any value I don't see why we would have
> them. 

Done.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (left):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:66: id
storeMock = [self setupMockStoreWithMediaConstraintString:@"960x480"];
On 2017/02/13 16:08:22, denicija-webrtc wrote:
> On 2017/02/13 15:57:36, magjed_webrtc wrote:
> > What happened to this line? It's not included in the xctest.
> 
> +1

storeMock is not used in this test.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (right):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:54:
XCTAssertEqual(string, @"640x480");
On 2017/02/13 16:08:22, denicija-webrtc wrote:
> Use XCTAssertEqualObjects here as well.

I wonder how this worked and the test passed?

[kjellander_webrtc, 2017-02-14 09:57:27]

lgtm with my comment addressed.
Please run at least one trybot per platform as well.

https://codereview.webrtc.org/2697603002/diff/60001/webrtc/webrtc.gni
File webrtc/webrtc.gni (right):

https://codereview.webrtc.org/2697603002/diff/60001/webrtc/webrtc.gni#newcode16
webrtc/webrtc.gni:16: import("//build/config/ios/rules.gni")
Please put this inside a
if (is_ios) {
}
block as well.

[kthelgason, 2017-02-14 09:58:52]

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

[commit-bot: I haz the power, 2017-02-14 09:59:03]

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

[magjed_webrtc, 2017-02-14 10:00:28]

Is the file name *_xctest.mm standard? For example, the name
ARDAppClient_xctest.mm looks weird to me.

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm (left):

https://codereview.webrtc.org/2697603002/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/tests/ARDSettingsModel_xctest.mm:66: id
storeMock = [self setupMockStoreWithMediaConstraintString:@"960x480"];
On 2017/02/14 09:48:46, kthelgason wrote:
> On 2017/02/13 16:08:22, denicija-webrtc wrote:
> > On 2017/02/13 15:57:36, magjed_webrtc wrote:
> > > What happened to this line? It's not included in the xctest.
> > 
> > +1
> 
> storeMock is not used in this test.

No, but doesn't the call to setupMockStoreWithMediaConstraintString affect
_model? I'll let Daniela comment on this.

[kthelgason, 2017-02-14 10:04:21]

On 2017/02/14 10:00:28, magjed_webrtc wrote:
> Is the file name *_xctest.mm standard? For example, the name
> ARDAppClient_xctest.mm looks weird to me.

I agree, but the files have to be named this way so that the test runner finds
them.

> > storeMock is not used in this test.
> 
> No, but doesn't the call to setupMockStoreWithMediaConstraintString affect
> _model? I'll let Daniela comment on this.

Yeah, it mocks the `settingsStore` method on `_model` but that's not important
in this particular test, at least as I understand it.

[daniela-webrtc, 2017-02-14 10:09:18]

On 2017/02/14 10:04:21, kthelgason wrote:
> On 2017/02/14 10:00:28, magjed_webrtc wrote:
> > Is the file name *_xctest.mm standard? For example, the name
> > ARDAppClient_xctest.mm looks weird to me.
> 
> I agree, but the files have to be named this way so that the test runner finds
> them.
> 
> > > storeMock is not used in this test.
> > 
> > No, but doesn't the call to setupMockStoreWithMediaConstraintString affect
> > _model? I'll let Daniela comment on this.
> 
> Yeah, it mocks the `settingsStore` method on `_model` but that's not important
> in this particular test, at least as I understand it.

It also stores the provided string into the store and that is what's being
tested in the second line of the test.
Please bring it back as the test without it is not essentially testing the right
thing.
Also, since it's confusing, we can do like so
__unused id storeMock = [self ...];

that way it will be clear that the storeMock is meant to be unused

[commit-bot: I haz the power, 2017-02-14 10:10:26]

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

[commit-bot: I haz the power, 2017-02-14 10:10:27]

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

[kthelgason, 2017-02-14 10:27:50]

Patchset #4 (id:80001) has been deleted

[kthelgason, 2017-02-14 10:30:22]

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

[commit-bot: I haz the power, 2017-02-14 10:30:26]

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

[kthelgason, 2017-02-14 10:43:07]

On 2017/02/14 10:09:18, denicija-webrtc wrote:
> On 2017/02/14 10:04:21, kthelgason wrote:
> > On 2017/02/14 10:00:28, magjed_webrtc wrote:
> > > Is the file name *_xctest.mm standard? For example, the name
> > > ARDAppClient_xctest.mm looks weird to me.
> > 
> > I agree, but the files have to be named this way so that the test runner
finds
> > them.
> > 
> > > > storeMock is not used in this test.
> > > 
> > > No, but doesn't the call to setupMockStoreWithMediaConstraintString affect
> > > _model? I'll let Daniela comment on this.
> > 
> > Yeah, it mocks the `settingsStore` method on `_model` but that's not
important
> > in this particular test, at least as I understand it.
> 
> It also stores the provided string into the store and that is what's being
> tested in the second line of the test.
> Please bring it back as the test without it is not essentially testing the
right
> thing.
> Also, since it's confusing, we can do like so
> __unused id storeMock = [self ...];
> 
> that way it will be clear that the storeMock is meant to be unused

Thanks for clarifying! Done.

[kthelgason, 2017-02-14 10:43:09]

On 2017/02/14 10:09:18, denicija-webrtc wrote:
> On 2017/02/14 10:04:21, kthelgason wrote:
> > On 2017/02/14 10:00:28, magjed_webrtc wrote:
> > > Is the file name *_xctest.mm standard? For example, the name
> > > ARDAppClient_xctest.mm looks weird to me.
> > 
> > I agree, but the files have to be named this way so that the test runner
finds
> > them.
> > 
> > > > storeMock is not used in this test.
> > > 
> > > No, but doesn't the call to setupMockStoreWithMediaConstraintString affect
> > > _model? I'll let Daniela comment on this.
> > 
> > Yeah, it mocks the `settingsStore` method on `_model` but that's not
important
> > in this particular test, at least as I understand it.
> 
> It also stores the provided string into the store and that is what's being
> tested in the second line of the test.
> Please bring it back as the test without it is not essentially testing the
right
> thing.
> Also, since it's confusing, we can do like so
> __unused id storeMock = [self ...];
> 
> that way it will be clear that the storeMock is meant to be unused

Thanks for clarifying! Done.

[commit-bot: I haz the power, 2017-02-14 12:38:52]

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

[commit-bot: I haz the power, 2017-02-14 12:38:53]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-02-14 12:48:51]

lgtm

[kthelgason, 2017-02-14 12:56:18]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2017-02-14 12:56:18]

The patchset sent to the CQ was uploaded after l-g-t-m from denicija@webrtc.org,
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2697603002/#ps100001 (title:
"fix nits")

[commit-bot: I haz the power, 2017-02-14 12:56:21]

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-02-14 12:58:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487076978208070,
"parent_rev": "e374e0139bc387b890fa7164ed3a9929e06bbb25", "commit_rev":
"4065a5762bdb2fd257586d85552206e2e776382f"}

[commit-bot: I haz the power, 2017-02-14 12:58:59]

Description was changed from

==========
Move iOS tests to XCTest from gtest.

This enables tighter integration with XCode tooling and is a prereq
for adding UI tests.

BUG=webrtc:7150
==========

to

==========
Move iOS tests to XCTest from gtest.

This enables tighter integration with XCode tooling and is a prereq
for adding UI tests.

BUG=webrtc:7150

Review-Url: https://codereview.webrtc.org/2697603002
Cr-Commit-Position: refs/heads/master@{#16609}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4065a5762bdb2fd257586d855...
==========

[commit-bot: I haz the power, 2017-02-14 12:59:00]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/4065a5762bdb2fd257586d855...