2010-12-14 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Dec 2010 21:48:40 +0000 (21:48 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Dec 2010 21:48:40 +0000 (21:48 +0000)
        Reviewed by Adam Barth.

        commit-queue should upload failure diffs when tests flake
        https://bugs.webkit.org/show_bug.cgi?id=51051

        To make this testable I needed to pipe FileSystem down onto tool.
        We've wanted it there for a long time anyway.

        This patch is kinda a big hack.  But we don't have a nice
        way to read results.html files.  I think this will need further
        revision before this code actually feels clean.

        As part of testing this change, I had to make MockBugzilla.create_bug
        actually return an id (like it should) which required updating
        a few other unit test results (for the better).

        The results_matching_keys change in layouttestresults/rebasline
        was an alternate path which I decided not to use in the end, but
        I left the change as it seemed an improvement.

        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
        * Scripts/webkitpy/common/net/layouttestresults.py:
        * Scripts/webkitpy/tool/bot/flakytestreporter.py:
        * Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py:
        * Scripts/webkitpy/tool/commands/queues.py:
        * Scripts/webkitpy/tool/commands/rebaseline.py:
        * Scripts/webkitpy/tool/main.py:
        * Scripts/webkitpy/tool/mocktool.py:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@74138 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
WebKitTools/Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py
WebKitTools/Scripts/webkitpy/common/net/layouttestresults.py
WebKitTools/Scripts/webkitpy/tool/bot/flakytestreporter.py
WebKitTools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py
WebKitTools/Scripts/webkitpy/tool/commands/bugfortest.py
WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py
WebKitTools/Scripts/webkitpy/tool/commands/queues.py
WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py
WebKitTools/Scripts/webkitpy/tool/main.py
WebKitTools/Scripts/webkitpy/tool/mocktool.py

index f48b845..c115679 100644 (file)
@@ -1,3 +1,34 @@
+2010-12-14  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        commit-queue should upload failure diffs when tests flake
+        https://bugs.webkit.org/show_bug.cgi?id=51051
+
+        To make this testable I needed to pipe FileSystem down onto tool.
+        We've wanted it there for a long time anyway.
+
+        This patch is kinda a big hack.  But we don't have a nice
+        way to read results.html files.  I think this will need further
+        revision before this code actually feels clean.
+
+        As part of testing this change, I had to make MockBugzilla.create_bug
+        actually return an id (like it should) which required updating
+        a few other unit test results (for the better).
+
+        The results_matching_keys change in layouttestresults/rebasline
+        was an alternate path which I decided not to use in the end, but
+        I left the change as it seemed an improvement.
+
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
+        * Scripts/webkitpy/common/net/layouttestresults.py:
+        * Scripts/webkitpy/tool/bot/flakytestreporter.py:
+        * Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py:
+        * Scripts/webkitpy/tool/commands/queues.py:
+        * Scripts/webkitpy/tool/commands/rebaseline.py:
+        * Scripts/webkitpy/tool/main.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+
 2010-12-15  Cosmin Truta  <ctruta@chromium.org>
 
         Reviewed by James Robinson.
index 0a59e22..d6210d5 100644 (file)
@@ -221,15 +221,16 @@ class Bugzilla(object):
         if not bug_id:
             return None
         content_type = "&ctype=xml" if xml else ""
-        return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url,
-                                           bug_id,
-                                           content_type)
+        return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url, bug_id, content_type)
 
     def short_bug_url_for_bug_id(self, bug_id):
         if not bug_id:
             return None
         return "http://webkit.org/b/%s" % bug_id
 
+    def add_attachment_url(self, bug_id):
+        return "%sattachment.cgi?action=enter&bugid=%s" % (self.bug_server_url, bug_id)
+
     def attachment_url_for_id(self, attachment_id, action="view"):
         if not attachment_id:
             return None
@@ -415,72 +416,104 @@ class Bugzilla(object):
                 self.authenticated = True
                 self.username = username
 
