Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050153003:
Update Unit test for AppRTCMobile app (Closed)
|
Created:
4 years, 6 months ago by afedor Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd a GN target for unit tests, get them working again and added a test.
BUG=webrtc:3417
Committed: https://crrev.com/bcc5d87f0932e96efdd90dc7fcc50ebab301ce62
Cr-Commit-Position: refs/heads/master@{#14959}
Patch Set 1 #Patch Set 2 : Rebase to current master #
Total comments: 10
Patch Set 3 : Update tests for AppRTCMobile app #
Total comments: 8
Patch Set 4 : Updates based on comments #
Total comments: 1
Patch Set 5 : Wrap test code build. Use containsString for SDP check #Patch Set 6 : Remove unneeded/undefined deps #Patch Set 7 : Fixes to pass tests on iOS simulator #
Total comments: 1
Patch Set 8 : Add test Info.plist #Patch Set 9 : Use existing Info.plist #
Total comments: 1
Patch Set 10 : Various requested cleanups #
Total comments: 9
Patch Set 11 : More cleanup requests #
Total comments: 5
Patch Set 12 : More styling #
Messages
Total messages: 58 (21 generated)
Description was changed from ========== Add local video view support to the AppRTCDemo app Also fixed the size of the video views, otherwise the window tends to suddenly change size every time WebRTC changes the streaming resolution. I'm not a UI guy, so I just made it work, not made it look pretty. Also got the ARDAppClient unit tests working and added a test. BUG=3417 ========== to ========== Add local video view support to the (Mac) AppRTCDemo app Also fixed the size of the video views, otherwise the window tends to suddenly change size every time WebRTC changes the streaming resolution. I'm not a UI guy, so I just made it work, not made it look pretty. Also got the ARDAppClient unit tests working and added a test. BUG=3417 ==========
adam.fedor@gmail.com changed reviewers: + tkchin@webrtc.org
Followup to the previous patch enabling AVFoundation support on Mac. FYI, curiously, the test I added passes even if there is no camera present. But perhaps that's the subject for a change in the SDK?
Updated due to a conflict with the current master. Any update on if this is the right approach?
tkchin@webrtc.org changed reviewers: + kthelgason@webrtc.org
sorry for the delay, I hadn't seen the mails If you check the reviewer name you can often ping reviewers directly as well (e.g. tkchin@webrtc.org) kthelgason has done some work here recently so this might be obsolete, but the unit test portion could still be valuable https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm (left): https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:306: NSString *expectedSdp = @("m=video 9 RTP/SAVPF 120 100 116 117 96\n" don't know how this got here but it's intended to be a single string https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:84: NSDate *startDate = [NSDate date]; Probably better to use media timing here for accuracy CACurrentMediaTime https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:89: error = [NSError errorWithDomain:@"ARDAppClient" make domain a global constant. org.webrtc.ARDAppClientTests https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:89: error = [NSError errorWithDomain:@"ARDAppClient" Not sure we need this error. We'd want to just do a test assertion since this error isn't going to be checked really. https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:90: code:101 likewise, global constant kARDAppClientTestsExpectationTimeoutError https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:209: connectedHandler:(void (^)(NSInvocation *))connectedHandler { would rather not be passing NSInvocation around to avoid use of reflection beyond what's needed for ocmock https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:227: connectedHandler(invocation); I feel like this should be a different handler instead of overloading the same one. Then you get to avoid the invocation check as well. https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:321: messageHandler:^(ARDSignalingMessage *message) { fix indentation https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:342: @interface NSString (WebRTCTests) I don't think we need an extra category for this since it's a single line.
On 2016/06/30 20:03:51, afedor wrote: > Updated due to a conflict with the current master. Any update on if this is the > right approach? You should try to rebase this as I think most of this is obsoleted by recent changes upstream. Though as tkchin@ pointed out the tests would be valuable to have. If you still want to pursue this we would certainly appreciate it. See my CL here for reference: https://codereview.webrtc.org/2283743003/
Also, the BUG field should be webrtc:3417, otherwise this refers to a chromium bug.
Description was changed from ========== Add local video view support to the (Mac) AppRTCDemo app Also fixed the size of the video views, otherwise the window tends to suddenly change size every time WebRTC changes the streaming resolution. I'm not a UI guy, so I just made it work, not made it look pretty. Also got the ARDAppClient unit tests working and added a test. BUG=3417 ========== to ========== Add local video view support to the (Mac) AppRTCDemo app Also fixed the size of the video views, otherwise the window tends to suddenly change size every time WebRTC changes the streaming resolution. I'm not a UI guy, so I just made it work, not made it look pretty. Also got the ARDAppClient unit tests working and added a test. BUG=webrtc:3417 ==========
Description was changed from ========== Add local video view support to the (Mac) AppRTCDemo app Also fixed the size of the video views, otherwise the window tends to suddenly change size every time WebRTC changes the streaming resolution. I'm not a UI guy, so I just made it work, not made it look pretty. Also got the ARDAppClient unit tests working and added a test. BUG=webrtc:3417 ========== to ========== Add a GN target for unit tests, get them working again and added a test. BUG=webrtc:3417 ==========
Updated to just include the Unit tests now. Comments?
kthelgason@webrtc.org changed reviewers: + denicija@webrtc.org
This lgtm. Thanks Adam. https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:381: ] This should be the declaration of deps at the top of the target block.
Few comments to address https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (left): https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:88: NSAssert(NO, @"Expectation timed out."); By removing the NSAssert here you'll break the `testSession` test. So either bring back the assertion here or make sure to add asserts to all the places where this method is called (i.e line 298) I personally, like this approach better where the validation responsibility is moved to the caller of the method. https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:89: error = [NSError errorWithDomain:@"ARDAppClient" I think you can use: error = [[NSError alloc] init]; I doubt that there will be any need in validating the actual error down the line. https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:317: - (void)testSession_ShouldGetLocalVideoTrackCallback { Please remove the underscore from the name of the method. I know that some developers use it to increase readability but we don't use it in our codebase. https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:330: } connectedHandler:^(NSInvocation *invocation){ Nit: move connectedHandler parameter to next line https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:370: Nit: remove extra line https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:381: EXPECT_TRUE([h264Desc.description webrtc_containsString: expectedSdpLines[0]] There seems to be no apparent change in the ARDSDPUtils class that might have broken this check. I'm curious what value does h264Desc have when you run the test?
Updated https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:381: EXPECT_TRUE([h264Desc.description webrtc_containsString: expectedSdpLines[0]] On 2016/10/17 11:33:31, denicija-webrtc wrote: > There seems to be no apparent change in the ARDSDPUtils class that might have > broken this check. > I'm curious what value does h264Desc have when you run the test? I get: "RTCSessionDescription:\n offer\n m=video 9 RTP/SAVPF 120 100 116 117 96\n a=rtpmap:120 H264/90000\n" so I coud test for that, or just change isEqualToString: to hasSuffix:, but I though I'd just test for the individual lines, since AFAIK the SDP doesn't have to be in a certain order, and those lines are all I need to verify.
On 2016/10/19 03:10:47, afedor wrote: > ... but I > though I'd just test for the individual lines, since AFAIK the SDP doesn't have > to be in a certain order, and those lines are all I need to verify. The attribute must come after the media description line, otherwise it's interpreted as a session-level attribute.
https://codereview.webrtc.org/2050153003/diff/60001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2050153003/diff/60001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:386: } Both the config and the target should be wrapped in if (rtc_include_tests) { ... }
OK. Also switched to using just 'containsString' for the SDP test
On 2016/10/19 14:29:50, afedor wrote: > OK. > > Also switched to using just 'containsString' for the SDP test lgtm. Thanks for this CL afedor@. Nicely done!
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11653) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11675) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/13894) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19233) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/17911)
Tested on iOS, Hopefully should pass now
Sorry, few more fixes to pass tests on IOS simulator. Also, FYI [[NSError alloc] init] is not allowed since it produces an invalid NSError instance. So I changed it back.
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Thanks Fedor, this is great! I just have one minor comment. I'd still like tkchin@ to review this as well. https://codereview.chromium.org/2050153003/diff/120001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.chromium.org/2050153003/diff/120001/webrtc/examples/BUILD.... webrtc/examples/BUILD.gn:363: if (rtc_include_tests) { This needs a custom plist to work with the iOS 10 SDK. See the AppRTCMobile target in the same file.
Add Info.plist for tests
On 2016/10/25 15:10:24, afedor wrote: > Add Info.plist for tests Sorry, I wasn't clear enough in my comment above. We already have an Info.plist file for this purpose, you just have to specify it in the target as is done with AppRTCMobile. Thanks!
On 2016/10/26 08:31:14, kthelgason wrote: > On 2016/10/25 15:10:24, afedor wrote: > > Add Info.plist for tests > > Sorry, I wasn't clear enough in my comment above. We already have an Info.plist > file for this purpose, you just have to specify it in the target as is done with > AppRTCMobile. > > Thanks! Wouldn't that one have the wrong Bundle Identifier?
On 2016/10/26 13:21:03, afedor wrote: > Wouldn't that one have the wrong Bundle Identifier? I'm not sure that matters. We already reuse that plist for other targets, and I prefer to not have a bunch of them that we have to update when something changes.
Uses the existing Info.plist
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2050153003/#ps160001 (title: "Use existing Info.plist")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9642)
On 2016/10/28 14:35:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9642) Thanks Fedor. We still need an lgtm from an owner for these. Ping tkchin@
On 2016/10/28 14:35:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9642) Thanks Fedor. We still need an lgtm from an owner for these. Ping tkchin@
On 2016/10/28 14:36:43, kthelgason wrote: > On 2016/10/28 14:35:22, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > presubmit on master.tryserver.webrtc (JOB_FAILED, > > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9642) > > Thanks Fedor. We still need an lgtm from an owner for these. > Ping tkchin@ I don't think my initial comments were addressed.
Sorry, I must have missed those. Not sure I completely understood your comments, so please check again. https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/tests/ARDAppClientTest.mm:209: connectedHandler:(void (^)(NSInvocation *))connectedHandler { On 2016/09/29 11:14:40, tkchin_webrtc wrote: > would rather not be passing NSInvocation around to avoid use of reflection > beyond what's needed for ocmock How about just passing back the selector name?
https://codereview.webrtc.org/2050153003/diff/160001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2050153003/diff/160001/webrtc/examples/BUILD.gn... webrtc/examples/BUILD.gn:368: rtc_test("test_apprtcmobile") { nit: apprtcmobile_tests https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:12: #import <QuartzCore/CoreAnimation.h> alphabetize https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:61: handler:(void (^)(BOOL didError))handler; This was meant to mirror XCTest interfaces so make our transition to XCTestCase easier. Please don't change to BOOL. https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:86: BOOL didErrorExp = NO; CFTimeInterval change looks good, but please return an error instead of a bool. https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:208: connectedHandler:(void (^)(NSString *))connectedHandler { instead of passing the name of the method, just introduce a new event handler e.g. ... messageHandler: connectedHandler: localVideoTrackHandler: This avoids all the selector magic and makes it more obvious what's going on. https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:322: [self expectationWithDescription:@"Caller PC connected."]; indent 4 https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:328: messageHandler:^(ARDSignalingMessage *message) { please follow formatting as in other methods do not use git cl format for ObjC++ files https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:336: [[RTCMediaConstraints alloc] initWithMandatoryConstraints:nil indent 4 https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:370: EXPECT_TRUE([h264Desc.description rangeOfString: expectedSdp].location != NSNotFound); nit: no space after : EXPECT_NE?
https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:61: handler:(void (^)(BOOL didError))handler; On 2016/11/07 19:14:21, tkchin_webrtc wrote: > This was meant to mirror XCTest interfaces so make our transition to XCTestCase > easier. Please don't change to BOOL. OK, but to maintain the interface, I should also probably change the signature back to use NSTimeInterval
On 2016/11/07 20:25:25, afedor wrote: > https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): > > https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:61: handler:(void > (^)(BOOL didError))handler; > On 2016/11/07 19:14:21, tkchin_webrtc wrote: > > This was meant to mirror XCTest interfaces so make our transition to > XCTestCase > > easier. Please don't change to BOOL. > > OK, but to maintain the interface, I should also probably change the signature > back to use NSTimeInterval Sounds good to me.
On 2016/11/07 20:38:29, tkchin_webrtc wrote: > On 2016/11/07 20:25:25, afedor wrote: > > > https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... > > File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): > > > > > https://codereview.webrtc.org/2050153003/diff/180001/webrtc/examples/objc/App... > > webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:61: handler:(void > > (^)(BOOL didError))handler; > > On 2016/11/07 19:14:21, tkchin_webrtc wrote: > > > This was meant to mirror XCTest interfaces so make our transition to > > XCTestCase > > > easier. Please don't change to BOOL. > > > > OK, but to maintain the interface, I should also probably change the signature > > back to use NSTimeInterval > > Sounds good to me. OK. I also made the other changes in the latest patch set. There were several previous comments about returning NSError in "waitForExpectation...". I don't see another way to do it besides returning a real error. Just throwing an NSAssert does not work as you just crash the test program with an uncaught exception. Unless you wanted a try/catch around the waitForExpectation calls, but that seems awkward.
Just some style nits remaining. https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:270: } connectedHandler:^(void){ nit: ^{ instead of ^(void) { https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:272: } localVideoTrackHandler:^(void){ ditto just ^{ no need for (void) here and elsewhere https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:326: [self expectationWithDescription:@"Caller got local video."]; 2 more spaces (4 from margin of ARDTestExpecation) https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:333: } connectedHandler:^(void) { add spaces after } to align the : for each arg https://codereview.webrtc.org/2050153003/diff/200001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:338: [[RTCMediaConstraints alloc] initWithMandatoryConstraints:nil ditto 2 more spaces
OK. Other styling issues updated.
On 2016/11/07 21:09:50, afedor wrote: > OK. Other styling issues updated. lgtm!
The CQ bit was checked by tkchin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2050153003/#ps220001 (title: "More styling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add a GN target for unit tests, get them working again and added a test. BUG=webrtc:3417 ========== to ========== Add a GN target for unit tests, get them working again and added a test. BUG=webrtc:3417 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add a GN target for unit tests, get them working again and added a test. BUG=webrtc:3417 ========== to ========== Add a GN target for unit tests, get them working again and added a test. BUG=webrtc:3417 Committed: https://crrev.com/bcc5d87f0932e96efdd90dc7fcc50ebab301ce62 Cr-Commit-Position: refs/heads/master@{#14959} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/bcc5d87f0932e96efdd90dc7fcc50ebab301ce62 Cr-Commit-Position: refs/heads/master@{#14959} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
