Refactor infra git libs and testing.

Refactor infra git libs and testing.

Takes advantage of new expect_tests features like autoloading and per-Test
coverage.

All 88 tests now run just by invoking `./test.py` in the root, and all
test names match the natural namespace of their packages.

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

[Vadim Sh., 2014-06-27 18:26:22]

So are we using rietveld or gerrit?

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/_...
File infra/libs/data_structures/__init__.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/_...
infra/libs/data_structures/__init__.py:5: from
infra.libs.data_structures.data_structures import freeze, thaw, FrozenDict
I think data_structures is toooo long.

infra.libs.types?
infra.libs.collections?

dunno...

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
File infra/libs/data_structures/data_structures.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:4: import collections
nit: \n

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:8: def freeze(obj):
if |obj| is already frozen, it makes a copy. In theory it's not necessary.

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:9: """Takes a jsonish object
|obj|, and returns an immutable version of it."""
There's no asserts anywhere that verify it's 'jsonish' object. I can pass object
of some user defined classes here as long as they define __hash__.

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:12: elif isinstance(obj, list):
tuples with non-frozen items will not be frozen.

elif isinstance(obj, (list, tuple))?

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:17: hash(obj)
assert getattr(obj, '__hash__' , None), 'Can\'t freeze unhashable type'

no need to actually calculate the hash, right?

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:27: return list(thaw(i) for i in
obj)
distinction between tuples and lists will be lost after freeze\thaw :(

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/d...
infra/libs/data_structures/data_structures.py:41: self._hash =
reduce(operator.xor,
Why not make it lazy?

https://codereview.chromium.org/355153002/diff/1/infra/libs/decorators/decora...
File infra/libs/decorators/decorators.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/decorators/decora...
infra/libs/decorators/decorators.py:5: class cached_property(object):
Mention somewhere that it's not thread safe.

https://codereview.chromium.org/355153002/diff/1/infra/libs/decorators/decora...
infra/libs/decorators/decorators.py:33: self.__name__ = fn.__name__
Use functools.update_wrapper, it's exactly what it was added for.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/__init__.py
File infra/libs/git2/__init__.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/__init__.py#...
infra/libs/git2/__init__.py:5: from infra.libs.git2.util import INVALID
why git2? :) Do we have git 1 in infra/infra? Alternatives: 'git', 'gitlib'..
dunno.

[agable, 2014-06-27 18:53:13]

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/t...
File infra/libs/data_structures/test/data_structures_test.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/data_structures/t...
infra/libs/data_structures/test/data_structures_test.py:73:
self.assertIsInstance(t, set)
Add TestFreezeThaw and assert that the start/end states are the same.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/__init__.py
File infra/libs/git2/__init__.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/__init__.py#...
infra/libs/git2/__init__.py:5: from infra.libs.git2.util import INVALID
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> why git2? :) Do we have git 1 in infra/infra? Alternatives: 'git', 'gitlib'..
> dunno.

Agreed -- I think now is the time to absorb git into these, and fix lkgr-finder
to use these instead.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/__init__.py#...
infra/libs/git2/__init__.py:10: from infra.libs.git2.git2 import Repo
Inconsistent between listing these all on their own lines vs. all on one line as
you do in data_structures/ and decorators/.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/config_ref.py
File infra/libs/git2/config_ref.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/config_ref.p...
infra/libs/git2/config_ref.py:8: 
nit: no newline

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/config_ref.p...
infra/libs/git2/config_ref.py:10: 
nit: two newlines.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/data/commit.py
File infra/libs/git2/data/commit.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/data/commit....
infra/libs/git2/data/commit.py:14: from infra.libs.git2.data.data import
Alterable
Do you really need data.data? Couldn't you just do infra.libs.git2.data and use
the __init__ like everyone else?

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/config_...
File infra/libs/git2/test/config_ref_test.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/config_...
infra/libs/git2/test/config_ref_test.py:6: 
nit: no newlines

I thought our policy was:
* builtin imports
* from builtin imports
* local imports
* from local imports

All of these are local package imports; no need for newlines between them.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/config_...
infra/libs/git2/test/config_ref_test.py:10: 
nit: two newlines

Also I'm not gonna comment on every instance of this from here on out :P

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/git2_te...
File infra/libs/git2/test/git2_test.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/git2_te...
infra/libs/git2/test/git2_test.py:23: O Z
Is Z a merge commit of H and O?

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/util_te...
File infra/libs/git2/test/util_test.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/test/util_te...
infra/libs/git2/test/util_test.py:16: self.assertTrue(not (util.INVALID ==
util.INVALID))
assertFalse

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/util.py
File infra/libs/git2/util.py (right):

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/util.py#newc...
infra/libs/git2/util.py:4: from cStringIO import StringIO
nit: newline

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/util.py#newc...
infra/libs/git2/util.py:7: class _Invalid(object):
I would consider putting INVALID in Data, but this works too.

