Code review comment for lp:~lifeless/bzr/test-speed

-----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-----