Add git/gerrit-on-borg utilities.

Add git/gerrit-on-borg utilities.

gob_util.py is a general-purpose library for communicating with the
gerrit-on-borg service.

testing_support/gerrit_test_case.py is a unittest framework for
testing code that interacts with gerrit.

R=vadimsh@chromium.org, cmp@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228965

[szager, 2013-10-10 19:12:08]

Vadim and I spoke offline about the difficulty of versioning this code if we
update chromite to use it.  I'm punting on that question for now: chromite can
continue to use its own gob_util.py, but other (non-chromeos) projects can use
this gerrit_util.py.

In the future, we may:

- Move the gerrit tools into their own repository that can be pinned in DEPS
and/or a manifest.

- Stabilize the gerrit_util.py interface enough that chromeos can import it from
tip-of-tree depot_tools without fear.  Note that the chromite unit tests already
rely on depot_tools/testing_support/gerrit-init.sh, so we've already taken steps
in this direction.

- Do nothing; chromite will continue to use its own implementation of this
functionality.


In any case, we want to move quickly towards migrating other projects to
gerrit-on-borg, so we need to have this functionality available.

[cmp, 2013-10-10 19:24:04]

On 2013/10/10 19:12:08, szager wrote:
> Vadim and I spoke offline about the difficulty of versioning this code if we
> update chromite to use it.  I'm punting on that question for now: chromite can
> continue to use its own gob_util.py, but other (non-chromeos) projects can use
> this gerrit_util.py.
> 
> In the future, we may:
> 
> - Move the gerrit tools into their own repository that can be pinned in DEPS
> and/or a manifest.

+1, let's plan to do this.

[Vadim Sh., 2013-10-10 21:55:46]

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode72
gerrit_util.py:72: conn = httplib.HTTPSConnection(host)
Can we choose between HTTPConnection and HTTPSConnection based on
GERRIT_PROTOCOL value?

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode353
gerrit_util.py:353: '%s label set to %s programmatically by chromite.' % (label,
value))
'by chromite' part is no longer correct.

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode358
gerrit_util.py:358: for review in jmsg.get('labels', {}).get('Commit-Queue',
{}).get('all', []):
Commit-Queue is ChromeOS specific label. Shouldn't it be just |label|?

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
File testing_support/gerrit_test_case.py (right):

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:46: import mock
Is it always available? Is it a requirement for gerrit_test_case.py to work?

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:60: import gerrit_util
I'm assume depot_tools/ is in sys.path? gerrit_test_case.py and gerrit_util.py
are in different directories.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:66: TEARDOWN = True
I also find auth.type = DEVELOPMENT_BECOME_ANY_ACCOUNT in gerrit.config very
helpful. It allows to login into Gerrit UI under whatever account I want.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:115: os.path.dirname(__file__),
'gerrit-init.sh')
__file__ can be a path relative to a cwd when python loaded this module. If cwd
changed since then it won't work.

Convert __file__ into absolute path when loading a module, i.e. on top of this
module:

THIS_DIR = os.path.dirname(os.path.abspath(__file__))

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:218: s = StringIO(response.read())
Can it use ReadHttpJsonResponse?

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:224: def _CloneProject(cls, name, path):
I see 3 different naming styles:
1) createProject
2) _CloneProject
3) _stop_gerrit

I personally don't like this. It's probably fine as long as public API is using
same style (it does).

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:285: 
why is this new line here?

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:414: cls.check_call(
Are we OK with touching the network?

We can probably find repo repository locally in .repo directory.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:419: cls.check_call(['git', 'clone', '-n',
cls.REPO_URL, repo_clone_path])
Maybe clone from mirror created above?

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:456: proj = 'testproj%d' % i
Can this bookkeeping be extracted into separate function used by both
_CloneProject and this code?

[szager, 2013-10-10 23:58:06]

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py
File gerrit_util.py (right):

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode72
gerrit_util.py:72: conn = httplib.HTTPSConnection(host)
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Can we choose between HTTPConnection and HTTPSConnection based on
> GERRIT_PROTOCOL value?

Great idea; done.

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode353
gerrit_util.py:353: '%s label set to %s programmatically by chromite.' % (label,
value))
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> 'by chromite' part is no longer correct.

Done.

https://codereview.chromium.org/26399002/diff/8001/gerrit_util.py#newcode358
gerrit_util.py:358: for review in jmsg.get('labels', {}).get('Commit-Queue',
{}).get('all', []):
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Commit-Queue is ChromeOS specific label. Shouldn't it be just |label|?

Yes, thanks, I fixed this in chromite already, but didn't copy it over.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
File testing_support/gerrit_test_case.py (right):

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:46: import mock
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Is it always available? Is it a requirement for gerrit_test_case.py to work?

