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..bfe47f1eb982abbbe1c7b886037803dff6e755d6 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,44 @@ using ::testing::Le;
 
 namespace copresence {
 
-class TestAudioPlayer : public AudioPlayer {
+class TestAudioManager : public AudioManager {

[Daniel Erat, 2014/10/18 21:21:18]

i agree that you should create an pure virtual AudioManagerInterface instead of
selectively overriding methods from the real implementation.

[rkc, 2014/10/20 16:45:37]

Though I agree that there are benefits to having a stub implementation
inheriting from a pure interface, my concern about that pattern aren't with
testing the original code (the AudioManager code will be tested separately in
its own test anyway). The concern is managing two sets of code to have the same
behavior. Currently the AudioManager behavior is simple, but if at a later date,
it starts to get more complex (as it definitely will), we now have to update the
stub to match the behavior of the original AudioManager. This now means we have
two copies of code to maintain, which need to mimic the same behavior, i.e., to
ensure that the stub does that, we really should have tests for the stub also.
This doesn't sound very optimal.

Overriding large parts of the implementation is also undesirable, so I've fixed
this by using the original AudioManager class instead.

I've changed the AudioPlayer and AudioRecorder classes instead to have a pure
interface, and have their stubs inherit from that interface. This behavior
duplication is acceptable since it is at the very lowest layer - these classes
are mimicking what the Audio hardware would have done instead - something we
intentionally want to duplicate.

[Charlie, 2014/10/20 20:06:54]

It's a little hard to talk about this without going through all the specifics.
But the general idea would be that if you're relying on the full behavior of the
AudioManager, then it becomes more of an integration test than a unit test.

The idea is exactly that you *shouldn't* have to duplicate AudioManager. You
just need a simple stub to verify that this class interacts with it correctly.
Yes, as the AudioManagerInterface grows, the stub will become slightly less
simple. But it should not track the complexity of the real class.

[rkc, 2014/10/20 20:17:53]

Its complexity may not grow at the same rate as the implementation, but the
complexity will grow. If it is possible to just use the real class instead, why
not?

[Charlie, 2014/10/20 20:33:05]

We talked about this already. See crbug/424781.

[rkc, 2014/10/21 01:25:04]

I don't necessarily agree with everything said in that bug.

  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&,

[Daniel Erat, 2014/10/18 21:21:18]

it's cleaner to keep Initialize methods in the implementation(s) rather than
putting them in the interface class. different implementations usually need
different things to be passed in at creation.

[rkc, 2014/10/20 16:45:38]

I'm confused about how this would work, maybe I am not reading this right. Could
you explain what you mean here a bit more?

The way I am looking at it; we'll have AudioManagerBase, which will provide the
start/stop play/record methods. Then there will be an AudioManagerImpl, which
will inherit from AudioManagerBase, that will actually implement these methods,
and will provide an additional Initialize method?

Which class will AudioDirectiveHandler hold a pointer to? If
AudioDirectiveHandler holds a pointer to AudioManagerBase, it can't really call
Initialize(), since that method won't exist in the interface. If
AudioDirectiveHandler holds a pointer to AudioManagerImpl, then it can't accept
any other classes that derive from AudioManagerBase as its audio manager.

(since I am actually using the original AudioManager class itself, this point is
moot here, but it still applies to the TestAudioPlayer/Recorder classes).

[Charlie, 2014/10/20 20:06:54]

Let's call it AudioManager or AudioManagerInterface instead of AudioManagerBase.

As for pointer types, one option would be:

scoped_ptr<AudioManager> manager = AudioManagerImpl::Create(...);

But I'm curious to see what Dan suggests.

[rkc, 2014/10/20 20:17:53]

That doesn't fix the issue. The issue is that the container class needs to be
able to initialize the derived class, without knowing that it is a derived class
(which is what allows us to swap out the implementation with the stub). If the
Intialize method or the ::Create method, does not exist in, or is different in,
the Stub versus the Impl class, the container class needs to call a specific
Impl version of the function. This means that the container class now will need
to know which version of the AudioManagerInterface class it is using - the Impl?
or the Stub? 

[Charlie, 2014/10/20 20:33:05]

By "container class", do you mean AudioDirectiveHandler? It's fine to have
AudioDirectiveHandler initialize the Impl class by default. For testing, add
another version of AudioDirectiveHandler::Initialize() that takes in a (stub)
AudioManager. The default Initialize() can then delegate to that one and pass an
AudioManagerImpl.

[rkc, 2014/10/21 01:25:04]

What's the difference between this and just creating and setting the stub audio
manager and then calling the original Initialize(). At this point, I am starting
to wonder how this discussion is actually contributing to the quality of the
code.

[Charlie, 2014/10/21 16:27:09]

set_*_for_testing() methods are sub-optimal because, as Dan points out,
test-specific code clutters the cc file and doesn't belong in the prod binary.

+                          const EncodeTokenCallback&) override {}
+  virtual void StartPlaying(AudioType type) override {
+    set_playing_for_testing(type, true);

[Daniel Erat, 2014/10/18 21:21:18]

it would also be cleaner if this stub class had its own member variables to
track the playing/recording state instead of tagging along with the real
implementation.

if you want to be able to test some of the real AudioManager implementation,
please move the code that actually communicates with audio hardware into a
delegate class. then you can create a real implementation of the delegate for
production and a stub implementation for tests.

[rkc, 2014/10/20 16:45:37]

The TestAudioPlayer/Recorder are technically delegates to the code that
communicates with the audio hardware.

   }
-  virtual void Stop() override { set_is_playing(false); }
-  virtual void Finalize() override { delete this; }
-
- 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; }
+  virtual void StopPlaying(AudioType type) override {
+    set_playing_for_testing(type, false);
+  }
+  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(TestAudioRecorder);
+  DISALLOW_COPY_AND_ASSIGN(TestAudioManager);
 };
 
 class AudioDirectiveHandlerTest : public testing::Test {
  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 +65,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);
+  }
+
+  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,30 +100,34 @@ 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
 // transmits/receives.
+// TODO(rkc): Write tests to move time back and forth and test functionality.
 
 }  // namespace copresence