Support multiple connected Android devices in low bandwidth audio test

Support multiple connected Android devices in low bandwidth audio test

Previously it was assumed that exactly one device is connected.
Now adb will get an argument with the device ID taken from the runner
script's stdout.

BUG=webrtc:7229
TBR=kjellander@webrtc.org
NOTRY=true

Review-Url: https://codereview.webrtc.org/2783343003
Cr-Commit-Position: refs/heads/master@{#17580}
Committed: https://chromium.googlesource.com/external/webrtc/+/abd101b91f693fc218d3def6f76e118c4f91c9bf

[oprypin_donotuse, 2017-03-31 12:00:40]

Description was changed from

==========
Support multiple connected Android devices in low bandwidth audio test

BUG=webrtc:7229
TBR=kjellander@webrtc.org
NOTRY=true
==========

to

==========
Support multiple connected Android devices in low bandwidth audio test

Previously it was assumed that exactly one device is connected.
Now adb will get an argument with the device ID taken from the runner
script's stdout.

BUG=webrtc:7229
TBR=kjellander@webrtc.org
NOTRY=true
==========

[oprypin_donotuse, 2017-03-31 12:00:56]

oprypin@google.com changed reviewers:
+ oprypin@google.com

[oprypin_webrtc, 2017-03-31 12:03:14]

oprypin@webrtc.org changed reviewers:
- oprypin@google.com

[oprypin_webrtc, 2017-04-03 11:50:27]

I'm not sure if the email went through correctly.
This is ready for review.

[kjellander_webrtc, 2017-04-03 12:00:26]

On 2017/04/03 11:50:27, oprypin_webrtc wrote:
> I'm not sure if the email went through correctly.
> This is ready for review.

Yeah, it's on my radar (last e-mail went though). I'll get to it shortly.

[kjellander_webrtc, 2017-04-03 13:07:43]

lgtm to unblock you, but please consider the suggested refactoring+test.

https://codereview.webrtc.org/2783343003/diff/1/webrtc/audio/test/low_bandwid...
File webrtc/audio/test/low_bandwidth_audio_test.py (right):

https://codereview.webrtc.org/2783343003/diff/1/webrtc/audio/test/low_bandwid...
webrtc/audio/test/low_bandwidth_audio_test.py:112: test_re = r'^' +
android_prefix_re + r'TEST (\w+) ([^ ]+?) ([^ ]+?)\s*$'
Could you extract the logic here into a function that will get it's own unit
test, using a log snippet from a real run? That way we can more easily see in
the future if this stops working, what assumptions it has on the format.

[oprypin_webrtc, 2017-04-04 08:41:50]

https://codereview.webrtc.org/2783343003/diff/1/webrtc/audio/test/low_bandwid...
File webrtc/audio/test/low_bandwidth_audio_test.py (right):

https://codereview.webrtc.org/2783343003/diff/1/webrtc/audio/test/low_bandwid...
webrtc/audio/test/low_bandwidth_audio_test.py:112: test_re = r'^' +
android_prefix_re + r'TEST (\w+) ([^ ]+?) ([^ ]+?)\s*$'
On 2017/04/03 13:07:43, kjellander_webrtc wrote:
> Could you extract the logic here into a function that will get it's own unit
> test, using a log snippet from a real run? That way we can more easily see in
> the future if this stops working, what assumptions it has on the format.

Thanks for the suggestion. This is now done. I would like to hear what you thing
about this addition, because it turned out quite noisy.

[kjellander_webrtc, 2017-04-04 13:10:19]

https://codereview.webrtc.org/2783343003/diff/20001/webrtc/audio/test/low_ban...
File webrtc/audio/test/low_bandwidth_audio_test.py (right):

https://codereview.webrtc.org/2783343003/diff/20001/webrtc/audio/test/low_ban...
webrtc/audio/test/low_bandwidth_audio_test.py:96:
subprocess.check_call(_LogCommand(adb_command))
Can you add deletion of the file after a successful pull as well?

[oprypin_webrtc, 2017-04-04 15:47:28]

https://codereview.webrtc.org/2783343003/diff/20001/webrtc/audio/test/low_ban...
File webrtc/audio/test/low_bandwidth_audio_test.py (right):

https://codereview.webrtc.org/2783343003/diff/20001/webrtc/audio/test/low_ban...
webrtc/audio/test/low_bandwidth_audio_test.py:96:
subprocess.check_call(_LogCommand(adb_command))
On 2017/04/04 13:10:19, kjellander_webrtc wrote:
> Can you add deletion of the file after a successful pull as well?

Done.

[kjellander_webrtc, 2017-04-04 19:01:59]

lgtm, but please add the directory to
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472 in
order to actually get the unit test to execute during presubmit.

[oprypin_webrtc, 2017-04-05 12:51:47]

On 2017/04/04 19:01:59, kjellander_webrtc wrote:
> lgtm, but please add the directory to
> https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472 in
> order to actually get the unit test to execute during presubmit.

It is possible to add only directories there, and the unittest files in them
must end with _test.py - but with the naming scheme I have going here, the
opposite is happening... I don't see a good solution :(

[kjellander_webrtc, 2017-04-05 12:54:48]

On 2017/04/05 12:51:47, oprypin_webrtc wrote:
> On 2017/04/04 19:01:59, kjellander_webrtc wrote:
> > lgtm, but please add the directory to
> > https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472
in
> > order to actually get the unit test to execute during presubmit.
> 
> It is possible to add only directories there, and the unittest files in them
> must end with _test.py - but with the naming scheme I have going here, the
> opposite is happening... I don't see a good solution :(

Ah, sorry about missing the fact that you have to rename your test file. Let's
just do that and add
join('webrtc', 'audio' ,'test')
to the list?

[kjellander_webrtc, 2017-04-05 12:55:51]

On 2017/04/05 12:54:48, kjellander_webrtc wrote:
> On 2017/04/05 12:51:47, oprypin_webrtc wrote:
> > On 2017/04/04 19:01:59, kjellander_webrtc wrote:
> > > lgtm, but please add the directory to
> > >
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472
> in
> > > order to actually get the unit test to execute during presubmit.
> > 
> > It is possible to add only directories there, and the unittest files in them
> > must end with _test.py - but with the naming scheme I have going here, the
> > opposite is happening... I don't see a good solution :(
> 
> Ah, sorry about missing the fact that you have to rename your test file. Let's
> just do that and add
> join('webrtc', 'audio' ,'test')
> to the list?

Or.. you could just add a os.walk entry for 'webrtc' as well, I think it will
execute very fast (ideally you'd measure it though). Our tree isn't that large.

[oprypin_webrtc, 2017-04-05 12:55:52]

On 2017/04/05 12:54:48, kjellander_webrtc wrote:
> On 2017/04/05 12:51:47, oprypin_webrtc wrote:
> > On 2017/04/04 19:01:59, kjellander_webrtc wrote:
> > > lgtm, but please add the directory to
> > >
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472
> in
> > > order to actually get the unit test to execute during presubmit.
> > 
> > It is possible to add only directories there, and the unittest files in them
> > must end with _test.py - but with the naming scheme I have going here, the
> > opposite is happening... I don't see a good solution :(
> 
> Ah, sorry about missing the fact that you have to rename your test file. Let's
> just do that and add
> join('webrtc', 'audio' ,'test')
> to the list?

That's fine but it will still try to run low_bandwidth_audio_test.py itself.

[kjellander_webrtc, 2017-04-05 13:42:40]

On 2017/04/05 12:55:52, oprypin_webrtc wrote:
> On 2017/04/05 12:54:48, kjellander_webrtc wrote:
> > On 2017/04/05 12:51:47, oprypin_webrtc wrote:
> > > On 2017/04/04 19:01:59, kjellander_webrtc wrote:
> > > > lgtm, but please add the directory to
> > > >
> https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#472
> > in
> > > > order to actually get the unit test to execute during presubmit.
> > > 
> > > It is possible to add only directories there, and the unittest files in
them
> > > must end with _test.py - but with the naming scheme I have going here, the
> > > opposite is happening... I don't see a good solution :(
> > 
> > Ah, sorry about missing the fact that you have to rename your test file.
Let's
> > just do that and add
> > join('webrtc', 'audio' ,'test')
> > to the list?
> 
> That's fine but it will still try to run low_bandwidth_audio_test.py itself.

Ah, that's certainly annoying since we'll have to update the buildbots as well
if we're going to rename that :)
Rename + move the unit test into a 'unittest' subdirectory then, and add that.
It's not the end of the world if the file will be named
low_bandwidth_audio_test_test.py

[oprypin_webrtc, 2017-04-07 06:18:47]

The CQ bit was checked by oprypin@webrtc.org

[oprypin_webrtc, 2017-04-07 06:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2783343003/#ps80001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-04-07 06:18:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-07 06:21:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491545927371930,
"parent_rev": "225bfc0971e2986bd42af5c0a6f5c70fa07b6dd3", "commit_rev":
"abd101b91f693fc218d3def6f76e118c4f91c9bf"}

[commit-bot: I haz the power, 2017-04-07 06:21:35]

Description was changed from

==========
Support multiple connected Android devices in low bandwidth audio test

Previously it was assumed that exactly one device is connected.
Now adb will get an argument with the device ID taken from the runner
script's stdout.

BUG=webrtc:7229
TBR=kjellander@webrtc.org
NOTRY=true
==========

to

==========
Support multiple connected Android devices in low bandwidth audio test

Previously it was assumed that exactly one device is connected.
Now adb will get an argument with the device ID taken from the runner
script's stdout.

BUG=webrtc:7229
TBR=kjellander@webrtc.org
NOTRY=true

Review-Url: https://codereview.webrtc.org/2783343003
Cr-Commit-Position: refs/heads/master@{#17580}
Committed:
https://chromium.googlesource.com/external/webrtc/+/abd101b91f693fc218d3def6f...
==========

[commit-bot: I haz the power, 2017-04-07 06:21:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/abd101b91f693fc218d3def6f...