factory external display test

factory external display test

BUG=chrome-os-partner:447
TEST= ran on duts w/ different external display connections

[mdhayter, 2010-07-16 04:36:07]

Not sure that the shell script needs to be this complicated. You probably just
want the modprobe and the final run of the ch7036_monitor The tests and log junk
are really just for the upstart context

http://codereview.chromium.org/2877021/diff/1/3
File client/deps/factory/chrontel.sh (right):

http://codereview.chromium.org/2877021/diff/1/3#newcode7
client/deps/factory/chrontel.sh:7: test -e /usr/bin/ch7036_monitor
Are you running the shell to exit on a command fail? If not then this test does
not do anything useful (in the upstart script the shell exits on first fail and
will not try to start the process if it is not there)

http://codereview.chromium.org/2877021/diff/1/3#newcode9
client/deps/factory/chrontel.sh:9: /usr/bin/ch7036_monitor -p
Same comment applies to this line. The -p causes the chrontel to be probed, the
exit code indicates if it was found.

http://codereview.chromium.org/2877021/diff/1/3#newcode25
client/deps/factory/chrontel.sh:25: /usr/bin/ch7036_monitor -v >
"$HOME/log/ch7036_monitor.$log_tag" &
Could you simplify by just creating one log file? Im not sure you are going to
use it?

[Tammo Spalink, 2010-07-19 17:46:12]

http://codereview.chromium.org/2877021/diff/6001/7001
File client/bin/factory.py (right):

http://codereview.chromium.org/2877021/diff/6001/7001#newcode20
client/bin/factory.py:20: # rooted in autotest dir
on further consideration, i think this could just as well be an dargs parameter
to each of the tests that need it -- and you can just put this file into
deps/factory...  with no need to define it here.

sound reasonable?

http://codereview.chromium.org/2877021/diff/6001/7001#newcode23
client/bin/factory.py:23: def has_display_audio():
this should probably also just be a dargs parameter to the corresponding test --
the plan is to have different control files, or at least diffs of control files
for different boards/hw-configs and hence it would be good to capture all of the
config specific stuff there

http://codereview.chromium.org/2877021/diff/6001/7002
File client/deps/factory/startx.sh (right):

http://codereview.chromium.org/2877021/diff/6001/7002#newcode33
client/deps/factory/startx.sh:33: DISPLAY=${DISPLAY} XAUTHORITY=${XAUTH_FILE} \
if you instead export both of these variables after the sleep loop, we can add
more callouts like this more cleanly later

http://codereview.chromium.org/2877021/diff/6001/7003
File client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py (right):

http://codereview.chromium.org/2877021/diff/6001/7003#newcode10
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:10: 
add extra blank line

http://codereview.chromium.org/2877021/diff/6001/7003#newcode21
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:21: 
add extra blank line

http://codereview.chromium.org/2877021/diff/6001/7003#newcode23
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:23: _LABEL_FONT =
pango.FontDescription('courier new condensed 20')
looks like you are not using this

http://codereview.chromium.org/2877021/diff/6001/7003#newcode27
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:27: 
add extra blank line

http://codereview.chromium.org/2877021/diff/6001/7003#newcode34
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:34: 
add extra blank line

http://codereview.chromium.org/2877021/diff/6001/7003#newcode59
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:59: label =
ful.make_label('', alignment=(0.5, 0.5), size=_LABEL_STATUS_SIZE)
>80 chars

http://codereview.chromium.org/2877021/diff/6001/7003#newcode63
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:63: eb.add(label)
is the expectation that we are doing mirroring?
regardless, it would probably be good to put up some kind of pattern (eg
bounding box) to indicate the desired resolution, so that we can verify it is
being properly/fully displayed

http://codereview.chromium.org/2877021/diff/6001/7003#newcode140
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:140: alignment=(0,
0.5), fg=ful.LABEL_COLORS['UNTESTED'])
>80 chars
and you should probably replace 'UNTESTED' with ful.UNTESTED

http://codereview.chromium.org/2877021/diff/6001/7003#newcode171
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:171: if
factory.has_display_audio():
like i mentioned, lets make this a dargs param

