[MERGE][bug #173010] initial branch over http: can show no ui activity for a very long time

Vincent Ladeuil v.ladeuil+lp at free.fr
Mon Dec 10 10:43:03 GMT 2007


>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> Vincent Ladeuil wrote:
    >> Thanks for your tweak vote and your inspiring review, but I think
    >> the further modifications are worth a second review, so here is
    >> the new patch.
    >> 
    >> Most significant points:
    >> 
    >> - tests redesigned and completed (thanks to --coverage even all
    >> the error handling is now covered),

    Andrew> Excellent!  The tests are a pleasure to read now.

My feeling and far easier to write too, but good to see
confirmation ;-)

    Andrew> (Hmm, if someone wanted to write a plugin/script that
    Andrew> could intersect a diff with a coverage report, so
    Andrew> that you could see if any lines you've added aren't
    Andrew> run by the test suite, that'd be really cool.)

I will be more interested for a start by some emacs helper that
will show me the lines that are not covered *now* when I edit the
file itself. I looked a bit a various files and some work is
still to be done.

Having played a bit with --coverage, I have two remarks:

- I encountered 2 failing tests when using it for the whole test
  suite (not investigated),

FAIL: test_transactions.TestReadOnlyTransaction.test_zero_size_cache
    not equal:
a = None
b = <bzrlib.tests.test_transactions.DummyWeave object at 0x6518ed0>


FAIL: test_transactions.TestWriteTransaction.test_zero_size_cache
    not equal:
a = None
b = <bzrlib.tests.test_transactions.DummyWeave object at 0x65117d0>


- having all the import lines and 'def method' lines pointed at
  is a minor nuisance but a sure sign that --coverage should be a
  global option (after all it will be good to verify that a given
  bzr command exercise some path code so there is no reason to
  restrict it to selftest).

<snip/>

    Andrew> I guess that bit is fine then.  A brief “Read until
    Andrew> EOF” comment to make it clear what the size < 0 case
    Andrew> is intended to do (and to remind readers like myself
    Andrew> ;).

Done. I often whine myself about such missing comments ;)

    Andrew> Do you test the behaviour of read(0), btw?
    Andrew> Probably nothing tries to do that, but it's an edge
    Andrew> case that might do funny things if we're not careful.
    Andrew> For that matter, f.seek(f.tell())/f.seek(0, 1) might
    Andrew> be another interesting case (a seek to the current
    Andrew> position).

This is indeed outside the expected uses, but since there was a
bug there (read(0) was in fact doing a read(-1) in some cases) I
dug more. After all, reviewers should get some reward too ;-)

This allowed me to document the behavior difference between seek
and read:
- seek is allowed across ranges,
- read is not allowed across ranges.

End of range was indeed an edge case worth documenting even if
only for posterity ;)

Since there is nothing to be read at the end of a range,
RangeFile set itself to the beginning of the next range but
*only* if a next range exists.

And *that* can be expressed quite simply with the new design, so
thanks for pushing me in that direction, I think these additional
tests illustrates the benefits of both the test refactoring and
the enhanced review feedback they allow.



More information about the bazaar mailing list