Redesign the copresence audio handlers.

components/copresence/handlers/audio/audio_directive_handler_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/copresence/handlers/audio/audio_directive_handler.h"
 
 #include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
-#include "components/copresence/mediums/audio/audio_player.h"
-#include "components/copresence/mediums/audio/audio_recorder.h"
-#include "components/copresence/test/audio_test_support.h"
-#include "media/base/audio_bus.h"
+#include "components/copresence/mediums/audio/audio_manager.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
 using ::testing::Le;
 
 namespace copresence {
 
-class TestAudioPlayer : public AudioPlayer {
+class TestAudioManager : public AudioManager {

[Charlie, 2014/10/17 22:42:25]

We probably shouldn't be subclassing production code to create a mock for
testing. This makes it unclear when the test uses the production behavior/state,
and when it uses the mock behavior. The gMock guide suggests mocking against
interfaces:

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

See also:

http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes

So we would have an AudioManager interface, and the real implementation would
live in AudioManagerImpl. This forces you to call out exactly the AudioManager
behavior you're depending on, and would also allow you to remove the
set_*_for_testing methods.

[rkc, 2014/10/18 00:21:54]

I agree that deriving a mock from a production class has pitfalls, but there are
plenty of cases where deriving from a a concrete class is beneficial -
particularly when you want to actually test the production code in the mocked
class as well.

We can discuss this at length offline if you wish. Leaving as is for now.

  public:
-  TestAudioPlayer() {}
-  virtual ~TestAudioPlayer() {}
+  TestAudioManager() : AudioManager() {}
+  virtual ~TestAudioManager() {}
 
-  // AudioPlayer overrides:
-  virtual void Initialize() override {}
-  virtual void Play(
-      const scoped_refptr<media::AudioBusRefCounted>& /* samples */) override {
-    set_is_playing(true);
+  // AudioManager overrides:
+  virtual void Initialize(const DecodeSamplesCallback&,
+                          const EncodeTokenCallback&) override{};

[Daniel Erat, 2014/10/17 22:25:59]

nit: space after 'override' and no trailing semicolon

[rkc, 2014/10/18 00:21:54]

Done.

+

[Daniel Erat, 2014/10/17 22:25:59]

nit: delete blank line

[rkc, 2014/10/18 00:21:54]

Done.

+  virtual void StartPlaying(AudioType type) override {
+    set_playing_for_testing(type, true);
   }
-  virtual void Stop() override { set_is_playing(false); }
-  virtual void Finalize() override { delete this; }
+

[Daniel Erat, 2014/10/17 22:25:59]

here too

[rkc, 2014/10/18 00:21:54]

Done.

+  virtual void StopPlaying(AudioType type) override {
+    set_playing_for_testing(type, false);
+  }
+

[Daniel Erat, 2014/10/17 22:25:59]

here too

[rkc, 2014/10/18 00:21:54]

Done.

+  virtual void StartRecording(AudioType type) override {
+    set_recording_for_testing(type, true);
+  }
+  virtual void StopRecording(AudioType type) override {
+    set_recording_for_testing(type, false);
+  }
+
+  virtual void SetToken(AudioType, const std::string&) override {}
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(TestAudioPlayer);
-};
-
-class TestAudioRecorder : public AudioRecorder {
- public:
-  TestAudioRecorder() : AudioRecorder(AudioRecorder::DecodeSamplesCallback()) {}
-  virtual ~TestAudioRecorder() {}
-
-  // AudioRecorder overrides:
-  virtual void Initialize() override {}
-  virtual void Record() override { set_is_recording(true); }
-  virtual void Stop() override { set_is_recording(false); }
-  virtual void Finalize() override { delete this; }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestAudioRecorder);
+  DISALLOW_COPY_AND_ASSIGN(TestAudioManager);
 };
 
 class AudioDirectiveHandlerTest : public testing::Test {

[Charlie, 2014/10/17 22:42:25]

I know this is a pain, but we really should add a test with a fake timer to
ensure that the directives are being timed out correctly. This seems not to be
working in the current implementation.

[rkc, 2014/10/18 00:21:54]

This is actually tested in the directive handler; but you're right, there should
be a test for that here too.
Added a TODO to get add this test. It will take non-trivial changes, I'd prefer
to do them in another CL.

[Charlie, 2014/10/18 00:44:11]

I don't follow. This *is* the test for the directive handler.

[rkc, 2014/10/18 00:51:30]

In the AudioDirectiveList tests, sorry.

  public:
   AudioDirectiveHandlerTest()
-      : directive_handler_(new AudioDirectiveHandler(
-            AudioRecorder::DecodeSamplesCallback(),
-            base::Bind(&AudioDirectiveHandlerTest::EncodeToken,
-                       base::Unretained(this)))) {
-    directive_handler_->set_player_audible_for_testing(new TestAudioPlayer());
-    directive_handler_->set_player_inaudible_for_testing(new TestAudioPlayer());
-    directive_handler_->set_recorder_for_testing(new TestAudioRecorder());
+      : directive_handler_(new AudioDirectiveHandler()) {
+    directive_handler_->set_audio_manager_for_testing(
+        make_scoped_ptr(new TestAudioManager()));
   }
   virtual ~AudioDirectiveHandlerTest() {}
 
   void DirectiveAdded() {}
 
  protected:
-  void EncodeToken(const std::string& token,
-                   bool audible,
-                   const AudioDirectiveHandler::SamplesCallback& callback) {
-    callback.Run(
-        token, audible, CreateRandomAudioRefCounted(0x1337, 1, 0x7331));
-  }
-
   copresence::TokenInstruction CreateTransmitInstruction(
       const std::string& token,
       bool audible) {
     copresence::TokenInstruction instruction;
     instruction.set_token_instruction_type(copresence::TRANSMIT);
     instruction.set_token_id(token);
     instruction.set_medium(audible ? AUDIO_AUDIBLE_DTMF
                                    : AUDIO_ULTRASOUND_PASSBAND);
     return instruction;
   }
 
-  copresence::TokenInstruction CreateReceiveInstruction() {
+  copresence::TokenInstruction CreateReceiveInstruction(bool audible) {
     copresence::TokenInstruction instruction;
     instruction.set_token_instruction_type(copresence::RECEIVE);
+    instruction.set_medium(audible ? AUDIO_AUDIBLE_DTMF
+                                   : AUDIO_ULTRASOUND_PASSBAND);
     return instruction;
   }
 
+  bool IsPlaying(AudioType type) {
+    return directive_handler_->audio_manager_->is_playing_for_testing(type);

[Charlie, 2014/10/17 22:42:25]

These _for_testing methods, too, really only belong in the mock/stub.

[rkc, 2014/10/18 00:21:54]

Acknowledged.

+  }
+
+  bool IsRecording(AudioType type) {
+    return directive_handler_->audio_manager_->is_recording_for_testing(type);
+  }
+
   // This order is important. We want the message loop to get created before
   // our the audio directive handler since the directive list ctor (invoked
   // from the directive handler ctor) will post tasks.
   base::MessageLoop message_loop_;
   scoped_ptr<AudioDirectiveHandler> directive_handler_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AudioDirectiveHandlerTest);
 };
 
 TEST_F(AudioDirectiveHandlerTest, Basic) {
   const base::TimeDelta kTtl = base::TimeDelta::FromMilliseconds(9999);
   directive_handler_->AddInstruction(
       CreateTransmitInstruction("token", true), "op_id1", kTtl);
   directive_handler_->AddInstruction(
       CreateTransmitInstruction("token", false), "op_id1", kTtl);
   directive_handler_->AddInstruction(
       CreateTransmitInstruction("token", false), "op_id2", kTtl);
   directive_handler_->AddInstruction(
-      CreateReceiveInstruction(), "op_id1", kTtl);
+      CreateReceiveInstruction(false), "op_id1", kTtl);
   directive_handler_->AddInstruction(
-      CreateReceiveInstruction(), "op_id2", kTtl);
+      CreateReceiveInstruction(true), "op_id2", kTtl);
   directive_handler_->AddInstruction(
-      CreateReceiveInstruction(), "op_id3", kTtl);
+      CreateReceiveInstruction(false), "op_id3", kTtl);
 
-  EXPECT_EQ(true, directive_handler_->player_audible_->IsPlaying());
-  EXPECT_EQ(true, directive_handler_->player_inaudible_->IsPlaying());
-  EXPECT_EQ(true, directive_handler_->recorder_->IsRecording());
+  EXPECT_TRUE(IsPlaying(AUDIBLE));
+  EXPECT_TRUE(IsPlaying(INAUDIBLE));
+  EXPECT_TRUE(IsRecording(AUDIBLE));
+  EXPECT_TRUE(IsRecording(INAUDIBLE));
 
   directive_handler_->RemoveInstructions("op_id1");
-  EXPECT_FALSE(directive_handler_->player_audible_->IsPlaying());
-  EXPECT_EQ(true, directive_handler_->player_inaudible_->IsPlaying());
-  EXPECT_EQ(true, directive_handler_->recorder_->IsRecording());
+  EXPECT_FALSE(IsPlaying(AUDIBLE));
+  EXPECT_TRUE(IsPlaying(INAUDIBLE));
+  EXPECT_TRUE(IsRecording(AUDIBLE));
+  EXPECT_TRUE(IsRecording(INAUDIBLE));
 
   directive_handler_->RemoveInstructions("op_id2");
-  EXPECT_FALSE(directive_handler_->player_inaudible_->IsPlaying());
-  EXPECT_EQ(true, directive_handler_->recorder_->IsRecording());
+  EXPECT_FALSE(IsPlaying(INAUDIBLE));
+  EXPECT_FALSE(IsRecording(AUDIBLE));
+  EXPECT_TRUE(IsRecording(INAUDIBLE));
 
   directive_handler_->RemoveInstructions("op_id3");
-  EXPECT_FALSE(directive_handler_->recorder_->IsRecording());
+  EXPECT_FALSE(IsRecording(INAUDIBLE));
 }
 
 // TODO(rkc): Write more tests that check more convoluted sequences of
 // transmits/receives.
 
 }  // namespace copresence