LAVA Android Test

Merge lp://qastaging/~liuyq0307/lava-android-test/support-custom-command into lp://qastaging/lava-android-test

support-custom-command
Merge into trunk

Proposed by Yongqin Liu on 2012-03-07

Status:	Merged
Merged at revision:	142
Proposed branch:	lp://qastaging/~liuyq0307/lava-android-test/support-custom-command
Merge into:	lp://qastaging/lava-android-test
Diff against target:	311 lines (+152/-18) 4 files modified lava_android_test/commands.py (+128/-8) lava_android_test/main.py (+5/-1) lava_android_test/testdef.py (+17/-9) setup.py (+2/-0)
To merge this branch:	bzr merge lp://qastaging/~liuyq0307/lava-android-test/support-custom-command
Related bugs:	Link a bug report
Related blueprints:	Support custom android command (Essential)

Reviewer	Date Requested	Status
Zygmunt Krynicki (community)	2012-03-07	Approve on 2012-03-09
Le Chi Thu (community)		Needs Fixing on 2012-03-08
Zach Pfeffer	2012-03-07	Pending
Review via email: mp+96381@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-03-07.

Description of the change

mainly 3 points:
1. add support for running android command that user specified
2. add run option for run command
3. change the parent directory of temporary directory to 777,
so that anyone can write and read for that directory.

And about the support for specified android command,
if it's acceptable, then I will modify lava-dispatcher and linaro-android-build-tools
to make it can be used on android-build.

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2012-03-07: Posted in a previous version of this proposal

18 + parser.add_argument('-O', '--run-option', help=("Specified in the job file for using in the real test action,"
19 + " so that we can customize some test when need"))

This is probably too long. I'd split it before help= if I were you. Running pep8 lava_android_test will tell you more.

+ except Exception as strerror:
37 + raise LavaCommandError("Test execution error: %s" % strerror)

By convention the exception should be named 'exc'. Using strerror is confusing as it may imply this is the C equivalent which it is not.

52 + parser.add_argument('-c', '--android-command', action='append', help=("Specified in the job file for using in the real test action,"
53 + " so that we can customize some test when need"))
54 + parser.add_argument('-p', '--parse-regex', help=("Specified the regular expression used for analyzing command output"))

Probably too long as well

74 + tip_msg = ''
75 + if self.args.serial:
76 + tip_msg = "Run following custom test(s) on device(%s):\n\t\t%s" % (self.args.serial, '\n\t\t'.join(ADB_SHELL_STEPS))
77 + else:
78 + tip_msg = "Run following custom test(s):\n\t\t%s" % ('\n\t\t'.join(ADB_SHELL_STEPS))

Line 74 is not needed. Python scoping will ensure that tip_msg is present after the if/else statement.

81 + inst = AndroidTestInstaller()
82 + run = AndroidTestRunner(adbshell_steps=ADB_SHELL_STEPS)
83 + parser = AndroidTestParser(pattern=PATTERN)
84 + test = AndroidTest(testname=test_name, installer=inst,
85 + runner=run, parser=parser)
86 + test.parser.results = {'test_results':[]}
87 + test.setadb(self.adb)

I cannot help to notice that we're duplicating part of lava-test. Could we consider depending on lava-test and reuse the TestArtifacts class defined there? I know this is a long topic so we can postpone this discussion but I want to reverse the trend so that we go towards unified APIs for both host-driven (lava-android-test, upcoming lava-uboot-test) and target-driven (lava-test) frameworks.

188 + #make every user can write/read this directory
189 + os.chmod(config.tempdir_host, 0777)

Why is this needed?

18	+ parser.add_argument('-O', '--run-option', help=("Specified in the job file for using in the real test action,"
19	+ " so that we can customize some test when need"))

This is probably too long. I'd split it before help= if I were you. Running pep8 lava_android_test will tell you more.

