Code review comment for lp:~gary/launchpad/zbuildout

On August 14, 2009, Gary Poster wrote:
> Gary Poster has proposed merging lp:~gary/launchpad/zbuildout into
> lp:launchpad.
>
> Requested reviews:
> Francis J. Lacoste (flacoste)
>
> This branch changes us to use source distributions for our zope-related
> dependencies.
>
> Because we still are using some packages in sourcecode that want to be
> egg-based distributions--particularly Twisted--we filter out pkg_resources
> warning in _pythonpath and other scripts. This prevents some test failures
> that do not want to see any output from running a command.
>
> There are many other changes, of course.
>
> - Many are driven by changes in reprs of various objects--validation errors
> (LaunchpadValidationError) and ClientForm response wrappers
> (httperror_seek_wrapper) in particular.
>
> - Others are for API changes, like in zope.testing
> (lib/canonical/testing/__init__.py and ).
>
> - I commented out the retry tests, as we have discussed.
>
> - Some tests expected zope files to be in the tree, so I had to switch to
> the pkg_resources API, or simply use more proper zcml in some cases.
>
> - There's a case or two of tokens needing to be converted from unicode to
> binary strings before they go into the URL, as with the
> lp:~gary/canonical-identity-provider/zbuildout branch that accompanies
> this one.
>
> - zope.schema set fields no longer support sets.Set, only the built-in set,
> which also resulted in some changes.
>
> - The transaction.begin API now does not expect to encounter an in-progress
> transaction (lib/canonical/launchpad/doc/storm.txt and
> lib/canonical/launchpad/webapp/ftests/test_adapter_permissions.txt).
>
> - I found a couple of tests that failed in isolation. Related changes are
> in lib/canonical/launchpad/doc/batch_navigation.txt and
> lib/canonical/launchpad/scripts/ftests/test_oops_prune.py .
>
> - I'm not entirely sure how
>
> lib/lp/translations/stories/standalone/xx-pofile-translate-html-tags-escape
>.txt ever passed before. The doctest syntax there was wrong.
>
> - I switched back to the feedvalidator solution that we had discussed way
> back when, rather than using the hacked version. I left a comment to
> describe the situation.
>
> - Some tests did a browser reload. That now has a different behavior,
> resubmitting forms and so on, so I reload the same URL instead.
>
> - I did a fly-by on
> lib/lp/soyuz/browser/tests/binarypackagerelease-views.txt because that was
> a [testfix] problem I diagnosed yesterday, and using queryMultiAdapter hid
> the problem.
>
> versions.cfg comments out version numbers only when my changes represent a
> *previous* version that what was specified before. Happy to remove those
> as well.
>
> For this branch, I made changes and releases in the following packages:
>
> - zope.testing (modified trunk, released and used as zope.testing 3.8.1)
> - zope.security (modified trunk, released and used as zope.security 3.7.1)
> - zope.app.publication (modified trunk, eventually to be 3.8.2; also
> modified 3.4.3, released and used as zope.app.publication 3.4.4)
>
> Diffs to the dependencies are below. For zope.testing and zope.security, I
> only show the commit that made the substantive changes, rather than the
> commits generated by the release process. zope.testing has the biggest
> changes from our branch because it significantly changed how it handled
> subprocesses from the version we were using in order to support being able
> to run layers in multiple processes, taking advantage of multiple cores.
>
> ############
> zope.testing
> ############
>
> Log message for revision 102634:
> reduce batching and improve output for subprocesses
>

>
> Modified: zope.testing/trunk/src/zope/testing/testrunner/formatter.py
> ===================================================================
> --- zope.testing/trunk/src/zope/testing/testrunner/formatter.py 2009-08-10
> 18:23:10 UTC (rev 102633) +++
> zope.testing/trunk/src/zope/testing/testrunner/formatter.py 2009-08-10
> 20:57:09 UTC (rev 102634) @@ -52,6 +52,9 @@
>
> progress = property(lambda self: self.options.progress)
> verbose = property(lambda self: self.options.verbose)
> + in_subprocess = property(
> + lambda self: self.options.resume_layer is not None and
> + self.options.processes > 1)
>
> def compute_max_width(self):
> """Try to determine the terminal width."""
> @@ -263,6 +266,12 @@
>
> elif self.verbose == 1:
> sys.stdout.write('.' * test.countTestCases())
> +
> + elif self.in_subprocess:
> + sys.stdout.write('.' * test.countTestCases())
> + # Give the parent process a new line so it sees the progress
> + # in a timely manner.
> + sys.stdout.write('\n')

Why not use sys.stdout.flush() here instead of using a newline?

> Added:
> zope.testing/trunk/src/zope/testing/testrunner/testrunner-layers-buff.txt
> =================================================================== ---
> zope.testing/trunk/src/zope/testing/testrunner/testrunner-layers-buff.txt
> (rev 0) +++

> +
> +Now we actually check the results we care about. We should see that there
> are +two pauses of about half a second, one around the first test and one
> around the +second. Before the change that this test verfies, there was a

'verifies'

Nice tests btw!

--
Francis J. Lacoste
<email address hidden>