Add unit test for AudioBus::CreateWrapper()

media/base/audio_bus_unittest.cc

Index: media/base/audio_bus_unittest.cc
diff --git a/media/base/audio_bus_unittest.cc b/media/base/audio_bus_unittest.cc
index 63dcb0cad0aaa85e0b47dc41c92df289a04d3828..4a007f9b093485b707d532ef162ce6336c3a32aa 100644
--- a/media/base/audio_bus_unittest.cc
+++ b/media/base/audio_bus_unittest.cc
@@ -132,6 +132,13 @@ TEST_F(AudioBusTest, CreateUsingAudioParameters) {
   VerifyReadWriteAndAlignment(bus.get());
 }
 
+// Verify CreateWrapper(...) method works as advertised.
+TEST_F(AudioBusTest, CreateWrapper) {
+  std::unique_ptr<AudioBus> bus = AudioBus::CreateWrapper(kChannels);
+  EXPECT_EQ(kChannels, bus->channels());
+  EXPECT_EQ(0, bus->frames());
+}
+
 // Verify an AudioBus created via wrapping a vector works as advertised.
 TEST_F(AudioBusTest, WrapVector) {
   data_.reserve(kChannels);
@@ -172,6 +179,28 @@ TEST_F(AudioBusTest, WrapMemory) {
             data.get() + data_size / sizeof(*data.get()));
 }
 
+// Verify SetChannelData(...) method works as advertised.
+TEST_F(AudioBusTest, SetChannelData) {
+  data_.reserve(kChannels);
+  for (int i = 0; i < kChannels; ++i) {
+    data_.push_back(static_cast<float*>(
+        base::AlignedAlloc(sizeof(*data_[i]), AudioBus::kChannelAlignment)));
+  }
+  std::unique_ptr<AudioBus> bus = AudioBus::CreateWrapper(kChannels);
+  for (int i = 0; i < bus->channels(); ++i)
+    bus->SetChannelData(i, data_[i]);
+
+  for (int i = 0; i < bus->channels(); ++i)
+    EXPECT_EQ(bus->channel(i), data_[i]);

[chfremer, 2017/04/27 16:55:14]

Would it make sense to add 

VerifyReadWriteAndAlignment(bus.get());

here?

[Chandan, 2017/04/27 18:43:07]

With number of frames in this test being always zero, a read/write on the
channel data in VerifyReadWriteAndAlignment() doesn't really do anything.

Alignment can be verified but we already have a DCHECK for the same inside
SetChannelData().

IMO, alignment verification in VerifyReadWriteAndAlignment() can be removed as
we either have a DCHECK or CHECK for it in each of the factory methods for
AudioBus creation. WDYT?

[chfremer, 2017/04/27 20:19:36]

Hmm, I feel that for the cases where AudioBus itself does the memory allocation,
it does make sense to verify the alignment in the tests. That is because
AudioBus makes the guarantee in its class-level description comment and the test
code verifies that without assuming anything about the implementation (including
the presence of DCHECKS).

[Chandan, 2017/04/28 06:07:29]

Sure. I will add Alignment verification for this test?

[chfremer, 2017/04/28 16:39:48]

Not needed for the tests involving AudioBus::CreateWrapper(), since here the
allocation does not happen inside AudioBus.

+}
+
+// Verify set_frames(...) method works as advertised.
+TEST_F(AudioBusTest, SetFrames) {
+  std::unique_ptr<AudioBus> bus = AudioBus::CreateWrapper(kChannels);
+  bus->set_frames(kFrameCount);
+  EXPECT_EQ(kFrameCount, bus->frames());

[chfremer, 2017/04/27 16:55:14]

Would it make sense to add 

VerifyReadWriteAndAlignment(bus.get());

here?

[Chandan, 2017/04/27 18:43:07]

With no channel data allocated in this test, verifying read/write/alignment
doesn't really make sense.

[chfremer, 2017/04/27 20:19:36]

True. I guess, when I added the TODO for adding tests, what I had in mind was a
test that verifies the scenario of an AudioBus instance created via
CreateWrapper and then actually being used for something.

Without assuming anything about the implementation, do test run passes of the
three new tests already guarantee that everything works when we do have channel
data?

[Chandan, 2017/04/28 06:07:29]

By 'everything' you mean read/write/alignment?

Implementation of CreateWrapper()/SetChannelData()/set_frames() is such that we
cannot verify 'everything' for each one of these individually, unlike other
factory methods.

In case we want to do that, then I think we need to add another test with all 3
methods together and verify everything w.r.t CreateWrapper.

Please let me know your opinion.

[chfremer, 2017/04/28 16:39:48]

Sorry about my vague language. Yes I meant read/write/alignment.
Excellent. A test with all 3 methods together is exactly what I was looking for.
The three individual tests are probably okay to keep as well, even though they
seem to be close to the threshold of test-worthiness. For example, SetFrames
basically tests only a setter/getter.

[Chandan, 2017/05/02 09:27:06]

Right. With the new test in place, these 3 individual tests do not really make
sense. The new test also verifies channel and frame count. Hence, I have removed
the individual tests. Uploaded patchset 2. PTAL.

+}
+
 // Simulate a shared memory transfer and verify results.
 TEST_F(AudioBusTest, CopyTo) {
   // Create one bus with AudioParameters and the other through direct values to