http://codereview.chromium.org/2877021/diff/6001/7004
File client/site_tests/suite_Factory/control (right):

http://codereview.chromium.org/2877021/diff/6001/7004#newcode120
client/site_tests/suite_Factory/control:120: label_zw='??? 顯示',
外接顯示

http://codereview.chromium.org/2877021/diff/6001/7004#newcode131
client/site_tests/suite_Factory/control:131: formal_name='factory_Audio',
this will likely conflict with your other CL

[tbroch1, 2010-07-20 00:44:03]

minus bounding box will look into that

http://codereview.chromium.org/2877021/diff/6001/7001
File client/bin/factory.py (right):

http://codereview.chromium.org/2877021/diff/6001/7001#newcode20
client/bin/factory.py:20: # rooted in autotest dir
On 2010/07/19 17:46:13, tammo wrote:
> on further consideration, i think this could just as well be an dargs
parameter
> to each of the tests that need it -- and you can just put this file into
> deps/factory...  with no need to define it here.
> 
> sound reasonable?

Done.

http://codereview.chromium.org/2877021/diff/6001/7001#newcode23
client/bin/factory.py:23: def has_display_audio():
On 2010/07/19 17:46:13, tammo wrote:
> this should probably also just be a dargs parameter to the corresponding test
--
> the plan is to have different control files, or at least diffs of control
files
> for different boards/hw-configs and hence it would be good to capture all of
the
> config specific stuff there

Done.

http://codereview.chromium.org/2877021/diff/6001/7002
File client/deps/factory/startx.sh (right):

http://codereview.chromium.org/2877021/diff/6001/7002#newcode33
client/deps/factory/startx.sh:33: DISPLAY=${DISPLAY} XAUTHORITY=${XAUTH_FILE} \
On 2010/07/19 17:46:13, tammo wrote:
> if you instead export both of these variables after the sleep loop, we can add
> more callouts like this more cleanly later

Done.

http://codereview.chromium.org/2877021/diff/6001/7003
File client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py (right):

http://codereview.chromium.org/2877021/diff/6001/7003#newcode10
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:10: 
On 2010/07/19 17:46:13, tammo wrote:
> add extra blank line

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode21
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:21: 
On 2010/07/19 17:46:13, tammo wrote:
> add extra blank line

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode23
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:23: _LABEL_FONT =
pango.FontDescription('courier new condensed 20')
On 2010/07/19 17:46:13, tammo wrote:
> looks like you are not using this

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode27
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:27: 
On 2010/07/19 17:46:13, tammo wrote:
> add extra blank line

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode34
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:34: 
On 2010/07/19 17:46:13, tammo wrote:
> add extra blank line

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode59
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:59: label =
ful.make_label('', alignment=(0.5, 0.5), size=_LABEL_STATUS_SIZE)
On 2010/07/19 17:46:13, tammo wrote:
> >80 chars

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode140
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:140: alignment=(0,
0.5), fg=ful.LABEL_COLORS['UNTESTED'])
On 2010/07/19 17:46:13, tammo wrote:
> >80 chars
> and you should probably replace 'UNTESTED' with ful.UNTESTED

Done.

http://codereview.chromium.org/2877021/diff/6001/7003#newcode171
client/site_tests/factory_ExtDisplay/factory_ExtDisplay.py:171: if
factory.has_display_audio():
On 2010/07/19 17:46:13, tammo wrote:
> like i mentioned, lets make this a dargs param

Done.

http://codereview.chromium.org/2877021/diff/6001/7004
File client/site_tests/suite_Factory/control (right):

http://codereview.chromium.org/2877021/diff/6001/7004#newcode120
client/site_tests/suite_Factory/control:120: label_zw='??? 顯示',
On 2010/07/19 17:46:13, tammo wrote:
> 外接顯示

Done.

http://codereview.chromium.org/2877021/diff/6001/7004#newcode131
client/site_tests/suite_Factory/control:131: formal_name='factory_Audio',
On 2010/07/19 17:46:13, tammo wrote:
> this will likely conflict with your other CL

Done.

[Tammo Spalink, 2010-08-03 11:10:26]

lgtm