+    def _commit_queue_flag(self, mark_for_landing, mark_for_commit_queue):
+        if mark_for_landing:
+            return '+'
+        elif mark_for_commit_queue:
+            return '?'
+        return 'X'
+
+    # FIXME: mark_for_commit_queue and mark_for_landing should be joined into a single commit_flag argument.
     def _fill_attachment_form(self,
                               description,
-                              patch_file_object,
-                              comment_text=None,
+                              file_object,
                               mark_for_review=False,
                               mark_for_commit_queue=False,
                               mark_for_landing=False,
-                              bug_id=None):
+                              is_patch=False,
+                              filename=None,
+                              mimetype=None):
         self.browser['description'] = description
-        self.browser['ispatch'] = ("1",)
+        if is_patch:
+            self.browser['ispatch'] = ("1",)
+        # FIXME: Should this use self._find_select_element_for_flag?
         self.browser['flag_type-1'] = ('?',) if mark_for_review else ('X',)
+        self.browser['flag_type-3'] = (self._commit_queue_flag(mark_for_landing, mark_for_commit_queue),)
+
+        filename = filename or "%s.patch" % timestamp()
+        mimetype = mimetype or "text/plain"
+        self.browser.add_file(file_object, mimetype, filename, 'data')
+
+    def _file_object_for_upload(self, file_or_string):
+        if hasattr(file_or_string, 'read'):
+            return file_or_string
+        # Only if file_or_string is not already encoded do we want to encode it.
+        if isinstance(file_or_string, unicode):
+            file_or_string = file_or_string.encode('utf-8')
+        return StringIO.StringIO(file_or_string)
+
+    # timestamp argument is just for unittests.
+    def _filename_for_upload(self, file_object, bug_id, extension="txt", timestamp=timestamp):
+        if hasattr(file_object, "name"):
+            return file_object.name
+        return "bug-%s-%s.%s" % (bug_id, timestamp(), extension)
+
+    def add_attachment_to_bug(self,
+                              bug_id,
+                              file_or_string,
+                              description,
+                              filename=None,
+                              comment_text=None):
+        self.authenticate()
+        log('Adding attachment "%s" to %s' % (description, self.bug_url_for_bug_id(bug_id)))
+        if self.dryrun:
+            log(comment_text)
+            return
 
-        if mark_for_landing:
-            self.browser['flag_type-3'] = ('+',)
-        elif mark_for_commit_queue:
-            self.browser['flag_type-3'] = ('?',)
-        else:
-            self.browser['flag_type-3'] = ('X',)
-
-        if bug_id:
-            patch_name = "bug-%s-%s.patch" % (bug_id, timestamp())
-        else:
-            patch_name ="%s.patch" % timestamp()
-
-        self.browser.add_file(patch_file_object,
-                              "text/plain",
-                              patch_name,
-                              'data')
+        self.browser.open(self.add_attachment_url(bug_id))
+        self.browser.select_form(name="entryform")
+        file_object = self._file_object_for_upload(file_or_string)
+        filename = filename or self._filename_for_upload(file_object, bug_id)
+        self._fill_attachment_form(description, file_object, filename=filename)
+        if comment_text:
+            log(comment_text)
+            self.browser['comment'] = comment_text
+        self.browser.submit()
 
+    # FIXME: The arguments to this function should be simplified and then
+    # this should be merged into add_attachment_to_bug
     def add_patch_to_bug(self,
                          bug_id,
-                         diff,
+                         file_or_string,
                          description,
                          comment_text=None,
                          mark_for_review=False,
                          mark_for_commit_queue=False,
                          mark_for_landing=False):
         self.authenticate()
-
-        log('Adding patch "%s" to %sshow_bug.cgi?id=%s' % (description,
-                                                           self.bug_server_url,
-                                                           bug_id))
+        log('Adding patch "%s" to %s' % (description, self.bug_url_for_bug_id(bug_id)))
 
         if self.dryrun:
             log(comment_text)
             return
 
