Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917006:
factory_Audio test created (Closed)
|
Created:
10 years, 5 months ago by tbroch1 Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, sosa+cc_chromium.org, seano, ericli, petkov+cc_chromium.org Base URL:
ssh://git@chromiumos-git/autotest.git Visibility:
Public. |
Descriptionfactory_Audio test created
BUG=chrome-os-partner id:343
TEST=Manual, factory test image bbot929
Patch Set 1 #
Total comments: 14
Patch Set 2 : tom + tammo feedback #
Total comments: 3
Patch Set 3 : make audio sample run_once param #
Total comments: 2
Patch Set 4 : tammo feedback ; relo default sample / use dargs #Patch Set 5 : remove result_file_path ref #Patch Set 6 : chg UI a bit to use keys r,p,s for record,play,sample #Patch Set 7 : no full-screen test widget, auto-playback recording #
Total comments: 12
Patch Set 8 : Add Chinese from Hung-Te #
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/2917006/diff/1/2 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/1/2#newcode15 client/site_tests/factory_Audio/factory_Audio.py:15: import logging Please sort them by alphabetic order. http://codereview.chromium.org/2917006/diff/1/2#newcode61 client/site_tests/factory_Audio/factory_Audio.py:61: **{'msgtext':'Speak into internal microphone near camera for several seconds', Break this line to fit 80 chars. http://codereview.chromium.org/2917006/diff/1/2#newcode112 client/site_tests/factory_Audio/factory_Audio.py:112: 'stdout: %s\nstderr: %s' % (result.stdout, result.stderr)) Exceptions in gtk handler are not propagated outside. My solution is to keep the error message in a variable and quit gtk. Then raise the exception outside gtk. Not sure any better solution. http://codereview.chromium.org/2917006/diff/1/2#newcode230 client/site_tests/factory_Audio/factory_Audio.py:230: Remove one empty line.
http://codereview.chromium.org/2917006/diff/1/2 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/1/2#newcode32 client/site_tests/factory_Audio/factory_Audio.py:32: if msgtext: trailing whitespace here and in a few places further down http://codereview.chromium.org/2917006/diff/1/2#newcode33 client/site_tests/factory_Audio/factory_Audio.py:33: label.set_text(msgtext + "\n\nRemember, " + _LABEL_RESPONSE_STR) there is also now a label convenience function in factory_ui_lib that might make this cleaner http://codereview.chromium.org/2917006/diff/1/2#newcode195 client/site_tests/factory_Audio/factory_Audio.py:195: os.chdir(self.srcdir) do you actually have stuff in srcdir? currently your patch does not create a srcdir, so this is not so nice
http://codereview.chromium.org/2917006/diff/1/2 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/1/2#newcode15 client/site_tests/factory_Audio/factory_Audio.py:15: import logging On 2010/07/15 08:08:18, Tom Wai-Hong Tam wrote: > Please sort them by alphabetic order. Done. http://codereview.chromium.org/2917006/diff/1/2#newcode32 client/site_tests/factory_Audio/factory_Audio.py:32: if msgtext: Done ... found autotest/utils/reident.py so will be using this to hopefully cutdown on pesky whitespace violations On 2010/07/15 16:43:07, tammo wrote: > trailing whitespace here and in a few places further down http://codereview.chromium.org/2917006/diff/1/2#newcode33 client/site_tests/factory_Audio/factory_Audio.py:33: label.set_text(msgtext + "\n\nRemember, " + _LABEL_RESPONSE_STR) On 2010/07/15 16:43:07, tammo wrote: > there is also now a label convenience function in factory_ui_lib that might make > this cleaner Done. http://codereview.chromium.org/2917006/diff/1/2#newcode61 client/site_tests/factory_Audio/factory_Audio.py:61: **{'msgtext':'Speak into internal microphone near camera for several seconds', On 2010/07/15 08:08:18, Tom Wai-Hong Tam wrote: > Break this line to fit 80 chars. Done. http://codereview.chromium.org/2917006/diff/1/2#newcode112 client/site_tests/factory_Audio/factory_Audio.py:112: 'stdout: %s\nstderr: %s' % (result.stdout, result.stderr)) This works for me ... in the sense that the exception is caught and makes the test fail (turns red). Question is should factory test die at that point? On 2010/07/15 08:08:18, Tom Wai-Hong Tam wrote: > Exceptions in gtk handler are not propagated outside. My solution is to keep the > error message in a variable and quit gtk. Then raise the exception outside gtk. > Not sure any better solution. http://codereview.chromium.org/2917006/diff/1/2#newcode195 client/site_tests/factory_Audio/factory_Audio.py:195: os.chdir(self.srcdir) On 2010/07/15 16:43:07, tammo wrote: > do you actually have stuff in srcdir? currently your patch does not create a > srcdir, so this is not so nice Done. http://codereview.chromium.org/2917006/diff/1/2#newcode230 client/site_tests/factory_Audio/factory_Audio.py:230: On 2010/07/15 08:08:18, Tom Wai-Hong Tam wrote: > Remove one empty line. Done.
http://codereview.chromium.org/2917006/diff/5001/6001 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/5001/6001#newcode81 client/site_tests/factory_Audio/factory_Audio.py:81: label = ful.make_label('', alignment=(0.5, 0.5), size=_LABEL_STATUS_SIZE) I'll get this >80 http://codereview.chromium.org/2917006/diff/5001/6001#newcode153 client/site_tests/factory_Audio/factory_Audio.py:153: alignment=(0, 0.5), fg=ful.LABEL_COLORS['UNTESTED']) I'll get this >80
http://codereview.chromium.org/2917006/diff/5001/6001 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/5001/6001#newcode50 client/site_tests/factory_Audio/factory_Audio.py:50: 'sample':factory.AUDIO_SAMPLE, as we discussed, this might be better moved to a run_one parameter
http://codereview.chromium.org/2917006/diff/12001/13001 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/12001/13001#newcode162 client/site_tests/factory_Audio/factory_Audio.py:162: sample=factory.AUDIO_SAMPLE): looks like you need to update factory.py and, if i am understanding correctly that you want to share this across tests, you probably should put the sample into deps/factory or something
http://codereview.chromium.org/2917006/diff/12001/13001 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/12001/13001#newcode162 client/site_tests/factory_Audio/factory_Audio.py:162: sample=factory.AUDIO_SAMPLE): On 2010/07/19 17:06:59, tammo wrote: > looks like you need to update factory.py and, if i am understanding correctly > that you want to share this across tests, you probably should put the sample > into deps/factory or something Done.
now that i have managed to run this test on a device -- it is always cool to see new functionality actually working, wow it makes noice, and you can record, very cool! wrt code review, some high level requests -- using the interaction model from factory_Display seems rather unnatural, specifically, (unlike display) i do not understand the need for a seperate full screen window .. also, the test activeness seems odd for record/playback combinations i would prefer folding the separate testing window into the existing testing window, simply removing the status list widget and temporality swapping in a widget that does the test .. this transition can be launched from the status list screen, but should persist until the user chooses pass/fail ; i do not think the status screen itself should accept pass/fail input .. once you have this non-temporary progression into a testing widget, you can smoothly combine together record/playback into a single test .. basically the test starts (with space hit) into recording mode until the user hits some key to terminate recording and switch immediately into playback, and after playback it waits for pass/fail before transitioning into the status list again sound workable?
On 2010/07/20 21:03:12, tammo wrote: > now that i have managed to run this test on a device -- it is always cool to see > new functionality actually working, wow it makes noice, and you can record, very > cool! > > wrt code review, some high level requests -- > > using the interaction model from factory_Display seems rather unnatural, > specifically, (unlike display) i do not understand the need for a seperate full > screen window .. also, the test activeness seems odd for record/playback > combinations > > i would prefer folding the separate testing window into the existing testing > window, simply removing the status list widget and temporality swapping in a > widget that does the test .. this transition can be launched from the status > list screen, but should persist until the user chooses pass/fail ; i do not > think the status screen itself should accept pass/fail input .. once you have > this non-temporary progression into a testing widget, you can smoothly combine > together record/playback into a single test .. basically the test starts (with > space hit) into recording mode until the user hits some key to terminate > recording and switch immediately into playback, and after playback it waits for > pass/fail before transitioning into the status list again > > sound workable? It does. Will model something closer to a traditional audio player w/ key presses for record, playback recording, playback provided audio then returning pass/fail based on that.
almost-lgtm can we not have the internal tests running full screen? they should fit just fine in the regular test widget space, and you can just replace the test selector widget with one for the individual tests... if i can be extra nit-picky :-) it would also seem nicer interface-wise if, after the recording is done, if the playback just happened automatically and did not require holding down keys... you could ask for pass/fail after each of the two playbacks.
lgtm i think we can leave the sample, although i agree with your reasoning that it principle we could remove it adding hung-te as a reviewer -- he can hopefully provide some chinese translations as comments
http://codereview.chromium.org/2917006/diff/29001/30002 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/29001/30002#newcode26 client/site_tests/factory_Audio/factory_Audio.py:26: _LABEL_START_STR = 'hit SPACE to start each audio test,\n\n' 按空白鍵開始各項聲音測試 http://codereview.chromium.org/2917006/diff/29001/30002#newcode65 client/site_tests/factory_Audio/factory_Audio.py:65: lab_str = 'Connect headset to device\n(Chinese)' 將耳機接上音源孔 http://codereview.chromium.org/2917006/diff/29001/30002#newcode67 client/site_tests/factory_Audio/factory_Audio.py:67: lab_str = 'Remove headset from device\n(Chinese)' 將耳機移開音源孔 http://codereview.chromium.org/2917006/diff/29001/30002#newcode71 client/site_tests/factory_Audio/factory_Audio.py:71: 'Press & hold \'r\' to record\n(Chinese)\n' + \ 壓住 \'r\' 鍵開始錄音 http://codereview.chromium.org/2917006/diff/29001/30002#newcode72 client/site_tests/factory_Audio/factory_Audio.py:72: '[Playback will follow]\n[(Chinese)]')) [之後會重播錄到的聲音] http://codereview.chromium.org/2917006/diff/29001/30002#newcode74 client/site_tests/factory_Audio/factory_Audio.py:74: 'Press & hold \'p\' to play sample\n(Chinese)')) 壓住 \'p\' 鍵以播放範例
http://codereview.chromium.org/2917006/diff/29001/30002 File client/site_tests/factory_Audio/factory_Audio.py (right): http://codereview.chromium.org/2917006/diff/29001/30002#newcode26 client/site_tests/factory_Audio/factory_Audio.py:26: _LABEL_START_STR = 'hit SPACE to start each audio test,\n\n' On 2010/07/28 08:37:43, Hung-Te wrote: > 按空白鍵開始各項聲音測試 Done. http://codereview.chromium.org/2917006/diff/29001/30002#newcode65 client/site_tests/factory_Audio/factory_Audio.py:65: lab_str = 'Connect headset to device\n(Chinese)' On 2010/07/28 08:37:43, Hung-Te wrote: > 將耳機接上音源孔 Done. http://codereview.chromium.org/2917006/diff/29001/30002#newcode67 client/site_tests/factory_Audio/factory_Audio.py:67: lab_str = 'Remove headset from device\n(Chinese)' On 2010/07/28 08:37:43, Hung-Te wrote: > 將耳機移開音源孔 Done. http://codereview.chromium.org/2917006/diff/29001/30002#newcode71 client/site_tests/factory_Audio/factory_Audio.py:71: 'Press & hold \'r\' to record\n(Chinese)\n' + \ On 2010/07/28 08:37:43, Hung-Te wrote: > 壓住 \'r\' 鍵開始錄音 Done. http://codereview.chromium.org/2917006/diff/29001/30002#newcode72 client/site_tests/factory_Audio/factory_Audio.py:72: '[Playback will follow]\n[(Chinese)]')) On 2010/07/28 08:37:43, Hung-Te wrote: > [之後會重播錄到的聲音] Done. http://codereview.chromium.org/2917006/diff/29001/30002#newcode74 client/site_tests/factory_Audio/factory_Audio.py:74: 'Press & hold \'p\' to play sample\n(Chinese)')) On 2010/07/28 08:37:43, Hung-Te wrote: > 壓住 \'p\' 鍵以播放範例 Done.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
