[MERGE] DirState pyrex helpers

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has voted +0.
> Status is now: Semi-approved
> Comment:
> (These are in kind of random order as I tried to page around to find the
> logical top.)
> 
> +      read the entire DirState (atm, all commands that effect the working
> +      tree, ``diff``, ``status``, etc).
> 
> Please spell out 'at the moment', and it's 'affect' for 'has an effect
> on'.  It would be good to also give indicative performance numbers.
> 

Ok.

> +class TestUsingCompiledIfAvailable(tests.TestCase):
> +    """Check that any compiled functions that are available are the
> default.
> +
> +    It is possible to have typos, etc in the import line, such that
> +    _dirstate_helpers_c is actually available, but the compiled
> functions are
> +    not being used.
> +    """
> 
> I'm glad you tested it, this is a pretty tidy way to guard against a
> possible mistake.  If there were many more interfaces done this way
> it might be better to have a list of names and iterate over them, but I
> think for just four this is simpler.  Indeed maybe we should have a test
> utility for "check there are parallel python and C implementations of
> foo.{bar,qux}"
> 
> +    _test_needs_features = [CompiledDirstateHelpersFeature]
> 
> (Incidentally) this is the kind of case where I'd like to have
> TestDependencyMissing, and have that be disallowed by selftest --strict.
> (See my other rfc mail.)
>

We could do that with our existing features + test runner. If you use
requireFeature it raises UnavailableFeature and passes that to
self.addNotSupported().
If you use:
 _test_needs_features = []

bzrlib.tests.TestCase just uses addNotSupported() directly. It calls startTest
and stopTest but never calls unittest.TestCase.run().

Anyway, in both places it would be a simple flag check to turn these into
'addError'.

> +
> +class TestCompiledCmpByDirs(TestCmpByDirs):
> +    """Test the pyrex implementation of cmp_by_dirs"""
> +
> +    _test_needs_features = [CompiledDirstateHelpersFeature]
> +
> +    def get_cmp_by_dirs(self):
> +        from bzrlib._dirstate_helpers_c import cmp_by_dirs_c
> +        return cmp_by_dirs_c
> 
> Could you please add a comment just explaining that this parameterizes the
> tests by overriding get_cmp_by_dirs - like in TestCompiledBisectPathLeft.
> 

Done for all classes.

...

> +
> +    def assertPositive(self, val):
> +        """Assert that val is greater than 0."""
> +        self.assertTrue(val > 0, 'expected a positive value, but got
> %s' % val)
> +
> +    def assertNegative(self, val):
> +        """Assert that val is less than 0."""
> +        self.assertTrue(val < 0, 'expected a negative value, but got
> %s' % val)
> +
> 
> Could you please put these on TestCase, so they'll be done only once.
> 
> 

Done.

> +# We cannot import the dirstate module, because it loads this module
> +# All we really need is the IN_MEMORY_MODIFIED constant
> +from bzrlib.dirstate import DirState
> +
> +
> 
> I don't understand, won't the from/import fail if there's a circular
> import?
> 

No. The trick is just not to import anything that hasn't finished being
defined. So if you have:

foo/
  __init__.py
  bar.py
  baz.py

if in 'bar.py' you do:

class Foo:
  pass

import foo.baz

And then in foo/baz.py you do:

from foo import bar

It will fail, because 'foo.bar' hasn't been fully defined yet. But if you do:
from foo.bar import Foo

It will succeed, because the class Foo *has* been fully defined.

Note that the other trick is to put the 'import foo.baz' *after* you've defined
Foo.


> I haven't finished reading the pyrex but I'll just post this to get it out
> there.  I didn't see any severe problems.
> 
>> Anyway, I don't really know how to do this sort of testing in a platform
>> independent manner. (On some forms of Linux you can read
> /proc/PID/status to
>> find your memory usage, etc)
> 
> I would recommend valgrind.  I'm not sure how well it will interact with
> Python's gc but it'd be worth a try.

It seems reasonable for manual testing, though I was hoping to put something
together for an auto-test. Something that would read a 20k entry dirstate a few
times, and make sure that the memory consumption/object count stays reasonable.
(If you have 20k entries, and then delete them, it should drop back down to
within a hundred of before).


> 
> The other thing I would suggest would be to check that the dirstate
> docstring is totally clear about the ordering, since this has caused
> problems in the past.
> 

I'm not sure what ordering you are mentioning. Maybe just that I'm importing
_dirstate_helpers_* at the bottom?

> 
> For details, see:
> http://bundlebuggy.aaronbentley.com/request/%3C4696AFBF.7010405%40arbash-meinel.com%3E

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGl4poJdeBCYSNAAMRAj3YAJ9IeseKFKCfmGZZkdUF/lat/nbg9wCdEYTk
y7jW91TLo/nQlSB1FEwG9+8=
=xDIX
-----END PGP SIGNATURE-----