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

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

 /*
  *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #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
 
 @implementation RTCAudioSessionTestDelegate
 
 - (void)audioSessionDidBeginInterruption:(RTCAudioSession *)session {
 }
 
 - (void)audioSessionDidEndInterruption:(RTCAudioSession *)session
@@ skipping 151 matching lines @@
 
 - (void)testAudioSessionActivation {
   RTCAudioSession *audioSession = [RTCAudioSession sharedInstance];
   EXPECT_EQ(0, audioSession.activationCount);
   [audioSession audioSessionDidActivate:[AVAudioSession sharedInstance]];
   EXPECT_EQ(1, audioSession.activationCount);
   [audioSession audioSessionDidDeactivate:[AVAudioSession sharedInstance]];
   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 {
 
 class AudioSessionTest : public ::testing::Test {
  protected:
   void TearDown() {
     RTCAudioSession *session = [RTCAudioSession sharedInstance];
     for (id<RTCAudioSessionDelegate> delegate : session.delegates) {
       [session removeDelegate:delegate];
@@ skipping 24 matching lines @@
 TEST_F(AudioSessionTest, RemoveDelegateOnDealloc) {
   RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init];
   [test testRemoveDelegateOnDealloc];
 }
 
 TEST_F(AudioSessionTest, AudioSessionActivation) {
   RTCAudioSessionTest *test = [[RTCAudioSessionTest alloc] init];
   [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