Merge lp:~gz/bzr/python_2.7_selftest_582113 into lp:bzr

[Martin Packman]

Some python2.7 unittest fixes (_WriteLnDecorator, etc)

Get bzr selftest working on Python 2.7 by resolving some incompatibilities with the many changes to the unittest module (now a package).

Removes use of unittest._WriteLnDecorator which isn't really very helpful anyway. Also switches to using testtools.TextTestResult which shouldn't break the output any more than the original testtools switch already has - there aren't new test failures*.

Getting this branch up on babune is probably a precursor to deciding if it's practical to support Python 2.7 with the bzr 2.2 release, I'm tending towards 'no' currently as this was a pain.

I've not tested this all that thoroughly, so if something breaks for you, please yell.

* Okay, there are new test failures on Python 2.7 but for other reasons.

[Barry Warsaw (barry)]

JAM and I were looking at this today, and started down the _WriteLnDecorator change you made (but you did it more thoroughly). I'm not really qualified to review the patch for bzr, but +1 on supporting Python 2.7 if you can do it without break promises for earlier Pythons.

[John A Meinel (jameinel)]

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

Martin [gz] wrote:
> Martin [gz] has proposed merging lp:~gz/bzr/python_2.7_selftest_582113 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #582113 Tests don't run under Python 2.7 (unittest._WritelnDecorator has moved)
> https://bugs.launchpad.net/bugs/582113
>
>
> Get bzr selftest working on Python 2.7 by resolving some incompatibilities with the many changes to the unittest module (now a package).
>
> Removes use of unittest._WriteLnDecorator which isn't really very helpful anyway. Also switches to using testtools.TextTestResult which shouldn't break the output any more than the original testtools switch already has - there aren't new test failures*.
>
> Getting this branch up on babune is probably a precursor to deciding if it's practical to support Python 2.7 with the bzr 2.2 release, I'm tending towards 'no' currently as this was a pain.
>
> I've not tested this all that thoroughly, so if something breaks for you, please yell.
>
> * Okay, there are new test failures on Python 2.7 but for other reasons.
>

- - basic_tests = super(TestLoader, self).loadTestsFromModule(module)
+ basic_tests = super(TestLoader, self).loadTestsFromModule(module,
+ # GZ 2010-07-19: Python 2.7 unittest also uses load_tests but
with a
+ # different incompatible signature so have to avoid
+ **(sys.version_info > (2, 7) and {"use_load_tests": False}
or {}))

^- spell this out, this is pretty ugly as is.

kwargs = {}
if sys.version >= (2, 7):
  # Python 2.7 unittest supports load_tests, but using an incompatible
  # signature, so disable it
  kwargs['use_load_tests'] = False
basic_tests = super(TestLoader, self).loadTestsFromModule(module,
    **kwargs)

- - unittest._TextTestResult.__init__(self, stream, descriptions,
verbosity)
+ testtools.TextTestResult.__init__(self, stream)

^- Losing the parameters seems iffy, but if you are sure it is correct.
...

