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

On Aug 18, 2009, at 4:57 PM, Francis J. Lacoste wrote:

> 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?

Because this feeds into the same new code in the parent process that
now has the readline behavior. I don't think a flush is going to help
if the listener is doing a readline. Maybe I could change the
listener behavior for this case so that it no longer does a readline,
and then I could do a flush here, but just sending a newline seemed
like a nice one-line fix with no downside that I saw.

>> 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'

Ah, good catch. :-) Changed in the packages trunk (though I'm not
bothering to make a new release :-) ).

> Nice tests btw!

Thanks. Still a chance that they will have intermittent failures, but
maybe I'll be lucky ;-)

Gary