test isolation: default ignores & repository acquisition

[John Arbash Meinel]

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

Vincent Ladeuil wrote:
...

>     jam> So I disagree with Vincent, which was my original
>     jam> example. It isn't a big deal when there are 2 tests. But
>     jam> when you have 2 pages of tests, tracking back to find
>     jam> which test case this is part of is difficult.  Tracking
>     jam> up 3 lines to see what setup is called is fairly
>     jam> immediately obvious.
> 
> 1) Shorter tests address your point, by making tests shorter you
> can see more in the same page. That means: put more code in
> setUp() and create higher level helpers.
> 
> 2) I don't buy the argument that tests have to be on the same
> page that their setup. It's a bit like saying that inheritance
> can be used only if all classes fit in the same page.

Having multiple layers increases abstraction. Which can obfuscate as
much as it clarifies. Sometimes abstraction helps because then you don't
need to think about the details. Sometimes it hurts because when you go
to find the details you need to switch context between 20 different
places before you get the whole picture.

...

>     jam> Now, all of those test cases have rather complex
>     jam> setup. They are rather hard tests to decipher.
> 
> Right, that make them good candidates for refactoring. Complex
> setup does not mean the test itself *has* to be hard to read (nor
> write), it mostly means we lack the right setup tools.

Possibly.

...

>     jam> I'm pretty sure that I could have used slightly more inheritance in some
>     jam> of the tests. For example the:
>     jam>   test_conflict_without_lca
>     jam>   test_other_deletes_lca_renames
>     jam>   test_executable_changes
>     jam>   test_create_symlink
>     jam>  ...
> 
>     jam> All use a similar shaped graph. And a couple of times the specific
>     jam> difference is probably in just the final commit.
> 
> "just in the final commit" sounds to me as:
> - common code in setUp()
> - final commit in the test

Yeah, though I think if you actually look at the details, that won't be
true. They all use the same basic X structure:
  A
  |\
  B C
  |X|
  D E
    |
    F

As that is the simple way to create multiple LCAs and most more complex
graphs boil down to that basic structure anyway.

There are a couple of tests that F is the only bit that is interesting
versus another test. But sometimes A, B, & C also have various states
under test.

I think the other big wart here is that TestCases cannot trivially
inherit from each other, as they bring their "def test_foo" attributes
with them. So you have to create a group of mixins to create
inheritance. And at that point, is inheritance really better than having
"make_foo" attributes.

I just ran into this again today, where I was going to try for more
classes rather than less. And I would have had to split off a new base
class that does nothing but provide the 'make_*' (or setUp() :).

The other argument for 'make' style tests is that you trade:

class TestFoo
...
  def setUp(self):
     self.object1 = [make object 1]
     self.object2 = [make object 2]

  def test_attribute(self):
     self.assertEqual(self.object1.foo(), bar)
     self.assertEqual(self.object1.foo(), bar)
     self.assertEqual(self.object1.foo(), bar)
     self.assertEqual(self.object1.foo(), bar)

for

  def test_attribute(self):
     object1 = self.make_object1()
     self.assertEqual(object1.foo(), bar)
     self.assertEqual(object1.foo(), bar)
     self.assertEqual(object1.foo(), bar)
     self.assertEqual(object1.foo(), bar)


ie, you get to have local variables rather than attributes, which means
you don't have to repeat 'self.' as often. Yes, you could do:

  def test_attribute(self):
     object1 = self.object1

However, if you do that, you've now added as much overhead to the test
as the simple "object1 = self.make_object1()" line. So all setUp() gives
you is magic attributes that you have to track down in a function whose
name exists 20 times in the file, rather than only 1 "make_X".


> 
>     jam> Anyway, it was a pain for me to have to care whether I
>     jam> had a disk backing to work with or not.
> 
>     jam> And figure out what tests could be done "cheaply" and
>     jam> what tests could not (for comparison, first runs 23
>     jam> cases in 2.4s = 104ms/test, OnDisk is 19 tests in 6.0s =
>     jam> 315ms/test)
> 
> But only because you tried (and rightly so !) to separate tests
> that could run without disk support from those that required disk
> support.
> 
> So here I'm tempted to say that you may not have to care if:
> 1) we could use MemoryTransport in more cases,
> 2) the merge code was more testable.

  3) I could have a lighter weight base class that let me selected
     whether I wanted to run a particular test in memory or on disk.

...
> 
>     jam> Now, potentially we could have turned all of these tests
>     jam> into individual classes, and moved the setup
>     jam> appropriately.
> 
> You generally find some commonality to group test by classes
> (even when doing "test after" instead of "test before").
> 
>     jam> Or made a *much* bigger setUp that would have to deal
>     jam> with 20 different permutations and then had the final
>     jam> assertion test just test one aspect of that large set
>     jam> up.
> 
> Different permutations is what parametrization is for.

I really think that parameterization would have hidden the details more
than it would have helped. Yes, parameterization could have done the
same permutations. We could write a test loader that does:

permutations = ["""foo = self.make_foo()
self.assertEqual('bar', foo)
""",
...
]

class TestAnything(TestCase):

  self.permutation = None #Set by load_tests()

  def test_everything(self):
     eval(self.permutation)

I'm intentionally going overboard to make a point. That parameterization
is an option. But being 'data driven' doesn't make all problems go away.
You're looking for a nice way to describe the problem, and the assertion.

...

>     jam> I certainly created the "builder.build_snapshot()"
>     jam> functionality explicitly to help me do the work, because
>     jam> managing 'tree' objects and knowing what revision I was
>     jam> on during 'tree.merge_from_branch' was killing me.
> 
>     jam> As is, there is a lot of voodoo there.
> 
> My gut feeling is that this is mainly due to the fact the code is
> not testable enough.

My gut feeling is that complex graph logic is ... complex. Not all
graphs collapse.

Sort of the things you get when going from 2D => 3D. In 3D you can have
a Mobius strip, which is a 2D object that wraps in on itself. (In 4D you
have the Klein bottle equivalent.)

Some concepts just don't *exist* at the lower levels of simplification.
In topology, in 2D you can have a 'donut' where there is a region inside
of another region, which is not connected to the background. In 3D you
can have a region which for any given 2D plane does not connect to the
background, but in 3D it *is* connected.

Anyway, I'm interested in expanding my testing abilities. I'm also
willing at times to punt and say "this is probably good enough". :)

Personally, I feel like class hierarchies are often a forced solution,
rather than an ideal one. (in languages without duck-typing, you're
certainly forced into creating a hierarchy if you want to maintain an
abstraction.)

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

iEYEARECAAYFAkq5mG8ACgkQJdeBCYSNAAPRmQCdFwwNk+FxJx24YBq+JJhnl1Cd
gz0AnRIu2W80/hVPWtckt0q2T77I7cgI
=bQ3m
-----END PGP SIGNATURE-----