[MERGE] Fix for bug 183831

[Robert Collins]

On Thu, 2009-04-02 at 22:16 +0200, Geoff Bache wrote:
> Oops, forgot to reply to all...
> 
> >> To me it would seem useful if
> >> --no-plugins was the default for
> >> "bzr selftest", as it seems like there is plenty of scope for this
> >> kind of thing to happen otherwise.
> >
> > bzr itself includes plugins, so a complete run of the test suite must
> > include plugins.  We also want to encourage plugin authors to keep their
> > tests passing.
> 
> Well OK, maybe we need some form of "bzr --no-nondefault-plugins
> selftest" then. If something doesn't come by default with Bazaar then
> it surely shouldn't be included by default in a test run of Bazaar
> (whether it's officially called a "plugin" or not isn't so interesting
> in this context). A user could have all sorts of plugins installed, in
> different stages of development, including some highly "alpha" ones
> (like I have). You can't hope that all the plugins ever written will
> always be compatible with 17000 tests that take half an hour to run,
> can you?

A couple of points here; the test suite has become quite slow recently,
we want to make it a lot faster. We've recently landed patches to
support parallel testing (--parallel=fork in bzr.dev, for instance)
which make a massive speedup. If you don't have parallel hardware, you
can also use the bzr-ec2test plugin with bzr.dev. 2 instances gets you a
90 second test suite once the instances are warmed up. And as a bonus,
they have no plugins to break things:).

That said, yes, we *do* expect that the plugins shipped to a users
machine will be passing all their own tests, and for any interfaces they
implement passing those interfaces as well. If they don't then there is
a good chance that the plugin will cause problems.


> "Their tests" as in "their runs of the tests". It would be nice if I
> as a newbie could run the tests and know that failures were my fault,
> rather than having to also run them on unchanged code and compare the
> output. It took some time
> for me to establish that the test failures were caused by my
> (non-default) plugins, I just wondered if you were interested in
> helping the next person in my shoes be aware of this issue more
> directly.

I am interested in helping people developing plugins do so safely and
quickly, and also in helping people wanting to contribute to the core do
so easily. We do have some tension between 'officially blessed' and 'J
random' plugins; I'm not sure of the best route forward. Like Aaron I
see value in having all the plugins present tested by default.

> >> Isn't the fact that it fixes this issue enough for it to be "better"...?
> >
> > When you're talking about changing the behaviour of a low-level function
> > in order to achieve a high-level effect, no it's not.
> 
> Well, this is quite a serious bug in my view, it's been open for
> nearly a year and has been independently reported 4 times (5 counting
> the first comment). I've personally run into it at least 3 times
> independently, one of which forced me to implement an ugly workaround
> for it in my test tool's integration with Bazaar.

We definitely want to fix it. I'm glad you're putting time into fixing
it - thank you.

> To me, it seems that fixing it at a low level will probably mean it
> stays fixed, whereas if you address it at higher levels you have to
> address a lot of different contexts separately to ensure
> osutils.relpath is never passed a symbolic link and its target as
> arguments. Which is going to be a lot more complicated and possibly
> lead to variants of the bug reappearing in the future.
> 
> Unless, of course, a measurable slowdown can be demonstrated (see later).
>
> >> 3 samples doesn't sound very
> >> much to me for something that only takes 2 seconds anyway.
> >
> > Best of 3 is our default practice for benchmarking.
> 
> Is there any documentation of why? It still doesn't sound very much to
> me even if it is your default practice. It surely ought to depend on
> the size of the operation? I've done a lot of benchmarking at work and
> have come to the view that cpu time changes less than 2% should be
> viewed as noise.
> 
> Anyway, I did "time repeat 100 bzr branches" and discovered that my
> fix improved total time by about 1% (!) I became a bit suspicious and
> added a print statement to osutils.relpath, which did not write
> anything. It seems osutils.relpath isn't called at all by "bzr
> branches".
> 
> Maybe we need another benchmark...

bzr branches is probably using urlutils.urljoin, which makes sense
because branches are entities we access entirely via url's.

The selftest --benchmark is largely unmaintained at the moment, we seem
to be relying on 'usertest' and 'timeit' tests. timeit does a variable
number of runs to get a reasonable sample period with a minimum number
of iterations to prevent slow operations just getting measured once.

And you can't implement urljoin with any reference to the
filesystem  :).

Your fix looks potentially ok to me. I suggest putting it in a branch
[done] and asking igc to run a usertest run over it vs its branch point
to see if there are changes, as noone seems to have a good off the cuff
answer.

Definitely though, we need more tests, because testing that add is fixed
is good, but the contract for the low level function is also being
changed and we should make sure that that doesn't regress by mistake.
bzrlib/tests/test_osutils.py has a TestOSUtils class that looks
reasonably appropriate for adding a few small fixtures to.

-Rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090403/5ef7419a/attachment-0001.pgp