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