Fair point; I got rid of all the mock stuff.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:60: import gerrit_util
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> I'm assume depot_tools/ is in sys.path? gerrit_test_case.py and gerrit_util.py
> are in different directories.

Just to be safe, derive path to gerrit_util.py from __file__.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:66: TEARDOWN = True
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> I also find auth.type = DEVELOPMENT_BECOME_ANY_ACCOUNT in gerrit.config very
> helpful. It allows to login into Gerrit UI under whatever account I want.

Added this to gerrit-init.sh

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:115: os.path.dirname(__file__),
'gerrit-init.sh')
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> __file__ can be a path relative to a cwd when python loaded this module. If
cwd
> changed since then it won't work.
> 
> Convert __file__ into absolute path when loading a module, i.e. on top of this
> module:
> 
> THIS_DIR = os.path.dirname(os.path.abspath(__file__))

Done.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:218: s = StringIO(response.read())
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Can it use ReadHttpJsonResponse?

The only problem is that ReadHttpJsonResponse demands an http status of 200 (or
404 in some cases).  This is one of those weird cases where the status is code
is something other than 200 on success.

I updated ReadHttpResponse and ReadHttpJsonResponse to add an
'expect_status=200' parameter, to handle this more gracefully.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:224: def _CloneProject(cls, name, path):
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> I see 3 different naming styles:
> 1) createProject
> 2) _CloneProject
> 3) _stop_gerrit
> 
> I personally don't like this. It's probably fine as long as public API is
using
> same style (it does).

Yeah, I'm open to suggestions here.  Basically:

_underscore_linux_hacker_style is used for purely internal methods that should
not be public.

_underscoreCamelCase is used for classmethods that shadow an instance method
without reference to self.  These methods could conceivably be called from
elsewhere, but it would be unusual to do so.  I could see changing these to
UpperCamelCase if you think that would be cleaner or clearer.

regularCamelCase is used for public methods that can be invoked via
self.method().  Sometimes, that's a true instance method; other times, it's a
classmethod.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:285: 
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> why is this new line here?

Removed.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:414: cls.check_call(
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Are we OK with touching the network?
> 
> We can probably find repo repository locally in .repo directory.

I also prefer that unit tests don't touch the network.  However, we've already
decided that gerrit-init.sh will download gerrit, so that principle is already
abandoned.

There is no reason to assume that this test code will be invoked from an
existing repo working directory.  Indeed, it's probably better if it's *not*
called from an existing repo checkout.

This could be improved very slightly by first invoking depot_tools/repo
(because, of course, depot_tools must be present); and then mirroring the
REPO_URL defined there.  But it hardly seems worth the effort to do that.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:419: cls.check_call(['git', 'clone', '-n',
cls.REPO_URL, repo_clone_path])
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Maybe clone from mirror created above?

Ah yes, that was the intention.  Fixed.

https://codereview.chromium.org/26399002/diff/8001/testing_support/gerrit_tes...
testing_support/gerrit_test_case.py:456: proj = 'testproj%d' % i
On 2013/10/10 21:55:47, Vadim Sh. wrote:
> Can this bookkeeping be extracted into separate function used by both
> _CloneProject and this code?

Done.

[Vadim Sh., 2013-10-11 18:25:38]

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
File testing_support/gerrit_test_case.py (right):

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
testing_support/gerrit_test_case.py:64: def _import_gerrit_util():
It's too complicated :( 

I looked at existing files in testing_support/, they already assume depot_tools
is in path (this one for example:
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/testing_support...)

Also imp.load_module reloads a module if it was already loaded. It may be
undesirable (if module's globals were modified for instance).

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
testing_support/gerrit_test_case.py:115: def getproto(cls):
It's not used.

[szager, 2013-10-11 18:48:37]

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
File testing_support/gerrit_test_case.py (right):

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
testing_support/gerrit_test_case.py:64: def _import_gerrit_util():
On 2013/10/11 18:25:39, Vadim Sh. wrote:
> It's too complicated :( 
> 
> I looked at existing files in testing_support/, they already assume
depot_tools
> is in path (this one for example:
>
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/testing_support...)
> 
> Also imp.load_module reloads a module if it was already loaded. It may be
> undesirable (if module's globals were modified for instance).

OK, back to plan A then.

https://codereview.chromium.org/26399002/diff/14001/testing_support/gerrit_te...
testing_support/gerrit_test_case.py:115: def getproto(cls):
On 2013/10/11 18:25:39, Vadim Sh. wrote:
> It's not used.

Whoops, I put that in while testing the import code that I just ripped out.

[szager1, 2013-10-14 21:14:41]

ping

[Vadim Sh., 2013-10-15 18:03:15]

lgtm

[szager1, 2013-10-16 19:45:37]

Committed patchset #5 manually as r228965.