Code review comment for lp:~jelmer/bzr/foreign-tests

If we are going to go this route, I would probably call it a ForeignBranchTestMixin

since it is meant as a second parent class. (eg, users will do:

class TestMyBranchImpl(TestCase, ForeignBranchTestMixin)

However, I think I would tweak a few things differently anyway.

1) It should probably be called "per_foreign" rather than "foreign"

2) I think I would use adaptation rather than inheritance. Yes, there are going to be *more* tests that will be needed for specific foreign branch implementations, but those can just be created in another test class.

I think having a flag, etc on register_format to indicate this is a foreign format, would be a great move towards making registering the format automatically queue it for the appropriate tests.

It makes it less susceptible to accidentally missing some tests, because you forgot to inherit from the new base test class.

It also means that when we add a new base test class, it automatically starts getting run across all of the current implementations, we don't need to change anything in bzr-svn or bzr-git.

3) I would also like to see things like:
class ForeignBranchFormatTests(object):
  format = None # set by XXXXX

  def test_foo(self):
    self.format.x

It makes it easier to figure out where things are happening.

4) For something like ForiegnBranch, I think the tests you have are ok, though
126 + def test_last_revision(self):
127 + (revno, revid) = self.branch.last_revision_info()
128 + self.assertIsInstance(revno, int)
129 + self.assertIsInstance(revid, str)
130 + self.assertEquals(revno, self.branch.revision_id_to_revno(revid))
131 + self.assertEquals(revid, self.branch.last_revision())

^- this is testing more "last_revision_info" not last_revision. And I certainly thought you wanted to get away from 'revno' since it cannot be easily computed at all times.

I think what we should be doing is designing a reasonable factory interface, so that you can have the test case say:

 def test_something(self):
   self.factory.give_me_a_branch_like_X()

(a branch with 1 commit, a branch with a few commits, a branch with merges, etc.)

There was some discussion raised by Martin on the mailing list, which didn't quite as planned, but I think foreign branch testing is a *great* place to implement some of this, since a lot of them are going to be read-only sources, and thus doing:
  self.tree.commit('foo')
to generate the shape isn't going to work.