Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(175)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 802403003: presubmit_unittest: Fix pylint errors. (Closed)

Created:
6 years ago by Raphael Kubo da Costa (rakuco)
Modified:
6 years ago
Reviewers:
iannucci, M-A Ruel, pgervais
CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master
Target Ref:
refs/heads/master
Project:
tools
Visibility:
Public.

Description

presubmit_unittest: Fix pylint errors. The recent pylint 1.3.1 and 1.4.0 upgrades have caused some new errors to be reported for presubmit_unittest: * presubmit_support.InputApi.AffectedFiles() expectes a parameter called |include_deletes|, not |include_deleted|. * The mock AffectedFiles() implementation in CannedChecksUnittest.testCannedCheckChangeHasNoTabs() had its signature updated to match the one in presubmit_support.InputApi, otherwise pylint would (erroneously) consider that this mock implementation was used in all other AffectedFiles() invocations in CannedChecksUnittest and complain that some parameters were missing. It makes more sense to do this than disable the check and miss real problems in the future. R=maruel@chromium.org, iannucci@chromium.org, pgervais@chromium.org BUG=443232 Committed: 293468

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M tests/presubmit_unittest.py View 3 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 15 (4 generated)
Raphael Kubo da Costa (rakuco)
6 years ago (2014-12-18 14:13:17 UTC) #1
M-A Ruel
eh, lgtm
6 years ago (2014-12-19 16:29:41 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/802403003/1
6 years ago (2014-12-19 16:30:11 UTC) #4
commit-bot: I haz the power
Presubmit check for 802403003-1 failed and returned exit status 1. Running presubmit commit checks ...
6 years ago (2014-12-19 16:33:09 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/802403003/1
6 years ago (2014-12-19 16:42:11 UTC) #8
commit-bot: I haz the power
Presubmit check for 802403003-1 failed and returned exit status 1. Running presubmit commit checks ...
6 years ago (2014-12-19 16:45:06 UTC) #10
pgervais
On 2014/12/19 16:45:06, I haz the power (commit-bot) wrote: > Presubmit check for 802403003-1 failed ...
6 years ago (2014-12-19 16:48:31 UTC) #11
Raphael Kubo da Costa (rakuco)
Committed patchset #1 (id:1) manually as r293468.
6 years ago (2014-12-19 17:04:17 UTC) #12
Raphael Kubo da Costa (rakuco)
On 2014/12/19 16:48:31, pgervais wrote: > It seems we have a bug in the test ...
6 years ago (2014-12-19 17:21:10 UTC) #13
pgervais
On 2014/12/19 17:21:10, Raphael Kubo da Costa (rakuco) wrote: > On 2014/12/19 16:48:31, pgervais wrote: ...
6 years ago (2014-12-19 17:47:04 UTC) #14
pgervais
6 years ago (2014-12-19 23:24:09 UTC) #15
Message was sent while issue was closed. 
On 2014/12/19 17:47:04, pgervais wrote:
> On 2014/12/19 17:21:10, Raphael Kubo da Costa (rakuco) wrote:
> > On 2014/12/19 16:48:31, pgervais wrote:
> > > It seems we have a bug in the test suite. The solution is to fix the
> > expectation
> > > in tests/git_commit_test.py:418 and replace 
> > > ('30d6fd5', 'master', 1, None)
> > > by 
> > > BranchesInfo(hash='30d6fd5', upstream='master', ahead= [truncated]...
> > > 
> > > This can be done in another CL if you want. Also feel free to ask someone
> else
> > > to do it.
> > 
> > I then get an actual failure:
> > ======================================================================
> > FAIL: testGetBranchesInfo (__main__.GitMutableFunctionsTest)
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> >   File "tests/git_common_test.py", line 423, in testGetBranchesInfo
> >     self.assertEquals(expected, actual)
> > AssertionError: {'': None, 'happybranch': BranchesInfo(hash='d8e473a',
> > upstream='master', ahead= [truncated]... != {'': None, 'happybranch':
> > BranchesInfo(hash='d8e473a', upstream='master', ahead= [truncated]...
> >   {'': None,
> >    'child': BranchesInfo(hash='d8e473a', upstream='happybranch', ahead=None,
> > behind=None),
> >    'happybranch': BranchesInfo(hash='d8e473a', upstream='master', ahead=1,
> > behind=None),
> >    'master': BranchesInfo(hash='1f12c7b', upstream='', ahead=None,
> behind=None),
> > -  'parent_gone': BranchesInfo(hash='1f12c7b', upstream='to_delete',
ahead=1,
> > behind=None),
> > ?                                                                          ^
 
>  
> >     ^^^^
> > 
> > +  'parent_gone': BranchesInfo(hash='1f12c7b', upstream='to_delete',
> > ahead=27926032, behind=170),
> > ?                                                                         
> > ^^^^^^^^         ^^^
> > 
> >    'to_delete': None}
> 
> I would say 'just tweak the expected value until test test passes' but I'm not
> sure it's the best thing to do here. In particular, there could be an actual
> error. iannucci@ is probably the person to talk to here.

Closing the loop after offline discussions and further investigation. 
The failure here is apparently caused by a bug in git 2.1.3 which is running on
the bots. 
This is why we get the obviously wrong value: ahead=27926032, behind=170.

The test passes with git 2.2.0 (and hopefully with 2.2.1). It also seems that it
passes with 1.9, but it has not been checked.

So the easiest fix is to upgrade to git 2.2.0, we'll take care of that for the
bots.

Powered by Google App Engine
This is Rietveld 408576698