diff --git a/owners.py b/owners.py index 7fa69e365..1a2a1ab73 100644 --- a/owners.py +++ b/owners.py @@ -2,7 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -"""A database of OWNERS files. +r"""A database of OWNERS files. OWNERS files indicate who is allowed to approve changes in a specific directory (or who is allowed to make changes without needing approval of another OWNER). @@ -62,6 +62,12 @@ import fnmatch import random import re +try: + # This fallback applies for all versions of Python before 3.3 + import collections.abc as collections_abc +except ImportError: + import collections as collections_abc + # If this is present by itself on a line, this means that everyone can review. EVERYONE = '*' @@ -80,9 +86,9 @@ def _assert_is_collection(obj): assert not isinstance(obj, str) # Module 'collections' has no 'Iterable' member # pylint: disable=no-member - if hasattr(collections, 'Iterable') and hasattr(collections, 'Sized'): - assert (isinstance(obj, collections.Iterable) and - isinstance(obj, collections.Sized)) + if hasattr(collections_abc, 'Iterable') and hasattr(collections_abc, 'Sized'): + assert (isinstance(obj, collections_abc.Iterable) and + isinstance(obj, collections_abc.Sized)) class SyntaxErrorInOwnersFile(Exception): diff --git a/owners_finder.py b/owners_finder.py index 61869347e..62fe355a8 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -9,6 +9,7 @@ from __future__ import print_function import os import copy import owners as owners_module +import random def first(iterable): @@ -161,10 +162,12 @@ class OwnersFinder(object): self.selected_owners = set() self.deselected_owners = set() - # Initialize owners queue, sort it by the score and name - self.owners_queue = sorted( - self.owners_to_files.keys(), - key=lambda owner: (self.owners_score[owner], owner)) + # Randomize owners' names so that if many reviewers have identical scores + # they will be randomly ordered to avoid bias. + owners = list(self.owners_to_files.keys()) + random.shuffle(owners) + self.owners_queue = sorted(owners, + key=lambda owner: self.owners_score[owner]) self.find_mandatory_owners() def select_owner(self, owner, findMandatoryOwners=True): diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index a520d9a2d..126cc3a2e 100755 --- a/tests/owners_finder_test.py +++ b/tests/owners_finder_test.py @@ -153,8 +153,12 @@ class OwnersFinderTests(_BaseTestCase): def test_reset(self): finder = self.defaultFinder() for _ in range(2): - self.assertEqual(finder.owners_queue, - [brett, darin, john, peter, ken, ben, tom]) + expected = [brett, darin, john, peter, ken, ben, tom] + # darin and john have equal cost, the others have distinct costs. + # If the owners_queue has those two swapped then swap them in expected. + if finder.owners_queue[1] != expected[1]: + expected[1], expected[2] = expected[2], expected[1] + self.assertEqual(finder.owners_queue, expected) self.assertEqual(finder.unreviewed_files, { 'base/vlog.h', 'chrome/browser/defaults.h', @@ -202,7 +206,13 @@ class OwnersFinderTests(_BaseTestCase): finder = self.defaultFinder() finder.select_owner(brett) - self.assertEqual(finder.owners_queue, [darin, john, peter, ken, tom]) + expected = [darin, john, peter, ken, tom] + # darin and john have equal cost, the others have distinct costs. + # If the owners_queue has those two swapped then swap them in expected. + if finder.owners_queue[0] == john: + expected[0], expected[1] = expected[1], expected[0] + + self.assertEqual(finder.owners_queue, expected) self.assertEqual(finder.selected_owners, {brett}) self.assertEqual(finder.deselected_owners, {ben}) self.assertEqual(finder.reviewed_by,