launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-testtools-content into launchpad:master.

Commit message:
Fix and simplify use of testtools.content

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/388908

The get_bytes callable passed to Content is supposed to return an iterable of bytes objects, not a single bytes object.  (On Python 2 we get away with this, because iterating over a bytes object yields one byte at a time; on Python 3 we don't, because iterating over a bytes object yields the integer value of each byte.)

Pass buffer_now=False to some attach_file calls so that we get the contents of the file at the end of the test, not the contents when the test detail is added.

Use helper methods in testtools.content to simplify various longhand constructions.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-testtools-content into launchpad:master.

diff --git a/lib/lp/answers/tests/test_questionjob.py b/lib/lp/answers/tests/test_questionjob.py
index 96c4ffc..48f9a47 100644
--- a/lib/lp/answers/tests/test_questionjob.py
+++ b/lib/lp/answers/tests/test_questionjob.py
@@ -330,8 +330,8 @@ class QuestionEmailJobTestCase(TestCaseWithFactory):
         out, err, exit_code = run_script(
             "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" % (
                 IQuestionEmailJobSource.getName()))
-        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
-        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: [out]))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: [err]))
         self.assertEqual(0, exit_code)
         self.assertTrue(
             'Traceback (most recent call last)' not in err)
diff --git a/lib/lp/buildmaster/tests/mock_slaves.py b/lib/lp/buildmaster/tests/mock_slaves.py
index df48937..2794137 100644
--- a/lib/lp/buildmaster/tests/mock_slaves.py
+++ b/lib/lp/buildmaster/tests/mock_slaves.py
@@ -28,8 +28,7 @@ import fixtures
 from lpbuildd.tests.harness import BuilddSlaveTestSetup
 import six
 from six.moves import xmlrpc_client
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import attach_file
 from twisted.internet import defer
 from twisted.web import xmlrpc
 