https://codereview.chromium.org/355153002/diff/1/infra/libs/git2/util.py#newc...
infra/libs/git2/util.py:59: 
nit: no newline

https://codereview.chromium.org/355153002/diff/1/test.py
File test.py (right):

https://codereview.chromium.org/355153002/diff/1/test.py#newcode11
test.py:11: from testing_support import expect_tests  # pylint: disable=F0401
I will make this MAGICAL

[iannucci, 2014-06-28 08:33:35]

ptal

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
File infra/libs/data_structures/__init__.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/__init__.py:5: from
infra.libs.data_structures.data_structures import freeze, thaw, FrozenDict
On 2014/06/27 18:26:21, Vadim Sh. wrote:
> I think data_structures is toooo long.
> 
> infra.libs.types?
> infra.libs.collections?
> 
> dunno...

types sgtm

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
File infra/libs/data_structures/data_structures.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:4: import collections
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> nit: \n

Done.

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:8: def freeze(obj):
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> if |obj| is already frozen, it makes a copy. In theory it's not necessary.

Don't think this is the case

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:9: """Takes a jsonish object
|obj|, and returns an immutable version of it."""
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> There's no asserts anywhere that verify it's 'jsonish' object. I can pass
object
> of some user defined classes here as long as they define __hash__. 

Clarified in the comment

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:12: elif isinstance(obj, list):
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> tuples with non-frozen items will not be frozen.
> 
> elif isinstance(obj, (list, tuple))?

Done.

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:17: hash(obj)
On 2014/06/27 18:26:21, Vadim Sh. wrote:
> assert getattr(obj, '__hash__' , None), 'Can\'t freeze unhashable type'
> 
> no need to actually calculate the hash, right?

it recurses, so 'guarantees' that all objects inside are also hashable.

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:27: return list(thaw(i) for i in
obj)
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> distinction between tuples and lists will be lost after freeze\thaw :(

disinclined to care... that seems like a much harder problem and these are
supposed to just be light helper methods. :)

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/data_struc...
infra/libs/data_structures/data_structures.py:41: self._hash =
reduce(operator.xor,
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> Why not make it lazy?

Added comment

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/decorators...
File infra/libs/decorators/decorators.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/decorators...
infra/libs/decorators/decorators.py:5: class cached_property(object):
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> Mention somewhere that it's not thread safe.

Done.

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/decorators...
infra/libs/decorators/decorators.py:33: self.__name__ = fn.__name__
On 2014/06/27 18:26:22, Vadim Sh. wrote:
> Use functools.update_wrapper, it's exactly what it was added for.

Done.

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/git2/__ini...
File infra/libs/git2/__init__.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/git2/__ini...
infra/libs/git2/__init__.py:5: from infra.libs.git2.util import INVALID
On 2014/06/27 18:53:11, agable wrote:
> On 2014/06/27 18:26:22, Vadim Sh. wrote:
> > why git2? :) Do we have git 1 in infra/infra? Alternatives: 'git',
'gitlib'..
> > dunno.
> 
> Agreed -- I think now is the time to absorb git into these, and fix
lkgr-finder
> to use these instead.

I'd rather do in a followup CL. AFAIK, neither lkgr nor git have any tests, so
I'd rather refactor them separately (preferably after, you know, adding tests
:p).

https://chromiumcodereview.appspot.com/355153002/diff/1/infra/libs/git2/__ini...
infra/libs/git2/__init__.py:10: from infra.libs.git2.git2 import Repo
On 2014/06/27 18:53:11, agable wrote:
> Inconsistent between listing these all on their own lines vs. all on one line
as
> you do in data_structures/ and decorators/.

I'm torn, but looks like google python style says 1/line, so I'll do that:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Import...

[Vadim Sh., 2014-06-30 17:24:12]

lgtm
but wait for Aaron too

https://codereview.chromium.org/355153002/diff/60001/infra/ext/__init__.py
File infra/ext/__init__.py (right):

https://codereview.chromium.org/355153002/diff/60001/infra/ext/__init__.py#ne...
infra/ext/__init__.py:26: import argcomplete
btw, you use yaml. Is pyyaml in ext?

https://codereview.chromium.org/355153002/diff/60001/infra/libs/types/__init_...
File infra/libs/types/__init__.py (right):

https://codereview.chromium.org/355153002/diff/60001/infra/libs/types/__init_...
infra/libs/types/__init__.py:5: from infra.libs.types.types import freeze
I don't understand how package __init__ works :( When is it executed? During its
execution, is the package already "defined"? And if so, looks like package
submodules should not rely on side effects of __init__.py...

[iannucci, 2014-06-30 23:03:04]

I think agable is OOO today, and I'd like to get this in... could his comments
be in a follow-up CL? I have a bunch of other stuff depending on this change :)