+ except Exception as strerror:
37	+ raise LavaCommandError("Test execution error: %s" % strerror)

By convention the exception should be named 'exc'. Using strerror is confusing as it may imply this is the C equivalent which it is not.

52	+ parser.add_argument('-c', '--android-command', action='append', help=("Specified in the job file for using in the real test action,"
53	+ " so that we can customize some test when need"))
54	+ parser.add_argument('-p', '--parse-regex', help=("Specified the regular expression used for analyzing command output"))

Probably too long as well

74	+ tip_msg = ''
75	+ if self.args.serial:
76	+ tip_msg = "Run following custom test(s) on device(%s):\n\t\t%s" % (self.args.serial, '\n\t\t'.join(ADB_SHELL_STEPS))
77	+ else:
78	+ tip_msg = "Run following custom test(s):\n\t\t%s" % ('\n\t\t'.join(ADB_SHELL_STEPS))

Line 74 is not needed. Python scoping will ensure that tip_msg is present after the if/else statement.

81	+ inst = AndroidTestInstaller()
82	+ run = AndroidTestRunner(adbshell_steps=ADB_SHELL_STEPS)
83	+ parser = AndroidTestParser(pattern=PATTERN)
84	+ test = AndroidTest(testname=test_name, installer=inst,
85	+ runner=run, parser=parser)
86	+ test.parser.results = {'test_results':[]}
87	+ test.setadb(self.adb)

188	+ #make every user can write/read this directory
189	+ os.chmod(config.tempdir_host, 0777)

Why is this needed?

review: Needs Fixing

Revision history for this message

Yongqin Liu (liuyq0307) wrote on 2012-03-07: Posted in a previous version of this proposal

>
> 81 + inst = AndroidTestInstaller()
> 82 + run = AndroidTestRunner(adbshell_steps=ADB_SHELL_STEPS)
> 83 + parser = AndroidTestParser(pattern=PATTERN)
> 84 + test = AndroidTest(testname=test_name, installer=inst,
> 85 + runner=run, parser=parser)
> 86 + test.parser.results = {'test_results':[]}
> 87 + test.setadb(self.adb)
>
> I cannot help to notice that we're duplicating part of lava-test. Could we
> consider depending on lava-test and reuse the TestArtifacts class defined
> there? I know this is a long topic so we can postpone this discussion but I
> want to reverse the trend so that we go towards unified APIs for both host-
> driven (lava-android-test, upcoming lava-uboot-test) and target-driven (lava-
> test) frameworks.
Yes, I agree this, especial we will have more similar test tools.
There are some common parts between lava-test and lava-android-test now.
        lava-tools
            ^
            |
     lava-test-base
     ^ ^ ^
     | | |
  Linux Android Others

I create a BP about this.
https://blueprints.launchpad.net/lava-android-test/+spec/linaro-android-test-redesign

> 188 + #make every user can write/read this directory
> 189 + os.chmod(config.tempdir_host, 0777)
>
> Why is this needed?
Now there is a problem like this:
1. when use lava-instance to run android job, it will create /tmp/lava-android-test with the apache user,
2. then when I run lava-android-test as a normal user, I can't create temporary directory in /tmp/lava-android-test in this case.
so I think we can change the /tmp/lava-android-test to 777 when create it, then normal user can also use that directory.

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2012-03-07: Posted in a previous version of this proposal

464 + help=("Specified the regular expression used for \
465 + analyzing command output"))

This will not work as you think. The general consensus is to use

("text text text"
" text text text" # repeat as many times as you want
" more text")

Thanks for the blueprint! +1 :)

1. when use lava-instance to run android job, it will create /tmp/lava-android-test with the apache user,

Huh? What? Could you explain how this happens please? Lava runs as a designated user. Apache should _never_ be part of that.

2. then when I run lava-android-test as a normal user, I can't create temporary directory in /tmp/lava-android-test in this case.
so I think we can change the /tmp/lava-android-test to 777 when create it, then normal user can also use that directory.

