[MERGE] repository parameterisation tests for external_reference support

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
+    for scenario in scenarios:
+        format = scenario[1]['repository_format']
+        # For remote repositories, we need at least one external 
reference
+        # capable format to test it: defer this until landing such a 
format.
+        # if isinstance(format, remote.RemoteRepositoryFormat):
+        #     scenario[1]['bzrdir_format'].repository_format =
+        if format.supports_external_lookups:
+            adapter.scenarios.append(scenario)
+

^- You only adapt for repositories which support external lookups. Which 
seems reasonable. Would it be nice to have an easy way to communicate to 
users what formats support it? (Like known failure tests for other 
formats?) Probably not, but just an idea. Right now when a format 
doesn't support something we get at least a TestSkipped rather than no 
info.


...

+    def test_break_lock(self):
+        base = self.make_repository('base')
+        repo = self.make_referring('referring', 'base')
+        unused_repo = repo.bzrdir.open_repository()
+        base.lock_write()
+        self.addCleanup(base.unlock)

v- Incomplete comment
+        # break_lock when locked should
+        repo.lock_write()
+        self.assertEqual(repo.get_physical_lock_status(),
+            unused_repo.get_physical_lock_status())
+        if not unused_repo.get_physical_lock_status():
+            # 'lock_write' has not taken a physical mutex out.
+            repo.unlock()
+            return
+        # we want a UI factory that accepts canned input for the tests:
+        # while SilentUIFactory still accepts stdin, we need to 
customise
+        # ours
+        self.old_factory = bzrlib.ui.ui_factory
+        self.addCleanup(self.restoreFactory)
+        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
+        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        unused_repo.break_lock()
+        self.assertRaises(errors.LockBroken, repo.unlock)

^- Do we have a repository format which supports external references and 
also takes out a physical lock on lock_write()? My understanding is that 
packs are the only supported format, and don't take a physical lock. So 
this is untested test code. It seems correct, but it is never called. We 
should at least put in a comment that there may be something subtly 
wrong with it.

...

+    def test_check_file_graph_across_external_boundary_ok(self):
+        tree = self.make_branch_and_tree('base')
+        self.build_tree(['base/file'])
+        tree.add(['file'], ['file-id'])
+        rev1_id = tree.commit('one')
+        referring = self.make_branch_and_tree('referring')
+        readonly_base = self.readonly_repository('base')
+ 
referring.branch.repository.add_fallback_repository(readonly_base)
+        self.build_tree_contents([('referring/file', 'change')])
+        rev2_id = referring.commit('two')
+        check_result = referring.branch.repository.check(
+            referring.branch.repository.all_revision_ids())
+        check_result.report_results(verbose=False)
+        log = self._get_log(keep_log_file=True)
+        self.assertFalse("inconsistent parents" in log)

^- This check is asserting a negative (something is not found in the 
log). Which can pass for the wrong reasons. Is it possible to turn it 
into a positive test? I'm also a little uncomfortable about having to 
serialize down to the text level to check for something. Why doesn't 
check_result have members you can inspect? (Not saying you have to fix 
all of check here, but this test seems pretty clumsy to me)


I think this patch is dependent on some others being merged, but 
otherwise it seems good.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1203640368.15574.310.camel%40lifeless-64%3E