@@ -308,11 +307,8 @@ class SlaveTestHelpers(fixtures.Fixture):
         :return: A `BuilddSlaveTestSetup` object.
         """
         tachandler = self.useFixture(LPBuilddSlaveTestSetup())
-        self.addDetail(
-            'xmlrpc-log-file',
-            Content(
-                UTF8_TEXT,
-                lambda: open(tachandler.logfile, 'r').readlines()))
+        attach_file(
+            self, tachandler.logfile, name='xmlrpc-log-file', buffer_now=False)
         return tachandler
 
     def getClientSlave(self, reactor=None, proxy=None,
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposallisting.py b/lib/lp/code/browser/tests/test_branchmergeproposallisting.py
index ab42d0d..5bc790e 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposallisting.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposallisting.py
@@ -10,8 +10,7 @@ __metaclass__ = type
 from datetime import datetime
 
 import pytz
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import text_content
 from testtools.matchers import LessThan
 import transaction
 from zope.component import getUtility
@@ -937,7 +936,7 @@ class ActiveReviewsPerformanceMixin:
         added_bmps = 4
         recorder1, view1 = self.createUserBMPsAndRecordQueries(base_bmps)
         self.assertEqual(base_bmps, view1.proposal_count)
-        self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
+        self.addDetail("r1tb", text_content(str(recorder1)))
         recorder2, view2 = self.createUserBMPsAndRecordQueries(
             base_bmps + added_bmps)
         self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
@@ -973,7 +972,7 @@ class ActiveReviewsPerformanceMixin:
         added_bmps = 4
         recorder1, view1 = self.createProductBMPsAndRecordQueries(base_bmps)
         self.assertEqual(base_bmps, view1.proposal_count)
-        self.addDetail("r1tb", Content(UTF8_TEXT, lambda: [str(recorder1)]))
+        self.addDetail("r1tb", text_content(str(recorder1)))
         recorder2, view2 = self.createProductBMPsAndRecordQueries(
             base_bmps + added_bmps)
         self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
diff --git a/lib/lp/registry/browser/tests/test_distroseries.py b/lib/lp/registry/browser/tests/test_distroseries.py
index 1f8d5d5..6f710df 100644
--- a/lib/lp/registry/browser/tests/test_distroseries.py
+++ b/lib/lp/registry/browser/tests/test_distroseries.py
@@ -13,6 +13,7 @@ from textwrap import TextWrapper
 from fixtures import FakeLogger
 from lazr.restful.interfaces import IJSONRequestCache
 from lxml import html
+import six
 from six.moves.urllib.parse import (
     urlencode,
     urlparse,
@@ -975,7 +976,7 @@ class TestDistroSeriesLocalDiffPerformance(TestCaseWithFactory,
                     list(prepare_statements(rec1)),
                     list(prepare_statements(rec2)))
                 for line in diff:
-                    yield "%s\n" % line
+                    yield six.ensure_binary("%s\n" % line)
 
             return statement_diff
 
diff --git a/lib/lp/registry/tests/test_person_merge_job.py b/lib/lp/registry/tests/test_person_merge_job.py
index d814978..cea0979 100644
--- a/lib/lp/registry/tests/test_person_merge_job.py
+++ b/lib/lp/registry/tests/test_person_merge_job.py
@@ -144,8 +144,8 @@ class TestPersonMergeJob(TestCaseWithFactory):
             "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" % (
                 IPersonMergeJobSource.getName()))
 
-        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
-        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: [out]))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: [err]))
 
         self.assertEqual(0, exit_code)
         IStore(self.from_person).invalidate()
diff --git a/lib/lp/registry/tests/test_productjob.py b/lib/lp/registry/tests/test_productjob.py
index f4fdf9d..38d8634 100644
--- a/lib/lp/registry/tests/test_productjob.py
+++ b/lib/lp/registry/tests/test_productjob.py
@@ -155,8 +155,8 @@ class DailyProductJobsTestCase(TestCaseWithFactory, CommercialHelpers):
         transaction.commit()
         stdout, stderr, retcode = run_script(
             'cronscripts/daily_product_jobs.py')
-        self.addDetail("stdout", Content(UTF8_TEXT, lambda: stdout))
-        self.addDetail("stderr", Content(UTF8_TEXT, lambda: stderr))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: [stdout]))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: [stderr]))
         self.assertEqual(0, retcode)
         self.assertIn('Requested 3 total product jobs.', stderr)
 
@@ -590,8 +590,8 @@ class CommericialExpirationMixin(CommercialHelpers):
         out, err, exit_code = run_script(
             "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" %
              self.JOB_SOURCE_INTERFACE.getName())
-        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
-        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: [out]))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: [err]))
         self.assertEqual(0, exit_code)
         self.assertTrue(
             'Traceback (most recent call last)' not in err)
diff --git a/lib/lp/registry/tests/test_sharingjob.py b/lib/lp/registry/tests/test_sharingjob.py
index 72d601c..bda6a52 100644
--- a/lib/lp/registry/tests/test_sharingjob.py
+++ b/lib/lp/registry/tests/test_sharingjob.py
@@ -210,8 +210,8 @@ class TestRunViaCron(TestCaseWithFactory):
         out, err, exit_code = run_script(
             "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" % (
                 job_type))
-        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
-        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: [out]))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: [err]))
         self.assertEqual(0, exit_code)
         self.assertTrue(
             'Traceback (most recent call last)' not in err)
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index 2662c01..405d03f 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -30,8 +30,7 @@ from storm.locals import (
     Storm,
     )
 from storm.store import Store
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import text_content
 from testtools.matchers import (
     AfterPreprocessing,
     Equals,
@@ -434,9 +433,7 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         self.log_buffer = six.StringIO()
         handler = logging.StreamHandler(self.log_buffer)
         self.log.addHandler(handler)
-        self.addDetail(
-            'garbo-log',
-            Content(UTF8_TEXT, lambda: [self.log_buffer.getvalue()]))
+        self.addDetail('garbo-log', text_content(self.log_buffer.getvalue()))
 
     def runFrequently(self, maximum_chunk_size=2, test_args=()):
         switch_dbuser('garbo_daily')
diff --git a/lib/lp/services/job/tests/test_celeryjob.py b/lib/lp/services/job/tests/test_celeryjob.py
index e96c5b4..22ec50e 100644
--- a/lib/lp/services/job/tests/test_celeryjob.py
+++ b/lib/lp/services/job/tests/test_celeryjob.py
@@ -6,8 +6,7 @@ from time import sleep
 
 from lazr.jobrunner.bin.clear_queues import clear_queues
 import six
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import text_content
 
 from lp.code.model.branchjob import BranchScanJob
 from lp.scripts.helpers import TransactionFreeOperation
@@ -71,8 +70,7 @@ class TestRunMissingJobs(TestCaseWithFactory):
                   (expected_len, actual_len))
 
     def addTextDetail(self, name, text):
-        content = Content(UTF8_TEXT, lambda: text)
-        self.addDetail(name, content)
+        self.addDetail(name, text_content(text))
 
     def test_find_missing_ready(self):
         """A job which is ready but not queued is "missing"."""
diff --git a/lib/lp/services/signing/testing/fixture.py b/lib/lp/services/signing/testing/fixture.py
index 0afa9d4..9b763bb 100644
--- a/lib/lp/services/signing/testing/fixture.py
+++ b/lib/lp/services/signing/testing/fixture.py
@@ -60,7 +60,8 @@ class SigningServiceFixture(TacTestFixture):
         self.addCleanup(lambda: os.path.exists(logfile) and os.unlink(logfile))
 
         content.attach_file(
-            self, logfile, "signing-log", content_type.UTF8_TEXT)
+            self, logfile, "signing-log", content_type.UTF8_TEXT,
+            buffer_now=False)
 
         factory = ObjectFactory()
         config_name = factory.getUniqueString()
diff --git a/lib/lp/services/webapp/tests/test_error.py b/lib/lp/services/webapp/tests/test_error.py
index 0c7359f..0bd9c22 100644
--- a/lib/lp/services/webapp/tests/test_error.py
+++ b/lib/lp/services/webapp/tests/test_error.py
@@ -16,8 +16,7 @@ from storm.exceptions import (
     DisconnectionError,
     OperationalError,
     )
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import text_content
 from testtools.matchers import (
     Equals,
     MatchesAny,
@@ -93,12 +92,13 @@ class TestDatabaseErrorViews(TestCase):
         # supposed to be listening on.  connect_ex returns 0 on success or an
         # errno otherwise.
         pg_port_status = str(socket.socket().connect_ex(('localhost', 5432)))
-        self.addDetail('postgres socket.connect_ex result',
-            Content(UTF8_TEXT, lambda: pg_port_status))
+        self.addDetail(
+            'postgres socket.connect_ex result', text_content(pg_port_status))
         bouncer_port_status = str(
             socket.socket().connect_ex(('localhost', bouncer.port)))
-        self.addDetail('pgbouncer socket.connect_ex result',
-            Content(UTF8_TEXT, lambda: bouncer_port_status))
+        self.addDetail(
+            'pgbouncer socket.connect_ex result',
+            text_content(bouncer_port_status))
 
     def retryConnection(self, url, bouncer, retries=60):
         """Retry to connect to *url* for *retries* times.
