Merge lp:~lifeless/bzr/subunit into lp:bzr

I'm proposing this branch to get feedback; its not necessarily ripe, though I'd be happy to land it as-is (there aren't any must-do items remaining for it).

This branch deletes a bunch of code:
 17 files changed, 180 insertions(+), 350 deletions(-)

It dos this by using testtools as a base class and using the defined extension points there to add bzr specific functionality.

The net effect of doing this:
 - bzr selftest --parallel will now show log files, as will --subunit and pqm if we enable subunit there again.
 - assertThat is available for bzr to use
 - bzr no longer needs an implementation of run(), addCleanup(). There may be more duplicate assertions, but I haven't been on a hunt to minimise the duplication.

testtools 0.10 will be needed (that isn't released yet, but jml wants to bump to -10 when we make run() extensible. I'm putting a branch for that up for review in a few minutes).

-Rob

2009/12/6 Robert Collins <email address hidden>:
> I'm proposing this branch to get feedback; its not necessarily ripe, though I'd be happy to land it as-is (there aren't any must-do items remaining for it).

Well done.

> === modified file 'NEWS'
> --- NEWS        2009-12-04 22:13:52 +0000
> +++ NEWS        2009-12-06 03:05:32 +0000
> @@ -237,6 +237,10 @@
>  Testing
>  *******
>
> +* ``bzrlib.tests.TestCase`` now subclasses ``testtools.testcase.TestCase``.
> +  This permits features in testtools such as getUniqueInteger and
> +  getUniqueString to be used. (Robert Collins)
> +
>  * -Dhpssvfs will now trigger on ``RemoteBzrDir._ensure_real``, providing
>   more debugging of VFS access triggers. (Robert Collins)

You really need to explicitly mention that testtools is now a
requirement for running the Bazaar test suite, and which version, and
preferably also the URL. It seems like this should also update the
install instructions and maybe setup.py?

Also worth checking that you get a sane message if it's missing.

Since testtools will be changing I'd think about putting in a version
number check after the import.

More later maybe.

--
Martin <http://launchpad.net/~mbp/>

I've added some polish and improved the news entry as per Martin's request.

Doesn't seem as big a code complexity win as I'd hope for adding a whole new dependency.

+traceback._some_str = _clever_some_str

Not sure monkeypatching a builtin module in the middle of a several thousand line script is a good idea.

On Mon, 2009-12-14 at 21:38 +0000, Martin [gz] wrote:
> Doesn't seem as big a code complexity win as I'd hope for adding a whole new dependency.

How big is big enough ?

> +traceback._some_str = _clever_some_str
>
> Not sure monkeypatching a builtin module in the middle of a several thousand line script is a good idea.

Sadly, python 2.x is broken in this regard: __str__ on an exception with
a (a_str,) as its args, does an implicit decode of the string. We work
around this in bzrlib.errors by subclassing exception and reimplementing
__str__, but we can't do that for all errors that may be raised by
tests.

-Rob

I plan to land this this afternoon or perhaps tomorrow. Testtools 0.9.2 has been released, and the package is in debian. I can upload that to e.g. the bzr beta PPA.

There is a subunit 0.0.3 release as well, which makes --parallel=fork, ec2test and plain --subunit include separate attachments for the log files (but the fallback behaviour in testtools will include those anyway just not structured as such.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> I plan to land this this afternoon or perhaps tomorrow. Testtools 0.9.2 has been released, and the package is in debian. I can upload that to e.g. the bzr beta PPA.
>
> There is a subunit 0.0.3 release as well, which makes --parallel=fork, ec2test and plain --subunit include separate attachments for the log files (but the fallback behaviour in testtools will include those anyway just not structured as such.

Do you have any idea whether this will destroy the win32 test suite? Or
mac, or anything that doesn't have a deb package infrastructure? I
realize you are antsy to move forward, but given I just got win32
running again, I'd rather not have a serious breakdown at this point. (I
expect it to *probably* be ok, if you can get the right packages installed.)

The comments you've received certainly haven't been "this looks good,
let's land it", so I'm a bit surprised you are pushing this hard. Maybe
I'm wrong, but it feels like the conversation hasn't finished yet.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksoTYIACgkQJdeBCYSNAAPGgACgson2YfKfVf+Qs8r0DpDMCJ3A
GiwAn3Jc9fkaAe229h9Kmbxo/aHdahrU
=tUCZ
-----END PGP SIGNATURE-----

After upgrading to the latest testtools and subunit, I'm able to run bzr selftest using this proposed merge. Still waiting for it to finish though.

Perhaps I should mention that I'm running this on Vista64 using a 32-bit version of ActivePython 2.6.

On Wed, 2009-12-16 at 03:03 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > I plan to land this this afternoon or perhaps tomorrow. Testtools 0.9.2 has been released, and the package is in debian. I can upload that to e.g. the bzr beta PPA.
> >
> > There is a subunit 0.0.3 release as well, which makes --parallel=fork, ec2test and plain --subunit include separate attachments for the log files (but the fallback behaviour in testtools will include those anyway just not structured as such.
>
> Do you have any idea whether this will destroy the win32 test suite?

Yes, it won't.

> Or
> mac, or anything that doesn't have a deb package infrastructure? I
> realize you are antsy to move forward, but given I just got win32
> running again, I'd rather not have a serious breakdown at this point. (I
> expect it to *probably* be ok, if you can get the right packages installed.)

All you need is python-testtools (available via pypi, easy_install,
launchpad tarball or bzr branch lp:testtools), and python
2.4/2.5/2.6/2.7/3.1/3.2.

> The comments you've received certainly haven't been "this looks good,
> let's land it", so I'm a bit surprised you are pushing this hard. Maybe
> I'm wrong, but it feels like the conversation hasn't finished yet.

Martin Pool provided some comments, which I've considered and acted on
most of. He said 'maybe more later', that was 9 days ago; Martin [gz]
said 'not as much benefit as expected', and I've asked how much he
expected.

I have no sense of ongoing discussion about this; its been blocked on
testtools releasing an appropriate release - which it has now, and given
the absence of negative reviews, am planning on exercising the soft
timeout our review process has rather than delaying indefinitely. There
has been discussion on list about the general issue of dependencies, and
the outcome there was 'put it up and we can discuss it' (as I recall) -
which is what I've done. I also recall Martin (Pool) saying somewhere
that he is convinced that testtools has useful features and active
maintainers and he is ok with a dependency on it.

Of course, if you are concerned about it I'll hold off landing it, but
I'd like some articulation of concerns so that I can act on them. This
is important to me because I don't like writing code twice, and bzr's
current unittest structure forces me to do so.

This patch doesn't do any OS specific changes, so its very unlikely to
have an OS specific fallout. Gordon's positive feedback is an indicator
that its probably ok.

-Rob

2009/12/16 Robert Collins <email address hidden>:
> Martin Pool provided some comments, which I've considered and acted on
> most of. He said 'maybe more later', that was 9 days ago;

I'm not +1 until I get to read it again - the maybe was whether I
would get to that soon. If other core reviewers approve before I do,
that's great.

--
Martin <http://launchpad.net/~mbp/>

On Wed, 2009-12-16 at 04:57 +0000, Martin Pool wrote:
> 2009/12/16 Robert Collins <email address hidden>:
> > Martin Pool provided some comments, which I've considered and acted on
> > most of. He said 'maybe more later', that was 9 days ago;
>
> I'm not +1 until I get to read it again - the maybe was whether I
> would get to that soon. If other core reviewers approve before I do,
> that's great.

Given your encouragement to us to not block, do you think 10 days is
'long enough' to land in the absence of negative feedback?

Its not even a particularly large patch - 18 files changed, 210
insertions(+), 412 deletions(-) - so I feel that the lack of a full
review is an indication that its not important in either a risk or
benefits sense to other folk.

As I said to John, I won't land it if folk have concerns, but neither do
I want to block on it simply because folk aren't excited enough to
review it.

-Rob

Matt's tiny nitpicks of the day:

25 === modified file 'bzrlib/osutils.py'

42 + if type(object) == str:

Types should be compared with "is".

71 === modified file 'bzrlib/tests/__init__.py'

156 + # pythons this goes sufficiently wrong that it is a bad idea. (
157 + # specifically a built in file with encoding 'UTF-8' will still try
158 + # to encode using ascii.

Where's the ")"? :D

198 +def _clever_some_str(value):
199 + try:
200 + return str(value)
201 + except:

Blanket except? That's scary.

290 + @staticmethod
291 + def _do_known_failure(self, result, e):

299 + @staticmethod
300 def _do_not_applicable(self, result, e):

308 + @staticmethod
309 + def _do_unsupported_or_skip(self, result, e):

I should probably read the testtools docs or something, but... Static methods that take "self"?

597 === modified file 'bzrlib/tests/blackbox/test_debug.py'

606 + log = u"".join(self.getDetails()['log'].iter_text())

It's a bit unpleasant to have that pasted all over the test suite. Though I guess it isn't duplicated *that* many times, given the number of tests...

8 +* New helper osutils.StreamWriter which encodes unicode objects but
9 + passes str objects straight through. This is used for selftest but
10 + may be useful for diff and other operations that generate mixed output.
11 + (Robert Collins)
12 +

Please let's not have two things called StreamWriter that behave differently. needsfixing. Something like UnicodeOrBytesToBytesWriter.

+ log = u"".join(self.getDetails()['log'].iter_text())

Agree with Matt, this is ugly to repeat. Couldn't we have a convenience function for it?

On Wed, 2009-12-16 at 06:05 +0000, Matt Nordhoff wrote:
>
> 198 +def _clever_some_str(value):
> 199 + try:
> 200 + return str(value)
> 201 + except:
>
> Blanket except? That's scary.

Look at traceback._some_str :P.

-Rob

On Wed, 2009-12-16 at 06:09 +0000, Martin Pool wrote:
> Review: Needs Fixing
> 8 +* New helper osutils.StreamWriter which encodes unicode objects but
> 9 + passes str objects straight through. This is used for selftest but
> 10 + may be useful for diff and other operations that generate mixed output.
> 11 + (Robert Collins)
> 12 +
>
> Please let's not have two things called StreamWriter that behave differently. needsfixing. Something like UnicodeOrBytesToBytesWriter.

Ok. I had called it StreamWriter because I plan to submit a patch to
python to fix this, as I believe it meets the /intent/ of the python
code more accurately, but I'm happy to rename it for now.

> + log = u"".join(self.getDetails()['log'].iter_text())
>
> Agree with Matt, this is ugly to repeat. Couldn't we have a convenience function for it?

Sure, I was intent on getting away from the tricky implementation we
had, but having done so can add back a get_log() happily. I'll do this
tomorrow.

-Rob

So. I've tested without subunit nor testtools => clean failure:

bzr: ERROR: No module named testtools
You may need to install this Python library separately.

We may want to polish things a bit on platforms where this may be harder to install, but nothing urgent.

I've tested with my current versions of subunit/testtools, horrible crashes :) The NEWS entry should
take care of that (I don't know how many people are currently using testtools/subunit and running
the test suite but they are likely to read the NEWS).

BUT, running with up-to-date versions is far too verbose for XFAILs, the traceback is nicely presented
thank you, I just don't want to see it when running with --parallel=fork.

I'm +1 for get_log() and renaming StreamWriter until your patch is accepted.

That being said, it's a pleasure to see many warts removed (don't tell me you need BZRTransformingResult
back to addres the above :0) , thanks for that.

I'll be running this branch on babune and comment later about the results.

babune is still running but at least one full test suite has run all tracked OSes (OSX still suspended)
So, there is nothing important to add except that skipped tests are also too verbose:

per_branch.test_bound_sftp.BoundSFTPBranch.test_unbinding(BzrBranchFormat6) SKIP 8ms
Text attachment: log
------------

------------
Text attachment: reason
------------
Not a local URL
------------

Hmm, weird, I don't get the same behavior everywhere, some runs handle XFAIL but not SKIP, some handle neither...

On Wed, 2009-12-16 at 15:44 +0000, Vincent Ladeuil wrote:
>
> I've tested with my current versions of subunit/testtools, horrible
> crashes :) The NEWS entry should
> take care of that (I don't know how many people are currently using
> testtools/subunit and running
> the test suite but they are likely to read the NEWS).

It would help me when reporting a problem with a patch if you can paste
what it looks like, then I can describe that easily for other people.

> BUT, running with up-to-date versions is far too verbose for XFAILs,
> the traceback is nicely presented
> thank you, I just don't want to see it when running with
> --parallel=fork.

Oh, that was an unintended behavioural change, will fix. It is detecting
it as xfail FWIW.

-Rob

On Wed, 2009-12-16 at 20:44 +0000, Vincent Ladeuil wrote:
> babune is still running but at least one full test suite has run all tracked OSes (OSX still suspended)
> So, there is nothing important to add except that skipped tests are also too verbose:

What platform :- ?

./bzr selftest --no-plugins
per_branch.test_bound_sftp.BoundSFTPBranch.test_unbinding
--parallel=fork
bzr
selftest: /home/robertc/source/baz/pending/working/bzr
   /home/robertc/source/baz/pending/working/bzrlib
   bzr-2.1.0dev5 python-2.6.4
Linux-2.6.31-16-generic-x86_64-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 8 tests in 0.630s

OK
8 tests skipped

On Wed, 2009-12-16 at 21:00 +0000, Robert Collins wrote:
>
> > BUT, running with up-to-date versions is far too verbose for XFAILs,
> > the traceback is nicely presented
> > thank you, I just don't want to see it when running with
> > --parallel=fork.
>
> Oh, that was an unintended behavioural change, will fix. It is
> detecting
> it as xfail FWIW.

Hah! Brilliant!

Read the code for addExpectedFailure

it is *meant* to show the backtrace.

But it doesn't. Why? because it uses ui.ui_factory.note, and during the
execution of a test (which is when the current code reports things with
exceptions), ui.ui_factory is a SilentUIFactory.

Global State, I heart thee.

Oh, I've also been able to delete expectFailure, another helper thats in
the base class I missed.

-Rob

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> On Wed, 2009-12-16 at 21:00 +0000, Robert Collins wrote:
>>> BUT, running with up-to-date versions is far too verbose for XFAILs,
>>> the traceback is nicely presented
>>> thank you, I just don't want to see it when running with
>>> --parallel=fork.
>> Oh, that was an unintended behavioural change, will fix. It is
>> detecting
>> it as xfail FWIW.
>
> Hah! Brilliant!
>
> Read the code for addExpectedFailure
>
> it is *meant* to show the backtrace.
>
> But it doesn't. Why? because it uses ui.ui_factory.note, and during the
> execution of a test (which is when the current code reports things with
> exceptions), ui.ui_factory is a SilentUIFactory.
>
> Global State, I heart thee.
>
> Oh, I've also been able to delete expectFailure, another helper thats in
> the base class I missed.
>
>
> -Rob

I'll note that I personally find the expectFailure syntax to be
backwards. At least, IMO, you should have an assert which *succeeds*
which proves that the failure is occurring. Rather than expecting the
assert to fail. Because the latter case means that it can fail for any
reason, and still cause the expectFailure to pass.

But, I guess we have what we have.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkspgTQACgkQJdeBCYSNAANxwACePhFkg+V01paJ363ziq6X3PCv
27QAoKW9xy6Mt+JD1fGLoEEX0hfO/LDK
=ycMf
-----END PGP SIGNATURE-----

2009/12/17 John A Meinel <email address hidden>:
> I'll note that I personally find the expectFailure syntax to be
> backwards. At least, IMO, you should have an assert which *succeeds*
> which proves that the failure is occurring. Rather than expecting the
> assert to fail. Because the latter case means that it can fail for any
> reason, and still cause the expectFailure to pass.
>
> But, I guess we have what we have.

I don't mind renaming it or adding an alternative.

What I liked about expectFailure (and I think I added it) is that you
can just add it as a kind of preposition once you have a failing test.
 It's true it's less precise.

--
Martin <http://launchpad.net/~mbp/>

Its actually pretty precise - it has to raise AssertionError : there are
three possibly behaviours:
 - the predicate works -> test failure
 - the predicate raises AssertionError -> test pass
 - the predicate raises some other exception -> test failure

-Rob

> Martin [gz]
> said 'not as much benefit as expected', and I've asked how much he
> expected.

Sorry, launchpad seems not to send email if you only comment, so unless I happen to check back I miss things.

> How big is big enough ?

Well, currently when my code that depends on bazaar.tests breaks I have to read a 4500 line file to understand why. With this change, I'll have to read a 4300 line file and a new module I've never looked at before.

> Sadly, python 2.x is broken in this regard:

Sure, but is this really the best place to fix it? And when it's core python being fiddled with you expect at least an upstream bug report linked prominently near the fiddling.

On Thu, 2009-12-17 at 02:17 +0000, Martin [gz] wrote:
> > Martin [gz]
> > said 'not as much benefit as expected', and I've asked how much he
> > expected.
>
> Sorry, launchpad seems not to send email if you only comment, so unless I happen to check back I miss things.

It should be mailing on every review-or-comment.

> > How big is big enough ?
>
> Well, currently when my code that depends on bazaar.tests breaks I have to read a 4500 line file to understand why. With this change, I'll have to read a 4300 line file and a new module I've never looked at before.
>
> > Sadly, python 2.x is broken in this regard:
>
> Sure, but is this really the best place to fix it? And when it's core python being fiddled with you expect at least an upstream bug report linked prominently near the fiddling.

If you have a better place to suggest, I'm happy to do it there.

python2.x won't be fixed in this regard, and 3.0 doesn't have the same
confusion AIUI.

-Rob

> It should be mailing on every review-or-comment.

Hm, still no email. There's a nice big box on the page saying who's subscribed, and not button saying "add me" either. I'll set the review type this time which should get me cc-ed.

> If you have a better place to suggest, I'm happy to do it there.

Well, as printing exceptions is a testing kind of thing, the new upstream testing package?

Just tried installing the testtools package, it has three failures on Jython and doesn't install on IronPython. Probably shallow, I'll find some time to look into it.

Martin [gz] wrote:
> Review: Abstain
>> It should be mailing on every review-or-comment.
>
> Hm, still no email. There's a nice big box on the page saying who's subscribed, and not button saying "add me" either. I'll set the review type this time which should get me cc-ed.

Yer still not on the list. Try subscribing to the source or destination
branch:

<https://code.launchpad.net/~lifeless/bzr/subunit>
<https://code.launchpad.net/~bzr-pqm/bzr/bzr.dev>
--
Matt Nordhoff

On Thu, 2009-12-17 at 02:47 +0000, Martin [gz] wrote:
> Review: Abstain
> > It should be mailing on every review-or-comment.
>
> Hm, still no email. There's a nice big box on the page saying who's subscribed, and not button saying "add me" either. I'll set the review type this time which should get me cc-ed.
>
> > If you have a better place to suggest, I'm happy to do it there.
>
> Well, as printing exceptions is a testing kind of thing, the new upstream testing package?

Its only needed if you have exceptions that aren't valid [default codec]
- its not something I'd want just importing a regular library package to
do, but I think its ok for an application to do.

> Just tried installing the testtools package, it has three failures on Jython and doesn't install on IronPython. Probably shallow, I'll find some time to look into it.

Please report bugs at launchpad.net/testtools - will be happy to look at
them.

-Rob

2009/12/17 Martin [gz] <email address hidden>:
> Review: Abstain
>> It should be mailing on every review-or-comment.
>
> Hm, still no email. There's a nice big box on the page saying who's subscribed, and not button saying "add me" either. I'll set the review type this time which should get me cc-ed.

See https://bugs.edge.launchpad.net/launchpad-code/+bug/497617

--
Martin <http://launchpad.net/~mbp/>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> Its actually pretty precise - it has to raise AssertionError : there are
> three possibly behaviours:
> - the predicate works -> test failure
> - the predicate raises AssertionError -> test pass
> - the predicate raises some other exception -> test failure
>
> -Rob

So that is true, except you write the test as 'assertEqual, 20' # fails

Which means that it could be 30, or it could be 100. When it often
should be failing in a known way. I think you can turn it around as
"assertNotEqual, 52". Though I had to think about it for a while, which
shows it isn't just obvious.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkspt/cACgkQJdeBCYSNAAOUSwCeJZS2heN4ZK0chKYVAXN/Zye5
RmgAoNG44xgHx+DSFn6PWhl7B/Rb9kXZ
=4Kc+
-----END PGP SIGNATURE-----