launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/simplify-buildout-bin-test into lp:launchpad.

Commit message:
Turn buildout-templates/bin/test.in into an entry point to a utility module.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/simplify-buildout-bin-test/+merge/323743

This brings us close to eradicating the use of z3c.recipe.filetemplate, which will simplify the upcoming conversion to pip.  All that remains is buildout-templates/_pythonpath.py.in.

Considerable care is needed with imports, because the config instance needs to be set before anything interesting is done with lp.services.config.  In practice this means that most imports need to be a little later in lp.scripts.utilities.test than one might normally expect, and (more subtly) that the utility module can't be under lp.testing because of the large number of non-trivial imports performed by lib/lp/testing/__init__.py.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/simplify-buildout-bin-test into lp:launchpad.

=== modified file 'README'
--- README	2011-02-18 18:43:16 +0000
+++ README	2017-05-08 12:04:30 +0000
@@ -46,8 +46,7 @@
   bin/, utilities/
     Where you will find scripts intended for developers and admins.  There's
     no rhyme or reason to what goes in bin/ and what goes in utilities/, so
-    take a look in both. bin/ will be empty in a fresh checkout, the actual
-    content lives in 'buildout-templates'.
+    take a look in both.
 
   configs/
     Configuration files for various kinds of Launchpad instances.
@@ -91,9 +90,7 @@
 of the ones that come up from time to time.
 
   buildout-templates/
-    Templates that are generated into actual files, normally bin/ scripts,
-    when buildout is run. If you want to change the behaviour of bin/test,
-    look here.
+    Templates that are generated into actual files when buildout is run.
 
   bzrplugins/
     Bazaar plugins used in running Launchpad.

=== removed directory 'buildout-templates/bin'
=== modified file 'doc/buildout.txt'
--- doc/buildout.txt	2014-01-30 15:04:06 +0000
+++ doc/buildout.txt	2017-05-08 12:04:30 +0000
@@ -549,7 +549,7 @@
 To add a file using the recipe, simply create mirrors of the source tree
 directories that you need under ``buildout-templates/``, and create a .in file
 template at the desired location.  Take a look at
-``buildout-templates/bin/`` for examples of what is possible.
+``buildout-templates/_pythonpath.py.in`` for an example of what is possible.
 
 .. _`z3c.recipe.filetemplate`: http://pypi.python.org/pypi/z3c.recipe.filetemplate
 

=== renamed file 'buildout-templates/bin/test.in' => 'lib/lp/scripts/utilities/test.py'
--- buildout-templates/bin/test.in	2017-01-20 01:46:34 +0000
+++ lib/lp/scripts/utilities/test.py	2017-05-08 12:04:30 +0000
@@ -1,4 +1,3 @@
-#!${buildout:executable} -S
 ##############################################################################
 #
 # Copyright (c) 2004 Zope Corporation and Contributors.
@@ -12,209 +11,211 @@
 # FOR A PARTICULAR PURPOSE.
 #
 ##############################################################################
-"""Test script
-"""
-
-# NOTE: This is a generated file.  The original is in
-# buildout-templates/bin/test.in
-
-import logging, os, re, sys, time, warnings
-
-# Initialize our paths.
-${python-relative-path-setup}
-
-
-# The working directory change is just so that the test script
-# can be invoked from places other than the root of the source
-# tree. This is very useful for IDE integration, so an IDE can
-# e.g. run the test that you are currently editing.
-BUILD_DIR = ${buildout:directory|path-repr}
-there = os.getcwd()
-os.chdir(BUILD_DIR)
-
-
-import sys
-sys.path.insert(0, ${scripts:parts-directory|path-repr})
-import site
-
-
-# Fix doctest so that it can handle mixed unicode and encoded output.
+"""Test script."""
+
 import doctest
