Add unit test for AudioBus::CreateWrapper()

Add unit test for AudioBus::CreateWrapper()

This CL adds unit test that verifies an AudioBus
created using AudioBus::CreateWrapper().

BUG=None

Review-Url: https://codereview.chromium.org/2845813004
Cr-Commit-Position: refs/heads/master@{#468698}
Committed: https://chromium.googlesource.com/chromium/src/+/99df95e150d6ac9b9c7e7a85a9261d609ae53b71

[Chandan, 2017-04-27 13:41:25]

c.padhi@samsung.com changed reviewers:
+ chfremer@chromium.org, dalecurtis@chromium.org

[Chandan, 2017-04-27 13:41:26]

PTAL. Thank you.

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

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:194: EXPECT_EQ(bus->channel(i), data_[i]);
Would it make sense to add 

VerifyReadWriteAndAlignment(bus.get());

here?

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
Would it make sense to add 

VerifyReadWriteAndAlignment(bus.get());

here?

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

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:194: EXPECT_EQ(bus->channel(i), data_[i]);
On 2017/04/27 16:55:14, chfremer wrote:
> Would it make sense to add 
> 
> VerifyReadWriteAndAlignment(bus.get());
> 
> here?

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?

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
On 2017/04/27 16:55:14, chfremer wrote:
> Would it make sense to add 
> 
> VerifyReadWriteAndAlignment(bus.get());
> 
> here?

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

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

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:194: EXPECT_EQ(bus->channel(i), data_[i]);
On 2017/04/27 18:43:07, Chandan wrote:
> On 2017/04/27 16:55:14, chfremer wrote:
> > Would it make sense to add 
> > 
> > VerifyReadWriteAndAlignment(bus.get());
> > 
> > here?
> 
> 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?

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).

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
On 2017/04/27 18:43:07, Chandan wrote:
> On 2017/04/27 16:55:14, chfremer wrote:
> > Would it make sense to add 
> > 
> > VerifyReadWriteAndAlignment(bus.get());
> > 
> > here?
> 
> With no channel data allocated in this test, verifying read/write/alignment
> doesn't really make sense.

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]

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:194: EXPECT_EQ(bus->channel(i), data_[i]);
On 2017/04/27 20:19:36, chfremer wrote:
> On 2017/04/27 18:43:07, Chandan wrote:
> > On 2017/04/27 16:55:14, chfremer wrote:
> > > Would it make sense to add 
> > > 
> > > VerifyReadWriteAndAlignment(bus.get());
> > > 
> > > here?
> > 
> > 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?
> 
> 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).

Sure. I will add Alignment verification for this test?

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
On 2017/04/27 20:19:36, chfremer wrote:
> On 2017/04/27 18:43:07, Chandan wrote:
> > On 2017/04/27 16:55:14, chfremer wrote:
> > > Would it make sense to add 
> > > 
> > > VerifyReadWriteAndAlignment(bus.get());
> > > 
> > > here?
> > 
> > With no channel data allocated in this test, verifying read/write/alignment
> > doesn't really make sense.
> 
> 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?

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]

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:194: EXPECT_EQ(bus->channel(i), data_[i]);
On 2017/04/28 06:07:29, Chandan wrote:
> On 2017/04/27 20:19:36, chfremer wrote:
> > On 2017/04/27 18:43:07, Chandan wrote:
> > > On 2017/04/27 16:55:14, chfremer wrote:
> > > > Would it make sense to add 
> > > > 
> > > > VerifyReadWriteAndAlignment(bus.get());
> > > > 
> > > > here?
> > > 
> > > 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?
> > 
> > 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).
> 
> Sure. I will add Alignment verification for this test?

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

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
On 2017/04/28 06:07:29, Chandan wrote:
> On 2017/04/27 20:19:36, chfremer wrote:
> > On 2017/04/27 18:43:07, Chandan wrote:
> > > On 2017/04/27 16:55:14, chfremer wrote:
> > > > Would it make sense to add 
> > > > 
> > > > VerifyReadWriteAndAlignment(bus.get());
> > > > 
> > > > here?
> > > 
> > > With no channel data allocated in this test, verifying
read/write/alignment
> > > doesn't really make sense.
> > 
> > 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?
> 
> By 'everything' you mean read/write/alignment?

Sorry about my vague language. Yes I meant 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.

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:20:30]

Description was changed from

==========
Add unit tests for AudioBus::CreateWrapper() and related methods

This CL adds unit tests for CreateWrapper(), SetChannelData() and
set_frames() methods from AudioBus.

BUG=None
==========

to

==========
Add unit test for AudioBus::CreateWrapper()

This CL adds unit test that verifies an AudioBus 
created using AudioBus::CreateWrapper().

BUG=None
==========

[Chandan, 2017-05-02 09:20:30]

c.padhi@samsung.com changed reviewers:
+ a.suchit@chromium.org

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

Please review.

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
File media/base/audio_bus_unittest.cc (right):

https://codereview.chromium.org/2845813004/diff/1/media/base/audio_bus_unitte...
media/base/audio_bus_unittest.cc:201: EXPECT_EQ(kFrameCount, bus->frames());
On 2017/04/28 16:39:48, chfremer wrote:
> On 2017/04/28 06:07:29, Chandan wrote:
> > On 2017/04/27 20:19:36, chfremer wrote:
> > > On 2017/04/27 18:43:07, Chandan wrote:
> > > > On 2017/04/27 16:55:14, chfremer wrote:
> > > > > Would it make sense to add 
> > > > > 
> > > > > VerifyReadWriteAndAlignment(bus.get());
> > > > > 
> > > > > here?
> > > > 
> > > > With no channel data allocated in this test, verifying
> read/write/alignment
> > > > doesn't really make sense.
> > > 
> > > 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?
> > 
> > By 'everything' you mean read/write/alignment?
> 
> Sorry about my vague language. Yes I meant 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.
> 
> 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.

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.

[a.suchit2, 2017-05-02 09:28:50]

The CQ bit was checked by a.suchit@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 09:29:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 11:23:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 11:23:48]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-05-02 16:47:39]

lgtm

[DaleCurtis, 2017-05-02 17:10:20]

lgtm

[Chandan, 2017-05-02 17:25:43]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-02 17:26:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 17:30:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493745943787460,
"parent_rev": "bd3351963fb4da3e31478d841b91a117583f9786", "commit_rev":
"99df95e150d6ac9b9c7e7a85a9261d609ae53b71"}

[commit-bot: I haz the power, 2017-05-02 17:32:35]

Description was changed from

==========
Add unit test for AudioBus::CreateWrapper()

This CL adds unit test that verifies an AudioBus 
created using AudioBus::CreateWrapper().

BUG=None
==========

to

==========
Add unit test for AudioBus::CreateWrapper()

This CL adds unit test that verifies an AudioBus
created using AudioBus::CreateWrapper().

BUG=None

Review-Url: https://codereview.chromium.org/2845813004
Cr-Commit-Position: refs/heads/master@{#468698}
Committed:
https://chromium.googlesource.com/chromium/src/+/99df95e150d6ac9b9c7e7a85a926...
==========

[commit-bot: I haz the power, 2017-05-02 17:32:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/99df95e150d6ac9b9c7e7a85a926...