[rfc] add a TestFactory class or concept
John Arbash Meinel
john at arbash-meinel.com
Tue Aug 18 15:20:14 BST 2009
-----BEGIN PGP SIGNED MESSAGE-----
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:
I would generally use the string form rather than doing:
rev1 = 'rev-1'
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:
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
'john at arbash-meinel.com-20090817221106-snef4s9g2f7zue60' not in
set(['john at arbash-meinel.com-20090817220821-wzul2cdupe88xi7t',
'john at arbash-meinel.com-20090817210105-k1ejuqmyaz4u2zfs'])
'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:
> 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.
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.build_snapshot('A', None, [
('add', ('', 'TREE_ROOT', 'directory', None)), # 
builder.build_snapshot('B', ['A'], )
builder.build_snapshot('C', ['A', 'B'], )
builder.build_snapshot('D', ['B'], )
builder.build_snapshot('E', ['C', 'D'], )
b = builder.get_branch()
tree = self.make_branch_and_tree('tree1')
tree2 = tree.bzrdir.sprout('tree2').open_workingtree()
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.
 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/
-----END PGP SIGNATURE-----
More information about the bazaar