https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/ext/__init_...
File infra/ext/__init__.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/ext/__init_...
infra/ext/__init__.py:26: import argcomplete
On 2014/06/30 17:24:12, Vadim Sh. wrote:
> btw, you use yaml. Is pyyaml in ext?

No not yet :(. I want to add it, but there are too many CLs flying around.
Presubmit in infra is disabled at the moment though, which is why I haven't had
people throwing sharp stuff at me.

https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/libs/types/...
File infra/libs/types/__init__.py (right):

https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/libs/types/...
infra/libs/types/__init__.py:5: from infra.libs.types.types import freeze
On 2014/06/30 17:24:12, Vadim Sh. wrote:
> I don't understand how package __init__ works :( When is it executed? During
its
> execution, is the package already "defined"? And if so, looks like package
> submodules should not rely on side effects of __init__.py...

The process, IIUC, is something like:
  add empty module to sys.modules
  set default __path__ on empty module
  run code in __init__ with module as globals()
  If you see an import during execution
    find module in sys.modules
    use __path__ to understand how to find sub modules

So, if you don't need to tweak __path__, then imports just work as intended. If
you do some imports, tweak __path__ and then do other imports, the second set of
imports will pick up the change to __path__ but the first set will not.

You can even import the same module during the import of that module. As long as
you don't access a member of that module which hasn't been assigned yet, it's
fine.

I just verified this with:
  # pkg/__int__.py
    import sys
    print 'in pkg', id(sys.modules[__name__])
    import pkg.foo
    print 'in pkg', pkg.foo, id(pkg.foo)
 
  # pkg/foo/__init__.py
    import sys
    print 'in foo', id(sys.modules[__name__])
    import pkg
    print 'in foo', pkg, id(pkg)

  # run.py
  import pkg

running run.py gives
  in pkg 4554776536
  in foo 4554789000
  in foo <module 'pkg' from '/.../pkg/__init__.pyc'> 4554776536
  in pkg <module 'pkg.foo' from '/.../pkg/foo/__init__.pyc'> 4554789000

[Vadim Sh., 2014-06-30 23:04:58]

On 2014/06/30 23:03:04, iannucci wrote:
> I think agable is OOO today, and I'd like to get this in... could his comments
> be in a follow-up CL? I have a bunch of other stuff depending on this change
:)
> 
Okay. To many flying CLs irritate me too.

>
https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/ext/__init_...
> File infra/ext/__init__.py (right):
> 
>
https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/ext/__init_...
> infra/ext/__init__.py:26: import argcomplete
> On 2014/06/30 17:24:12, Vadim Sh. wrote:
> > btw, you use yaml. Is pyyaml in ext?
> 
> No not yet :(. I want to add it, but there are too many CLs flying around.
> Presubmit in infra is disabled at the moment though, which is why I haven't
had
> people throwing sharp stuff at me.
> 
>
https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/libs/types/...
> File infra/libs/types/__init__.py (right):
> 
>
https://chromiumcodereview.appspot.com/355153002/diff/60001/infra/libs/types/...
> infra/libs/types/__init__.py:5: from infra.libs.types.types import freeze
> On 2014/06/30 17:24:12, Vadim Sh. wrote:
> > I don't understand how package __init__ works :( When is it executed? During
> its
> > execution, is the package already "defined"? And if so, looks like package
> > submodules should not rely on side effects of __init__.py...
> 
> The process, IIUC, is something like:
>   add empty module to sys.modules
>   set default __path__ on empty module
>   run code in __init__ with module as globals()
>   If you see an import during execution
>     find module in sys.modules
>     use __path__ to understand how to find sub modules
> 
> So, if you don't need to tweak __path__, then imports just work as intended.
If
> you do some imports, tweak __path__ and then do other imports, the second set
of
> imports will pick up the change to __path__ but the first set will not.
> 
> You can even import the same module during the import of that module. As long
as
> you don't access a member of that module which hasn't been assigned yet, it's
> fine.
> 
> I just verified this with:
>   # pkg/__int__.py
>     import sys
>     print 'in pkg', id(sys.modules[__name__])
>     import pkg.foo
>     print 'in pkg', pkg.foo, id(pkg.foo)
>  
>   # pkg/foo/__init__.py
>     import sys
>     print 'in foo', id(sys.modules[__name__])
>     import pkg
>     print 'in foo', pkg, id(pkg)
> 
>   # run.py
>   import pkg
> 
> running run.py gives
>   in pkg 4554776536
>   in foo 4554789000
>   in foo <module 'pkg' from '/.../pkg/__init__.pyc'> 4554776536
>   in pkg <module 'pkg.foo' from '/.../pkg/foo/__init__.pyc'> 4554789000

[iannucci, 2014-06-30 23:05:06]

Oh, I see agable has unresolved comments. I'll fix those then.

[iannucci, 2014-07-24 06:13:18]

Replaced with https://codereview.chromium.org/413983003