Changeset 52645 in webkit


Ignore:
Timestamp:
Dec 29, 2009 10:46:22 PM (14 years ago)
Author:
eric@webkit.org
Message:

2009-12-29 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

Split out BugzillaQueries class from Bugzilla
​https://bugs.webkit.org/show_bug.cgi?id=33042

  • Scripts/modules/bugzilla.py:
    • Split out BugzillaQueries from Bugzilla.
    • Try to isolate self.bugzilla usage into helper functions whenever possible.
    • Add a bunch of FIXMEs.
    • Rename fetch_bug_ids_from_needs_commit_list to fetch_bug_ids_from_pending_commit_list
  • Scripts/modules/bugzilla_unittest.py:
    • Create a new BugzillaQueriesTest testcase and move logic there.
  • Scripts/modules/buildsteps_unittest.py:
    • Use Bug 75 instead of 1 since bug 1 doesn't actually exist.
  • Scripts/modules/commands/queries.py:
    • Update to use bugzilla.queries
  • Scripts/modules/commands/queues.py:
    • Ditto.
  • Scripts/modules/commands/upload.py:
    • Ditto.
  • Scripts/modules/mock_bugzillatool.py:
    • Add a MockBugzillaQueries.
    • Make patches and bugs global privates.
    • Let _id_to_object_dictionary take a variable argument list instead of an array.
