[MERGE] Fix stacking tests applicability

Vincent Ladeuil v.ladeuil+lp at free.fr
Mon Dec 29 11:33:24 GMT 2008


>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:

    jam> John Arbash Meinel has voted comment.
    jam> Status is now: Semi-approved
    jam> Comment:
    jam> (sorry, I hit enter by accident)

    jam> If I understand correctly, the basic change you made is to switch:
    jam>         target = source.bzrdir.sprout('target').open_branch()

    jam> so that if it raises UnstackableBranchFormat it translates that
    jam> error into "TestNotApplicable". And you did this for several
    jam> different tests. Is that correct?

No.

I tried to make the loom plugin implements unstackable formats
based on the failing tests with the idea that our tests may be
buggy because we didn't really exercise them against such
formats.

Doing so I came across cases where some exceptions where raised
and were the same exceptions caught in the following lines.

And without trying to diagnose further I made the slight changes
you point out.

    jam> IMO, that is not the correct fix. Consider the causes:

    jam> 1) source.bzrdir.sprout('target')

    jam> This doesn't seem like something that should *ever raise
    jam> UnstackableBranchFormat.

But it was...

    jam> If there is a "default_stack_on" policy, but the branch
    jam> format doesn't support stacking, it should just not
    jam> stack. And if it does support stacking, then everything
    jam> should just work.

    jam> Note that the individual tests can then later say stuff like
    jam> "get_stacked_on_url()" should either raise an exception, or give
    jam> this specific
    jam> result. But, IMO, sprout() should not be failing.


    jam> 2) source.bzrdir.sprout('target').open_branch()

    jam> This should *definitely* never raise
    jam> UnstackableBranchFormat. Once you have
    jam> created the branch from sprout() you *better* be able to open
    jam> it. Or we have
    jam> created something explicitly broken as a result of sprout(). And
    jam> I would *want*
    jam> a failing test letting me know that "sprout()" is broken for this
    jam> format.


Both remarks sound perfectly valid, so I suspect my rude
implementation of an unstackable loom format was buggy and the
tests caught that.

Note that Andrew proposed another approach to fix loom regarding
the failing tests for which I sent a patch that is pending review
in the loom project ;-)

So I think we should forget that approach and look at

 https://bugs.launchpad.net/bugs/309730

and its consequences for looms instead.

BB:vetoed

        Vincent



More information about the bazaar mailing list