-
-_RealSpoofOut = doctest._SpoofOut
-
-class _SpoofOut(doctest._SpoofOut):
-
-    def write(self, value):
-        if isinstance(value, unicode):
-            value = value.encode('utf8')
-        _RealSpoofOut.write(self, value)
-
-doctest._SpoofOut = _SpoofOut
-
-
-CUSTOM_SITE_DIR = ${scripts:parts-directory|path-repr}
-
-# Make tests run in a timezone no launchpad developers live in.
-# Our tests need to run in any timezone.
-# (This is no longer actually required, as PQM does this.)
-os.environ['TZ'] = 'Asia/Calcutta'
-time.tzset()
-
-# Httplib2 0.7 started validating SSL certificates, and the test suite uses a
-# self-signed certificate. So disable it with an env variable
-os.environ['LP_DISABLE_SSL_CERTIFICATE_VALIDATION'] = '1'
-
-# Storm's C extensions should already be enabled from lp_sitecustomize.py,
-# which our custom sitecustomize.py ran.
-assert os.environ['STORM_CEXTENSIONS'] == '1'
-
-# Make sure our site.py is the one that subprocesses use.
-os.environ['PYTHONPATH'] = CUSTOM_SITE_DIR
-
-# Set a flag if this is the main testrunner process
-if len(sys.argv) > 1 and sys.argv[1] == '--resume-layer':
-    main_process = False
-else:
-    main_process = True
-
-# Install the import fascist import hook and atexit handler.
-from lp.scripts.utilities import importfascist
-importfascist.install_import_fascist()
-
-# Install the warning handler hook and atexit handler.
-from lp.scripts.utilities import warninghandler
-warninghandler.install_warning_handler()
-
-# Ensure that atexit handlers are executed on TERM.
+import os
+import random
+import re
 import signal
-def exit_with_atexit_handlers(*ignored):
-    sys.exit(-1 * signal.SIGTERM)
-signal.signal(signal.SIGTERM, exit_with_atexit_handlers)
-
-# Tell lp.services.config to use the testrunner config instance.
-from lp.services.config import config
-config.setInstance('testrunner')
-config.generate_overrides()
-
-# Remove this module's directory from path, so that zope.testbrowser
-# can import pystone from test:
-sys.path[:] = [p for p in sys.path if os.path.abspath(p) != BUILD_DIR]
-
-# Turn on psycopg debugging wrapper
-#import lp.services.database.debug
-#lp.services.database.debug.install()
-
-# Unset the http_proxy environment variable, because we're going to make
-# requests to localhost and we don't wand this to be proxied.
-try:
-    os.environ.pop('http_proxy')
-except KeyError:
-    pass
-
-# Suppress accessability warning because the test runner does not have UI.
-os.environ['GTK_MODULES'] = ''
-
-# Silence spurious warnings. Note that this does not propagate to subprocesses
-# so this is not always as easy as it seems. Warnings caused by our code that
-# need to be silenced should have an accompanied Bug reference.
-#
-warnings.filterwarnings(
-    'ignore', 'PyCrypto', RuntimeWarning, 'twisted[.]conch[.]ssh',
-    )
-warnings.filterwarnings(
-    'ignore', 'twisted.python.plugin', DeprecationWarning,
-    )
-warnings.filterwarnings(
-    'ignore', 'zope.testing.doctest', DeprecationWarning,
-    )
-warnings.filterwarnings(
-    'ignore', 'bzrlib.*was deprecated', DeprecationWarning,
-    )
-# The next one is caused by a lamosity in python-openid.  The following change
-# to openid/server/server.py would make the warning filter unnecessary:
-# 978c974,974
-# >         try:
-# >             namespace = request.message.getOpenIDNamespace()
-# >         except AttributeError:
-# >             namespace = request.namespace
-# >         self.fields = Message(namespace)
-# ---
-# <         self.fields = Message(request.namespace)
-warnings.filterwarnings(
-    'ignore',
-    (r'The \"namespace\" attribute of CheckIDRequest objects is deprecated.\s+'
-     r'Use \"message.getOpenIDNamespace\(\)\" instead'),
-    DeprecationWarning
-)
-# This warning will be triggered if the beforeTraversal hook fails. We
-# want to ensure it is not raised as an error, as this will mask the real
-# problem.
-warnings.filterwarnings(
-    'always',
-    re.escape('clear_request_started() called outside of a request'),
-    UserWarning
-    )
-# Unicode warnings are always fatal
-warnings.filterwarnings('error', category=UnicodeWarning)
-
-# shortlist() raises an error when it is misused.
-warnings.filterwarnings('error', r'shortlist\(\)')
-
-from lp.testing import pgsql
-# If this is removed, make sure lp.testing.pgsql is updated
-# because the test harness there relies on the Connection wrapper being
-# installed.
-pgsql.installFakeConnect()
+import sys
+import time
+import warnings
 
 from zope.testing import testrunner
 from zope.testing.testrunner import options
 