Location:
trunk/WebKitTools
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r52644 r52645  
     12009-12-29  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        Split out BugzillaQueries class from Bugzilla
     6        https://bugs.webkit.org/show_bug.cgi?id=33042
     7
     8        * Scripts/modules/bugzilla.py:
     9         - Split out BugzillaQueries from Bugzilla.
     10         - Try to isolate self.bugzilla usage into helper functions whenever possible.
     11         - Add a bunch of FIXMEs.
     12         - Rename fetch_bug_ids_from_needs_commit_list to fetch_bug_ids_from_pending_commit_list
     13        * Scripts/modules/bugzilla_unittest.py:
     14         - Create a new BugzillaQueriesTest testcase and move logic there.
     15        * Scripts/modules/buildsteps_unittest.py:
     16         - Use Bug 75 instead of 1 since bug 1 doesn't actually exist.
     17        * Scripts/modules/commands/queries.py:
     18         - Update to use bugzilla.queries
     19        * Scripts/modules/commands/queues.py:
     20         - Ditto.
     21        * Scripts/modules/commands/upload.py:
     22         - Ditto.
     23        * Scripts/modules/mock_bugzillatool.py:
     24         - Add a MockBugzillaQueries.
     25         - Make patches and bugs global privates.
     26         - Let _id_to_object_dictionary take a variable argument list instead of an array.
     27
    1282009-12-29  Daniel Bates  <dbates@webkit.org>
    229

  • trunk/WebKitTools/Scripts/modules/bugzilla.py

    r52642 r52645  
    8282
    8383
     84# A container for all of the logic for making a parsing buzilla queries.
     85class BugzillaQueries(object):
     86    def __init__(self, bugzilla):
     87        self.bugzila = bugzilla
     88
     89    def _load_query(self, query):
     90        full_url = "%s%s" % (self.bugzilla.bug_server_url, query)
     91        return self.bugzilla.browser.open(full_url)
     92
     93    def _fetch_bug_ids_advanced_query(self, query):
     94        soup = BeautifulSoup(self._load_query(query))
     95        # The contents of the <a> inside the cells in the first column happen to be the bug id.
     96        return [int(bug_link_cell.find("a").string) for bug_link_cell in soup('td', "first-child")]
     97
     98    def _parse_attachment_ids_request_query(self, page):
     99        digits = re.compile("\d+")
     100        attachment_href = re.compile("attachment.cgi\?id=\d+&action=review")
     101        attachment_links = SoupStrainer("a", href=attachment_href)
     102        return [int(digits.search(tag["href"]).group(0)) for tag in BeautifulSoup(page, parseOnlyThese=attachment_links)]
     103
     104    def _fetch_attachment_ids_request_query(self, query):
     105        return self._parse_attachment_ids_request_query(self._load_query(query))
     106
     107    # List of all r+'d bugs.
     108    def fetch_bug_ids_from_pending_commit_list(self):
     109        needs_commit_query_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B"
     110        return self._fetch_bug_ids_advanced_query(needs_commit_query_url)
     111
     112    def fetch_patches_from_pending_commit_list(self):
     113        # FIXME: This should not have to go through self.bugzilla
     114        return sum([self.bugzilla.fetch_reviewed_patches_from_bug(bug_id) for bug_id in self.fetch_bug_ids_from_pending_commit_list()], [])
     115
     116    def fetch_bug_ids_from_commit_queue(self):
     117        commit_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B"
     118        return self._fetch_bug_ids_advanced_query(commit_queue_url)
     119
     120    def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
     121        # FIXME: Once reject_invalid_patches is moved out of this function this becomes a simple list comprehension using fetch_bug_ids_from_commit_queue.
     122        patches_to_land = []
     123        for bug_id in self.fetch_bug_ids_from_commit_queue():
     124            # FIXME: This should not have to go through self.bugzilla
     125            patches = self.bugzilla.fetch_commit_queue_patches_from_bug(bug_id, reject_invalid_patches)
     126            patches_to_land += patches
     127        return patches_to_land
     128
     129    def _fetch_bug_ids_from_review_queue(self):
     130        review_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review?"
     131        return self._fetch_bug_ids_advanced_query(review_queue_url)
     132
     133    def fetch_patches_from_review_queue(self, limit=None):
     134        # FIXME: We should probably have a self.fetch_bug to minimize the number of self.bugzilla calls.
     135        return sum([self.bugzilla.fetch_bug(bug_id).unreviewed_patches() for bug_id in self._fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
     136
     137    # FIXME: Why do we have both fetch_patches_from_review_queue and fetch_attachment_ids_from_review_queue??
     138    # NOTE: This is also the only client of _fetch_attachment_ids_request_query
     139    def fetch_attachment_ids_from_review_queue(self):
     140        review_queue_url = "request.cgi?action=queue&type=review&group=type"
     141        return self._fetch_attachment_ids_request_query(review_queue_url)
     142
     143
    84144class Bugzilla(object):
    85145    def __init__(self, dryrun=False, committers=CommitterList()):
    86146        self.dryrun = dryrun
    87147        self.authenticated = False
     148        self.queries = BugzillaQueries(self)
    88149
    89150        # FIXME: We should use some sort of Browser mock object when in dryrun mode (to prevent any mistakes).
    … …  
    252313        return commit_queue_patches
    253314
    254     def _fetch_bug_ids_advanced_query(self, query):
    255         page = self.browser.open(query)
    256         soup = BeautifulSoup(page)
    257         # The contents of the <a> inside the cells in the first column happen to be the bug id.
    258         return [int(bug_link_cell.find("a").string) for bug_link_cell in soup('td', "first-child")]
    259 
    260     def _parse_attachment_ids_request_query(self, page):
    261         digits = re.compile("\d+")
    262         attachment_href = re.compile("attachment.cgi\?id=\d+&action=review")
    263         attachment_links = SoupStrainer("a", href=attachment_href)
    264         return [int(digits.search(tag["href"]).group(0)) for tag in BeautifulSoup(page, parseOnlyThese=attachment_links)]
    265 
    266     def _fetch_attachment_ids_request_query(self, query):
    267         return self._parse_attachment_ids_request_query(self.browser.open(query))
    268 
    269     def fetch_bug_ids_from_commit_queue(self):
    270         commit_queue_url = self.bug_server_url + "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B"
    271         return self._fetch_bug_ids_advanced_query(commit_queue_url)
    272 
    273     # List of all r+'d bugs.
    274     def fetch_bug_ids_from_needs_commit_list(self):
    275         needs_commit_query_url = self.bug_server_url + "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B"
    276         return self._fetch_bug_ids_advanced_query(needs_commit_query_url)
    277 
    278     def fetch_bug_ids_from_review_queue(self):
    279         review_queue_url = self.bug_server_url + "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review?"
    280         return self._fetch_bug_ids_advanced_query(review_queue_url)
    281 
    282     def fetch_attachment_ids_from_review_queue(self):
    283         review_queue_url = self.bug_server_url + "request.cgi?action=queue&type=review&group=type"
    284         return self._fetch_attachment_ids_request_query(review_queue_url)
    285 
    286     def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
    287         patches_to_land = []
    288         for bug_id in self.fetch_bug_ids_from_commit_queue():
    289             patches = self.fetch_commit_queue_patches_from_bug(bug_id, reject_invalid_patches)
    290             patches_to_land += patches
    291         return patches_to_land
    292 
    293     def fetch_patches_from_pending_commit_list(self):
    294         return sum([self.fetch_reviewed_patches_from_bug(bug_id) for bug_id in self.fetch_bug_ids_from_needs_commit_list()], [])
    295 
    296     def fetch_patches_from_review_queue(self, limit=None):
    297         return sum([self.fetch_bug(bug_id).unreviewed_patches() for bug_id in self.fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
    298 
    299315    def authenticate(self):
    300316        if self.authenticated:

  • trunk/WebKitTools/Scripts/modules/bugzilla_unittest.py

    r52641 r52645  
    3030
    3131from modules.committers import CommitterList, Reviewer, Committer
    32 from modules.bugzilla import Bugzilla, parse_bug_id
     32from modules.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id
    3333from modules.outputcapture import OutputCapture
    3434
    … …  
    224224        self.assertEquals(27314, bugzilla._parse_bug_id_from_attachment_page(self._sample_attachment_detail_page))
    225225
     226    def test_add_cc_to_bug(self):
     227        bugzilla = Bugzilla()
     228        bugzilla.browser = MockBrowser()
     229        bugzilla.authenticate = lambda: None
     230        expected_stderr = "Adding ['adam@example.com'] to the CC list for bug 42\n"
     231        OutputCapture().assert_outputs(self, bugzilla.add_cc_to_bug, [42, ["adam@example.com"]], expected_stderr=expected_stderr)
     232
     233    def test_flag_permission_rejection_message(self):
     234        bugzilla = Bugzilla()
     235        expected_messsage="""foo@foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
     236
     237- If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
     238
     239- If you have review rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your review rights."""
     240        self.assertEqual(bugzilla._flag_permission_rejection_message("foo@foo.com", "review"), expected_messsage)
     241
     242
     243class BugzillaQueriesTest(unittest.TestCase):
    226244    _sample_request_page = """
    227245<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
    … …  
    272290
    273291    def test_request_page_parsing(self):
    274         bugzilla = Bugzilla()
    275         self.assertEquals([40511, 40722, 40723], bugzilla._parse_attachment_ids_request_query(self._sample_request_page))
    276 
    277     def test_add_cc_to_bug(self):
    278         bugzilla = Bugzilla()
    279         bugzilla.browser = MockBrowser()
    280         bugzilla.authenticate = lambda: None
    281         expected_stderr = "Adding ['adam@example.com'] to the CC list for bug 42\n"
    282         OutputCapture().assert_outputs(self, bugzilla.add_cc_to_bug, [42, ["adam@example.com"]], expected_stderr=expected_stderr)
    283 
    284     def test_flag_permission_rejection_message(self):
    285         bugzilla = Bugzilla()
    286         expected_messsage="""foo@foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
    287 
    288 - If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
    289 
    290 - If you have review rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your review rights."""
    291         self.assertEqual(bugzilla._flag_permission_rejection_message("foo@foo.com", "review"), expected_messsage)
    292 
     292        queries = BugzillaQueries(None)
     293        self.assertEquals([40511, 40722, 40723], queries._parse_attachment_ids_request_query(self._sample_request_page))
    293294
    294295if __name__ == '__main__':

  • trunk/WebKitTools/Scripts/modules/buildsteps_unittest.py

    r52640 r52645  
    3939        capture = OutputCapture()
    4040        step = UpdateChangeLogsWithReviewerStep(MockBugzillaTool(), [])
    41         expected_stderr = "0 reviewed patches on bug 1, cannot infer reviewer.\n"
    42         capture.assert_outputs(self, step._guess_reviewer_from_bug, [1], expected_stderr=expected_stderr)
     41        expected_stderr = "0 reviewed patches on bug 75, cannot infer reviewer.\n"
     42        capture.assert_outputs(self, step._guess_reviewer_from_bug, [75], expected_stderr=expected_stderr)
    4343
    4444

  • trunk/WebKitTools/Scripts/modules/commands/queries.py

    r52295 r52645  
    4444
    4545    def execute(self, options, args, tool):
    46         bug_ids = tool.bugs.fetch_bug_ids_from_commit_queue()
     46        bug_ids = tool.bugs.queries.fetch_bug_ids_from_commit_queue()
    4747        for bug_id in bug_ids:
    4848            print "%s" % bug_id
    … …  
    5555
    5656    def execute(self, options, args, tool):
    57         patches = tool.bugs.fetch_patches_from_commit_queue()
     57        patches = tool.bugs.queries.fetch_patches_from_commit_queue()
    5858        log("Patches in commit queue:")
    5959        for patch in patches:
    … …  
    8383
    8484    def execute(self, options, args, tool):
    85         patches = tool.bugs.fetch_patches_from_pending_commit_list()
     85        patches = tool.bugs.queries.fetch_patches_from_pending_commit_list()
    8686        patches_needing_cq = filter(self._needs_commit_queue, patches)
    8787        if options.bugs:
    … …  
    101101
    102102    def execute(self, options, args, tool):
    103         patch_ids = tool.bugs.fetch_attachment_ids_from_review_queue()
     103        patch_ids = tool.bugs.queries.fetch_attachment_ids_from_review_queue()
    104104        log("Patches pending review:")
    105105        for patch_id in patch_ids:

  • trunk/WebKitTools/Scripts/modules/commands/queues.py

    r52430 r52645  
    134134
    135135    def next_work_item(self):
    136         patches = self.tool.bugs.fetch_patches_from_commit_queue(reject_invalid_patches=True)
     136        patches = self.tool.bugs.queries.fetch_patches_from_commit_queue(reject_invalid_patches=True)
    137137        if not patches:
    138138            self._update_status("Empty queue")
    … …  
    188188
    189189    def fetch_potential_patch_ids(self):
    190         return self.tool.bugs.fetch_attachment_ids_from_review_queue()
     190        return self.tool.bugs.queries.fetch_attachment_ids_from_review_queue()
    191191
    192192    def status_server(self):

  • trunk/WebKitTools/Scripts/modules/commands/upload.py

    r52641 r52645  
    8484
    8585    def execute(self, options, args, tool):
    86         for bug_id in tool.bugs.fetch_bug_ids_from_needs_commit_list():
     86        for bug_id in tool.bugs.queries.fetch_bug_ids_from_pending_commit_list():
    8787            self._assign_bug_to_last_patch_attacher(bug_id)
    8888

  • trunk/WebKitTools/Scripts/modules/mock_bugzillatool.py

    r52641 r52645  
    3333from modules.bugzilla import Bug
    3434
    35 def _id_to_object_dictionary(objects):
     35def _id_to_object_dictionary(*objects):
    3636    dictionary = {}
    3737    for thing in objects:
    … …  
    3939    return dictionary
    4040
    41 class MockBugzilla(Mock):
    42     patch1 = {
    43         "id" : 197,
    44         "bug_id" : 42,
    45         "url" : "http://example.com/197",
    46         "is_obsolete" : False,
    47         "reviewer" : "Reviewer1",
    48         "attacher_email" : "Contributer1",
    49     }
    50     patch2 = {
    51         "id" : 128,
    52         "bug_id" : 42,
    53         "url" : "http://example.com/128",
    54         "is_obsolete" : False,
    55         "reviewer" : "Reviewer2",
    56         "attacher_email" : "eric@webkit.org",
    57     }
    58     bug_server_url = "http://example.com"
    59     unassigned_email = "unassigned@example.com"
    60 
     41# FIXME: The ids shoudl be 1, 2, 3 instead of crazy numbers.
     42_patch1 = {
     43    "id" : 197,
     44    "bug_id" : 42,
     45    "url" : "http://example.com/197",
     46    "is_obsolete" : False,
     47    "is_patch" : True,
     48    "reviewer" : "Reviewer1",
     49    "attacher_email" : "Contributer1",
     50}
     51_patch2 = {
     52    "id" : 128,
     53    "bug_id" : 42,
     54    "url" : "http://example.com/128",
     55    "is_obsolete" : False,
     56    "is_patch" : True,
     57    "reviewer" : "Reviewer2",
     58    "attacher_email" : "eric@webkit.org",
     59}
     60
     61# This must be defined before we define the bugs, thus we don't use MockBugzilla.unassigned_email directly.
     62_unassigned_email = "unassigned@example.com"
     63
     64# FIXME: The ids should be 1, 2, 3 instead of crazy numbers.
     65_bug1 = {
     66    "id" : 42,
     67    "assigned_to_email" : _unassigned_email,
     68    "attachments" : [_patch1, _patch2],
     69}
     70_bug2 = {
     71    "id" : 75,
     72    "assigned_to_email" : "foo@foo.com",
     73    "attachments" : [],
     74}
     75_bug3 = {
     76    "id" : 76,
     77    "assigned_to_email" : _unassigned_email,
     78    "attachments" : [],
     79}
     80
     81class MockBugzillaQueries(Mock):
    6182    def fetch_bug_ids_from_commit_queue(self):
    6283        return [42, 75]
    … …  
    6687
    6788    def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
    68         return [self.patch1, self.patch2]
    69 
    70     def fetch_bug_ids_from_needs_commit_list(self):
     89        return [_patch1, _patch2]
     90
     91    def fetch_bug_ids_from_pending_commit_list(self):
    7192        return [42, 75, 76]
    72 
    73     bug1 = {
    74         "id" : 42,
    75         "assigned_to_email" : unassigned_email,
    76         "attachments" : [patch1, patch2],
    77     }
    78     bug2 = {
    79         "id" : 75,
    80         "assigned_to_email" : "foo@foo.com",
    81         "attachments" : [],
    82     }
    83     bug3 = {
    84         "id" : 76,
    85         "assigned_to_email" : unassigned_email,
    86         "attachments" : [],
    87     }
    88 
    89     bug_cache = _id_to_object_dictionary([bug1, bug2, bug3])
     93   
     94    def fetch_patches_from_pending_commit_list(self):
     95        return [_patch1, _patch2]
     96
     97
     98class MockBugzilla(Mock):
     99    bug_server_url = "http://example.com"
     100    unassigned_email = _unassigned_email
     101    bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3)
     102    attachment_cache = _id_to_object_dictionary(_patch1, _patch2)
     103    queries = MockBugzillaQueries()
    90104
    91105    def fetch_bug(self, bug_id):
    92106        return Bug(self.bug_cache.get(bug_id))
    93107
    94     def fetch_patches_from_pending_commit_list(self):
    95         return [self.patch1, self.patch2]
    96 
    97108    def fetch_reviewed_patches_from_bug(self, bug_id):
    98         if bug_id == 42:
    99             return [self.patch1, self.patch2]
    100         return []
    101 
     109        return self.fetch_patches_from_bug(bug_id) # Return them all for now.
     110
     111    def fetch_attachment(self, attachment_id):
     112        return self.attachment_cache[attachment_id] # This could be changed to .get() if we wish to allow failed lookups.
     113
     114    # NOTE: Functions below this are direct copies from bugzilla.py
    102115    def fetch_patches_from_bug(self, bug_id):
    103         if bug_id == 42:
    104             return [self.patch1, self.patch2]
    105         return None
    106 
    107     def fetch_attachment(self, attachment_id):
    108         if attachment_id == 197:
    109             return self.patch1
    110         if attachment_id == 128:
    111             return self.patch2
    112         raise Exception("Bogus attachment_id in fetch_attachment.")
    113 
     116        return self.fetch_bug(bug_id).patches()
     117   
    114118    def bug_url_for_bug_id(self, bug_id):
    115119        return "%s/%s" % (self.bug_server_url, bug_id)
Note: See TracChangeset for help on using the changeset viewer.