review: Needs Information

Revision history for this message

Yongqin Liu (liuyq0307) wrote on 2012-03-08:

>>1. when use lava-instance to run android job, it will create /tmp/lava-android-test with the apache user,
>Huh? What? Could you explain how this happens please? Lava runs as a designated user. Apache should _never_ be part of that.
I don't know why, but the directory created by lava-instance is owned by root on my machine.
so when I run lava-android-test from command line as user liuyq, I can't create temporary directory in that directory.

Following is the error information:

11:56:10 liuyq:lava-deployment-tool$ ll /tmp/lava-android-test/ -1d
drwxr-xr-x 2 root root 4096 2012-03-08 11:55 /tmp/lava-android-test//
11:56:13 liuyq:lava-deployment-tool$ lava-android-test list-installed
Traceback (most recent call last):
  File "/usr/bin/lava-android-test", line 9, in <module>
    load_entry_point('lava-android-test==0.0.7', 'console_scripts', 'lava-android-test')()
  File "/usr/lib/pymodules/python2.7/lava_android_test/main.py", line 51, in main
    shutil.rmtree(config.tempdir_host)
  File "/usr/lib/python2.7/shutil.py", line 253, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/lib/python2.7/shutil.py", line 251, in rmtree
    os.rmdir(path)
OSError: [Errno 1] Operation not permitted: '/tmp/lava-android-test'
11:56:37 liuyq:lava-deployment-tool$

Revision history for this message

Yongqin Liu (liuyq0307) wrote on 2012-03-08:

> 464 + help=("Specified the regular expression used for \
> 465 + analyzing command output"))
>
> This will not work as you think. The general consensus is to use
>
> ("text text text"
> " text text text" # repeat as many times as you want
> " more text")

It seems work, but about the output of the usage example, do you have any suggestion?

below is the output of "lava-android-test run-custom -h" command:

$ lava-android-test run-custom -h
usage: lava-android-test run-custom [-h] [-q] [-Q] [-s serial]
[-c ANDROID_COMMAND] [-p PARSE_REGEX]
[-o FILE]

optional arguments:
  -h, --help show this help message and exit
  -q, --quiet Be less verbose about undertaken actions
  -Q, --quiet-subcommands
                        Hide the output of all sub-commands (including tests)
  -c ANDROID_COMMAND, --android-command ANDROID_COMMAND
                        Specified in the job file for using in the real test
                        action, so that we can customize some test when need
  -p PARSE_REGEX, --parse-regex PARSE_REGEX
                        Specified the regular expression used for analyzing
                        command output

specify device serial number:
  -s serial, --serial serial
                        specify the device with serialnumber that this command
                        will berun on

specify the bundle output file:
  -o FILE, --output FILE
                        After running the test parse the result artefacts,
                        fuse them with the initial bundle and finally save the
                        complete bundle to the specified FILE.

program:: lava-android-test run-custom -c 'command1' -c 'command2' -p 'parse-
regex1' program:: lava-android-test run test-id -s device_serial program::
lava-android-test run test-id -s device_serial -o outputfile
$

Revision history for this message

Le Chi Thu (le-chi-thu) wrote on 2012-03-08:

Can you split this patch to 2 patches ? One for reformatting the code. The other one is for adding new functionality.

review: Needs Fixing

Revision history for this message

Yongqin Liu (liuyq0307) wrote on 2012-03-08:

> Can you split this patch to 2 patches ? One for reformatting the code. The
> other one is for adding new functionality.

Thanks for your suggestion.
I split it, and the style branch MP is https://code.launchpad.net/~liuyq0307/lava-android-test/improve-style/+merge/96560

And I will resubmit this MP later

Revision history for this message

Yongqin Liu (liuyq0307) wrote on 2012-03-09:

help updated it to only including function for support of custom android command.
Please help to review this.