Launchpad itself

Merge lp://qastaging/~bjornt/launchpad/bug-495397 into lp://qastaging/launchpad

  1. bug-495397
  2. Merge into devel
Proposed by Björn Tillenius
Status: Merged
Approved by: Gary Poster
Approved revision: not available
Merged at revision: not available
Proposed branch: lp://qastaging/~bjornt/launchpad/bug-495397
Merge into: lp://qastaging/launchpad
Diff against target: 43 lines (+14/-1)
1 file modified
lib/canonical/testing/layers.py (+14/-1)
To merge this branch: bzr merge lp://qastaging/~bjornt/launchpad/bug-495397
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email: mp+16007@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Björn Tillenius (bjornt) wrote :

Add a fake fileno() method to sys.stdin, if it doesn't exist.

When running different test layers, the test runner starts a new
sub-process, if it encounters a layer that can't be torn down
in-process. When this happens, sys.stdin is replaced with a
FakeInputContinueGenerator instance. This instance doesn't have a fileno
method, making it impossible for Windmill to start Firefox. By adding a
fake fileno() method everything works as expected. I made it return
None, since I would assume that if anything tried to use it, things
would break, rather than silently work in an odd way.

--
Björn Tillenius | https://launchpad.net/~bjornt

Revision history for this message
Gary Poster (gary) wrote :

merge-conditional

Hi Björn. This looks good. Thank you. My only concerns are these.

(1) Should we be more careful to only do what we expect to be doing? IOW, I'd feel better if we had

    if not safe_hasattr(sys.stdin, 'fileno'):
        assert isinstance(sys.stdin,
                          zope.testing.testrunner.runner.FakeInputContinueGenerator), (
            'Unexpected value for sys.stdin')

(2) Should we try getting this upstream? It seems reasonable.

Gary

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

As discussed on IRC...

On Fri, Dec 11, 2009 at 03:08:50PM -0000, Gary Poster wrote:
> Review: Approve
> merge-conditional
>
> Hi Björn. This looks good. Thank you. My only concerns are these.
>
> (1) Should we be more careful to only do what we expect to be doing? IOW, I'd feel better if we had
>
> if not safe_hasattr(sys.stdin, 'fileno'):
> assert isinstance(sys.stdin,
> zope.testing.testrunner.runner.FakeInputContinueGenerator), (
> 'Unexpected value for sys.stdin')

I added the assert, it's good to do a sanity check.

> (2) Should we try getting this upstream? It seems reasonable.

Ideally, yes. I'll file a bug and propose a patch.

--
Björn Tillenius | https://launchpad.net/~bjornt

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/testing/layers.py'
2--- lib/canonical/testing/layers.py 2009-12-09 06:32:33 +0000
3+++ lib/canonical/testing/layers.py 2009-12-11 15:19:13 +0000
4@@ -73,18 +73,21 @@
5 import transaction
6 import wsgi_intercept
7
8+from lazr.restful.utils import safe_hasattr
9+
10 from windmill.bin.admin_lib import (
11 start_windmill, teardown as windmill_teardown)
12
13-from zope.app.publication.httpfactory import chooseClasses
14 import zope.app.testing.functional
15 import zope.publisher.publish
16+from zope.app.publication.httpfactory import chooseClasses
17 from zope.app.testing.functional import FunctionalTestSetup, ZopePublication
18 from zope.component import getUtility, provideUtility
19 from zope.component.interfaces import ComponentLookupError
20 from zope.security.management import getSecurityPolicy
21 from zope.security.simplepolicies import PermissiveSecurityPolicy
22 from zope.server.logger.pythonlogger import PythonLogger
23+from zope.testing.testrunner.runner import FakeInputContinueGenerator
24
25 from canonical.lazr import pidfile
26 from canonical.config import CanonicalConfig, config, dbconfig
27@@ -1715,6 +1718,16 @@
28 # base_url. With no base_url, we can't create the config
29 # file windmill needs.
30 return
31+ # If we're running in a bin/test sub-process, sys.stdin is
32+ # replaced by FakeInputContinueGenerator, which doesn't have a
33+ # fileno method. When Windmill starts Firefox,
34+ # sys.stdin.fileno() is called, so we add such a method here, to
35+ # prevent it from breaking. By returning None, we should ensure
36+ # that it doesn't try to use the return value for anything.
37+ if not safe_hasattr(sys.stdin, 'fileno'):
38+ assert isinstance(sys.stdin, FakeInputContinueGenerator), (
39+ "sys.stdin (%r) doesn't have a fileno method." % sys.stdin)
40+ sys.stdin.fileno = lambda: None
41 # Windmill needs a config file on disk.
42 config_text = dedent("""\
43 START_FIREFOX = True