Handle the PrimeLint SSL read timeout better.

Handle the PrimeLint SSL read timeout better.

It takes more than 10 seconds to lint some CLs. Silently
ignore the expected timeout.

R=maruel@chromium.org,iannucci@chromium.org
BUG=290402
TEST=none

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

[M-A Ruel, 2014-01-09 02:08:14]

I'll let Robbie decide.

https://codereview.chromium.org/130493002/diff/1/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130493002/diff/1/gcl.py#newcode404
gcl.py:404: try:
You could remove the call completely.

[wtc, 2014-01-09 15:07:34]

https://codereview.chromium.org/130493002/diff/1/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130493002/diff/1/gcl.py#newcode404
gcl.py:404: try:

On 2014/01/09 02:08:14, M-A Ruel wrote:
> You could remove the call completely.

Does "git cl upload" also run lint job on Rietveld? If not, I guess gcl.py
doesn't need to do this.

Do you know if '/lint/issue%s_%s' really lints the files in the background, as
the comment on line 401 and the function name "PrimeLint" imply? The root cause
of this bug may be that '/lint/issue%s_%s' is waiting for the completion of
lint.

[M-A Ruel, 2014-01-09 15:15:35]

On 2014/01/09 15:07:34, wtc wrote:
> https://codereview.chromium.org/130493002/diff/1/gcl.py
> File gcl.py (right):
> 
> https://codereview.chromium.org/130493002/diff/1/gcl.py#newcode404
> gcl.py:404: try:
> 
> On 2014/01/09 02:08:14, M-A Ruel wrote:
> > You could remove the call completely.
> 
> Does "git cl upload" also run lint job on Rietveld? If not, I guess gcl.py
> doesn't need to do this.

No it doesn't.

> Do you know if '/lint/issue%s_%s' really lints the files in the background, as
> the comment on line 401 and the function name "PrimeLint" imply? The root
cause
> of this bug may be that '/lint/issue%s_%s' is waiting for the completion of
> lint.

I never touched the linting code personally.

[wtc, 2014-01-09 17:58:27]

Please review patch set 2. Thanks.

https://codereview.chromium.org/130493002/diff/1/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130493002/diff/1/gcl.py#newcode404
gcl.py:404: try:

Thanks for the reply.

I suspect the "background work" and the "PrimeLint" function name are referring
to the fact that we invoke '/lint/issue%s_%s' with a short timeout. The timeout
needs to be long enough to ensure the lint job gets started, but we are not
interested in waiting for the completion of lint.

So I've changed the timeout back to 10 seconds, and just silently ignore the
expected timeout error.

[iannucci, 2014-01-09 18:59:10]

On 2014/01/09 17:58:27, wtc wrote:
> Please review patch set 2. Thanks.
> 
> https://codereview.chromium.org/130493002/diff/1/gcl.py
> File gcl.py (right):
> 
> https://codereview.chromium.org/130493002/diff/1/gcl.py#newcode404
> gcl.py:404: try:
> 
> Thanks for the reply.
> 
> I suspect the "background work" and the "PrimeLint" function name are
referring
> to the fact that we invoke '/lint/issue%s_%s' with a short timeout. The
timeout
> needs to be long enough to ensure the lint job gets started, but we are not
> interested in waiting for the completion of lint.
> 
> So I've changed the timeout back to 10 seconds, and just silently ignore the
> expected timeout error.

The linting code is pretty dodgy, I have this on my TODO list of things to
improve.

This should really be done with a taskqueue task when a patchset is added, and
not by the client.

Anyway, this lgtm for now.

[commit-bot: I haz the power, 2014-01-09 20:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/130493002/80001

[commit-bot: I haz the power, 2014-01-09 20:04:41]

Presubmit check for 130493002-80001 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 **
tests/gcl_unittest.py (1.06s) failed
.........................['ssl']
F.
======================================================================
FAIL: testMembersChanged (__main__.GclUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/third_party/pymox/mox.py",
line 1618, in new_method
    func(self, *args, **kwargs)
  File "tests/gcl_unittest.py", line 115, in testMembersChanged
    self.compareMembers(gcl, members)
  File "/b/commit-queue/workdir/tools/depot_tools/testing_support/super_mox.py",
line 78, in compareMembers
    self.assertEqual(actual_members, expected_members)
AssertionError: Lists differ: ['CMDchange', 'CMDchanges', 'C... != ['CMDchange',
'CMDchanges', 'C...

First differing element 71:
ssl
string

First list contains 1 additional elements.
First extra element 78:
urllib2

Diff is 1458 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 27 tests in 0.059s

FAILED (failures=1)


Presubmit checks took 183.8s to calculate.

[commit-bot: I haz the power, 2014-01-09 20:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/130493002/150001

[commit-bot: I haz the power, 2014-01-09 20:41:56]

Change committed as 243968

[wtc, 2014-01-09 20:46:47]

Robbie: I had to add 'ssl' to tests/gcl_unittest.py to pass the presubmit
checks. Please do a post-commit review of patch set 3. Thanks.

[iannucci, 2014-01-09 22:27:20]

https://codereview.chromium.org/130493002/diff/150001/tests/gcl_unittest.py
File tests/gcl_unittest.py (right):

https://codereview.chromium.org/130493002/diff/150001/tests/gcl_unittest.py#n...
tests/gcl_unittest.py:111: 'ssl', 'string', 'subprocess2', 'sys', 'tempfile',
'time',
LOL this test is so bogus....

lgtm