test isolation: default ignores & repository acquisition

[John Arbash Meinel]

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

Vincent Ladeuil wrote:
>>>>>> "martin" == Martin Pool <mbp at canonical.com> writes:
> 
>     martin> 2009/9/18 Vincent Ladeuil <v.ladeuil+lp at free.fr>:
>     >> Exactly what I pointed in an another mail, the tests shouldn't
>     >> start with a local setup if that could be factored out at the
>     >> class level, thanks for demonstrating my point.
>     >> 
>     >> Having setUp() methods makes smaller and more readable
>     >> tests. Smaller tests provides better defect localization, by
>     >> design.
> 
>     martin> I think separating out common code is (of course) good, but I do not
>     martin> feel very convinced that putting it in setUp(), as opposed to an
>     martin> explicitly called make*() method is a good idea.  
> 
>     martin> It is less explicit,
> 
> I disagree :)
> 
> The intent is made more explicit in the class *name*.

So I disagree with Vincent, which was my original example. It isn't a
big deal when there are 2 tests. But when you have 2 pages of tests,
tracking back to find which test case this is part of is difficult.
Tracking up 3 lines to see what setup is called is fairly immediately
obvious.


> 
> Can you show me examples where your approach give benefits, and
> which ones[1] ?

So my direct set of test cases where this was an issue was:
bzrlib.tests.test_merge.py
  TestMergerEntriesLCA
and
  TestMergerEntriesLCAOnDisk

Now, all of those test cases have rather complex setup. They are rather
hard tests to decipher. They were just about as hard to create, and I
tried to do a decent job of explaining them in comments. (Not to mention
that the final "assertEqual" test is way to complex, and the api is full
of far too many tuples... :)

I'm pretty sure that I could have used slightly more inheritance in some
of the tests. For example the:
  test_conflict_without_lca
  test_other_deletes_lca_renames
  test_executable_changes
  test_create_symlink
 ...

All use a similar shaped graph. And a couple of times the specific
difference is probably in just the final commit.

Anyway, it was a pain for me to have to care whether I had a disk
backing to work with or not. And figure out what tests could be done
"cheaply" and what tests could not (for comparison, first runs 23 cases
in 2.4s = 104ms/test, OnDisk is 19 tests in 6.0s = 315ms/test)


Also note that there is a TestMergerInMemory() which has a similar split
out for things that I could test without creating a WT.

And note that TestMergerEntriesLCAOnDisk can't inherit from the same
hierarchy as TestMergerEntriesLCA because it needs the WithTransport
base class. (Arguably the whole thing could be pulled out to the side as
mixins that don't get their final base until the very end.)

Now, potentially we could have turned all of these tests into individual
classes, and moved the setup appropriately.

Or made a *much* bigger setUp that would have to deal with 20 different
permutations and then had the final assertion test just test one aspect
of that large set up.

I don't have great answers here. And if you can show me a way to make
those tests easier to read/maintain/develop I'd be very happy to see it.
I certainly created the "builder.build_snapshot()" functionality
explicitly to help me do the work, because managing 'tree' objects and
knowing what revision I was on during 'tree.merge_from_branch' was
killing me.

As is, there is a lot of voodoo there. Though I'm also pleased that I
actually did do TDD as much as possible there, and didn't fix things I
knew were wrong until I could get a test to exercise that code.

John
=:->


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

iEYEARECAAYFAkq0QtkACgkQJdeBCYSNAAMAaACg11e+sdqWKU2Em/n5Cq6GFdVY
iPYAoMDxIiShlG2w7y0GO5k1/qDvQHFq
=jkTI
-----END PGP SIGNATURE-----