launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24129
[Merge] ~cjwatson/launchpad:modern-ztk-prepare into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:modern-ztk-prepare into launchpad:master.
Commit message:
Fix various problems upgrading to modern ZTK
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/375697
I've been working on upgrading Launchpad to modern versions of its Zope Toolkit dependencies. This bundles a collection of small fixes discovered in the process, all of which are needed to make tests pass after the upgrade. This doesn't actually apply any package upgrades; that will come later.
I'd particularly like to get the change to lp.services.webapp.tests.test_session landed before upgrading, since that will allow us to verify that session IDs match before and after upgrading zope.session.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:modern-ztk-prepare into launchpad:master.
diff --git a/lib/lp/app/stories/basics/xx-request-expired.txt b/lib/lp/app/stories/basics/xx-request-expired.txt
index 2e3306a..fe21067 100644
--- a/lib/lp/app/stories/basics/xx-request-expired.txt
+++ b/lib/lp/app/stories/basics/xx-request-expired.txt
@@ -8,9 +8,6 @@ causing a soft timeout to be logged.
>>> from lp.services.config import config
>>> from textwrap import dedent
- >>> from lp.testing.fixture import CaptureOops
- >>> capture = CaptureOops()
- >>> capture.setUp()
>>> test_data = dedent("""
... [database]
... db_statement_timeout: 1
@@ -28,11 +25,9 @@ causing a soft timeout to be logged.
<title>Error: Timeout</title>
...
- >>> capture.oopses[0]['type']
+ >>> oops_capture.oopses[-1]['type']
'RequestExpired'
- >>> capture.cleanUp()
-
Let's reset the config value we changed:
>>> base_test_data = config.pop('base_test_data')
diff --git a/lib/lp/app/stories/basics/xx-soft-timeout.txt b/lib/lp/app/stories/basics/xx-soft-timeout.txt
index 65fa191..9a2e175 100644
--- a/lib/lp/app/stories/basics/xx-soft-timeout.txt
+++ b/lib/lp/app/stories/basics/xx-soft-timeout.txt
@@ -39,9 +39,6 @@ If we set soft_request_timeout to some value, the page will take
slightly longer then the soft_request_timeout value to generate, thus
causing a soft timeout to be logged.
- >>> from lp.testing.fixture import CaptureOops
- >>> capture = CaptureOops()
- >>> capture.setUp()
>>> from textwrap import dedent
>>> test_data = (dedent("""
... [database]
@@ -57,13 +54,9 @@ causing a soft timeout to be logged.
...
Soft timeout threshold is set to 1 ms. This page took ... ms to render.
- >>> capture.oopses[0]['type']
+ >>> oops_capture.oopses[-1]['type']
'SoftRequestTimeout'
-Unregister from the oops collection:
-
- >>> capture.cleanUp()
-
Since the page rendered correctly, we assume it's a soft timeout error,
since otherwise we would have gotten an OOPS page when we tried to
render the page.
diff --git a/lib/lp/bugs/doc/externalbugtracker.txt b/lib/lp/bugs/doc/externalbugtracker.txt
index 2a902f4..1e3a99a 100644
--- a/lib/lp/bugs/doc/externalbugtracker.txt
+++ b/lib/lp/bugs/doc/externalbugtracker.txt
@@ -750,17 +750,13 @@ We temporarily silence the logging from this function because we're not
interested in it. Again, the watch's lastchecked field will also be
updated.
- >>> from lp.testing.fixture import CaptureOops
- >>> capture = CaptureOops()
- >>> capture.setUp()
-
>>> external_bugtracker.initialize_remote_bugdb_error = None
>>> for error in [UnparsableBugData, Exception]:
... example_bugwatch.lastchecked = None
... external_bugtracker.get_remote_status_error = error
... bug_watch_updater.updateBugWatches(
... external_bugtracker, [example_bugwatch])
- ... oops = capture.oopses[-1]
+ ... oops = oops_capture.oopses[-1]
... print("%s: %s (%s; %s)" % (
... example_bugwatch.last_error_type.title,
... example_bugwatch.lastchecked is not None,
@@ -768,8 +764,6 @@ updated.
Unparsable Bug: True (OOPS-...; http://bugs.example.com/show_bug.cgi?id=1)
Unknown: True (OOPS-...; http://bugs.example.com/show_bug.cgi?id=1)
- >>> capture.cleanUp()
-
Using `LookupTree` to map statuses
----------------------------------
diff --git a/lib/lp/code/interfaces/gitref.py b/lib/lp/code/interfaces/gitref.py
index 16198eb..3c5f693 100644
--- a/lib/lp/code/interfaces/gitref.py
+++ b/lib/lp/code/interfaces/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Git reference ("ref") interfaces."""
@@ -419,7 +419,9 @@ class IGitRefEdit(Interface):
# _schema_circular_imports.py.
value_type=InlineObject(schema=Interface),
description=_(dedent("""\
- The new list of grants for this reference. For example::
+ The new list of grants for this reference.
+
+ For example::
[
{
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 2fca251..386ad62 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -812,7 +812,9 @@ class IGitRepositoryEdit(IWebhookTarget):
# _schema_circular_imports.py.
value_type=InlineObject(schema=Interface),
description=_(dedent("""\
- The new list of rules for this repository. For example::
+ The new list of rules for this repository.
+
+ For example::
[
{
diff --git a/lib/lp/registry/interfaces/poll.py b/lib/lp/registry/interfaces/poll.py
index 75a6af2..8f630d6 100644
--- a/lib/lp/registry/interfaces/poll.py
+++ b/lib/lp/registry/interfaces/poll.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__all__ = [
@@ -171,7 +171,9 @@ class IPoll(Interface):
raise Invalid(
"A poll cannot close at the time (or before) it opens.")
now = datetime.now(pytz.UTC)
- twelve_hours_ahead = now + timedelta(hours=12)
+ # Allow a bit of slack to account for time between form creation and
+ # validation.
+ twelve_hours_ahead = now + timedelta(hours=12) - timedelta(seconds=60)
start_date = poll.dateopens.astimezone(pytz.UTC)
if start_date < twelve_hours_ahead:
raise Invalid(
diff --git a/lib/lp/services/fields/__init__.py b/lib/lp/services/fields/__init__.py
index ee9e16f..6bbc0b3 100644
--- a/lib/lp/services/fields/__init__.py
+++ b/lib/lp/services/fields/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -65,7 +65,10 @@ from lazr.uri import (
URI,
)
from zope.component import getUtility
-from zope.interface import implementer
+from zope.interface import (
+ implementer,
+ Interface,
+ )
from zope.schema import (
Bool,
Bytes,
@@ -83,7 +86,6 @@ from zope.schema.interfaces import (
IBytes,
IDate,
IDatetime,
- Interface,
IObject,
IText,
ITextLine,
diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
index 635e075..800d303 100644
--- a/lib/lp/services/job/runner.py
+++ b/lib/lp/services/job/runner.py
@@ -267,6 +267,17 @@ class BaseRunnableJob(BaseRunnableJobSource):
response = self.runViaCelery(ignore_result)
if not ignore_result:
BaseRunnableJob.celery_responses.append(response)
+ # transaction >= 1.6.0 removes data managers from the
+ # transaction before calling after-commit hooks; this means that
+ # the transaction begun in runViaCelery to look up details of
+ # the job doesn't get committed or rolled back, and so
+ # subsequent SQL statements executed by the caller confusingly
+ # see a database snapshot from before the Celery job was run,
+ # even if they use the block_on_job test helper. Since
+ # runViaCelery never issues any data-modifying statements
+ # itself, the least confusing thing to do here is to just roll
+ # back its transaction.
+ transaction.abort()
def celeryRunOnCommit(self):
"""Configure transaction so that commit runs this job via Celery."""
diff --git a/lib/lp/services/mail/mbox.py b/lib/lp/services/mail/mbox.py
index 0ae2444..1767523 100644
--- a/lib/lp/services/mail/mbox.py
+++ b/lib/lp/services/mail/mbox.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""An IMailer that stores messages in a specified mbox file."""
@@ -69,3 +69,10 @@ class MboxMailer:
chained_mailer = getUtility(IMailer, self.mailer)
chained_mailer.send(fromaddr, toaddrs, message)
return message_id
+
+ def vote(self, fromaddr, toaddrs, message):
+ pass
+
+ def abort(self):
+ # We don't do any work until send() is called, so aborting is trivial.
+ pass
diff --git a/lib/lp/services/mail/stub.py b/lib/lp/services/mail/stub.py
index 98d40da..1492057 100644
--- a/lib/lp/services/mail/stub.py
+++ b/lib/lp/services/mail/stub.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""A stub IMailer for use in development and unittests."""
@@ -52,6 +52,13 @@ class StubMailer:
sendmail = getUtility(IMailer, self.mailer)
sendmail.send(self.from_addr, self.to_addrs, message)
+ def vote(self, fromaddr, toaddrs, message):
+ pass
+
+ def abort(self):
+ # We don't do any work until send() is called, so aborting is trivial.
+ pass
+
test_emails = []
@@ -67,3 +74,10 @@ class TestMailer:
def send(self, from_addr, to_addrs, message):
test_emails.append((from_addr, to_addrs, message))
+
+ def vote(self, fromaddr, toaddrs, message):
+ pass
+
+ def abort(self):
+ # We don't do any work until send() is called, so aborting is trivial.
+ pass
diff --git a/lib/lp/services/scripts/__init__.py b/lib/lp/services/scripts/__init__.py
index 51456c2..d4a362d 100644
--- a/lib/lp/services/scripts/__init__.py
+++ b/lib/lp/services/scripts/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Library functions for use in all scripts.
@@ -20,10 +20,10 @@ import os
import sys
import threading
+import zope.component.hooks
from zope.configuration.config import ConfigurationMachine
from zope.security.management import setSecurityPolicy
import zope.sendmail.delivery
-import zope.site.hooks
from lp.services.config import config
from lp.services.database.postgresql import ConnectionString
@@ -76,7 +76,7 @@ def execute_zcml_for_scripts(use_web_security=False):
from zope.configuration import xmlconfig
# Hook up custom component architecture calls
- zope.site.hooks.setHooks()
+ zope.component.hooks.setHooks()
# Load server-independent site config
context = ConfigurationMachine()
diff --git a/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled b/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
index aca50bb..5940282 100644
--- a/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
+++ b/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
@@ -82,9 +82,6 @@ exception, and a time machine.
Now we actually demonstrate the behaviour. The view did raise a TimeoutError.
- >>> from lp.testing.fixture import CaptureOops
- >>> capture = CaptureOops()
- >>> capture.setUp()
>>> browser = setupBrowser()
>>> browser.open('http://launchpad.test/doom_test')
Traceback (most recent call last):
@@ -98,12 +95,11 @@ The exception view did not.
The OOPS has the SQL from the main view.
- >>> print capture.oopses[0]['timeline']
+ >>> print oops_capture.oopses[-1]['timeline']
[(0, 0, 'SELECT ... FROM ... WHERE ...'), ...]
All that's left is to clean up after ourselves.
- >>> capture.cleanUp()
>>> sm.unregisterAdapter(
... DoomedView, required=(Interface, IBrowserRequest),
... provided=IBrowserView, name='doom_test')
diff --git a/lib/lp/services/webapp/tests/test_session.py b/lib/lp/services/webapp/tests/test_session.py
index 91c2bfa..b99ff4f 100644
--- a/lib/lp/services/webapp/tests/test_session.py
+++ b/lib/lp/services/webapp/tests/test_session.py
@@ -1,10 +1,13 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
import datetime
+import hashlib
+import hmac
from testtools import TestCase
from testtools.matchers import Contains
+from zope.session.session import digestEncode
from lp.services.webapp.login import (
isFreshLogin,
@@ -67,6 +70,23 @@ class TestLaunchpadCookieClientIdManager(TestCase):
dict(request.response.getHeaders())['Set-Cookie'],
Contains('; httponly;'))
+ def test_stable_client_id(self):
+ # Changing the HMAC algorithm used for client IDs would invalidate
+ # existing sessions, so be careful that that doesn't happen
+ # accidentally across upgrades.
+ request = LaunchpadTestRequest()
+ idmanager = LaunchpadCookieClientIdManager()
+ idmanager._secret = 'secret'
+ data = b'random'
+ s = digestEncode(hashlib.sha1(data).digest())
+ mac = hmac.new(
+ s, idmanager.secret.encode(), digestmod=hashlib.sha1).digest()
+ sid = (s + digestEncode(mac)).decode()
+ idmanager.setRequestId(request, sid)
+ # getRequestId will only return the previously-set ID if it was
+ # generated using the correct secret with the correct algorithm.
+ self.assertEqual(sid, idmanager.getRequestId(request))
+
class TestSessionRelatedFunctions(TestCaseWithFactory):
diff --git a/lib/lp/soyuz/doc/gina.txt b/lib/lp/soyuz/doc/gina.txt
index 76b7098..1f773df 100644
--- a/lib/lp/soyuz/doc/gina.txt
+++ b/lib/lp/soyuz/doc/gina.txt
@@ -123,11 +123,9 @@ Let's set up the filesystem:
And give it a spin:
- >>> from lp.testing.fixture import CaptureOops
>>> gina_proc = [sys.executable, 'scripts/gina.py', '-q',
... 'hoary', 'breezy']
- >>> with CaptureOops():
- ... proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
+ >>> proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
Check STDERR for the errors we expected:
@@ -515,8 +513,7 @@ run.
>>> gina_proc = [sys.executable, 'scripts/gina.py', '-q',
... 'hoary', 'breezy']
- >>> with CaptureOops():
- ... proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
+ >>> proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
>>> print(proc.stderr.read())
ERROR Error processing package files for clearlooks
...
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 39d2725..f0310f3 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -64,7 +64,6 @@ from zope.component import (
getUtility,
)
from zope.security.proxy import (
- builtin_isinstance,
Proxy,
ProxyFactory,
removeSecurityProxy,
@@ -4955,7 +4954,7 @@ def is_security_proxied_or_harmless(obj):
"""Check that the object is security wrapped or a harmless object."""
if obj is None:
return True
- if builtin_isinstance(obj, Proxy):
+ if isinstance(obj, Proxy):
return True
if type(obj) in unwrapped_types:
return True
diff --git a/lib/lp/testing/matchers.py b/lib/lp/testing/matchers.py
index ec9d22b..86266e6 100644
--- a/lib/lp/testing/matchers.py
+++ b/lib/lp/testing/matchers.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -38,10 +38,7 @@ from zope.interface.exceptions import (
DoesNotImplement,
)
from zope.interface.verify import verifyObject
-from zope.security.proxy import (
- builtin_isinstance,
- Proxy,
- )
+from zope.security.proxy import Proxy
from lp.services.database.sqlbase import flush_database_caches
from lp.services.webapp import canonical_url
@@ -249,7 +246,7 @@ class IsProxied(Matcher):
return "Is proxied."
def match(self, matchee):
- if not builtin_isinstance(matchee, Proxy):
+ if not isinstance(matchee, Proxy):
return IsNotProxied(matchee)
return None
diff --git a/lib/lp/testing/systemdocs.py b/lib/lp/testing/systemdocs.py
index 5a34004..d609bc8 100644
--- a/lib/lp/testing/systemdocs.py
+++ b/lib/lp/testing/systemdocs.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Infrastructure for setting up doctests."""
@@ -43,6 +43,7 @@ from lp.testing import (
verifyObject,
)
from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fixture import CaptureOops
from lp.testing.views import (
create_initialized_view,
create_view,
@@ -91,6 +92,41 @@ class StdoutHandler(Handler):
record.levelname, record.name, self.format(record)))
+def setUpStdoutLogging(test, prev_set_up=None,
+ stdout_logging_level=logging.INFO):
+ if prev_set_up is not None:
+ prev_set_up(test)
+ log = StdoutHandler('')
+ log.setLoggerLevel(stdout_logging_level)
+ log.install()
+ test.globs['log'] = log
+ # Store as instance attribute so we can uninstall it.
+ test._stdout_logger = log
+
+
+def tearDownStdoutLogging(test, prev_tear_down=None):
+ if prev_tear_down is not None:
+ prev_tear_down(test)
+ reset_logging()
+ test._stdout_logger.uninstall()
+
+
+def setUpOopsCapturing(test, prev_set_up=None):
+ oops_capture = CaptureOops()
+ oops_capture.setUp()
+ test.globs['oops_capture'] = oops_capture
+ # Store as instance attribute so we can clean it up.
+ test._oops_capture = oops_capture
+ if prev_set_up is not None:
+ prev_set_up(test)
+
+
+def tearDownOopsCapturing(test, prev_tear_down=None):
+ if prev_tear_down is not None:
+ prev_tear_down(test)
+ test._oops_capture.cleanUp()
+
+
def LayeredDocFileSuite(paths, id_extensions=None, **kw):
"""Create a DocFileSuite, optionally applying a layer to it.
@@ -119,29 +155,15 @@ def LayeredDocFileSuite(paths, id_extensions=None, **kw):
stdout_logging_level = kw.pop('stdout_logging_level', logging.INFO)
if stdout_logging:
- kw_setUp = kw.get('setUp')
-
- def setUp(test):
- if kw_setUp is not None:
- kw_setUp(test)
- log = StdoutHandler('')
- log.setLoggerLevel(stdout_logging_level)
- log.install()
- test.globs['log'] = log
- # Store as instance attribute so we can uninstall it.
- test._stdout_logger = log
-
- kw['setUp'] = setUp
-
- kw_tearDown = kw.get('tearDown')
-
- def tearDown(test):
- if kw_tearDown is not None:
- kw_tearDown(test)
- reset_logging()
- test._stdout_logger.uninstall()
-
- kw['tearDown'] = tearDown
+ kw['setUp'] = partial(
+ setUpStdoutLogging, prev_set_up=kw.get('setUp'),
+ stdout_logging_level=stdout_logging_level)
+ kw['tearDown'] = partial(
+ tearDownStdoutLogging, prev_tear_down=kw.get('tearDown'))
+
+ kw['setUp'] = partial(setUpOopsCapturing, prev_set_up=kw.get('setUp'))
+ kw['tearDown'] = partial(
+ tearDownOopsCapturing, prev_tear_down=kw.get('tearDown'))
layer = kw.pop('layer', None)
suite = doctest.DocFileSuite(*paths, **kw)