-        self.browser.open("%sattachment.cgi?action=enter&bugid=%s" % (
-                          self.bug_server_url, bug_id))
+        self.browser.open(self.add_attachment_url(bug_id))
         self.browser.select_form(name="entryform")
-
-        # _fill_attachment_form expects a file-like object
-        # Patch files are already binary, so no encoding needed.
-        assert(isinstance(diff, str))
-        patch_file_object = StringIO.StringIO(diff)
+        file_object = self._file_object_for_upload(file_or_string)
+        filename = self._filename_for_upload(file_object, bug_id, extension="patch")
         self._fill_attachment_form(description,
-                                   patch_file_object,
+                                   file_object,
                                    mark_for_review=mark_for_review,
                                    mark_for_commit_queue=mark_for_commit_queue,
                                    mark_for_landing=mark_for_landing,
-                                   bug_id=bug_id)
+                                   is_patch=True,
+                                   filename=filename)
         if comment_text:
             log(comment_text)
             self.browser['comment'] = comment_text
         self.browser.submit()
 
+    # FIXME: There has to be a more concise way to write this method.
     def _check_create_bug_response(self, response_html):
         match = re.search("<title>Bug (?P<bug_id>\d+) Submitted</title>",
                           response_html)
@@ -516,6 +549,7 @@ class Bugzilla(object):
         log('Creating bug with title "%s"' % bug_title)
         if self.dryrun:
             log(bug_description)
+            # FIXME: This will make some paths fail, as they assume this returns an id.
             return
 
         self.browser.open(self.bug_server_url + "enter_bug.cgi?product=WebKit")
@@ -531,7 +565,7 @@ class Bugzilla(object):
             self.browser["cc"] = cc
         if blocked:
             self.browser["blocked"] = unicode(blocked)
-        if assignee == None:
+        if not assignee:
             assignee = self.username
         if assignee and not self.browser.find_control("assigned_to").disabled:
             self.browser["assigned_to"] = assignee
@@ -547,7 +581,8 @@ class Bugzilla(object):
                     patch_description,
                     patch_file_object,
                     mark_for_review=mark_for_review,
-                    mark_for_commit_queue=mark_for_commit_queue)
+                    mark_for_commit_queue=mark_for_commit_queue,
+                    is_patch=True)
 
         response = self.browser.submit()
 
index 2e73220..1d08ca5 100644 (file)
@@ -28,6 +28,7 @@
 
 import unittest
 import datetime
+import StringIO
 
 from .bugzilla import Bugzilla, BugzillaQueries, parse_bug_id
 
@@ -278,6 +279,24 @@ ZEZpbmlzaExvYWRXaXRoUmVhc29uOnJlYXNvbl07Cit9CisKIEBlbmQKIAogI2VuZGlmCg==
         extra_stderr = "Did not reopen bug 42, it appears to already be open with status ['NEW'].\n"
         self._assert_reopen(item_names=["NEW", "RESOLVED"], selected_index=0, extra_stderr=extra_stderr)
 
