Merge lp:~lifeless/bzr/test-speed into lp:~bzr/bzr/trunk-old

This branch:
* Use copy, not deepcopy, to clone tests in test parameterisation.
* Make tests for finish_bzr_subprocess that really only care about the
   interface use StubProcess.
* Remove duplicated test_start_and_stop_working_dir test.
* Remove unnecessary use of an SFTP server connection to test the
   behaviour of TestCase.make_branch_and_tree.
* Refactor test_suite to make stubbing out the list of tests to load
possible without sacrificing coverage.

-Rob

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/test-speed into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This branch:
> * Use copy, not deepcopy, to clone tests in test parameterisation.

^- My primary concern is whether this is actually valid. Obviously you
believe so or you wouldn't have done it.

My understanding is that the main difference is things like:

 l = [[1]]
 lcopy = copy(l)
 ldeepcopy = deepcopy(l)
 l[0] is lcopy[0]
 l[0] is not ldeepcopy[0]

Now, I don't think we store any state in member variables that would end
up persisting between tests. I suppose if we did then you would get test
failures?

How much of an impact does this actually have? If it is .02s out 10s
loading, then it isn't really worthwhile versus the safety. If it is
more than 10% then it seems reasonable.

> * Make tests for finish_bzr_subprocess that really only care about the
> interface use StubProcess.
> * Remove duplicated test_start_and_stop_working_dir test.
> * Remove unnecessary use of an SFTP server connection to test the
> behaviour of TestCase.make_branch_and_tree.
> * Refactor test_suite to make stubbing out the list of tests to load
> possible without sacrificing coverage.

This seems like a lot of churn that is somewhat likely to cause
conflicts (as the data has now been moved). If you find it a lot easier,
I guess that is ok. I've always found:

^V B B B I #

(block select, down page, insert before, comment char)

to work well for stubbing out lots of tests at once.

You can even use

^V % I #

(jump to matching parenthesis/brace/etc) to block out the whole group in
just a couple keystrokes. And I know you use ViM :)

>
> -Rob

Anyway, other than the copy/deepcopy question, this doesn't seem to
reduce code coverage (note that I didn't check to make sure all the test
names were still present after the move.) So I'll defer to your
judgement about the rest.

 review: approve

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

iEYEARECAAYFAkqVQ84ACgkQJdeBCYSNAANDswCfRoctFvjboQvSDWaPkqdN5zuo
0y8AnREcXttKeXhpdj71zA/KpgMRRsZg
=aRNZ
-----END PGP SIGNATURE-----

> This branch:
> * Use copy, not deepcopy, to clone tests in test parameterisation.
> * Make tests for finish_bzr_subprocess that really only care about the
> interface use StubProcess.
> * Remove duplicated test_start_and_stop_working_dir test.
> * Remove unnecessary use of an SFTP server connection to test the
> behaviour of TestCase.make_branch_and_tree.
> * Refactor test_suite to make stubbing out the list of tests to load
> possible without sacrificing coverage.
>

Cough, that's two or three submissions crammed into one, here.

I approve them all, but yet.

I think the copy usage is fine, setUp() is really the test constructor so nothing occurring before that should have an impact on a test.

That may be worth adding a comment though.

You may want to check if your subprocess modifications (removing some bzr --version abusive uses) have addressed the bug about running the test suite in a bound branch.

On Wed, 2009-08-26 at 14:18 +0000, John A Meinel wrote:
> Review: Approve
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > Robert Collins has proposed merging lp:~lifeless/bzr/test-speed into lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> >
> > This branch:
> > * Use copy, not deepcopy, to clone tests in test parameterisation.
>
> ^- My primary concern is whether this is actually valid. Obviously you
> believe so or you wouldn't have done it.

Its only the test objects we copy:
so
foo = Test("method")
case1 = deepcopy(foo)
apply_attributes_here
becomes
foo = Test("method")
case1 = copy(foo)
apply_attributes_here

There aren't any attributes on the test at all; attributes on the class
will still be on the class -
foo.bar = gam
wouldn't be affecting them anyway.

So its pretty safe.

> > * Refactor test_suite to make stubbing out the list of tests to load
> > possible without sacrificing coverage.
>
> This seems like a lot of churn that is somewhat likely to cause
> conflicts (as the data has now been moved). If you find it a lot easier,
> I guess that is ok. I've always found:

Its stubbed out by the test suite now, rather than by hand ;). So editor
stubbing is a bit different :).

> Anyway, other than the copy/deepcopy question, this doesn't seem to
> reduce code coverage (note that I didn't check to make sure all the test
> names were still present after the move.) So I'll defer to your
> judgement about the rest.

Thanks, landing.

-Rob