[MERGE] Various test fixes and tweaks for packs.

[Robert Collins]

On Mon, 2007-09-17 at 16:40 +0200, Jelmer Vernooij wrote:
> Am Freitag, den 14.09.2007, 12:27 +1000 schrieb Robert Collins:
> > As the topic says, this is just a bunch of test tweaks I did in the
> > packs branch.
> 
> > @@ -272,7 +276,11 @@
> >          wt = self.make_branch_and_tree('source')
> >          wt.commit('A', allow_pointless=True, rev_id='A')
> >          repo = wt.branch.repository
> > -        repo.sign_revision('A', gpg.LoopbackGPGStrategy(None))
> > +        repo.lock_write()
> > +        repo.start_write_group()
> > +        repo.sign_revision('A', bzrlib.gpg.LoopbackGPGStrategy(None))
>                                    ^^^^ why the change to bzrlib.gpg ?
> That seems inconsistent with what the rest of that file uses. 

copy n paste as I changed it; will drop the bzrlib.

> > === modified file
> > 'bzrlib/tests/repository_implementations/test_repository.py'
> > ---
> > bzrlib/tests/repository_implementations/test_repository.py  2007-08-21
> > 03:40:50 +0000
> > +++
> > bzrlib/tests/repository_implementations/test_repository.py  2007-09-14
> > 02:19:41 +0000
> > @@ -234,13 +234,6 @@
> >          new_signature = wt.branch.repository.get_signature_text('A')
> >          self.assertEqual(old_signature, new_signature)
> >  
> > -    def test_exposed_versioned_files_are_marked_dirty(self):
> > -        repo = self.make_repository('.')
> > -        repo.lock_write()
> > -        inv = repo.get_inventory_weave()
> > -        repo.unlock()
> > -        self.assertRaises(errors.OutSideTransaction, inv.add_lines,
> > 'foo', [], [])
> > -
> ^^^ I'd rather see this test not be removed because it is still valid
> for non-pack formats. Perhaps change the assertRaises to check for
> (errors.OutSideTransaction, errors.NotWriteLocked) ?

So there are several problems with this test in concept, that drove me
to remove it.

Consider mozilla - 55K versioned files. Commit does the following:
lock()
loop:
 examine a file 
 open the vf object
 add a record
 close the vf object
unlock()

now, the previous hand holding that is partly embodied by the test that
I'm proposing we nuke requires tracking by all the vf objects of the
state of the repository to see whether it is legitimate or not.
Currently the tracking is a bit awkward, but I guess it can be fixed.
However I came to the conclusion that this isn't something the interface
should require.

What the interface should require here is the success cases:
 - take a read lock, get a vf, use it - it works
 - take a write lock, a write group, get a vf, use it - it works

What each implementation is responsible for is safety for data with its
disk format.
For knits/weaves:
 - they use a callback to cause an error when a write is attempted after
   one succeeded.
 - failing to do this can cause data corruption
For packs:
 - they supply a callback to do writes when the 'vf' object is created, 
   and that callback is common to all the 'vf' objects, and gets
   invalidated naturally during the end of data insertion - calling it
   will not cause data corruption.

So with packs its a lot easier to cause an attempt to write to fail once
the transaction has ended. But it is tricky to cause this to meet the
currently defined exception to be raised (if I remember correctly, the
error raised is something like AttributeError - not something you want
to routinely be catching). And there is very little benefit in this case
in requiring a regular error to be raised, because correct API use will
never cause the error case to trip; its not something outside influences
can cause.

I can make the test optional based on a repository attribute if you
like, or reinstate it as a non-interface test for weaves and knits.

-Rob



-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070918/411d2f21/attachment-0001.pgp