+from lp.scripts.utilities import (
+    importfascist,
+    warninghandler,
+    )
+from lp.services.config import config
+
+
+def fix_doctest_output():
+    # Fix doctest so that it can handle mixed unicode and encoded output.
+    _RealSpoofOut = doctest._SpoofOut
+
+    class _SpoofOut(doctest._SpoofOut):
+
+        def write(self, value):
+            if isinstance(value, unicode):
+                value = value.encode('utf8')
+            _RealSpoofOut.write(self, value)
+
+    doctest._SpoofOut = _SpoofOut
+
+
+def configure_environment():
+    # Make tests run in a timezone no launchpad developers live in.
+    # Our tests need to run in any timezone.
+    # (This is no longer actually required, as PQM does this.)
+    os.environ['TZ'] = 'Asia/Calcutta'
+    time.tzset()
+
+    # Httplib2 0.7 started validating SSL certificates, and the test suite
+    # uses a self-signed certificate, so disable it with an env variable.
+    os.environ['LP_DISABLE_SSL_CERTIFICATE_VALIDATION'] = '1'
+
+    # Storm's C extensions should already be enabled from
+    # lp_sitecustomize.py, which our custom sitecustomize.py ran.
+    assert os.environ['STORM_CEXTENSIONS'] == '1'
+
+    # Install the import fascist import hook and atexit handler.
+    importfascist.install_import_fascist()
+
+    # Install the warning handler hook and atexit handler.
+    warninghandler.install_warning_handler()
+
+    # Ensure that atexit handlers are executed on TERM.
+    def exit_with_atexit_handlers(*ignored):
+        sys.exit(-1 * signal.SIGTERM)
+    signal.signal(signal.SIGTERM, exit_with_atexit_handlers)
+
+    # Tell lp.services.config to use the testrunner config instance.
+    config.setInstance('testrunner')
+    config.generate_overrides()
+
+    # Remove this module's directory from path, so that zope.testbrowser
+    # can import pystone from test:
+    sys.path[:] = [p for p in sys.path if os.path.abspath(p) != config.root]
+
+    # Turn on psycopg debugging wrapper
+    #import lp.services.database.debug
+    #lp.services.database.debug.install()
+
+    # Unset the http_proxy environment variable, because we're going to make
+    # requests to localhost and we don't want this to be proxied.
+    os.environ.pop('http_proxy', None)
+
+    # Suppress accessibility warning because the test runner does not have UI.
+    os.environ['GTK_MODULES'] = ''
+
+
+def filter_warnings():
+    # Silence spurious warnings. Note that this does not propagate to
+    # subprocesses so this is not always as easy as it seems. Warnings
+    # caused by our code that need to be silenced should have an accompanied
+    # Bug reference.
+    warnings.filterwarnings(
+        'ignore', 'PyCrypto', RuntimeWarning, 'twisted[.]conch[.]ssh',
+        )
+    warnings.filterwarnings(
+        'ignore', 'twisted.python.plugin', DeprecationWarning,
+        )
+    warnings.filterwarnings(
+        'ignore', 'zope.testing.doctest', DeprecationWarning,
+        )
+    warnings.filterwarnings(
+        'ignore', 'bzrlib.*was deprecated', DeprecationWarning,
+        )
+    # The next one is caused by an infelicity in python-openid.  The
+    # following change to openid/server/server.py would make the warning
+    # filter unnecessary:
+    # 978c974,974
+    # >         try:
+    # >             namespace = request.message.getOpenIDNamespace()
+    # >         except AttributeError:
+    # >             namespace = request.namespace
+    # >         self.fields = Message(namespace)
+    # ---
+    # <         self.fields = Message(request.namespace)
+    warnings.filterwarnings(
+        'ignore',
+        (r'The \"namespace\" attribute of CheckIDRequest objects is '
+         r'deprecated.\s+'
+         r'Use \"message.getOpenIDNamespace\(\)\" instead'),
+        DeprecationWarning
+    )
+    # This warning will be triggered if the beforeTraversal hook fails. We
+    # want to ensure it is not raised as an error, as this will mask the
+    # real problem.
+    warnings.filterwarnings(
+        'always',
+        re.escape('clear_request_started() called outside of a request'),
+        UserWarning
+        )
+    # Unicode warnings are always fatal
+    warnings.filterwarnings('error', category=UnicodeWarning)
+
+    # shortlist() raises an error when it is misused.
+    warnings.filterwarnings('error', r'shortlist\(\)')
+
+
+def install_fake_pgsql_connect():
+    from lp.testing import pgsql
+    # If this is removed, make sure lp.testing.pgsql is updated
+    # because the test harness there relies on the Connection wrapper being
+    # installed.
+    pgsql.installFakeConnect()
+
+
+def randomise_listdir():
+    # Monkey-patch os.listdir to randomise the results.
+    original_listdir = os.listdir
+
+    def listdir(path):
+        """Randomise the results of os.listdir.
+
+        It uses random.shuffle to randomise os.listdir results.  This way
+        tests relying on unstable ordering will have a higher chance to fail
+        in the development environment.
+        """
+        directory_contents = original_listdir(path)
+        random.shuffle(directory_contents)
+        return directory_contents
+
+    os.listdir = listdir
+
+
 defaults = {
     # Find tests in the tests and ftests directories
     'tests_pattern': '^f?tests$',
-    'test_path': [${buildout:directory/lib|path-repr}],
+    'test_path': [os.path.join(config.root, 'lib')],
     'package': ['canonical', 'lp', 'devscripts', 'launchpad_loggerhead'],
     'layer': ['!(YUIAppServerLayer)'],
     'require_unique_ids': True,
     }
 
