From 37740e2bc9b2628d28f1959f78901168fcacd6ad Mon Sep 17 00:00:00 2001 From: Bruce Dawson Date: Thu, 14 Nov 2019 00:27:44 +0000 Subject: [PATCH] Randomize results of git cl owners git cl owners orders owners by score with alphabetization being the tie breaker. This leads to some owners being suggested far more often than others. Adding a tiny amount of randomization to the scoring leads to an even distribution of equally qualified reviewers. Less qualified reviewers will still be sorted into distinct buckets - the randomness is too small to do anything except break ties. The tests were updated so that they can tolerate the randomness, but only for breaking ties. Bug: 1024083 Change-Id: If7d39d1b3bbd980b80b46ab3f62c65215309bdc8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1913642 Commit-Queue: Bruce Dawson Reviewed-by: Anthony Polito Reviewed-by: Edward Lesmes --- owners.py | 14 ++++++++++---- owners_finder.py | 11 +++++++---- tests/owners_finder_test.py | 16 +++++++++++++--- 3 files changed, 30 insertions(+), 11 deletions(-) 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,