[MERGE] Add support for previewing the result of a merge

Andrew Bennetts andrew at canonical.com
Fri Jan 4 05:52:46 GMT 2008


John Arbash Meinel wrote:
> Aaron Bentley wrote:
[...]
> >>> +    def test_iter_changes(self):
> >>> +        revision_tree = self.create_tree()
> >>> +        root = revision_tree.inventory.root.file_id
> >>> +        preview = TransformPreview(revision_tree)
> >>> +        a_trans_id = preview.trans_id_file_id('a-id')
> >>> +        preview.delete_contents(a_trans_id)
> >>> +        preview.create_file('b content', a_trans_id)
> >>> +        preview_tree = preview.get_preview_tree()
> >>> +        self.assertEqual([('a-id', ('a', 'a'), True, (True, True),
> >>> +                          (root, root), ('a', 'a'), ('file', 'file'),
> >>> +                          (False, False))],
> >>> +                          list(preview_tree._iter_changes(revision_tree)))
> >>> +        # Test unsupported parameters
> >>> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
> >>> +                              preview_tree)
> >  
> >> ^- This seems like a test that you would really rather have as a set of
> >> tests, rather than one big conglomeration.
> > 
> > I don't agree.  It's all testing the same thing.  Having to build the
> > same fixture repeatedly just to test that a given option raises
> > ValueError seems like a waste of runtime to me.
> 
> I'll let the more test focused people (like Robert) chime in here. But
> my understanding is that all-in-one tests like this are brittle. It
> breaks in the middle, which doesn't show you if the rest is working or
> not. You get 1 test failure which might actually be 5 things that need
> to be fixed. Conversely if you are expecting side effects (not true in
> this case), you may get a false failure. (It failed because of the set
> up, but what you were actually testing in that test would have succeeded.)

I tend to agree with John here.  My motivation is more clarity than brittleness.
I find several short, very specific test methods are easier to understand than
one big test method that does several things.  This is especially true when the
time comes to add a test for another condition: with a conglomeration test
method it is hard to decide what the scope of it should be, and thus if a new
test ought to be part of the conglomeration or a new method.  Similarly, when
trying to get an overview of the existing tests by reading the code, I find it
easier if I can just skim for lots of “def test_this_condition” and “def
test_that_condition” rather than having to read a single large test method
itself in detail.

So I find single (narrow) purpose test methods easier to work with.  They also
force the author to think of a name for each condition, rather than just
sticking yet another assert in with no explanation.  (Although in this case
you've commented the various conditions quite well.)

-Andrew.




More information about the bazaar mailing list