-# Monkey-patch os.listdir to randomise the results
-original_listdir = os.listdir
-
-import random
-
-def listdir(path):
-    """Randomise the results of os.listdir.
-
-    It uses random.suffle to randomise os.listdir results, this way tests
-    relying on unstable ordering will have a higher chance to fail in the
-    development environment.
-    """
-    directory_contents = original_listdir(path)
-    random.shuffle(directory_contents)
-    return directory_contents
-
-os.listdir = listdir
-
-
-from lp.services.testing.customresult import filter_tests, patch_find_tests
-
-
-if __name__ == '__main__':
+
+def main():
+    # The working directory change is just so that the test script
+    # can be invoked from places other than the root of the source
+    # tree. This is very useful for IDE integration, so an IDE can
+    # e.g. run the test that you are currently editing.
+    there = os.getcwd()
+    os.chdir(config.root)
+
+    fix_doctest_output()
+    configure_environment()
+    filter_warnings()
+    install_fake_pgsql_connect()
+    randomise_listdir()
+
+    # The imports at the top of this file must avoid anything that reads
+    # from Launchpad config. Now that we've set the correct config instance,
+    # we can safely import the rest.
+    from lp.services.testing.customresult import (
+        filter_tests,
+        patch_find_tests,
+        )
+    from lp.services.testing import profiled
+
     # Extract arguments so we can see them too. We need to strip
     # --resume-layer and --default stuff if found as get_options can't
     # handle it.
     if len(sys.argv) > 1 and sys.argv[1] == '--resume-layer':
+        main_process = False
         args = list(sys.argv)
-        args.pop(1) # --resume-layer
-        args.pop(1) # The layer name
-        args.pop(1) # The resume number
+        args.pop(1)  # --resume-layer
+        args.pop(1)  # The layer name
+        args.pop(1)  # The resume number
         while len(args) > 1 and args[1] == '--default':
-            args.pop(1) # --default
-            args.pop(1) # The default value
+            args.pop(1)  # --default
+            args.pop(1)  # The default value
         args.insert(0, sys.argv[0])
     else:
+        main_process = True
         args = sys.argv
 
     # thunk across to parallel support if needed.
@@ -250,7 +251,6 @@
             options.parser.defaults[name] = value
 
     # Turn on Layer profiling if requested.
-    from lp.services.testing import profiled
     if local_options.verbose >= 3 and main_process:
         profiled.setup_profiling()
 

=== modified file 'setup.py'
--- setup.py	2017-01-21 13:42:28 +0000
+++ setup.py	2017-05-08 12:04:30 +0000
@@ -175,6 +175,7 @@
             'run-testapp = lp.scripts.runlaunchpad:start_testapp',
             'sprite-util = lp.scripts.utilities.spriteutil:main',
             'start_librarian = lp.scripts.runlaunchpad:start_librarian',
+            'test = lp.scripts.utilities.test:main',
             'twistd = twisted.scripts.twistd:run',
             'watch_jsbuild = lp.scripts.utilities.js.watchjsbuild:main',
             'with-xvfb = lp.scripts.utilities.withxvfb:main',