Don't call unconfigureWebRTCSession if setConfiguration fails. webrtc:7471

webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm

Index: webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm
diff --git a/webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm b/webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm
index 3f55077ed213a1ac6aed75e6befb2def545d7c49..d8937ab211fdb6be3cad67f71d21a90b874fd6b7 100644
--- a/webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm
+++ b/webrtc/modules/audio_device/ios/objc/RTCAudioSessionTest.mm
@@ -9,11 +9,13 @@
  */
 
 #import <Foundation/Foundation.h>
+#import <OCMock/OCMock.h>
 
 #include "webrtc/test/gtest.h"
 
 #import "webrtc/modules/audio_device/ios/objc/RTCAudioSession.h"
 #import "webrtc/modules/audio_device/ios/objc/RTCAudioSession+Private.h"
+#import "webrtc/modules/audio_device/ios/objc/RTCAudioSessionConfiguration.h"
 
 @interface RTCAudioSessionTestDelegate : NSObject <RTCAudioSessionDelegate>
 @end
@@ -185,6 +187,56 @@
   EXPECT_EQ(0, audioSession.activationCount);
 }
 
+// Hack - fixes OCMVerify link error

[tkchin_webrtc, 2017/04/18 17:53:28]

what is the link error?

[jtt_webrtc, 2017/04/18 18:53:54]

// Link error is: Undefined symbols for architecture i386:
// "OCMMakeLocation(objc_object*, char const*, int)", referenced from:
// -[RTCAudioSessionTest testConfigureWebRTCSession] in RTCAudioSessionTest.o
// ld: symbol(s) not found for architecture i386
// REASON: https://github.com/erikdoe/ocmock/issues/238

[tkchin_webrtc, 2017/04/18 21:06:33]

Was that fixed upstream? Can we update ocmock?

+OCMLocation *OCMMakeLocation(id testCase, const char *fileCString, int line){
+  return [OCMLocation locationWithTestCase:testCase file:[NSString

[tkchin_webrtc, 2017/04/18 17:53:28]

align :

[jtt_webrtc, 2017/04/18 18:53:54]

Done.

+      stringWithUTF8String:fileCString] line:line];
+}
+
+- (void)testConfigureWebRTCSession {
+  NSError *error;

[tkchin_webrtc, 2017/04/18 17:53:28]

nit: init to nil

[jtt_webrtc, 2017/04/18 18:53:54]

Defaults to nil. Will add it anyway.

+
+  void (^setActiveBlock)(NSInvocation *invocation) = ^(NSInvocation *invocation) {
+    __autoreleasing NSError **retError;

[tkchin_webrtc, 2017/04/18 17:53:28]

Do we really need to specify autoreleasing?

[jtt_webrtc, 2017/04/18 18:53:54]

Yes, otherwise it'll throw a build error.

[tkchin_webrtc, 2017/04/18 21:06:33]

what's the build error? (for my education)

+    [invocation getArgument:&retError atIndex:4];
+    *retError = [NSError errorWithDomain:@"AVAudioSession" code:561017449 userInfo:nil];

[henrika_webrtc, 2017/04/18 18:14:11]

Where does these details come from?

[jtt_webrtc, 2017/04/18 18:53:54]

Acknowledged.

+    BOOL failure = NO;
+    [invocation setReturnValue:&failure];
+  };
+
+  id mockAVAudioSession = OCMPartialMock([AVAudioSession sharedInstance]);
+  OCMExpect([[mockAVAudioSession ignoringNonObjectArgs]

[tkchin_webrtc, 2017/04/18 17:53:28]

nit: stub everything you need first, and then place expectations before
callsites

[jtt_webrtc, 2017/04/18 18:53:54]

Done.

+      setActive:YES withOptions:0 error:((NSError __autoreleasing **)[OCMArg anyPointer])]).
+      andDo(setActiveBlock);
+  OCMStub([[mockAVAudioSession ignoringNonObjectArgs]
+      setActive:YES withOptions:0 error:((NSError __autoreleasing **)[OCMArg anyPointer])]).
+      andDo(setActiveBlock);
+
+  id mockAudioSession = OCMPartialMock([RTCAudioSession sharedInstance]);
+  OCMExpect([mockAudioSession session]).andReturn(mockAVAudioSession);
+  OCMStub([mockAudioSession session]).andReturn(mockAVAudioSession);
+
+  RTCAudioSession *audioSession = mockAudioSession;
+  EXPECT_EQ(0, audioSession.activationCount);
+  [audioSession lockForConfiguration];
+  EXPECT_TRUE([audioSession checkLock:nil]);
+  // Force failure, so activationCount should remain 0

[tkchin_webrtc, 2017/04/18 17:53:28]

nit: this isn't technically forcing a failure - the failure is forced by the
setActive block above. Clarify the comment.

[jtt_webrtc, 2017/04/18 18:53:54]

Done.

+  EXPECT_FALSE([audioSession configureWebRTCSession:&error]);
+  EXPECT_EQ(0, audioSession.activationCount);
+
+  id session = audioSession.session;
+  EXPECT_EQ(session, mockAVAudioSession);
+  EXPECT_EQ(FALSE, [mockAVAudioSession setActive:YES withOptions:0 error:&error]);

[tkchin_webrtc, 2017/04/18 17:53:28]

don't use FALSE -> NO

[jtt_webrtc, 2017/04/18 18:53:54]

Done.

+  [audioSession unlockForConfiguration];
+
+  OCMVerify([mockAudioSession session]);
+  OCMVerify([[mockAVAudioSession ignoringNonObjectArgs] setActive:YES withOptions:0 error:&error]);
+  OCMVerify([[mockAVAudioSession ignoringNonObjectArgs] setActive:NO withOptions:0 error:&error]);
+
+  [mockAVAudioSession stopMocking];
+  [mockAudioSession stopMocking];
+}
+
 @end
 
 namespace webrtc {
@@ -229,5 +281,9 @@ TEST_F(AudioSessionTest, AudioSessionActivation) {
   [test testAudioSessionActivation];
 }
 
+TEST_F(AudioSessionTest, ConfigureWebRTCSession) {
+  RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init];

[tkchin_webrtc, 2017/04/18 21:06:33]

Do all these need to be in an autoreleasepool? I don't remember if I made a
change so that all gUnit unit tests automatically have one - I do remember
considering it at one point.

+  [test testConfigureWebRTCSession];
+}
 
 }  // namespace webrtc