+    def test_file_object_for_upload(self):
+        bugzilla = Bugzilla()
+        file_object = StringIO.StringIO()
+        unicode_tor = u"WebKit \u2661 Tor Arne Vestb\u00F8!"
+        utf8_tor = unicode_tor.encode("utf-8")
+        self.assertEqual(bugzilla._file_object_for_upload(file_object), file_object)
+        self.assertEqual(bugzilla._file_object_for_upload(utf8_tor).read(), utf8_tor)
+        self.assertEqual(bugzilla._file_object_for_upload(unicode_tor).read(), utf8_tor)
+
+    def test_filename_for_upload(self):
+        bugzilla = Bugzilla()
+        mock_file = Mock()
+        mock_file.name = "foo"
+        self.assertEqual(bugzilla._filename_for_upload(mock_file, 1234), 'foo')
+        mock_timestamp = lambda: "now"
+        filename = bugzilla._filename_for_upload(StringIO.StringIO(), 1234, extension="patch", timestamp=mock_timestamp)
+        self.assertEqual(filename, "bug-1234-now.patch")
+
 
 class BugzillaQueriesTest(unittest.TestCase):
     _sample_request_page = """
index a7b3b0a..15e95ce 100644 (file)
@@ -85,6 +85,8 @@ class LayoutTestResults(object):
     def parsed_results(self):
         return self._parsed_results
 
+    def results_matching_keys(self, result_keys):
+        return sorted(sum([tests for key, tests in self._parsed_results.items() if key in result_keys], []))
+
     def failing_tests(self):
-        failing_keys = [self.fail_key, self.crash_key, self.timeout_key]
-        return sorted(sum([tests for key, tests in self._parsed_results.items() if key in failing_keys], []))
+        return self.results_matching_keys([self.fail_key, self.crash_key, self.timeout_key])
index 1a18026..cca21d8 100644 (file)
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import codecs
 import logging
 import platform
+import os.path
 
 from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults
 from webkitpy.common.config import urls
@@ -48,7 +50,7 @@ class FlakyTestReporter(object):
         return set([commit_info.author().bugzilla_email() for commit_info in commit_infos if commit_info.author()])
 
     def _bugzilla_email(self):
-        # This is kinda a funny way to get the bugzilla email,
+        # FIXME: This is kinda a funny way to get the bugzilla email,
         # we could also just create a Credentials object directly
         # but some of the Credentials logic is in bugzilla.py too...
         self._tool.bugs.authenticate()
@@ -126,6 +128,16 @@ If you would like to track this test fix with another bug, please close this bug
         flake_message = "The %s just saw %s flake while processing attachment %s on bug %s." % (self._bot_name, flaky_test, patch.id(), patch.bug_id())
         return "%s\n%s" % (flake_message, self._bot_information())
 
+    def _results_diff_path_for_test(self, flaky_test):
+        # FIXME: This is a big hack.  We should get this path from results.json
+        # except that old-run-webkit-tests doesn't produce a results.json
+        # so we just guess at the file path.
+        results_path = self._tool.port().layout_tests_results_path()
+        results_directory = os.path.dirname(results_path)
+        test_path = os.path.join(results_directory, flaky_test)
+        (test_path_root, _) = os.path.splitext(test_path)
+        return "%s-diffs.txt" % test_path_root
+
     def _follow_duplicate_chain(self, bug):
         while bug.is_closed() and bug.duplicate_of():
             bug = self._tool.bugs.fetch_bug(bug.duplicate_of())
@@ -145,12 +157,23 @@ If you would like to track this test fix with another bug, please close this bug
             latest_flake_message = self._latest_flake_message(flaky_test, patch)
             author_emails = self._author_emails_for_test(flaky_test)
             if not bug:
+                _log.info("Bug does not already exist for %s, creating." % flaky_test)
                 flake_bug_id = self._create_bug_for_flaky_test(flaky_test, author_emails, latest_flake_message)
             else:
                 bug = self._follow_duplicate_chain(bug)
                 self._update_bug_for_flaky_test(bug, latest_flake_message)
                 flake_bug_id = bug.id()
-
+            # FIXME: Ideally we'd only make one comment per flake, not two.  But that's not possible
+            # in all cases (e.g. when reopening), so for now we do the attachment in a second step.
+            results_diff_path = self._results_diff_path_for_test(flaky_test)
+            # Check to make sure that the path makes sense.
+            # Since we're not actually getting this path from the results.html
+            # there is a high probaility it's totally wrong.
+            if self._tool.filesystem.exists(results_diff_path):
+                results_diff = self._tool.filesystem.read_binary_file(results_diff_path)
+                self._tool.bugs.add_attachment_to_bug(flake_bug_id, results_diff, "Failure diff from bot", filename="failure.diff")
+            else:
+                _log.error("%s does not exist as expected, not uploading." % results_diff_path)
             message += "%s bug %s%s\n" % (flaky_test, flake_bug_id, self._optional_author_string(author_emails))
 
         message += "The %s is continuing to process your patch." % self._bot_name
index 3aafe13..98769ce 100644 (file)
@@ -29,6 +29,7 @@
 import unittest
 
 from webkitpy.common.config.committers import Committer
+from webkitpy.common.system.filesystem_mock import MockFileSystem
 from webkitpy.common.system.outputcapture import OutputCapture
 from webkitpy.tool.bot.flakytestreporter import FlakyTestReporter
 from webkitpy.tool.mocktool import MockTool, MockStatusServer
@@ -76,6 +77,9 @@ The bots will update this with information from each new failure.
 
 If you would like to track this test fix with another bug, please close this bug as a duplicate.
 
+component: Tools / Tests
+cc: test@test.com
+blocked: 50856
 """
         OutputCapture().assert_outputs(self, reporter._create_bug_for_flaky_test, ['foo/bar.html', ['test@test.com'], 'FLAKE_MESSAGE'], expected_stderr=expected_stderr)
 
@@ -91,8 +95,9 @@ If you would like to track this test fix with another bug, please close this bug
         reporter = FlakyTestReporter(tool, 'dummy-queue')
         self.assertEqual(reporter._bot_information(), "Bot: MockBotId  Port: MockPort  Platform: MockPlatform 1.0")
 
-    def test_create_bug_for_flaky_test(self):
+    def test_report_flaky_tests_creating_bug(self):
         tool = MockTool()
+        tool.filesystem = MockFileSystem({"/mock/foo/bar.diff": "mock"})
         reporter = FlakyTestReporter(tool, 'dummy-queue')
         reporter._lookup_bug_for_flaky_test = lambda bug_id: None
         patch = tool.bugs.fetch_attachment(197)
@@ -118,7 +123,7 @@ MOCK bug comment: bug_id=42, cc=None
 --- Begin comment ---
 The dummy-queue encountered the following flaky tests while processing attachment 197:
 
-foo/bar.html bug None (author: abarth@webkit.org)
+foo/bar.html bug 78 (author: abarth@webkit.org)
 The dummy-queue is continuing to process your patch.
 --- End comment ---
 
@@ -131,4 +136,8 @@ The dummy-queue is continuing to process your patch.
         self.assertEqual(reporter._optional_author_string(["foo@bar.com"]), " (author: foo@bar.com)")
         self.assertEqual(reporter._optional_author_string(["a@b.com", "b@b.com"]), " (authors: a@b.com and b@b.com)")
 
+    def test_results_diff_path_for_test(self):
+        reporter = FlakyTestReporter(MockTool(), 'dummy-queue')
+        self.assertEqual(reporter._results_diff_path_for_test("test.html"), "/mock/test-diffs.txt")
+
     # report_flaky_tests is also tested by queues_unittest
index e0c4971..36aa6b5 100644 (file)
@@ -42,6 +42,7 @@ class BugForTest(AbstractDeclarativeCommand):
         search_string = args[0]
         bug = reporter._lookup_bug_for_flaky_test(search_string)
         if bug:
+            bug = reporter._follow_duplicate_chain(bug)
             print "%5s %s" % (bug.id(), bug.title())
         else:
             print "No bugs found matching '%s'" % search_string
index 0a8f022..3748a8f 100644 (file)
@@ -185,7 +185,7 @@ component: MOCK component
 cc: MOCK cc
 blocked: 42
 Running prepare-ChangeLog