diff --git a/lib/lp/testing/matchers.py b/lib/lp/testing/matchers.py
index d8b080d..fcdba23 100644
--- a/lib/lp/testing/matchers.py
+++ b/lib/lp/testing/matchers.py
@@ -21,9 +21,9 @@ __all__ = [
     ]
 
 from lazr.lifecycle.snapshot import Snapshot
+import six
 from testtools import matchers
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from testtools.content import text_content
 from testtools.matchers import (
     DocTestMatches as OriginalDocTestMatches,
     Equals,
@@ -213,8 +213,7 @@ class _MismatchedQueryCount(Mismatch):
             if backtrace is not None:
                 result.append(backtrace.rstrip())
                 result.append(u'.' * 70)
-        result = [item.encode('UTF-8') for item in result]
-        return Content(UTF8_TEXT, lambda: [b'\n'.join(result)])
+        return text_content('\n'.join(result))
 
     def get_details(self):
         details = {}
@@ -401,8 +400,7 @@ class SoupMismatch(Mismatch):
         self.soup_content = soup_content
 
     def get_details(self):
-        content = unicode(self.soup_content).encode('utf8')
-        return {'content': Content(UTF8_TEXT, lambda: [content])}
+        return {'content': text_content(six.text_type(self.soup_content))}
 
 
 class MissingElement(SoupMismatch):
diff --git a/lib/lp/testing/swift/fixture.py b/lib/lp/testing/swift/fixture.py
index e6ea41b..55cf8f3 100644
--- a/lib/lp/testing/swift/fixture.py
+++ b/lib/lp/testing/swift/fixture.py
@@ -53,7 +53,8 @@ class SwiftFixture(TacTestFixture):
         self.addCleanup(lambda: os.path.exists(logfile) and os.unlink(logfile))
 
         testtools.content.attach_file(
-            self, logfile, 'swift-log', testtools.content_type.UTF8_TEXT)
+            self, logfile, 'swift-log', testtools.content_type.UTF8_TEXT,
+            buffer_now=False)
 
         service_config = dedent("""\
             [librarian_server]