Code review comment for lp:~gz/bzr/python_2.7_selftest_582113

-----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,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))

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

In general, I'm happy with the direction this is going, just needs a bit
of conversation and possibly tweaking before landing.

John
=:->

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

iEYEARECAAYFAkxFm2cACgkQJdeBCYSNAANvQwCfTTC1rujtH3SRUbjJseULaYto
VF0AnRN04WMehq1T+TcVru8EJQ86Izjz
=k4yd
-----END PGP SIGNATURE-----