Code review comment for lp://qastaging/~liuyq0307/linaro-android-build-tools/support-timeout

Revision history for this message
Yongqin Liu (liuyq0307) wrote :

> Hi Yongqin,
>
> thanks for working on this. Overall the merge proposal looks good to me, just
> a couple of really small comments.
>
> On Fri, Jun 15, 2012 at 10:44 AM, Paul Sokolovsky <email address hidden>
> wrote:
> >
> > +    # Set the default timeout for all test,
> > +    # if this value it not set, then use the 3600 seconds as the default
> value
> > +    default_timeout = os.environ.get("DEFAULT_TIMEOUT", 3600)

> Just a small typo here: s/it not/is not
> Also, even if not introduced by you, can you please fix these two errors from
> pep8:
>
> post-build-lava.py:44:15: E203 whitespace before ':'
> post-build-lava.py:185:80: E501 line too long (100 characters)

Thanks, the above typo and this two lines have been updated.

> >     config_json = {"job_name": build_url,
> >                    "image_type": 'android',
> > -                   "timeout": 18000,
> > +                   "timeout": int(default_timeout),
> >                    "actions": actions
> >                   }
>
> The old timeout here was set to 18000 seconds, now you set a default value of
> 3600: has it been discussed somewhere to lower the timeout?
For this, I have asked about it in lava team, the timeout is not used before.
This is the first time that it will be used.
And from the tests we have now, mainly less than 1 hour.
So I set it to 3600.

But it's seems set it to 18000 is more reasonable.
1. keep it as it was before, although not used before
2. only the CTS test need to be specifically set.
   **The CTS test will need more time, about 10H.

Now I changed it to 18000

« Back to merge proposal