[rfc] add a TestFactory class or concept

[John Arbash Meinel]

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

Aaron Bentley wrote:
> Martin Pool wrote:
>>> Martin Pool wrote:
>>>> Lots of tests are run on some specific values; for instance they make
>>>> a branch and then do some operations on it.
>>> We need specific values in order to make assertions about them, and to
>>> provide legibility.  Could you please explain what about this is bad?
>> I will try.
> 
>> It's not quite true that you need specific values in order to make
>> assertions about them: a very common case is that we commit a revision
>> and then check that its id is retrieved later, without ever hardcoding
>> the revision id.
> 
> I think this is a good example, because it gets at a lot of the issues.
> 
> I don't think we're making assertions about specific values in that
> case.  In an example where we were making assertions about specific
> values, we would specify a revision-id, and then later test that the
> literal revision-id had been used.
> 
> So if you'd like to argue instead that we rarely need to make assertions
> about specific values, I'd certainly agree it's not a need.
> 
> We can test almost everything by allowing the the revision-id to be
> auto-generated and storing it in a variable.  (We can't test that
> specifying a revision-id works, of course.)  But I find that the
> indirection through that variable name makes the test more abstract,
> less concrete, and harder to follow.

Sometimes that is certainly true. Interestingly enough, I've found the
opposite to be true with the new 'key' based ids that we have. Specifically:

  wt.commit('foo', rev_id='rev-1')

I would generally use the string form rather than doing:
  rev1 = 'rev-1'
  wt.commit('foo', rev_id=rev1)
or
  rev1 = wt.commit('foo')

However, with keys, I have to do:

  vf.add_lines(('key1',), [], lines)
  vf.add_lines(('key2',), [('key1',)], lines)
  vf.add_lines(('key3',), [('key1',), ('key2',)], lines)

And I find I would rather do:
  key1 = ('key1',)
  key2 = ('key2',)
  key3 = ('key3',)
  vf.add_lines(key1, [], lines)
  vf.add_lines(key2, [key1], lines)
  vf.add_lines(key3, [key1, key2], lines)

I think a lot of it is the sheer amount of noise that is in:
 [('key1',), ('key2',)]

Which has as 5 punctuation characters for every 4 value characters.
('',) vs key1

I will also say that tests are easier to debug when the handle is 4-5
characters long and the *same* with every test run, rather than getting
back:
 'john at arbash-meinel.com-20090817221106-snef4s9g2f7zue60' not in
 set(['john at arbash-meinel.com-20090817220821-wzul2cdupe88xi7t',
      'john at arbash-meinel.com-20090817210105-k1ejuqmyaz4u2zfs'])

vs
 'rev-2a' not in set(['rev1', 'rev2'])

...


> 2. Cleanup must be manually specified.
> 
> Many test fixtures need to be cleaned up-- they need locks released,
> etc.  A method of TestCaseWithTransport can arrange for cleanup using
> addCleanup, but a method of a test factory like Launchpad's cannot use
> this facility.  This means that clients must manually specify cleanup.
> 
> We could alter the design so that the factory was initialized with a
> cleanup facility.  We could alter the design so that the factory also
> provided cleanup, and TestCase.tearDown would call it.  (Though I'd
> worry about doing things out of order then.)

You can simply have the factory constructor take the test case as a
parameter, and then have functions call 'self.test.addCleanup()'.

Alternatively the members themselves:

self.factory.makeFoo(self, ...)

...

> BranchBuilder-- well, no compelling case has been given to me for using
> it.  It wouldn't be appropriate for me to comment further until I
> understand what it's meant for.
> 
> Aaron

At a minimum, it is a *hell* of a lot easier to create ancestry graphs
that have merges in them, rather than using clone/merge/commit. Consider:

builder = self.make_branch_builder('source')
builder.start_series()
builder.build_snapshot('A', None, [
  ('add', ('', 'TREE_ROOT', 'directory', None)),  # [1]
  ])
builder.build_snapshot('B', ['A'], [])
builder.build_snapshot('C', ['A', 'B'], [])
builder.build_snapshot('D', ['B'], [])
builder.build_snapshot('E', ['C', 'D'], [])
builder.finish_series()
b = builder.get_branch()

versus

tree = self.make_branch_and_tree('tree1')
tree.add(['file'], ['file-id'])
# tree.set_root_id()
tree.commit('A', rev_id='A')
tree2 = tree.bzrdir.sprout('tree2').open_workingtree()
tree2.commit('B', rev_id='B')
tree1.merge_from_branch(tree2.branch)
tree1.commit('C', rev_id='C')
tree2.commit('D', rev_id='D')
tree1.merge_from_branch(tree2.branch)
tree1.comimt('E', rev_id='E')

The biggest problem with the 'tree.merge_from_branch' scenario is that
you have to trace back to see what the actual last-modified revision is
for the tree in question. With 2 trees, it isn't terrible. But we have
tests that have 3 and 4 trees.

John
=:->



[1] tree.commit() does automatically version a tree root for you. Which
is something that is a bit of a pain with branch builder. In that this
line gets repeated in pretty much every test.

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

iEYEARECAAYFAkqKuJ4ACgkQJdeBCYSNAANqugCgw252fRheqfIXpd9h3tjNuYiE
SU8AoKVoBN9Ba1HGTI+V+HVLrvhB9d0i
=+yQW
-----END PGP SIGNATURE-----