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

Thanks for addressing the review, I have few more.

1) Please retain the boot_linaro_image stuff as it was before you made the
changes, since this block does not change, I would like to have it in the
dict itself.

2) I did not see the 80 columns rule being followed, for ex line no: 99
spans to 100 columns.

3) I guess you did not see the following comments in the previous review

(i)
"
>
re.compile("(?P<testcase>\w+)(\((?P<options>[^\)]+)\))?(:(?P<timeout>\d+))?")
>

The regex above should not match first part(:100) of the following
LAVA_TEST_PLAN=":100, test_bcd".
The above regex matches :100 as testcase which is invalid.
"

(ii)

> +
> + 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.

could you please address all of them and resubmit.

On Tue, May 8, 2012 at 5:53 PM, Le Chi Thu <email address hidden> wrote:

> Review: Resubmit
>
> ltp take options - see the $OPTIONS macro in the test definition file.
> RUNSTEPS = ['cd build && sudo ./runltp $(OPTIONS)']
>
> http://bazaar.launchpad.net/~linaro-validation/lava-test/trunk/view/head:/lava_test/test_definitions/ltp.py
>
> Now the json file is correct generate.
> --
>
> 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 reviewing the proposed merge of
> lp:~le-chi-thu/linaro-ci/add-testplan-support into lp:linaro-ci.
>

--
Thanks and Regards,
Deepti
Infrastructure Team Member, Linaro Platform Teams
Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog