Attachment 285598 Details for Bug 160585 – Added the comment, also moved logging initialization to queueengine.py from queues.py

[patch] Added the comment, also moved logging initialization to queueengine.py from queues.py

ews-log-improved-v4 (text/plain), 20.29 KB, created by Aakash Jain on 2016-08-08 16:07:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Aakash Jain

Created: 2016-08-08 16:07:12 PDT

Size: 20.29 KB

patch

obsolete

>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 204267)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,51 @@
>+2016-08-08  Aakash Jain  <aakash_jain@apple.com>
>+
>+        Some EWS console logs doesn't go to log file
>+        https://bugs.webkit.org/show_bug.cgi?id=160585
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Scripts/webkitpy/common/system/logutils.py:
>+        (configure_logger_to_log_to_file): Return the handler so as to enable caller to remove it later.
>+        (remove_handler_from_logger): Removes the handler from the logger.
>+        (FileSystemHandler._open): Ensure that we open logfile in append mode in order to avoid
>+        any possible overwriting.
>+        * Scripts/webkitpy/common/system/filesystem.py:
>+        (FileSystem.open_text_file_for_writing): Add should_append parameter to append to file. 
>+        * Scripts/webkitpy/common/system/filesystem_mock.py:
>+        (MockFileSystem.open_text_file_for_writing): Same.
>+        * Scripts/webkitpy/common/system/filesystem_unittest.py:
>+        (RealFileSystemTest.test_read_and_write_text_file): Removed unused variable 'hex_equivalent'.
>+        (RealFileSystemTest.test_append_to_text_file): Added new unit test for testing append functionality.
>+        * Scripts/webkitpy/tool/bot/queueengine.py:
>+        (QueueEngine._begin_logging): Configure the python logger to log to file.
>+        (QueueEngine._stopping): Stop logging to file.
>+        * Scripts/webkitpy/tool/commands/queues.py:
>+        (AbstractQueue._log_directory): Reverting to os.path.join as we don't have host object.
>+        (AbstractQueue.queue_log_path): Same.
>+        (AbstractQueue.begin_work_queue): Removed logging initialization, it is now being done in QueueEngine.
>+        (AbstractQueue.__init__): Removed host parameter, not required anymore, it was required by logging initialization
>+        which moved to QueueEngine now.
>+        (PatchProcessingQueue.__init__): Same.
>+        (CommitQueue.__init__): Same.
>+        (AbstractReviewQueue.__init__): Same.
>+        (StyleQueue.__init__): Same.
>+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
>+        (TestCommitQueue): Removed host parameter.
>+        (TestCommitQueue.__init__): Same.
>+        (AbstractPatchQueueTest.test_next_patch): Same.
>+        (PatchProcessingQueueTest.test_upload_results_archive_for_patch): Same.
>+        (test_commit_queue_failure): Same.
>+        (mock_run_webkit_patch):
>+        (MockCommitQueueTask.results_from_patch_test_run): Same.
>+        (test_rollout_lands): Same.
>+        (test_non_valid_patch): Same.
>+        (test_auto_retry): Same.
>+        (test_style_queue_with_watch_list_exception): Same.
>+        (TestQueue.__init__): Deleted.
>+        (TestReviewQueue.__init__): Deleted.
>+        (TestFeederQueue.__init__): Deleted.
>+
> 2016-08-08  Matt Baker  <mattbaker@apple.com>
> 
>         Unreviewed: moved myself to the reviewers list.
>Index: Tools/Scripts/webkitpy/common/system/filesystem.py
>===================================================================
>--- Tools/Scripts/webkitpy/common/system/filesystem.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/common/system/filesystem.py	(working copy)
>@@ -235,7 +235,9 @@ class FileSystem(object):
>         # not being seekable. See http://stackoverflow.com/questions/1510188/can-seek-and-tell-work-with-utf-8-encoded-documents-in-python .
>         return codecs.open(path, 'r', 'utf8', errors)
> 
>-    def open_text_file_for_writing(self, path):
>+    def open_text_file_for_writing(self, path, should_append=False):
>+        if should_append:
>+            return codecs.open(path, 'a', 'utf8')
>         return codecs.open(path, 'w', 'utf8')
> 
>     def open_stdin(self):
>Index: Tools/Scripts/webkitpy/common/system/filesystem_mock.py
>===================================================================
>--- Tools/Scripts/webkitpy/common/system/filesystem_mock.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/common/system/filesystem_mock.py	(working copy)
>@@ -337,7 +337,7 @@ class MockFileSystem(object):
>             self._raise_not_found(path)
>         return ReadableTextFileObject(self, path, self.files[path])
> 
>-    def open_text_file_for_writing(self, path):
>+    def open_text_file_for_writing(self, path, should_append=False):
>         return WritableTextFileObject(self, path)
> 
>     def read_text_file(self, path):
>Index: Tools/Scripts/webkitpy/common/system/filesystem_unittest.py
>===================================================================
>--- Tools/Scripts/webkitpy/common/system/filesystem_unittest.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/common/system/filesystem_unittest.py	(working copy)
>@@ -187,7 +187,6 @@ class RealFileSystemTest(unittest.TestCa
>         text_path = None
> 
>         unicode_text_string = u'\u016An\u012Dc\u014Dde\u033D'
>-        hex_equivalent = '\xC5\xAA\x6E\xC4\xAD\x63\xC5\x8D\x64\x65\xCC\xBD'
>         try:
>             text_path = tempfile.mktemp(prefix='tree_unittest_')
>             file = fs.open_text_file_for_writing(text_path)
>@@ -203,6 +202,31 @@ class RealFileSystemTest(unittest.TestCa
>             if text_path and fs.isfile(text_path):
>                 os.remove(text_path)
> 
>+    def test_append_to_text_file(self):
>+        fs = FileSystem()
>+        text_path = None
>+
>+        unicode_text_string1 = u'\u016An\u012Dc\u014Dde\u033D'
>+        unicode_text_string2 = 'Hello'
>+        try:
>+            text_path = tempfile.mktemp(prefix='tree_unittest_')
>+            file = fs.open_text_file_for_writing(text_path)
>+            file.write(unicode_text_string1)
>+            file.close()
>+
>+            file = fs.open_text_file_for_writing(text_path, should_append=True)
>+            file.write(unicode_text_string2)
>+            file.close()
>+
>+            file = fs.open_text_file_for_reading(text_path)
>+            read_text = file.read()
>+            file.close()
>+
>+            self.assertEqual(read_text, unicode_text_string1 + unicode_text_string2)
>+        finally:
>+            if text_path and fs.isfile(text_path):
>+                os.remove(text_path)
>+
>     def test_read_and_write_file(self):
>         fs = FileSystem()
>         text_path = None
>Index: Tools/Scripts/webkitpy/common/system/logutils.py
>===================================================================
>--- Tools/Scripts/webkitpy/common/system/logutils.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/common/system/logutils.py	(working copy)
>@@ -223,6 +223,11 @@ def configure_logger_to_log_to_file(logg
>     handler.setFormatter(formatter)
> 
>     logger.addHandler(handler)
>+    return handler
>+
>+
>+def remove_handler_from_logger(logger, handler):
>+    logger.removeHandler(handler)
> 
> 
> class FileSystemHandler(FileHandler):
>@@ -232,4 +237,4 @@ class FileSystemHandler(FileHandler):
>         FileHandler.__init__(self, filename)
> 
>     def _open(self):
>-        return self.filesystem.open_text_file_for_writing(self.filename)
>+        return self.filesystem.open_text_file_for_writing(self.filename, should_append=True)
>Index: Tools/Scripts/webkitpy/tool/bot/queueengine.py
>===================================================================
>--- Tools/Scripts/webkitpy/tool/bot/queueengine.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/tool/bot/queueengine.py	(working copy)
>@@ -33,6 +33,8 @@ import traceback
> 
> from datetime import datetime, timedelta
> 
>+from webkitpy.common.host import Host
>+from webkitpy.common.system import logutils
> from webkitpy.common.system.executive import ScriptError
> from webkitpy.common.system.outputtee import OutputTee
> 
>@@ -126,12 +128,16 @@ class QueueEngine:
>     def _stopping(self, message):
>         _log.info("\n%s" % message)
>         self._delegate.stop_work_queue(message)
>+        logutils.remove_handler_from_logger(logging.getLogger("webkitpy"), self._log_handler)
>         # Be careful to shut down our OutputTee or the unit tests will be unhappy.
>         self._ensure_work_log_closed()
>         self._output_tee.remove_log(self._queue_log)
> 
>     def _begin_logging(self):
>-        self._queue_log = self._output_tee.add_log(self._delegate.queue_log_path())
>+        _queue_log_path = self._delegate.queue_log_path()
>+        # We are using logging.getLogger("webkitpy") instead of _log since we want to capture all messages logged from webkitpy modules.
>+        self._log_handler = logutils.configure_logger_to_log_to_file(logging.getLogger("webkitpy"), _queue_log_path, Host().filesystem)
>+        self._queue_log = self._output_tee.add_log(_queue_log_path)
>         self._work_log = None
> 
>     def _open_work_log(self, work_item):
>Index: Tools/Scripts/webkitpy/tool/commands/queues.py
>===================================================================
>--- Tools/Scripts/webkitpy/tool/commands/queues.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/tool/commands/queues.py	(working copy)
>@@ -41,10 +41,8 @@ from StringIO import StringIO
> 
> from webkitpy.common.config.committervalidator import CommitterValidator
> from webkitpy.common.config.ports import DeprecatedPort
>-from webkitpy.common.host import Host
> from webkitpy.common.net.bugzilla import Attachment
> from webkitpy.common.net.statusserver import StatusServer
>-from webkitpy.common.system import logutils
> from webkitpy.common.system.executive import ScriptError
> from webkitpy.tool.bot.botinfo import BotInfo
> from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate
>@@ -68,7 +66,7 @@ class AbstractQueue(Command, QueueEngine
>     _fail_status = "Fail"
>     _error_status = "Error"
> 
>-    def __init__(self, options=None, host=Host()):  # Default values should never be collections (like []) as default values are shared between invocations
>+    def __init__(self, options=None):  # Default values should never be collections (like []) as default values are shared between invocations
>         options_list = (options or []) + [
>             make_option("--no-confirm", action="store_false", dest="confirm", default=True, help="Do not ask the user for confirmation before running the queue.  Dangerous!"),
>             make_option("--exit-after-iteration", action="store", type="int", dest="iterations", default=None, help="Stop running the queue after iterating this number of times."),
>@@ -78,7 +76,6 @@ class AbstractQueue(Command, QueueEngine
>         self._iteration_count = 0
>         if not hasattr(self, 'architecture'):
>             self.architecture = None
>-        self.host = host
> 
>     def _cc_watchers(self, bug_id):
>         try:
>@@ -112,18 +109,17 @@ class AbstractQueue(Command, QueueEngine
>         return command_output
> 
>     def _log_directory(self):
>-        return self.host.filesystem.join("..", "%s-logs" % self.name)
>+        return os.path.join("..", "%s-logs" % self.name)
> 
>     # QueueEngineDelegate methods
> 
>     def queue_log_path(self):
>-        return self.host.filesystem.join(self._log_directory(), "%s.log" % self.name)
>+        return os.path.join(self._log_directory(), "%s.log" % self.name)
> 
>     def work_item_log_path(self, work_item):
>         raise NotImplementedError, "subclasses must implement"
> 
>     def begin_work_queue(self):
>-        logutils.configure_logger_to_log_to_file(_log, self.queue_log_path(), self.host.filesystem)
>         _log.info("CAUTION: %s will discard all local changes in \"%s\"" % (self.name, self._tool.scm().checkout_root))
>         if self._options.confirm:
>             response = self._tool.user.prompt("Are you sure?  Type \"yes\" to continue: ")
>@@ -263,10 +259,9 @@ class PatchProcessingQueue(AbstractPatch
>     # Subclasses must override.
>     port_name = None
> 
>-    def __init__(self, options=None, host=Host()):
>+    def __init__(self, options=None):
>         self._port = None  # We can't instantiate port here because tool isn't avaialble.
>-        self.host = host
>-        AbstractPatchQueue.__init__(self, options, host=host)
>+        AbstractPatchQueue.__init__(self, options)
> 
>     # FIXME: This is a hack to map between the old port names and the new port names.
>     def _new_port_name_from_old(self, port_name, platform):
>@@ -318,10 +313,9 @@ class PatchProcessingQueue(AbstractPatch
> 
> 
> class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTaskDelegate):
>-    def __init__(self, commit_queue_task_class=CommitQueueTask, host=Host()):
>-        self.host = host
>+    def __init__(self, commit_queue_task_class=CommitQueueTask):
>         self._commit_queue_task_class = commit_queue_task_class
>-        PatchProcessingQueue.__init__(self, host=host)
>+        PatchProcessingQueue.__init__(self)
> 
>     name = "commit-queue"
>     port_name = "mac"
>@@ -436,9 +430,8 @@ class CommitQueue(PatchProcessingQueue, 
> 
> class AbstractReviewQueue(PatchProcessingQueue, StepSequenceErrorHandler):
>     """This is the base-class for the EWS queues and the style-queue."""
>-    def __init__(self, options=None, host=Host()):
>-        self.host = host
>-        PatchProcessingQueue.__init__(self, options, host=host)
>+    def __init__(self, options=None):
>+        PatchProcessingQueue.__init__(self, options)
> 
>     def review_patch(self, patch):
>         raise NotImplementedError("subclasses must implement")
>@@ -470,9 +463,8 @@ class AbstractReviewQueue(PatchProcessin
> class StyleQueue(AbstractReviewQueue, StyleQueueTaskDelegate):
>     name = "style-queue"
> 
>-    def __init__(self, host=Host()):
>-        self.host = host
>-        AbstractReviewQueue.__init__(self, host=host)
>+    def __init__(self):
>+        AbstractReviewQueue.__init__(self)
> 
>     def review_patch(self, patch):
>         task = StyleQueueTask(self, patch)
>Index: Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
>===================================================================
>--- Tools/Scripts/webkitpy/tool/commands/queues_unittest.py	(revision 204267)
>+++ Tools/Scripts/webkitpy/tool/commands/queues_unittest.py	(working copy)
>@@ -31,7 +31,6 @@ import StringIO
> 
> from webkitpy.common.checkout.scm import CheckoutNeedsUpdate
> from webkitpy.common.checkout.scm.scm_mock import MockSCM
>-from webkitpy.common.host_mock import MockHost
> from webkitpy.common.net.layouttestresults import LayoutTestResults
> from webkitpy.common.net.bugzilla import Attachment
> from webkitpy.common.system.outputcapture import OutputCapture
>@@ -48,7 +47,7 @@ from webkitpy.tool.mocktool import MockT
> 
> class TestCommitQueue(CommitQueue):
>     def __init__(self, tool=None):
>-        CommitQueue.__init__(self, host=MockHost())
>+        CommitQueue.__init__(self)
>         if tool:
>             self.bind_to_tool(tool)
>         self._options = MockOptions(confirm=False, parent_command="commit-queue", port=None)
>@@ -63,23 +62,14 @@ class TestCommitQueue(CommitQueue):
> class TestQueue(AbstractPatchQueue):
>     name = "test-queue"
> 
>-    def __init__(self):
>-        AbstractPatchQueue.__init__(self, host=MockHost())
>-
> 
> class TestReviewQueue(AbstractReviewQueue):
>     name = "test-review-queue"
> 
>-    def __init__(self):
>-        AbstractReviewQueue.__init__(self, host=MockHost())
>-
> 
> class TestFeederQueue(FeederQueue):
>     _sleep_duration = 0
> 
>-    def __init__(self):
>-        FeederQueue.__init__(self, host=MockHost())
>-
> 
> class AbstractQueueTest(CommandsTest):
>     def test_log_directory(self):
>@@ -163,7 +153,7 @@ MOCK: submit_to_ews: 10002
> 
> class AbstractPatchQueueTest(CommandsTest):
>     def test_next_patch(self):
>-        queue = AbstractPatchQueue(host=MockHost())
>+        queue = AbstractPatchQueue()
>         tool = MockTool()
>         queue.bind_to_tool(tool)
>         queue._options = Mock()
>@@ -181,7 +171,7 @@ class AbstractPatchQueueTest(CommandsTes
> 
> class PatchProcessingQueueTest(CommandsTest):
>     def test_upload_results_archive_for_patch(self):
>-        queue = PatchProcessingQueue(host=MockHost())
>+        queue = PatchProcessingQueue()
>         queue.name = "mock-queue"
>         tool = MockTool()
>         queue.bind_to_tool(tool)
>@@ -270,7 +260,7 @@ MOCK: release_work_item: commit-queue 10
>             "handle_script_error": "ScriptError error message\n\nMOCK output\n",
>             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.\n\nMock error message'\n",
>         }
>-        self.assert_queue_outputs(CommitQueue(host=MockHost()), tool=tool, expected_logs=expected_logs)
>+        self.assert_queue_outputs(CommitQueue(), tool=tool, expected_logs=expected_logs)
> 
>     def test_commit_queue_failure(self):
>         expected_logs = {
>@@ -286,7 +276,7 @@ MOCK: release_work_item: commit-queue 10
>             "handle_script_error": "ScriptError error message\n\nMOCK output\n",
>             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.\n\nMock error message'\n",
>         }
>-        queue = CommitQueue(host=MockHost())
>+        queue = CommitQueue()
> 
>         def mock_run_webkit_patch(command):
>             if command[0] == 'clean' or command[0] == 'update':
>@@ -318,7 +308,7 @@ MOCK: release_work_item: commit-queue 10
>             def results_from_patch_test_run(self, patch):
>                 return LayoutTestResults([test_results.TestResult("mock_test_name.html", failures=[test_failures.FailureTextMismatch()])], did_exceed_test_failure_limit=False)
> 
>-        queue = CommitQueue(MockCommitQueueTask, host=MockHost())
>+        queue = CommitQueue(MockCommitQueueTask)
> 
>         def mock_run_webkit_patch(command):
>             if command[0] == 'clean' or command[0] == 'update':
>@@ -357,7 +347,7 @@ MOCK: release_work_item: commit-queue 10
>             "handle_script_error": "ScriptError error message\n\nMOCK output\n",
>             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.\n\nMock error message'\n",
>         }
>-        self.assert_queue_outputs(CommitQueue(host=MockHost()), tool=tool, expected_logs=expected_logs)
>+        self.assert_queue_outputs(CommitQueue(), tool=tool, expected_logs=expected_logs)
> 
>     def test_rollout_lands(self):
>         tool = MockTool()
>@@ -382,7 +372,7 @@ MOCK: release_work_item: commit-queue 10
>             "handle_script_error": "ScriptError error message\n\nMOCK output\n",
>             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10005' with comment 'Rejecting attachment 10005 from commit-queue.\n\nMock error message'\n",
>         }
>-        self.assert_queue_outputs(CommitQueue(host=MockHost()), tool=tool, work_item=rollout_patch, expected_logs=expected_logs)
>+        self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_logs=expected_logs)
> 
>     def test_non_valid_patch(self):
>         tool = MockTool()
>@@ -393,10 +383,10 @@ MOCK: release_work_item: commit-queue 10
> MOCK: release_work_item: commit-queue 10007
> """,
>         }
>-        self.assert_queue_outputs(CommitQueue(host=MockHost()), tool=tool, work_item=patch, expected_logs=expected_logs)
>+        self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=patch, expected_logs=expected_logs)
> 
>     def test_auto_retry(self):
>-        queue = CommitQueue(host=MockHost())
>+        queue = CommitQueue()
>         options = Mock()
>         options.parent_command = "commit-queue"
>         tool = AlwaysCommitQueueTool()
>@@ -508,7 +498,7 @@ MOCK: release_work_item: style-queue 100
>             "handle_script_error": "MOCK output\n",
>         }
>         tool = MockTool(executive_throws_when_run=set(['check-style']))
>-        self.assert_queue_outputs(StyleQueue(host=MockHost()), expected_logs=expected_logs, tool=tool)
>+        self.assert_queue_outputs(StyleQueue(), expected_logs=expected_logs, tool=tool)
> 
>     def test_style_queue_with_watch_list_exception(self):
>         expected_logs = {
>@@ -533,7 +523,7 @@ MOCK: release_work_item: style-queue 100
>             "handle_script_error": "MOCK output\n",
>         }
>         tool = MockTool(executive_throws_when_run=set(['apply-watchlist-local']))
>-        self.assert_queue_outputs(StyleQueue(host=MockHost()), expected_logs=expected_logs, tool=tool)
>+        self.assert_queue_outputs(StyleQueue(), expected_logs=expected_logs, tool=tool)
> 
>     def test_non_valid_patch(self):
>         tool = MockTool()
>@@ -544,4 +534,4 @@ MOCK: release_work_item: style-queue 100
> MOCK: release_work_item: style-queue 10007
> """,
>         }
>-        self.assert_queue_outputs(StyleQueue(host=MockHost()), tool=tool, work_item=patch, expected_logs=expected_logs)
>+        self.assert_queue_outputs(StyleQueue(), tool=tool, work_item=patch, expected_logs=expected_logs)

Actions: View | Formatted Diff | Diff

Attachments on bug 160585: 285393 | 285469 | 285598