Changeset 74138 in webkit


Ignore:
Timestamp:
Dec 15, 2010 1:48:40 PM (13 years ago)
Author:
eric@webkit.org
Message:

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:
Location:
trunk/WebKitTools
Files:
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r74136 r74138  
     12010-12-14  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        commit-queue should upload failure diffs when tests flake
     6        https://bugs.webkit.org/show_bug.cgi?id=51051
     7
     8        To make this testable I needed to pipe FileSystem down onto tool.
     9        We've wanted it there for a long time anyway.
     10
     11        This patch is kinda a big hack.  But we don't have a nice
     12        way to read results.html files.  I think this will need further
     13        revision before this code actually feels clean.
     14
     15        As part of testing this change, I had to make MockBugzilla.create_bug
     16        actually return an id (like it should) which required updating
     17        a few other unit test results (for the better).
     18
     19        The results_matching_keys change in layouttestresults/rebasline
     20        was an alternate path which I decided not to use in the end, but
     21        I left the change as it seemed an improvement.
     22
     23        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
     24        * Scripts/webkitpy/common/net/layouttestresults.py:
     25        * Scripts/webkitpy/tool/bot/flakytestreporter.py:
     26        * Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py:
     27        * Scripts/webkitpy/tool/commands/queues.py:
     28        * Scripts/webkitpy/tool/commands/rebaseline.py:
     29        * Scripts/webkitpy/tool/main.py:
     30        * Scripts/webkitpy/tool/mocktool.py:
     31
    1322010-12-15  Cosmin Truta  <ctruta@chromium.org>
    233

  • trunk/WebKitTools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py

    r73991 r74138  
    222222            return None
    223223        content_type = "&ctype=xml" if xml else ""
    224         return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url,
    225                                            bug_id,
    226                                            content_type)
     224        return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url, bug_id, content_type)
    227225
    228226    def short_bug_url_for_bug_id(self, bug_id):
    … …  
    230228            return None
    231229        return "http://webkit.org/b/%s" % bug_id
     230
     231    def add_attachment_url(self, bug_id):
     232        return "%sattachment.cgi?action=enter&bugid=%s" % (self.bug_server_url, bug_id)
    232233
    233234    def attachment_url_for_id(self, attachment_id, action="view"):
    … …  
    416417                self.username = username
    417418
     419    def _commit_queue_flag(self, mark_for_landing, mark_for_commit_queue):
     420        if mark_for_landing:
     421            return '+'
     422        elif mark_for_commit_queue:
     423            return '?'
     424        return 'X'
     425
     426    # FIXME: mark_for_commit_queue and mark_for_landing should be joined into a single commit_flag argument.
    418427    def _fill_attachment_form(self,
    419428                              description,
    420                               patch_file_object,
    421                               comment_text=None,
     429                              file_object,
    422430                              mark_for_review=False,
    423431                              mark_for_commit_queue=False,
    424432                              mark_for_landing=False,
    425                               bug_id=None):
     433                              is_patch=False,
     434                              filename=None,
     435                              mimetype=None):
    426436        self.browser['description'] = description
    427         self.browser['ispatch'] = ("1",)
     437        if is_patch:
     438            self.browser['ispatch'] = ("1",)
     439        # FIXME: Should this use self._find_select_element_for_flag?
    428440        self.browser['flag_type-1'] = ('?',) if mark_for_review else ('X',)
    429 
    430         if mark_for_landing:
    431             self.browser['flag_type-3'] = ('+',)
    432         elif mark_for_commit_queue:
    433             self.browser['flag_type-3'] = ('?',)
    434         else:
    435             self.browser['flag_type-3'] = ('X',)
    436 
    437         if bug_id:
    438             patch_name = "bug-%s-%s.patch" % (bug_id, timestamp())
    439         else:
    440             patch_name ="%s.patch" % timestamp()
    441 
    442         self.browser.add_file(patch_file_object,
    443                               "text/plain",
    444                               patch_name,
    445                               'data')
    446 
     441        self.browser['flag_type-3'] = (self._commit_queue_flag(mark_for_landing, mark_for_commit_queue),)
     442
     443        filename = filename or "%s.patch" % timestamp()
     444        mimetype = mimetype or "text/plain"
     445        self.browser.add_file(file_object, mimetype, filename, 'data')
     446
     447    def _file_object_for_upload(self, file_or_string):
     448        if hasattr(file_or_string, 'read'):
     449            return file_or_string
     450        # Only if file_or_string is not already encoded do we want to encode it.
     451        if isinstance(file_or_string, unicode):
     452            file_or_string = file_or_string.encode('utf-8')
     453        return StringIO.StringIO(file_or_string)
     454
     455    # timestamp argument is just for unittests.
     456    def _filename_for_upload(self, file_object, bug_id, extension="txt", timestamp=timestamp):
     457        if hasattr(file_object, "name"):
     458            return file_object.name
     459        return "bug-%s-%s.%s" % (bug_id, timestamp(), extension)
     460
     461    def add_attachment_to_bug(self,
     462                              bug_id,
     463                              file_or_string,
     464                              description,
     465                              filename=None,
     466                              comment_text=None):
     467        self.authenticate()
     468        log('Adding attachment "%s" to %s' % (description, self.bug_url_for_bug_id(bug_id)))
     469        if self.dryrun:
     470            log(comment_text)
     471            return
     472
     473        self.browser.open(self.add_attachment_url(bug_id))
     474        self.browser.select_form(name="entryform")
     475        file_object = self._file_object_for_upload(file_or_string)
     476        filename = filename or self._filename_for_upload(file_object, bug_id)
     477        self._fill_attachment_form(description, file_object, filename=filename)
     478        if comment_text:
     479            log(comment_text)
     480            self.browser['comment'] = comment_text
     481        self.browser.submit()
     482
     483    # FIXME: The arguments to this function should be simplified and then
     484    # this should be merged into add_attachment_to_bug
    447485    def add_patch_to_bug(self,
    448486                         bug_id,
    449                          diff,
     487                         file_or_string,
    450488                         description,
    451489                         comment_text=None,
    … …  
    454492                         mark_for_landing=False):
    455493        self.authenticate()
    456 
    457         log('Adding patch "%s" to %sshow_bug.cgi?id=%s' % (description,
    458                                                            self.bug_server_url,
    459                                                            bug_id))
    460 
    461         if self.dryrun:
    462             log(comment_text)
    463             return
    464 
    465         self.browser.open("%sattachment.cgi?action=enter&bugid=%s" % (
    466                           self.bug_server_url, bug_id))
     494        log('Adding patch "%s" to %s' % (description, self.bug_url_for_bug_id(bug_id)))
     495
     496        if self.dryrun:
     497            log(comment_text)
     498            return
     499
     500        self.browser.open(self.add_attachment_url(bug_id))
    467501        self.browser.select_form(name="entryform")
    468 
    469         # _fill_attachment_form expects a file-like object
    470         # Patch files are already binary, so no encoding needed.
    471         assert(isinstance(diff, str))
    472         patch_file_object = StringIO.StringIO(diff)
     502        file_object = self._file_object_for_upload(file_or_string)
     503        filename = self._filename_for_upload(file_object, bug_id, extension="patch")
    473504        self._fill_attachment_form(description,
    474                                    patch_file_object,
     505                                   file_object,
    475506                                   mark_for_review=mark_for_review,
    476507                                   mark_for_commit_queue=mark_for_commit_queue,
    477508                                   mark_for_landing=mark_for_landing,
    478                                    bug_id=bug_id)
     509                                   is_patch=True,
     510                                   filename=filename)
    479511        if comment_text:
    480512            log(comment_text)
    … …  
    482514        self.browser.submit()
    483515
     516    # FIXME: There has to be a more concise way to write this method.
    484517    def _check_create_bug_response(self, response_html):
    485518        match = re.search("<title>Bug (?P<bug_id>\d+) Submitted</title>",
    … …  
    517550        if self.dryrun:
    518551            log(bug_description)
     552            # FIXME: This will make some paths fail, as they assume this returns an id.
    519553            return
    520554
    … …  
    532566        if blocked:
    533567            self.browser["blocked"] = unicode(blocked)
    534         if assignee == None:
     568        if not assignee:
    535569            assignee = self.username
    536570        if assignee and not self.browser.find_control("assigned_to").disabled:
    … …  
    548582                    patch_file_object,
    549583                    mark_for_review=mark_for_review,
    550                     mark_for_commit_queue=mark_for_commit_queue)
     584                    mark_for_commit_queue=mark_for_commit_queue,
     585                    is_patch=True)
    551586
    552587        response = self.browser.submit()

  • trunk/WebKitTools/Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py

    r73950 r74138  
    2929import unittest
    3030import datetime
     31import StringIO
    3132
    3233from .bugzilla import Bugzilla, BugzillaQueries, parse_bug_id
    … …  
    278279        extra_stderr = "Did not reopen bug 42, it appears to already be open with status ['NEW'].\n"
    279280        self._assert_reopen(item_names=["NEW", "RESOLVED"], selected_index=0, extra_stderr=extra_stderr)
     281
     282    def test_file_object_for_upload(self):
     283        bugzilla = Bugzilla()
     284        file_object = StringIO.StringIO()
     285        unicode_tor = u"WebKit \u2661 Tor Arne Vestb\u00F8!"
     286        utf8_tor = unicode_tor.encode("utf-8")
     287        self.assertEqual(bugzilla._file_object_for_upload(file_object), file_object)
     288        self.assertEqual(bugzilla._file_object_for_upload(utf8_tor).read(), utf8_tor)
     289        self.assertEqual(bugzilla._file_object_for_upload(unicode_tor).read(), utf8_tor)
     290
     291    def test_filename_for_upload(self):
     292        bugzilla = Bugzilla()
     293        mock_file = Mock()
     294        mock_file.name = "foo"
     295        self.assertEqual(bugzilla._filename_for_upload(mock_file, 1234), 'foo')
     296        mock_timestamp = lambda: "now"
     297        filename = bugzilla._filename_for_upload(StringIO.StringIO(), 1234, extension="patch", timestamp=mock_timestamp)
     298        self.assertEqual(filename, "bug-1234-now.patch")
    280299
    281300

  • trunk/WebKitTools/Scripts/webkitpy/common/net/layouttestresults.py

    r71571 r74138  
    8686        return self._parsed_results
    8787
     88    def results_matching_keys(self, result_keys):
     89        return sorted(sum([tests for key, tests in self._parsed_results.items() if key in result_keys], []))
     90
    8891    def failing_tests(self):
    89         failing_keys = [self.fail_key, self.crash_key, self.timeout_key]
    90         return sorted(sum([tests for key, tests in self._parsed_results.items() if key in failing_keys], []))
     92        return self.results_matching_keys([self.fail_key, self.crash_key, self.timeout_key])

  • trunk/WebKitTools/Scripts/webkitpy/tool/bot/flakytestreporter.py

    r73991 r74138  
    2727# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
    2828
     29import codecs
    2930import logging
    3031import platform
     32import os.path
    3133
    3234from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults
    … …  
    4951
    5052    def _bugzilla_email(self):
    51         # This is kinda a funny way to get the bugzilla email,
     53        # FIXME: This is kinda a funny way to get the bugzilla email,
    5254        # we could also just create a Credentials object directly
    5355        # but some of the Credentials logic is in bugzilla.py too...
    … …  
    127129        return "%s\n%s" % (flake_message, self._bot_information())
    128130
     131    def _results_diff_path_for_test(self, flaky_test):
     132        # FIXME: This is a big hack.  We should get this path from results.json
     133        # except that old-run-webkit-tests doesn't produce a results.json
     134        # so we just guess at the file path.
     135        results_path = self._tool.port().layout_tests_results_path()
     136        results_directory = os.path.dirname(results_path)
     137        test_path = os.path.join(results_directory, flaky_test)
     138        (test_path_root, _) = os.path.splitext(test_path)
     139        return "%s-diffs.txt" % test_path_root
     140
    129141    def _follow_duplicate_chain(self, bug):
    130142        while bug.is_closed() and bug.duplicate_of():
    … …  
    146158            author_emails = self._author_emails_for_test(flaky_test)
    147159            if not bug:
     160                _log.info("Bug does not already exist for %s, creating." % flaky_test)
    148161                flake_bug_id = self._create_bug_for_flaky_test(flaky_test, author_emails, latest_flake_message)
    149162            else:
    … …  
    151164                self._update_bug_for_flaky_test(bug, latest_flake_message)
    152165                flake_bug_id = bug.id()
    153 
     166            # FIXME: Ideally we'd only make one comment per flake, not two.  But that's not possible
     167            # in all cases (e.g. when reopening), so for now we do the attachment in a second step.
     168            results_diff_path = self._results_diff_path_for_test(flaky_test)
     169            # Check to make sure that the path makes sense.
     170            # Since we're not actually getting this path from the results.html
     171            # there is a high probaility it's totally wrong.
     172            if self._tool.filesystem.exists(results_diff_path):
     173                results_diff = self._tool.filesystem.read_binary_file(results_diff_path)
     174                self._tool.bugs.add_attachment_to_bug(flake_bug_id, results_diff, "Failure diff from bot", filename="failure.diff")
     175            else:
     176                _log.error("%s does not exist as expected, not uploading." % results_diff_path)
    154177            message += "%s bug %s%s\n" % (flaky_test, flake_bug_id, self._optional_author_string(author_emails))
    155178

  • trunk/WebKitTools/Scripts/webkitpy/tool/bot/flakytestreporter_unittest.py

    r73838 r74138  
    3030
    3131from webkitpy.common.config.committers import Committer
     32from webkitpy.common.system.filesystem_mock import MockFileSystem
    3233from webkitpy.common.system.outputcapture import OutputCapture
    3334from webkitpy.tool.bot.flakytestreporter import FlakyTestReporter
    … …  
    7778If you would like to track this test fix with another bug, please close this bug as a duplicate.
    7879
     80component: Tools / Tests
     81cc: test@test.com
     82blocked: 50856
    7983"""
    8084        OutputCapture().assert_outputs(self, reporter._create_bug_for_flaky_test, ['foo/bar.html', ['test@test.com'], 'FLAKE_MESSAGE'], expected_stderr=expected_stderr)
    … …  
    9296        self.assertEqual(reporter._bot_information(), "Bot: MockBotId  Port: MockPort  Platform: MockPlatform 1.0")
    9397
    94     def test_create_bug_for_flaky_test(self):
     98    def test_report_flaky_tests_creating_bug(self):
    9599        tool = MockTool()
     100        tool.filesystem = MockFileSystem({"/mock/foo/bar.diff": "mock"})
    96101        reporter = FlakyTestReporter(tool, 'dummy-queue')
    97102        reporter._lookup_bug_for_flaky_test = lambda bug_id: None
    … …  
    119124The dummy-queue encountered the following flaky tests while processing attachment 197:
    120125
    121 foo/bar.html bug None (author: abarth@webkit.org)
     126foo/bar.html bug 78 (author: abarth@webkit.org)
    122127The dummy-queue is continuing to process your patch.
    123128--- End comment ---
    … …  
    132137        self.assertEqual(reporter._optional_author_string(["a@b.com", "b@b.com"]), " (authors: a@b.com and b@b.com)")
    133138
     139    def test_results_diff_path_for_test(self):
     140        reporter = FlakyTestReporter(MockTool(), 'dummy-queue')
     141        self.assertEqual(reporter._results_diff_path_for_test("test.html"), "/mock/test-diffs.txt")
     142
    134143    # report_flaky_tests is also tested by queues_unittest

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/bugfortest.py

    r73991 r74138  
    4343        bug = reporter._lookup_bug_for_flaky_test(search_string)
    4444        if bug:
     45            bug = reporter._follow_duplicate_chain(bug)
    4546            print "%5s %s" % (bug.id(), bug.title())
    4647        else:

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py

    r73951 r74138  
    186186blocked: 42
    187187Running prepare-ChangeLog
    188 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
     188MOCK add_patch_to_bug: bug_id=78, description=ROLLOUT of r852, mark_for_review=False, mark_for_commit_queue=True, mark_for_landing=False
    189189-- Begin comment --
    190190Any 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.

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/queues.py

    r73691 r74138  
    294294
    295295    # FIXME: This exists for mocking, but should instead be mocked via
    296     # some sort of tool.filesystem() object.
     296    # tool.filesystem.read_text_file.  They have different error handling at the moment.
    297297    def _read_file_contents(self, path):
    298298        try:

  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py

    r73776 r74138  
    8989
    9090    def _tests_to_update(self, build):
    91         parsed_results = build.layout_test_results().parsed_results()
    92         # FIXME: This probably belongs as API on LayoutTestResults
    93         # but .failing_tests() already means something else.
    94         failing_tests = parsed_results[LayoutTestResults.fail_key]
     91        failing_tests = build.layout_test_results().results_matching_keys([LayoutTestResults.fail_key])
    9592        return self._tool.user.prompt_with_list("Which test(s) to rebaseline:", failing_tests, can_choose_multiple=True)
    9693

  • trunk/WebKitTools/Scripts/webkitpy/tool/main.py

    r73804 r74138  
    4242from webkitpy.common.net.statusserver import StatusServer
    4343from webkitpy.common.system.executive import Executive
     44from webkitpy.common.system.filesystem import FileSystem
    4445from webkitpy.common.system.platforminfo import PlatformInfo
    4546from webkitpy.common.system.user import User
    … …  
    7273        self.executive = Executive()
    7374        self._irc = None
     75        self.filesystem = FileSystem()
    7476        self._port = None
    7577        self.user = User()

  • trunk/WebKitTools/Scripts/webkitpy/tool/mocktool.py

    r73991 r74138  
    3434from webkitpy.common.checkout.scm import CommitMessage
    3535from webkitpy.common.net.bugzilla import Bug, Attachment
     36from webkitpy.common.system.deprecated_logging import log
     37from webkitpy.common.system.filesystem_mock import MockFileSystem
    3638from webkitpy.thirdparty.mock import Mock
    37 from webkitpy.common.system.deprecated_logging import log
    3839
    3940
    … …  
    293294        if blocked:
    294295            log("blocked: %s" % blocked)
     296        return 78
    295297
    296298    def quips(self):
    … …  
    642644        return "MockPort"
    643645
     646    def layout_tests_results_path(self):
     647        return "/mock/results.html"
     648
    644649class MockTestPort1(object):
    645650
    … …  
    672677        self.buildbot = MockBuildBot()
    673678        self.executive = MockExecutive(should_log=log_executive)
     679        self.filesystem = MockFileSystem()
    674680        self._irc = None
    675681        self.user = MockUser()
Note: See TracChangeset for help on using the changeset viewer.