- - self.printErrors()
- - self.stream.writeln(self.separator2)
- - self.stream.writeln("%s %d test%s in %.3fs" % (actionTaken,
+ # GZ 2010-07-19: Seems testtools has no printErrors method, and
though
+ # the parent class method is similar have to
duplicate
+ self._show_list('ERROR', self.errors)
+ self._show_list('FAIL', self.failures)
+ self.stream.write(self.sep2)
+ self.stream.write("%s %d test%s in %.3fs\n\n" % (actionTaken,
                             run, run != 1 and "s" or "", timeTaken))
- - self.stream.writeln()

^- I'm happy to get rid of 'writeln()' though I wonder how much of
*unittest* expects it to be there. unittest._TextTestResult certainly
seems to.
I'm just concerned that we are technically breaking compatibility in
ways we won't find until a test fails, or we get an exception, or some
other non-standard case.

self.separator2 => self.sep2 also seems to be an odd change. (Either way
this seems to be something that should be private)

At the least, shouldn't we pull this code block into our own helper as a
replacement for 'printErrors()' ?

...

@@ -878...

review: Needs Fixing

[Vincent Ladeuil (vila)]

As a data point:

  http://babune.ladeuil.net:24842/job/selftest-maverick/6/console

This was with python2.7 from ppa:doko/toolchain

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> As a data point:
>
> http://babune.ladeuil.net:24842/job/selftest-maverick/6/console
>
> This was with python2.7 from ppa:doko/toolchain
>

Is this his code or trunk?

Looks like we got a unicode decoding exception which broke the subunit
toolchain...

John
=:->

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

iEYEARECAAYFAkxFz6cACgkQJdeBCYSNAAMltwCg2d/IjNPPP2meEh6Myy9heT7p
4WQAoNS5a5qQPPCC6onyjKKCAcAUYafL
=gyUh
-----END PGP SIGNATURE-----

[Martin Packman (gz)]

> JAM and I were looking at this today, and started down the _WriteLnDecorator
> change you made (but you did it more thoroughly). I'm not really qualified to
> review the patch for bzr, but +1 on supporting Python 2.7 if you can do it
> without break promises for earlier Pythons.

I've filed a few more bugs with a python2.7 tag, most of these are also due to bazaar extending unittest, but might be relevent for a few other projects.

> ^- spell this out, this is pretty ugly as is.

It's a fair cop guv. In my defence, this code is due to be ripped out when a resolution for bug 607412 is decided on.

> ^- Losing the parameters seems iffy, but if you are sure it is correct.

This and the following couple of comments are this way because the testtools testresult classes are a parallel implementation, not unittest subclasses.

> ^- I'm happy to get rid of 'writeln()' though I wonder how much of
> *unittest* expects it to be there. unittest._TextTestResult certainly
> seems to.

Only unittest classes we're no longer using expect a writeln method.

> self.separator2 => self.sep2 also seems to be an odd change. (Either way
> this seems to be something that should be private)

I presume the name change was done in testtools because the values are not strictly equivalent, unittest doesn't include the newline.

> At the least, shouldn't we pull this code block into our own helper as a
> replacement for 'printErrors()' ?

Yes, that's one option. I'd like testtools to improve here instead, its TextTestResult.stopTestRun method is nearly identical to the bzr one, just doesn't have any way of adding in the bzr specific bits by subclassing, so this is all duplication. Filed bug 607435 against testtools on that.

> ^- Do you have more info on this? loosing messages seems like a rather
> bad thing.

That's bug 607400 - filed after I wrote the code, which is why the lack of bug numbers, can add those in.

> As a data point:
>
> http://babune.ladeuil.net:24842/job/selftest-maverick/6/console
>
> This was with python2.7 from ppa:doko/toolchain

Ack, wasn't expecting that fast a response, thanks! The problem there is it needs the fix for testtools bug 604187 which hasn't been merged yet - or the previous testtools release would work as well I think.

[Vincent Ladeuil (vila)]

This is clearly going in the right direction but can't be landed as is so marking WIP.

Let's make sure we get the dependencies on testtools/subunit right too or it will be hard to review.

[Martin Packman (gz)]

Addressed John's point about the ugly workaround spelling, and replaced a few more unittest.TestResult method calls I somehow missed, which maybe caused the babune problems.

Elsewhere have fixed another testtools bug I'm to blame for, and Jonathan Lange has released 0.9.5 but there may still be another issue there or in subunit that needs shaking out. Another babune run with that updated as well would be great.

[John A Meinel (jameinel)]

Seems fine to me. I'm not sure what we need from testtools for python 2.7 support. Any chance you could clarify?

review: Approve

[Martin Packman (gz)]

The bit of my last comment talking about testtools was mostly addressed to Vincent - trying to run this branch with 2.7 on babune had some python-or-bzr-or-testtools-or-subunit fallout with streams leaking into other streams.

To answer you exact question, we don't need anything more from testtools for 2.7. Using their TextTestRunner avoids some unittest backwards compatibility problems, but there are still some other minor issues which I've filed bugs for.

[John A Meinel (jameinel)]

sent to pqm by email

[Martin Packman (gz)]

This was a prerequisite for the other two _python_2.7_ branches so was silently merged when the first of those was anyway.

=== modified file 'bzrlib/tests/TestUtil.py'
--- bzrlib/tests/TestUtil.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/TestUtil.py	2010-08-05 18:13:49 +0000
@@ -135,7 +135,13 @@
         >>>         result.addTests([test, test])
         >>>     return result
         """
-        basic_tests = super(TestLoader, self).loadTestsFromModule(module)
+        if sys.version_info < (2, 7):
+            basic_tests = super(TestLoader, self).loadTestsFromModule(module)
+        else:
+            # GZ 2010-07-19: Python 2.7 unittest also uses load_tests but with
+            #                a different and incompatible signature
+            basic_tests = super(TestLoader, self).loadTestsFromModule(module,
+                use_load_tests=False)
         load_tests = getattr(module, "load_tests", None)
         if load_tests is not None:
             return load_tests(basic_tests, module, self)

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-08-05 05:41:34 +0000
+++ bzrlib/tests/__init__.py	2010-08-05 18:13:49 +0000
@@ -141,7 +141,7 @@
 SUBUNIT_SEEK_CUR = 1
 
 
-class ExtendedTestResult(unittest._TextTestResult):
+class ExtendedTestResult(testtools.TextTestResult):
     """Accepts, reports and accumulates the results of running tests.
 
     Compared to the unittest version this class adds support for
@@ -168,7 +168,7 @@
         :param bench_history: Optionally, a writable file object to accumulate
             benchmark results.
         """
-        unittest._TextTestResult.__init__(self, stream, descriptions, verbosity)
+        testtools.TextTestResult.__init__(self, stream)
         if bench_history is not None:
             from bzrlib.version import _get_bzr_source_tree
             src_tree = _get_bzr_source_tree()
@@ -201,11 +201,13 @@
         actionTaken = "Ran"
         stopTime = time.time()
         timeTaken = stopTime - self.startTime
-        self.printErrors()
-        self.stream.writeln(self.separator2)
-        self.stream.writeln("%s %d test%s in %.3fs" % (actionTaken,
+        # GZ 2010-07-19: Seems testtools has no printErrors method, and though
+        #                the parent class method is similar have to duplicate
+        self._show_list('ERROR', self.errors)
+        self._show_list('FAIL', self.failures)
+        self.stream.write(self.sep2)
+        self.stream.write("%s %d test%s in %.3fs\n\n" % (actionTaken,
                             run, run != 1 and "s" or "", timeTaken))
-        self.stream.writeln()
         if not self.wasSuccessful():
             self.stream.write("FAILED (")
             failed, errored = map(len, (self.failures, self.errors))
@@ -218,20 +220,20 @@
                 if failed or errored: self.stream.write(", ")
                 self.stream.write("known_failure_count=%d" %
                     self.known_failure_count)
-            self.stream.writeln(")")
+            self.stream.write(")\n")
         else:
             if self.known_failure_count:
-                self.stream.writeln("OK (known_failures=%d)" %
+                self.stream.write("OK (known_failures=%d)\n" %
                     self.known_failure_count)
             else:
-                self.stream.writeln("OK")
+                self.stream.write("OK\n")
         if self.skip_count > 0:
             skipped = self.skip_count
-            self.stream.writeln('%d test%s skipped' %
+            self.stream.write('%d test%s skipped\n' %
                                 (skipped, skipped != 1 and "s" or ""))
         if self.unsupported:
             for feature, count in sorted(self.unsupported.items()):
-                self.stream.writeln("Missing feature '%s' skipped %d tests." %
+                self.stream.write("Missing feature '%s' skipped %d tests.\n" %
                     (feature, count))
         if self._strict:
             ok = self.wasStrictlySuccessful()
@@ -279,7 +281,7 @@
         return what
 
     def startTest(self, test):
-        unittest.TestResult.startTest(self, test)
+        super(ExtendedTestResult, self).startTest(test)
         if self.count == 0:
             self.startTests()
         self.report_test_start(test)
@@ -323,7 +325,7 @@
         fails with an unexpected error.
         """
         self._post_mortem()
-        unittest.TestResult.addError(self, test, err)
+        super(ExtendedTestResult, self).addError(test, err)
         self.error_count += 1
         self.report_error(test, err)
         if self.stop_early:
@@ -337,7 +339,7 @@
         fails because e.g. an assert() method failed.
         """
         self._post_mortem()
-        unittest.TestResult.addFailure(self, test, err)
+        super(ExtendedTestResult, self).addFailure(test, err)
         self.failure_count += 1
         self.report_failure(test, err)
         if self.stop_early:
@@ -357,7 +359,7 @@
                     test.id()))
         self.report_success(test)
         self._cleanupLogFile(test)
-        unittest.TestResult.addSuccess(self, test)
+        super(ExtendedTestResult, self).addSuccess(test)
         test._log_contents = ''
 
     def addExpectedFailure(self, test, err):
@@ -551,40 +553,40 @@
         return '%s%s' % (indent, err[1])
 
     def report_error(self, test, err):
-        self.stream.writeln('ERROR %s\n%s'
+        self.stream.write('ERROR %s\n%s\n'
                 % (self._testTimeString(test),
                    self._error_summary(err)))
 
     def report_failure(self, test, err):
-        self.stream.writeln(' FAIL %s\n%s'
+        self.stream.write(' FAIL %s\n%s\n'
                 % (self._testTimeString(test),
                    self._error_summary(err)))
 
     def report_known_failure(self, test, err):
-        self.stream.writeln('XFAIL %s\n%s'
+        self.stream.write('XFAIL %s\n%s\n'
                 % (self._testTimeString(test),
                    self._error_summary(err)))
 
     def report_success(self, test):
-        self.stream.writeln('   OK %s' % self._testTimeString(test))
+        self.stream.write('   OK %s\n' % self._testTimeString(test))
         for bench_called, stats in getattr(test, '_benchcalls', []):
-            self.stream.writeln('LSProf output for %s(%s, %s)' % bench_called)
+            self.stream.write('LSProf output for %s(%s, %s)\n' % bench_called)
             stats.pprint(file=self.stream)
         # flush the stream so that we get smooth output. This verbose mode is
         # used to show the output in PQM.
         self.stream.flush()
 
     def report_skip(self, test, reason):
-        self.stream.writeln(' SKIP %s\n%s'
+        self.stream.write(' SKIP %s\n%s\n'
                 % (self._testTimeString(test), reason))
 
     def report_not_applicable(self, test, reason):
-        self.stream.writeln('  N/A %s\n    %s'
+        self.stream.write('  N/A %s\n    %s\n'
                 % (self._testTimeString(test), reason))
 
     def report_unsupported(self, test, feature):
         """test cannot be run because feature is missing."""
-        self.stream.writeln("NODEP %s\n    The feature '%s' is not available."
+        self.stream.write("NODEP %s\n    The feature '%s' is not available.\n"
                 %(self._testTimeString(test), feature))
 
 
@@ -619,7 +621,7 @@
             encode = codec.encode
         stream = osutils.UnicodeOrBytesToBytesWriter(encode, stream)
         stream.encoding = new_encoding
-        self.stream = unittest._WritelnDecorator(stream)
+        self.stream = stream
         self.descriptions = descriptions
         self.verbosity = verbosity
         self._bench_history = bench_history

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-07-23 18:45:31 +0000
+++ bzrlib/tests/test_selftest.py	2010-08-05 18:13:49 +0000
@@ -801,7 +801,7 @@
         self.requireFeature(test_lsprof.LSProfFeature)
         result_stream = StringIO()
         result = bzrlib.tests.VerboseTestResult(
-            unittest._WritelnDecorator(result_stream),
+            result_stream,
             descriptions=0,
             verbosity=2,
             )
@@ -862,7 +862,7 @@
         # verbose test output formatting
         result_stream = StringIO()
         result = bzrlib.tests.VerboseTestResult(
-            unittest._WritelnDecorator(result_stream),
+            result_stream,
             descriptions=0,
             verbosity=2,
             )
@@ -878,6 +878,9 @@
         output = result_stream.getvalue()[prefix:]
         lines = output.splitlines()
         self.assertContainsRe(lines[0], r'XFAIL *\d+ms$')
+        if sys.version_info > (2, 7):
+            self.expectFailure("_ExpectedFailure on 2.7 loses the message",
+                self.assertNotEqual, lines[1], '    ')
         self.assertEqual(lines[1], '    foo')
         self.assertEqual(2, len(lines))
 
@@ -917,7 +920,7 @@
         # verbose test output formatting
         result_stream = StringIO()
         result = bzrlib.tests.VerboseTestResult(
-            unittest._WritelnDecorator(result_stream),
+            result_stream,
             descriptions=0,
             verbosity=2,
             )
@@ -1419,7 +1422,7 @@
         sample_test = TestTestCase("method_that_times_a_bit_twice")
         output_stream = StringIO()
         result = bzrlib.tests.VerboseTestResult(
-            unittest._WritelnDecorator(output_stream),
+            output_stream,
             descriptions=0,
             verbosity=2)
         sample_test.run(result)