[PING] updated benchmark display

[John Arbash Meinel]

Carl Friedrich Bolz wrote:
> holger krekel wrote:
>> On Tue, Aug 15, 2006 at 12:02 +1000, Robert Collins wrote:
>>> There are some general stylistic issues with the patch:
>>>  - please use self.assertTHING rather than 'assert' in the test suite.
>>> We like our tests to run with -O [in principal]. More importantly is the
>>> consistency with the rest of bzrlib and the improved diagnostics we get.
>>>  - please put docstrings or comments in your tests explaining what and
>>> why you are testing. They are rather bare at the moment.
>>>  - please put docstrings on your functions outside the test suite too!
>>>  - you have some duplicate code fragments in the tests that could well
>>> be factored out - making the tests easier to read.
>>>  - please put docstrings on classes. These are really essential when
>>> trying to understand how you are intending to tie the various components
>>> together.
>> please find a much updated diff attached and some
>> selected inline comments further below.  Our branch
>> is also available at 
>>
>>     http://codespeak.net/bzr/benchmark-display.merge1 
> 
> Could someone please look at this branch again? We think it is close to
> merging and Martin said on IRC that he liked the output. It is
> registered at launchpad as bench-display. I also attached the diff.
> 
> Cheers,
> 
> Carl Friedrich
> 

There are quite a few PEP8 violations, but I could clean those up.

I'm a little more concerned about how it was implemented, though.

It seems that most things are in plain tools/*. Which is okay, though if
they really are an aggregate, then they probably should be in their own
subdir.

And then you have a test that runs, and expects to be able to import
'tools'. Which will break if bzr is installed (because it doesn't
install the 'tools' directory).

I'm also concerned that you are importing 'tools/benchmark_report' which
is importing Image, ImageDraw, etc.

I assume these come from PIL. However, I don't know that we want an
explicit dependency on PIL for bzr. Which would be a good reason why the
benchmark display stuff should be a plugin.

It can still provide test cases because we can use:
bzrlib.plugins.<plugin_name>.test_suite()

To get any extra tests that you would want to run.

I really don't want to merge stuff into the core that adds dependencies
on stuff that isn't needed for standard operation.

I also wonder why you need to use 'pyxml' at all, rather than using
cElementTree. If it makes things easier for you, that is fine. But
cElementTree represents documents as a list of elements. It seems to be
a pretty good (and quite fast) api. And it seems to make more sense to
re-use the one we already have, rather than adding another one.

So the #1 thing in my mind, is that this should probably be a plugin.
The #2 is that it needs some PEP8 cleanup. And the #3 is at least an
explanation why we want to bring in extra dependencies.

If we decide not to do it as a plugin, then we need to add a lot of
checking, so that the test suite doesn't fail if someone doesn't have
PIL installed.

As it stands, I don't think they could even run the test suite, because
they will get an ImportError while it tries to load all the possible
test cases.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060831/a6767e25/attachment.pgp