Add test coverage for CrasAudioHandler.

Add test coverage for CrasAudioHandler.
Separate CrasAudioClientStubImpl out of cras_audio_client.cc into its own files.
Remove MockCrasAudioHandler.

BUG=233266
R=stevenjb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214224

[jennyz, 2013-07-22 22:23:00]

rahul, PTAL the cl, thanks! stevenjb is out of office until Thursday.

[jennyz, 2013-07-23 20:19:02]

james, both rkc@ and stevenjb@ seems not here this week, would you please review
my cl?

[rkc, 2013-07-23 20:37:42]

Mostly looks fine - I think we can do away with having a stub preferences
handler though, there are better ways to handle it. I'd rather that we keep our
write to preferences code versus save the settings to a map since that gives us
more test coverage.


Still looking over the unit tests in detail.

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/audio_devices_...
File chromeos/audio/audio_devices_pref_handler_stub.h (right):

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/audio_devices_...
chromeos/audio/audio_devices_pref_handler_stub.h:20: class CHROMEOS_EXPORT
AudioDevicesPrefHandlerStub
Instead of stubbing this class out, can we instead provide the ability to use a
mock/stub preferences to the main AudioDevicesPrefHandler class?

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:167: device_list->clear();
A DCHECK(device_list) might be appropriate right before the ->clear().

[rkc, 2013-07-23 20:50:53]

Tests look good overall. There are a few missing pieces that I noticed though
but should be relatively simple to add.

