Added a default timeout to rietveld requests.

Added a default timeout to rietveld requests.

BUG=345117
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=252452

[Paweł Hajdan Jr., 2014-02-21 00:49:29]

Looks good, some comments.

https://codereview.chromium.org/174313002/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/174313002/diff/1/rietveld.py#newcode370
rietveld.py:370: # Code below assumes timeout=None by default; make sure it's
set
nit: Let's be more precise and say rpc_server (or the class name) instead of
"code below", especially that the distance is fairly big so it's not obvious
which code this is referring to and why the code in question just wasn't fixed.

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py
File tests/rietveld_test.py (right):

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py#newco...
tests/rietveld_test.py:18: from testing_support import auto_stub
nit: Please put it in one "block" with "from testing_support.patches_data"
import above.

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py#newco...
tests/rietveld_test.py:455: except ProbeException as e:
Use assertRaises instead. This also allows you to inspect the exception, but
seems more idiomatic. Same below.

[Sergey Berezin, 2014-02-21 01:30:56]

https://codereview.chromium.org/174313002/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/174313002/diff/1/rietveld.py#newcode370
rietveld.py:370: # Code below assumes timeout=None by default; make sure it's
set
On 2014/02/21 00:49:30, Paweł Hajdan Jr. wrote:
> nit: Let's be more precise and say rpc_server (or the class name) instead of
> "code below", especially that the distance is fairly big so it's not obvious
> which code this is referring to and why the code in question just wasn't
fixed.

Done.

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py
File tests/rietveld_test.py (right):

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py#newco...
tests/rietveld_test.py:18: from testing_support import auto_stub
On 2014/02/21 00:49:30, Paweł Hajdan Jr. wrote:
> nit: Please put it in one "block" with "from testing_support.patches_data"
> import above.

Done.

https://codereview.chromium.org/174313002/diff/1/tests/rietveld_test.py#newco...
tests/rietveld_test.py:455: except ProbeException as e:
On 2014/02/21 00:49:30, Paweł Hajdan Jr. wrote:
> Use assertRaises instead. This also allows you to inspect the exception, but
> seems more idiomatic. Same below.

Done.

[Paweł Hajdan Jr., 2014-02-21 01:40:20]

LGTM

[Sergey Berezin, 2014-02-21 01:41:07]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyberezin@chromium.org/174313002/6...

[commit-bot: I haz the power, 2014-02-21 01:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:43:54]

Presubmit check for 174313002-60001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/rietveld.py
    depot_tools/tests/rietveld_test.py

Presubmit checks took 161.3s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.
[85258 refs]

[Dirk Pranke, 2014-02-21 01:52:37]

lgtm

[Sergey Berezin, 2014-02-21 01:53:11]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:53:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyberezin@chromium.org/174313002/6...

[commit-bot: I haz the power, 2014-02-21 01:55:32]

Change committed as 252452