Redesign the copresence audio handlers.

components/copresence/handlers/audio/audio_directive_handler_unittest.cc

Index: components/copresence/handlers/audio/audio_directive_handler_unittest.cc
diff --git a/components/copresence/handlers/audio/audio_directive_handler_unittest.cc b/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
index 31c5f58adb911f6e5c7ccdeebf939b366de15c86..9864e2c711509a5195f0aab0542c6e549ca5bf88 100644
--- a/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
+++ b/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
@@ -5,11 +5,9 @@
 #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"
 
@@ -18,62 +16,48 @@ 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() {}
-
-  // AudioPlayer overrides:
-  virtual void Initialize() override {}
-  virtual void Play(
-      const scoped_refptr<media::AudioBusRefCounted>& /* samples */) override {
-    set_is_playing(true);
+  TestAudioManager() : AudioManager() {}
+  virtual ~TestAudioManager() {}
+
+  // 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.

- private:
-  DISALLOW_COPY_AND_ASSIGN(TestAudioPlayer);
-};
+  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.

-class TestAudioRecorder : public AudioRecorder {
- public:
-  TestAudioRecorder() : AudioRecorder(AudioRecorder::DecodeSamplesCallback()) {}
-  virtual ~TestAudioRecorder() {}
+  virtual void StartRecording(AudioType type) override {
+    set_recording_for_testing(type, true);
+  }
+  virtual void StopRecording(AudioType type) override {
+    set_recording_for_testing(type, false);
+  }
 
-  // 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; }
+  virtual void SetToken(AudioType, const std::string&) override {}
 
  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) {
@@ -85,12 +69,22 @@ class AudioDirectiveHandlerTest : public testing::Test {
     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.
@@ -110,27 +104,30 @@ TEST_F(AudioDirectiveHandlerTest, Basic) {
   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