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

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