. We want to add negative tests with invalid devices to make sure that we
gracefully handle the case when things go horribly wrong.
. We need to test audio switching a bit more, particularly complicated cases
with multiple devices of the same type, in which case 'last plugged in time'
comes into play while decided device priority.
. We need to add tests which verify that once we switch the audio device, the
mute and volume settings are switched to the value they should be for the new
device. Doing this back and forth many times between many devices would be an
additional good test.

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler_unittest.cc:55: const AudioNode kUSBMic(
We want to add some invalid devices to add negative tests. We should also add a
HDMI device and add additional devices which share the same type (for example 2
USB headphones).

[James Cook, 2013-07-23 23:57:01]

Please let me know if you still need me to review.

[jennyz, 2013-07-24 00:04:40]

On 2013/07/23 23:57:01, James Cook (Chromium) wrote:
> Please let me know if you still need me to review.

rahul has reviewed it, so you're free now.

[jennyz, 2013-07-24 21:12:14]

On 2013/07/23 20:37:42, Rahul Chaturvedi wrote:
> Mostly looks fine - I think we can do away with having a stub preferences
> handler though, there are better ways to handle it. I'd rather that we keep
our
> write to preferences code versus save the settings to a map since that gives
us
> more test coverage.
This test covers CrasAudioHandler, it just needs to make sure it saves audio
preference as expected. For AudioPrefHandler, it should has its own test cases
to cover its unit tests. I don't try to cover the test of both classes.

> 
> 
> Still looking over the unit tests in detail.
> 
>
https://codereview.chromium.org/19861002/diff/1/chromeos/audio/audio_devices_...
> File chromeos/audio/audio_devices_pref_handler_stub.h (right):
> 
>
https://codereview.chromium.org/19861002/diff/1/chromeos/audio/audio_devices_...
> chromeos/audio/audio_devices_pref_handler_stub.h:20: class CHROMEOS_EXPORT
> AudioDevicesPrefHandlerStub
> Instead of stubbing this class out, can we instead provide the ability to use
a
> mock/stub preferences to the main AudioDevicesPrefHandler class?
> 
>
https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
> File chromeos/audio/cras_audio_handler.cc (right):
> 
>
https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
> chromeos/audio/cras_audio_handler.cc:167: device_list->clear();
> A DCHECK(device_list) might be appropriate right before the ->clear().

[jennyz, 2013-07-24 21:12:25]

I added more test cases to switching to Bluetooth, HDMI and USB devices, and
also the test case for handling multiple devices with the same type.

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:167: device_list->clear();
On 2013/07/23 20:37:43, Rahul Chaturvedi wrote:
> A DCHECK(device_list) might be appropriate right before the ->clear().
> 

I think it is OK for client to pass in an non-empty device_list object, as long
as we cleared it inside GetAudioDevices, it is good.

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler_unittest.cc:55: const AudioNode kUSBMic(
On 2013/07/23 20:50:53, Rahul Chaturvedi wrote:
> We want to add some invalid devices to add negative tests. We should also add
a
> HDMI device and add additional devices which share the same type (for example
2
> USB headphones).

The audio node may give us some bogus audio device type, which is categorized to
"OTHER" type, but not invalid. I added the test to make sure CrasAudioHandler
will handle it gracefully.

[jennyz, 2013-07-24 21:31:10]

Added more test cases. PTAL!

[jennyz, 2013-07-25 18:42:28]

stevenjb@, rkc@ is sick, can you take a look at this cl? Thanks!

[stevenjb, 2013-07-26 17:23:30]

About half of the base files failed to upload. I commented on the files that I
could.

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/audio_devi...
File chromeos/audio/audio_devices_pref_handler_stub.h (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/audio_devi...
chromeos/audio/audio_devices_pref_handler_stub.h:17: typedef std::map<uint64,
int> AudioDeviceVolumeGain;
Define these inside the class

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:17: using chromeos::AudioNode;
We could put this entire file inside namespace chromeos

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:258: CrasAudioClientStubImpl*
cras_audio_client_stub_;  // Not owned.
Unused?

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:267: const size_t
kNumStubAudioDevices = 2;
Maybe use audio_nodes.size() instead, here and below?

https://codereview.chromium.org/19861002/diff/15002/chromeos/dbus/cras_audio_...
File chromeos/dbus/cras_audio_client_stub_impl.cc (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/dbus/cras_audio_...
chromeos/dbus/cras_audio_client_stub_impl.cc:58: node_list_.push_back(node_5);
nit: Add blank line

[jennyz, 2013-07-26 18:14:43]

I rebased and upload the patch with nits fixed. Can you see all the files now?

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/audio_devi...
File chromeos/audio/audio_devices_pref_handler_stub.h (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/audio_devi...
chromeos/audio/audio_devices_pref_handler_stub.h:17: typedef std::map<uint64,
int> AudioDeviceVolumeGain;
On 2013/07/26 17:23:30, stevenjb (chromium) wrote:
> Define these inside the class

Done.

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:17: using chromeos::AudioNode;
On 2013/07/26 17:23:30, stevenjb (chromium) wrote:
> We could put this entire file inside namespace chromeos

Done.

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:258: CrasAudioClientStubImpl*
cras_audio_client_stub_;  // Not owned.
On 2013/07/26 17:23:30, stevenjb (chromium) wrote:
> Unused?
It is used in both SetUpCrasAudioHandler and ChangeAudioNodes.

https://codereview.chromium.org/19861002/diff/15002/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:267: const size_t
kNumStubAudioDevices = 2;
On 2013/07/26 17:23:30, stevenjb (chromium) wrote:
> Maybe use audio_nodes.size() instead, here and below?

Done.

https://codereview.chromium.org/19861002/diff/15002/chromeos/dbus/cras_audio_...
File chromeos/dbus/cras_audio_client_stub_impl.cc (right):

https://codereview.chromium.org/19861002/diff/15002/chromeos/dbus/cras_audio_...
chromeos/dbus/cras_audio_client_stub_impl.cc:58: node_list_.push_back(node_5);
On 2013/07/26 17:23:30, stevenjb (chromium) wrote:
> nit: Add blank line

Done.

[stevenjb, 2013-07-26 21:03:17]

lgtm

https://codereview.chromium.org/19861002/diff/45001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/45001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:19: using chromeos::AudioNode;
Not needed

[jennyz, 2013-07-26 21:07:40]

https://codereview.chromium.org/19861002/diff/45001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/19861002/diff/45001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler_unittest.cc:19: using chromeos::AudioNode;
On 2013/07/26 21:03:17, stevenjb (chromium) wrote:
> Not needed

Done.

[commit-bot: I haz the power, 2013-07-26 21:31:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/19861002/29003

[commit-bot: I haz the power, 2013-07-26 23:42:03]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-26 23:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/19861002/62001

[commit-bot: I haz the power, 2013-07-28 00:42:44]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[jennyz, 2013-07-29 21:41:36]

Committed patchset #5 manually as r214224 (presubmit successful).