Code review comment for lp:~le-chi-thu/linaro-ci/add-testplan-support

Thanks for the changes. Please see my comments inline.

On Fri, May 4, 2012 at 7:38 PM, Le Chi Thu <email address hidden> wrote:

> Le Chi Thu has proposed merging
> lp:~le-chi-thu/linaro-ci/add-testplan-support into lp:linaro-ci.
>
> Requested reviews:
> Deepti B. Kalakeri (deeptik)
>
> For more details, see:
>
> https://code.launchpad.net/~le-chi-thu/linaro-ci/add-testplan-support/+merge/104743<https://code.launchpad.net/%7Ele-chi-thu/linaro-ci/add-testplan-support/+merge/104743>
>
> Add support to define test plan for lava.
>
> the format is : LAVA_TEST_PLAN=<testcase>(<options>):<timeout>,...
> options and timeout is optional.
>
> Examples:
> LAVA_TEST_PLAN=ltp,pwrmgmt
> LAVA_TEST_PLAN=test_abc(option_abc):100
>

Could you give me an example of the options_abc ?

>
> if the LAVA_TEST_PLAN is not define, the default will be "ltp,pwrmgmt". It
> mean the current jobs do not need to update.
>

The default test that needs to be run should as well include
boot_linaro_image. Without booting the image how will you be able to run
the tests ?

> --
>
> https://code.launchpad.net/~le-chi-thu/linaro-ci/add-testplan-support/+merge/104743<https://code.launchpad.net/%7Ele-chi-thu/linaro-ci/add-testplan-support/+merge/104743>
> You are requested to review the proposed merge of
> lp:~le-chi-thu/linaro-ci/add-testplan-support into lp:linaro-ci.
>
> === modified file 'get_latest_ci_hwpack'
> --- get_latest_ci_hwpack 2012-04-04 12:14:43 +0000
> +++ get_latest_ci_hwpack 2012-05-04 14:07:20 +0000
> @@ -5,6 +5,7 @@
>
> Usage: ./gentemplate <target> <hwpack_type> <image_type>
> """
> +import re
>
> from find_latest import find_latest_hwpack, find_latest_rootfs,
> find_ci_latest
> import json
> @@ -15,8 +16,7 @@
> job_name = os.getenv("KERNEL_JOB_NAME", "unknown") + '_' + \
> os.getenv("HWPACK_BUILD_DATE", "unknown") + '_daily test'
>
> -
> -
> +testplan = os.getenv("LAVA_TEST_PLAN", "ltp,pwrmgmt")
>
> template = {
> "timeout": 20000,
> @@ -25,36 +25,6 @@
> "command": "deploy_linaro_image"
> },
> {
> - "command": "lava_test_install",
> - "parameters":
> - {
> - "tests": [
> - "ltp",
> - "pwrmgmt"
> - ],
> - "timeout": 4000
> - }
> - },
> - {
> - "command": "boot_linaro_image"
> - },
>

This should not be deleted, as we need to boot the image and then can we
run the tests.

> - {
> - "command": "lava_test_run",
> - "parameters":
> - {
> - "test_name": "ltp",
> - "timeout": 4800
> - }
> - },
> - {
> - "command": "lava_test_run",
> - "parameters":
> - {
> - "test_name": "pwrmgmt",
> - "timeout": 4800
> - }
> - },
> - {
> "command": "submit_results",
> "parameters": {
> "stream": os.getenv("BUNDLE_STREAM_NAME", "/anonymous/plars/"),
> @@ -71,6 +41,9 @@
> hwpack_dir_url=os.getenv("KERNEL_JOB_NAME", "unknown")
> hwpack_url_path = os.path.join(url, hwpack_parent_url)
> hwpack_url_path = os.path.join(hwpack_url_path, hwpack_dir_url)
> +
> +
> +
>

Please delete the blank lines, you can leave a single blank line if
required and follow it with a comment
explaining the next section details.

> hwpack_url = find_ci_latest(hwpack_url_path)
> rootfs_id, rootfs_url = find_latest_rootfs(imagetype)
>
> @@ -101,4 +74,38 @@
>
> template['actions'][0]['metadata'] = metadata
>
> +testcases = testplan.split(',')
> +
> +regex =
> re.compile("(?P<testcase>\w+)(\((?P<options>[^\)]+)\))?(:(?P<timeout>\d+))?")
>

The regex above should not match LAVA_TEST_PLAN=":100, test_bcd".
The above regex matches :100 as testcase which is invalid.

+
> +# append lava_test_install command
> +
> +lava_test_install_command = {}
> +lava_test_install_command["command"] = "lava_test_install"
> +lava_test_install_command["parameters"] = { "tests" : [], "timeout" :
> 4000 }
> +
> +for testcase in testcases:
> + r = regex.search(testcase)
> +
> lava_test_install_command["parameters"]["tests"].append(r.group("testcase"))
> +
> +template['actions'].append(lava_test_install_command)
> +
> +# append lava_test_run commands
> +
> +for testcase in testcases:
> + r = regex.search(testcase)
> +
> + timeout = 4800
> + if r.group('timeout'):
> + timeout = r.group('timeout')
> +
> + lava_test_run_command = {}
> + lava_test_run_command["command"] = "lava_test_run"
> + lava_test_run_command["parameters"] = { "test_name" :
> r.group('testcase'), "timeout" : timeout }
>

Try to restrict the lines to 80 column rule.

> +
> + if r.group("options"):
> + lava_test_run_command["parameters"]["test_options"] =
> r.group("options")
> +
> + template['actions'].append(lava_test_run_command)
> +
>

It would be good if combined both the for loops into one for loop.

> print json.dumps(template, indent=2)
>
>
>
There is a new version of get_ci_hwpack available now, please rebase your
changes on top of it.

--
Thanks and Regards,
Deepti