launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/twisted-logging into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #820510 in Launchpad itself: "hard to turn on extra logging for twisted job runner"
  https://bugs.launchpad.net/launchpad/+bug/820510

For more details, see:
https://code.launchpad.net/~abentley/launchpad/twisted-logging/+merge/72082

= Summary =
Fix bug #820510: hard to turn on extra logging for twisted job runner

== Proposed fix ==
Add --log-twisted flag for debugging.

== Pre-implementation notes ==
None

== Implementation details ==
All scripts that use the JobCronScript will support the --log-twisted flag if
their job runner is the TwistedJobRunner.

== Tests ==
bin/test -t log_twisted -t test_script_runs

== Demo and Q/A ==
None


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/job/tests/test_runner.py
  lib/lp/services/job/runner.py
  lib/lp/code/scripts/tests/test_merge_proposal_jobs.py
-- 
https://code.launchpad.net/~abentley/launchpad/twisted-logging/+merge/72082
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/twisted-logging into lp:launchpad.

=== modified file 'lib/lp/code/scripts/tests/test_merge_proposal_jobs.py'
--- lib/lp/code/scripts/tests/test_merge_proposal_jobs.py	2011-08-13 04:07:10 +0000
+++ lib/lp/code/scripts/tests/test_merge_proposal_jobs.py	2011-08-18 17:40:25 +0000
@@ -5,6 +5,7 @@
 
 """Test the sendbranchmail script"""
 
+from testtools.matchers import MatchesRegex
 import transaction
 
 from canonical.launchpad.scripts.tests import run_script
@@ -25,14 +26,25 @@
         job = make_runnable_incremental_diff_job(self)
         transaction.commit()
         retcode, stdout, stderr = run_script(
-            'cronscripts/merge-proposal-jobs.py', [])
+            'cronscripts/merge-proposal-jobs.py', ['--log-twisted'])
         self.assertEqual(0, retcode)
         self.assertEqual('', stdout)
-        self.assertEqual(
+        matches_expected = MatchesRegex(
             'INFO    Creating lockfile:'
             ' /var/lock/launchpad-merge-proposal-jobs.lock\n'
             'INFO    Running through Twisted.\n'
-            'INFO    Running GenerateIncrementalDiffJob (ID %d).\n'
-            'INFO    Ran 1 GenerateIncrementalDiffJob jobs.\n' % job.job.id,
-            stderr)
+            'Log opened.\n'
+            'INFO    Log opened.\n'
+            'ProcessPool stats:\n'
+            'INFO    ProcessPool stats:\n'
+            '\tworkers: 0\n'
+            'INFO    \tworkers: 0\n'
+            '(.|\n)*'
+            'INFO    Running GenerateIncrementalDiffJob \(ID %d\).\n'
+            '(.|\n)*'
+            'INFO    STOPPING: \'\'\n'
+            'Main loop terminated.\n'
+            'INFO    Main loop terminated.\n'
+            'INFO    Ran 1 GenerateIncrementalDiffJob jobs.\n' % job.job.id)
+        self.assertThat(stderr, matches_expected)
         self.assertEqual(JobStatus.COMPLETED, job.status)

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2011-08-14 23:11:45 +0000
+++ lib/lp/services/job/runner.py	2011-08-18 17:40:25 +0000
@@ -580,15 +580,22 @@
             rely on the subclass providing `config_name` and
             `source_interface`.
         """
+        self._runner_class = runner_class
         super(JobCronScript, self).__init__(
             name=name, dbuser=None, test_args=test_args)
-        self._runner_class = runner_class
+        self.log_twisted = getattr(self.options, 'log_twisted', False)
         if not commandline_config:
             return
         self.config_name = self.args[0]
         self.source_interface = import_source(
             self.config_section.source_interface)
 
+    def add_my_options(self):
+        if self.runner_class is TwistedJobRunner:
+            self.parser.add_option(
+                '--log-twisted', action='store_true', default=False,
+                help='Enable extra Twisted logging.')
+
     @property
     def dbuser(self):
         return self.config_section.dbuser
@@ -617,8 +624,11 @@
             # utility default to using the [error_reports] config.
             errorlog.globalErrorUtility.configure(self.config_name)
         job_source = getUtility(self.source_interface)
+        kwargs = {}
+        if self.log_twisted:
+            kwargs['_log_twisted'] = True
         runner = self.runner_class.runFromSource(
-            job_source, self.dbuser, self.logger)
+            job_source, self.dbuser, self.logger, **kwargs)
         for name, count in self.job_counts(runner.completed_jobs):
             self.logger.info('Ran %d %s jobs.', count, name)
         for name, count in self.job_counts(runner.incomplete_jobs):

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2011-08-16 00:24:00 +0000
+++ lib/lp/services/job/tests/test_runner.py	2011-08-18 17:40:25 +0000
@@ -647,3 +647,23 @@
             cronscript.main()
         finally:
             errorlog.globalErrorUtility = old_errorlog
+
+    def test_log_twisted_option_for_twisted_runner(self):
+        """TwistedJobRunner creates --log-twisted flag."""
+        jcs = JobCronScript(TwistedJobRunner, test_args=[])
+        self.assertIsNot(None, getattr(jcs.options, 'log_twisted', None))
+
+    def test_no_log_twisted_option_for_plain_runner(self):
+        """JobRunner has no --log-twisted flag."""
+        jcs = JobCronScript(JobRunner, test_args=[])
+        self.assertIs(None, getattr(jcs.options, 'log_twisted', None))
+
+    def test_log_twisted_flag(self):
+        """--log-twisted sets JobCronScript.log_twisted True."""
+        jcs = JobCronScript(TwistedJobRunner, test_args=['--log-twisted'])
+        self.assertTrue(jcs.log_twisted)
+
+    def test_no_log_twisted_flag(self):
+        """No --log-twisted sets JobCronScript.log_twisted False."""
+        jcs = JobCronScript(TwistedJobRunner, test_args=[])
+        self.assertFalse(jcs.log_twisted)