[MERGE] Speedup selftest by loading a previously saved test list

[Robert Collins]

On Wed, 2008-01-16 at 15:55 +0100, Vincent Ladeuil wrote:
> >>>>> "robert" == Robert Collins <robertc at robertcollins.net> writes:
> 
>     robert> On Fri, 2008-01-11 at 00:16 +0100, Vincent Ladeuil wrote:
>     >> This patch add two options to the selftest command:
>     >> 
>     >> --save-list <file> 
>     >> --load-list <file>
>     >> 
>     >> You use the first one to define a set of tests you want to run
>     >> repeatedly and use the second one to avoid loading the whole test
>     >> suite:
>     robert> ...
>     >> - test inheriting from a class defined inside a function (or
>     >> using a lambda as an attribute) can't be pickled either (but
>     >> see the patch for some obvious fixes in the bzr core tests),
>     >> 
>     >> But overall I think more than 99% of tests can be used with this
>     >> mechanism.
> 
>     robert> I feel pretty uncomfortable about this.
> 
> What I've tested so far: 99.76% of our core test suite are
> serialized reliably (10530 out of 10561 tests) see below.
> 
>     robert> You have to assume a great deal to be sure that tests
>     robert> will execute the correct test when you are pickling
>     robert> their state.
> 
> Serialized before setUp is run. I assume that each test is
> initialized by setUp and setUp only regarding its state.

Depending on what you mean by initialized this can be very false in some
tests using the pyunit test protocol, which we try to adhere to for
interoperability with other python libraries (so that bzrlib tests are
friendly to other runners (when bzrlib is embedded), and vice verca (so
that plugins that use features from other projects can mix in their test
classes safely)). 

>     robert> I'd rather not see this come in, it feels basically
>     robert> wrong. You should be able to get nearly the same
>     robert> performance by saving just the names and asking for
>     robert> just those tests.
> 
> Been there, thought about that, can't be used for parametrized
> tests (~6000 out of the ~10000 existing tests) which requires
> some serialization for parameters anyway.

I don't see why paramterised tests won't work well with a few tweaks to
the loading api.

> ,----

> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py	2008-01-09 00:51:01 +0000
> +++ bzrlib/bzrdir.py	2008-01-16 12:00:41 +0000
> @@ -1670,6 +1670,13 @@
>  
>      _lock_class = lockable_files.TransportLock
>  
> +    def __eq__(self, other):
> +        if other.__class__ is not self.__class__:
> +            return False
> +        if other.repository_format != self.repository_format:
> +            return False
> +        return True
> +
>      def get_format_string(self):
>          """See BzrDirFormat.get_format_string()."""
>          return "Bazaar-NG branch, format 5\n"
> @@ -1729,6 +1736,13 @@
>  
>      _lock_class = lockable_files.TransportLock
>  
> +    def __eq__(self, other):
> +        if other.__class__ is not self.__class__:
> +            return False
> +        if other.repository_format != self.repository_format:
> +            return False
> +        return True
> +
>      def get_format_string(self):
>          """See BzrDirFormat.get_format_string()."""
>          return "Bazaar-NG branch, format 6\n"

It's better not to have __eq__ methods on objects unless they really are
Value objects; in this case its arguable about whether the Format stack
of objects are Value objects or not at this point; 

>     robert> bb:reject
> 
> Still ???

Its undeniably faster. But there is a raft of complications not address
at all by your patch, and which addressing will add complexity that is
at best confusing to the test environment.

I'll change my vote on a discussion about /correctness and simplicity
for developers/ here, not about speed. pickling and state skew is an
insidious problem when you are dealing with carefully managed data, let
alone tests.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080117/78cc52f5/attachment-0001.pgp