-MOCK add_patch_to_bug: bug_id=None, description=ROLLOUT of r852, mark_for_review=False, mark_for_commit_queue=True, mark_for_landing=False
+MOCK add_patch_to_bug: bug_id=78, description=ROLLOUT of r852, mark_for_review=False, mark_for_commit_queue=True, mark_for_landing=False
 -- Begin comment --
 Any committer can land this patch automatically by marking it commit-queue+.  The commit-queue will build and test the patch before landing to ensure that the rollout will be successful.  This process takes approximately 15 minutes.
 
index 858713e..e15555f 100644 (file)
@@ -293,7 +293,7 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
         return self._update_status(message, patch=patch, results_file=failure_log)
 
     # FIXME: This exists for mocking, but should instead be mocked via
-    # some sort of tool.filesystem() object.
+    # tool.filesystem.read_text_file.  They have different error handling at the moment.
     def _read_file_contents(self, path):
         try:
             with codecs.open(path, "r", "utf-8") as open_file:
index 35388ee..8c4b997 100644 (file)
@@ -88,10 +88,7 @@ class Rebaseline(AbstractDeclarativeCommand):
         shutil.move(downloaded_file, local_file)
 
     def _tests_to_update(self, build):
-        parsed_results = build.layout_test_results().parsed_results()
-        # FIXME: This probably belongs as API on LayoutTestResults
-        # but .failing_tests() already means something else.
-        failing_tests = parsed_results[LayoutTestResults.fail_key]
+        failing_tests = build.layout_test_results().results_matching_keys([LayoutTestResults.fail_key])
         return self._tool.user.prompt_with_list("Which test(s) to rebaseline:", failing_tests, can_choose_multiple=True)
 
     def _results_url_for_test(self, build, test):
index 53a699b..0006e87 100755 (executable)
@@ -41,6 +41,7 @@ from webkitpy.common.net.buildbot import BuildBot
 from webkitpy.common.net.irc.ircproxy import IRCProxy
 from webkitpy.common.net.statusserver import StatusServer
 from webkitpy.common.system.executive import Executive
+from webkitpy.common.system.filesystem import FileSystem
 from webkitpy.common.system.platforminfo import PlatformInfo
 from webkitpy.common.system.user import User
 from webkitpy.layout_tests import port
@@ -71,6 +72,7 @@ class WebKitPatch(MultiCommandTool):
         self.buildbot = BuildBot()
         self.executive = Executive()
         self._irc = None
+        self.filesystem = FileSystem()
         self._port = None
         self.user = User()
         self._scm = None
index 7b33719..9133b37 100644 (file)
@@ -33,8 +33,9 @@ from webkitpy.common.config.committers import CommitterList, Reviewer
 from webkitpy.common.checkout.commitinfo import CommitInfo
 from webkitpy.common.checkout.scm import CommitMessage
 from webkitpy.common.net.bugzilla import Bug, Attachment
-from webkitpy.thirdparty.mock import Mock
 from webkitpy.common.system.deprecated_logging import log
+from webkitpy.common.system.filesystem_mock import MockFileSystem
+from webkitpy.thirdparty.mock import Mock
 
 
 def _id_to_object_dictionary(*objects):
@@ -292,6 +293,7 @@ class MockBugzilla(Mock):
             log("cc: %s" % cc)
         if blocked:
             log("blocked: %s" % blocked)
+        return 78
 
     def quips(self):
         return ["Good artists copy. Great artists steal. - Pablo Picasso"]
@@ -641,6 +643,9 @@ class MockPort(Mock):
     def name(self):
         return "MockPort"
 
+    def layout_tests_results_path(self):
+        return "/mock/results.html"
+
 class MockTestPort1(object):
 
     def skips_layout_test(self, test_name):
@@ -671,6 +676,7 @@ class MockTool(object):
         self.bugs = MockBugzilla()
         self.buildbot = MockBuildBot()
         self.executive = MockExecutive(should_log=log_executive)
+        self.filesystem = MockFileSystem()
         self._irc = None
         self.user = MockUser()
         self._scm = MockSCM()