[MERGE] Reconcile can fix bad parent references, Andrew's version

Andrew Bennetts andrew at canonical.com
Wed Oct 10 07:10:25 BST 2007


Martin Pool wrote:
> Martin Pool has voted tweak.
> Status is now: Conditionally approved
> Comment:
> +    def make_populated_repository(self, factory):
> +        """Create a new repository populated by the given factory."""
> +        repo = self.make_repository('broken-repo')
> +        repo.lock_write()
> +        try:
> +            repo.start_write_group()
> +            try:
> +                factory(repo)
> +                repo.commit_write_group()
> +                return repo
> +            except:
> +                repo.abort_write_group()
> +                raise
> +        finally:
> +            repo.unlock()
> 
> (comment only) It would be nice to have a more general facility in 
> tests:
> 
>   self.call_in_write_group(repo, factory)
> 
> It's probably not good to have on the public Repository interface but
> might be nice in tests.

Actually, it might be good to have for more than just tests.
RepoFetcher.__init__ would benefit from a call_in_write_group function.

> +      checked, and possibly reconciled ASAP.  (Aaron Bentley)
> 
> Andrew should be credited here too.

Oops!  Fixed :)

> === added file 'bzrlib/tests/repository_implementations/test_check.py'
> +from bzrlib.repository import _RevisionTextVersionCache
> 
> _RevisionTextVersionCache doesn't seem to be used in this file.

It's not; but it is used by bzrlib/check.py and bzrlib/reconcile.py.  As you
comment below it's hard to find a sensible location for this sort of thing.  I
guess part of the problem is that reconcile and check want to share some code,
but currently the closest point common both is the repository module.  Hence
this class was added here by Aaron.  I'm open to suggestions for a better
location.

> +class _RevisionTextVersionCache(object):
> +    """A cache of the versionedfile versions for revision and 
> file-id"""
> +
> 
> +
> +class WeaveChecker(object):
> +
> 
> (comment) Because those two classes are only used in fairly special 
> cases it
> seems a bit unfortunate to have them making this file larger, just from 
> the
> point of view of ease of reading.  Maybe there is no other sensible
> location; we know that one file per class is too slow.

I agree.  (see also previous comment).

> Is WeaveChecker really weave-specific?  It seems more like a 
> VersionedFileChecker?

Hmm, I guess not.  I've renamed it to VersionedFileChecker.

> +    result = TestSuite()
> +    loader = TestLoader()
> +
> +    for module_name, appliers in test_repository_implementations:
> +        tests = loader.loadTestsFromModuleNames([prefix + module_name])
> +        suite = TestSuite()
> +        suite.addTests(tests)
> +        for applier in appliers:
> +            new_suite = TestSuite()
> +            for test in iter_suite_tests(suite):
> +                new_suite.addTests(applier.adapt(test))
> +            suite = new_suite
> +        result.addTests(suite)
> 
> This seems a bit generic to have inline here.  Could you not generate 
> the non-broken scenarios in the normal way, then add in
> 
>   multiply_tests_from_modules(['....test_check_reconcile'],
>     reconcile_scenario_iter)
> 
> where that later one generates the cross product of formats and 
> brokenness
> scenarios?

Hmm, I did it that way because Robert's review requested it :/

I'm not sure I understand your suggestion.  It sounds like you're proposing
iterating back over the suite of all repository_implementations tests
paramaterised by format, and then building a new suite where the tests are
either copied unchanged or further multiplied by another scenario if their name
matches a particular pattern?  That sounds a bit ugly and perhaps fragile to me.
It also sounds likely to be a bit slower to generate the test_suite, which is a
real concern I think given the speed of “bzr selftest --list-only”.

I'd be happy to move the loop to somewhere reusable (probably alongside the
existing adapt_modules function?).  The signature could be:

def multiply_tests_from_modules(modules_and_their_adapters, loader, suite):
    """...

    :param modules_and_their_adapters: a list of tuples like
         (module name, [scenaro applier, ...]).  Each tuple specifies a module
         name and the sequence of scenarios to multiply the tests in that module
         by.
    ...
    """

Although I wonder if decoupling the “load tests from a module” part from the
“multiply tests” part would make the adaption easier to deal with; that coupling
is part of the reason why I couldn't simply call the existing helper functions.

I guess I'm not sure what exactly you're objecting to here.  Is it just that
there is some potentially reusable code here that isn't factored out, or the way
the list of module-and-list-of-scenarios is being expressed, or something else?

Maybe you just want to see something like:

    # Parameterise most modules by repository format
    adapt_modules(test_repository_implementations,
                  format_applier, loader, result)

    # Parameterise test_check_reconcile by both repository format *and* by
    # broken-repository scenario.
    scenario_suite = TestSuite()
    adapt_modules(
        ['bzrlib.tests.repository_implementations.test_broken']
        format_adapter, loader, scenario_suite)
    for test in iter_suite_tests(scenario_suite):
        result.addTests(broken_scenario_applier.adapt(test))

(where the definition of format_applier and broken_scenario_applier are the same
as in my last bundle.)

That's essentially what I had before Robert's review (although I also
amalgamated the multiplying by disk format and by the remote format, which fixed
a fair bit of ugly duplication).

-Andrew.




More information about the bazaar mailing list