Use weighted random selection; add project reviewers
* Allow Rust reviewers to have different number (quota)
of randomly assigned projects.
* People owning K projects got their quota reduced by K.
* Add more project owners (reviewers).
* Sort a set of reviewers before adding all of them.
* Add and separate unit tests.
* Add TEST_MAPPING with the reviewers presubmit test.
* Separate reviewers test in its own file;
old external_updater test does not run with atest or TH.
Bug: 162988933
Test: out/host/*/testcases/*/*/external_updater_test
Test: out/host/*/testcases/*/*/external_updater_reviewers_test
Test: out/.../bin/external_updater update --branch_and_commit --push_change <some_project>
Change-Id: I375bf3d216f213452aacbedf844d1a1015310e7b
diff --git a/Android.bp b/Android.bp
index 59bda78..e867089 100644
--- a/Android.bp
+++ b/Android.bp
@@ -69,13 +69,37 @@
},
}
+python_defaults {
+ name: "external_updater_defaults",
+ libs: [
+ "external_updater_lib",
+ ],
+ version: {
+ py2: {
+ enabled: false,
+ },
+ py3: {
+ enabled: true,
+ },
+ },
+}
+
python_test_host {
name: "external_updater_test",
+ defaults: ["external_updater_defaults"],
main: "external_updater_test.py",
srcs: [
"external_updater_test.py",
],
- libs: [
- "external_updater_lib",
+ test_suites: ["general-tests"],
+}
+
+python_test_host {
+ name: "external_updater_reviewers_test",
+ defaults: ["external_updater_defaults"],
+ main: "external_updater_reviewers_test.py",
+ srcs: [
+ "external_updater_reviewers_test.py",
],
+ test_suites: ["general-tests"],
}
diff --git a/TEST_MAPPING b/TEST_MAPPING
new file mode 100644
index 0000000..00c285b
--- /dev/null
+++ b/TEST_MAPPING
@@ -0,0 +1,9 @@
+{
+ "presubmit": [
+ // "external_updater_test" runs alone but not with atest
+ {
+ "name": "external_updater_reviewers_test",
+ "host": true
+ }
+ ]
+}
diff --git a/external_updater_reviewers_test.py b/external_updater_reviewers_test.py
new file mode 100644
index 0000000..e055296
--- /dev/null
+++ b/external_updater_reviewers_test.py
@@ -0,0 +1,141 @@
+# Copyright (C) 2020 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Unit tests for external updater reviewers."""
+
+from typing import List, Mapping, Set
+import unittest
+
+import reviewers
+
+
+class ExternalUpdaterReviewersTest(unittest.TestCase):
+ """Unit tests for external updater reviewers."""
+
+ def setUp(self):
+ super().setUp()
+ # save constants in reviewers
+ self.saved_proj_reviewers = reviewers.PROJ_REVIEWERS
+ self.saved_rust_reviewers = reviewers.RUST_REVIEWERS
+ self.saved_rust_reviewer_list = reviewers.RUST_REVIEWER_LIST
+ self.saved_num_rust_projects = reviewers.NUM_RUST_PROJECTS
+ self.saved_rust_crate_owners = reviewers.RUST_CRATE_OWNERS
+
+ def tearDown(self):
+ super().tearDown()
+ # restore constants in reviewers
+ reviewers.PROJ_REVIEWERS = self.saved_proj_reviewers
+ reviewers.RUST_REVIEWERS = self.saved_rust_reviewers
+ reviewers.RUST_REVIEWER_LIST = self.saved_rust_reviewer_list
+ reviewers.NUM_RUST_PROJECTS = self.saved_num_rust_projects
+ reviewers.RUST_CRATE_OWNERS = self.saved_rust_crate_owners
+
+ def _collect_reviewers(self, num_runs, proj_path):
+ counters = {}
+ for _ in range(num_runs):
+ name = reviewers.find_reviewers(proj_path)
+ if name in counters:
+ counters[name] += 1
+ else:
+ counters[name] = 1
+ return counters
+
+ def test_reviewers_types(self):
+ # Check type of PROJ_REVIEWERS
+ self.assertIsInstance(reviewers.PROJ_REVIEWERS, Mapping)
+ for key, value in reviewers.PROJ_REVIEWERS.items():
+ self.assertIsInstance(key, str)
+ if isinstance(value, Set) or isinstance(value, List):
+ map(lambda x: self.assertIsInstance(x, str), value)
+ else:
+ self.assertIsInstance(value, str)
+ # Check element types of the reviewers list and map.
+ self.assertIsInstance(reviewers.RUST_REVIEWERS, Mapping)
+ for (x, n) in reviewers.RUST_REVIEWERS.items():
+ self.assertIsInstance(x, str)
+ self.assertIsInstance(n, int)
+
+ def test_reviewers_constants(self):
+ # There should be enough people in the reviewers pool.
+ self.assertGreaterEqual(len(reviewers.RUST_REVIEWERS), 3)
+ # The NUM_RUST_PROJECTS should not be too small.
+ self.assertGreaterEqual(reviewers.NUM_RUST_PROJECTS, 50)
+ self.assertGreaterEqual(reviewers.NUM_RUST_PROJECTS,
+ len(reviewers.RUST_CRATE_OWNERS))
+ # chh@ should be in many projects and not in RUST_REVIEWER_LIST
+ self.assertGreaterEqual(len(reviewers.RUST_REVIEWER_LIST), 3)
+ self.assertNotIn("[email protected]", reviewers.RUST_REVIEWER_LIST)
+ self.assertIn("[email protected]", reviewers.RUST_REVIEWERS)
+ # Assume no project reviewers and recreate RUST_REVIEWER_LIST
+ reviewers.PROJ_REVIEWERS = {}
+ reviewers.RUST_REVIEWER_LIST = reviewers.create_rust_reviewer_list()
+ sum_projects = sum(reviewers.RUST_REVIEWERS.values())
+ self.assertEqual(sum_projects, len(reviewers.RUST_REVIEWER_LIST))
+ self.assertGreaterEqual(sum_projects, reviewers.NUM_RUST_PROJECTS)
+
+ def test_reviewers_randomness(self):
+ # Check random selection of reviewers.
+ # This might fail when the random.choice function is extremely unfair.
+ # With N * 20 tries, each reviewer should be picked at least twice.
+ # Assume no project reviewers and recreate RUST_REVIEWER_LIST
+ reviewers.PROJ_REVIEWERS = {}
+ reviewers.RUST_REVIEWER_LIST = reviewers.create_rust_reviewer_list()
+ num_tries = len(reviewers.RUST_REVIEWERS) * 20
+ counters = self._collect_reviewers(num_tries, "rust/crates/libc")
+ self.assertEqual(len(counters), len(reviewers.RUST_REVIEWERS))
+ map(lambda n: self.assertGreaterEqual(n, 10), counters.values())
+ self.assertEqual(sum(counters.values()), num_tries)
+
+ def test_project_reviewers(self):
+ # For specific projects, select only the specified reviewers.
+ reviewers.PROJ_REVIEWERS = {
+ "rust/crates/p1": "[email protected]",
+ "rust/crates/p_any": ["[email protected]", "[email protected]"],
+ "rust/crates/p_all": {"z@g", "[email protected]", "[email protected]"},
+ }
+ counters = self._collect_reviewers(20, "external/rust/crates/p1")
+ self.assertEqual(len(counters), 1)
+ self.assertTrue(counters["[email protected]"], 20)
+ counters = self._collect_reviewers(20, "external/rust/crates/p_any")
+ self.assertEqual(len(counters), 2)
+ self.assertGreater(counters["[email protected]"], 2)
+ self.assertGreater(counters["[email protected]"], 2)
+ self.assertTrue(counters["[email protected]"] + counters["[email protected]"], 20)
+ counters = self._collect_reviewers(20, "external/rust/crates/p_all")
+ # {x, y, z} reviewers should be sorted
+ self.assertEqual(counters["[email protected],[email protected],r=z@g"], 20)
+
+ def test_weighted_reviewers(self):
+ # Test create_rust_reviewer_list
+ reviewers.PROJ_REVIEWERS = {
+ "any_p1": "x@g", # 1 for x@g
+ "any_p2": {"xyz", "x@g"}, # 1 for x@g, xyz is not a rust reviewer
+ "any_p3": {"abc", "x@g"}, # 0.5 for "abc" and "x@g"
+ }
+ reviewers.RUST_REVIEWERS = {
+ "x@g": 5, # ceil(5 - 2.5) = 3
+ "abc": 2, # ceil(2 - 0.5) = 2
+ }
+ reviewer_list = reviewers.create_rust_reviewer_list()
+ self.assertEqual(reviewer_list, ["x@g", "x@g", "x@g", "abc", "abc"])
+ # Error case: if nobody has project quota, reset everyone to 1.
+ reviewers.RUST_REVIEWERS = {
+ "x@g": 1, # ceil(1 - 2.5) = -1
+ "abc": 0, # ceil(0 - 0.5) = 0
+ }
+ reviewer_list = reviewers.create_rust_reviewer_list()
+ self.assertEqual(reviewer_list, ["x@g", "abc"]) # everyone got 1
+
+
+if __name__ == "__main__":
+ unittest.main(verbosity=2)
diff --git a/external_updater_test.py b/external_updater_test.py
index 32e2918..c274be5 100644
--- a/external_updater_test.py
+++ b/external_updater_test.py
@@ -13,11 +13,9 @@
# limitations under the License.
"""Unit tests for external updater."""
-from typing import List, Mapping, Set
import unittest
import github_archive_updater
-import reviewers
class ExternalUpdaterTest(unittest.TestCase):
@@ -45,63 +43,6 @@
expected_url = prefix + "archive/ver-1.0.zip"
self.assertEqual(url, expected_url)
- def collect_reviewers(self, num_runs, proj_path):
- counters = {}
- for _ in range(num_runs):
- name = reviewers.find_reviewers(proj_path)
- if name in counters:
- counters[name] += 1
- else:
- counters[name] = 1
- return counters
-
- def test_reviewers(self):
- # There should be enough people in the reviewers pool.
- self.assertGreaterEqual(len(reviewers.RUST_REVIEWERS), 3)
- # Check element types of the reviewers list and map.
- self.assertIsInstance(reviewers.RUST_REVIEWERS, List)
- for x in reviewers.RUST_REVIEWERS:
- self.assertIsInstance(x, str)
- self.assertIsInstance(reviewers.PROJ_REVIEWERS, Mapping)
- for key, value in reviewers.PROJ_REVIEWERS.items():
- self.assertIsInstance(key, str)
- if isinstance(value, Set) or isinstance(value, List):
- for x in value:
- self.assertIsInstance(x, str)
- else:
- self.assertIsInstance(value, str)
- # Check random selection of reviewers.
- # This might fail when the random.choice function is extremely unfair.
- # With N * 20 tries, each reviewer should be picked at least twice.
- counters = self.collect_reviewers(len(reviewers.RUST_REVIEWERS) * 20,
- "rust/crates/no_such_project")
- self.assertEqual(len(counters), len(reviewers.RUST_REVIEWERS))
- for key, value in counters.items():
- self.assertGreaterEqual(value, 2)
- # For specific projects, select only the specified reviewers.
- saved_reviewers = reviewers.PROJ_REVIEWERS
- reviewers.PROJ_REVIEWERS = {
- "rust/crates/p1": "[email protected]",
- "rust/crates/p_any": ["[email protected]", "[email protected]"],
- "rust/crates/p_all": {"[email protected]", "[email protected]"},
- }
- counters = self.collect_reviewers(20, "external/rust/crates/p1")
- self.assertEqual(len(counters), 1)
- self.assertTrue(counters["[email protected]"], 20)
- counters = self.collect_reviewers(20, "external/rust/crates/p_any")
- self.assertEqual(len(counters), 2)
- self.assertGreater(counters["[email protected]"], 2)
- self.assertGreater(counters["[email protected]"], 2)
- self.assertTrue(counters["[email protected]"] + counters["[email protected]"], 20)
- counters = self.collect_reviewers(20, "external/rust/crates/p_all")
- counted = 0
- if "[email protected],[email protected]" in counters:
- counted += counters["[email protected],[email protected]"]
- if "[email protected],[email protected]" in counters:
- counted += counters["[email protected],[email protected]"]
- self.assertEqual(counted, 20)
- reviewers.PROJ_REVIEWERS = saved_reviewers
-
if __name__ == "__main__":
- unittest.main()
+ unittest.main(verbosity=2)
diff --git a/reviewers.py b/reviewers.py
index 35ba323..40443fc 100644
--- a/reviewers.py
+++ b/reviewers.py
@@ -11,37 +11,137 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-"""Find main reviewers for git push command."""
+"""Find main reviewers for git push commands."""
+import math
import random
from typing import List, Mapping, Set, Union
-# Note that for randomly-pick-one reviewers, we put them in a List[str]
+# To randomly pick one of multiple reviewers, we put them in a List[str]
# to work with random.choice efficiently.
-# For pick-all reviewers, use a Set[str] to distinguish with List[str].
+# To pick all of multiple reviewers, we use a Set[str].
# A ProjMapping maps a project path string to
-# (1) a single reviewer email address as a string
-# (2) multiple reviewer email addresses in a List; one to be randomly picked
-# (3) multiple reviewer email addresses in a Set; all to be picked
+# (1) a single reviewer email address as a string, or
+# (2) a List of multiple reviewers to be randomly picked, or
+# (3) a Set of multiple reviewers to be all added.
ProjMapping = Mapping[str, Union[str, List[str], Set[str]]]
-# Project specific reviewers.
-PROJ_REVIEWERS: ProjMapping = {
+# Upgrades of Rust futures-* crates should be reviewed/tested together,
+# not split to radomly different people. In additional to one rust-dev
+# reviewer, we need one crosvm-dev reviewer to check the impact to crosvm.
+RUST_FUTURES_REVIEWERS: Set[str] = {'[email protected]', '[email protected]'}
+
+# Rust crate owners (reviewers).
+RUST_CRATE_OWNERS: ProjMapping = {
'rust/crates/aho-corasick': '[email protected]',
'rust/crates/anyhow': '[email protected]',
- # more could be added later
+ 'rust/crates/bindgen': '[email protected]',
+ 'rust/crates/bytes': '[email protected]',
+ 'rust/crates/cexpr': '[email protected]',
+ 'rust/crates/cfg-if': '[email protected]',
+ 'rust/crates/clang-sys': '[email protected]',
+ 'rust/crates/futures': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-channel': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-core': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-executor': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-io': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-macro': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-sink': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-task': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/futures-util': RUST_FUTURES_REVIEWERS,
+ 'rust/crates/glob': '[email protected]',
+ 'rust/crates/lazycell': '[email protected]',
+ 'rust/crates/lazy_static': '[email protected]',
+ 'rust/crates/libloading': '[email protected]',
+ 'rust/crates/log': '[email protected]',
+ 'rust/crates/nom': '[email protected]',
+ 'rust/crates/once_cell': '[email protected]',
+ 'rust/crates/peeking_take_while': '[email protected]',
+ 'rust/crates/pin-project': '[email protected]',
+ 'rust/crates/pin-project-internal': '[email protected]',
+ 'rust/crates/protobuf': '[email protected]',
+ 'rust/crates/protobuf-codegen': '[email protected]',
+ 'rust/crates/regex': '[email protected]',
+ 'rust/crates/regex-syntax': '[email protected]',
+ 'rust/crates/rustc-hash': '[email protected]',
+ 'rust/crates/shlex': '[email protected]',
+ 'rust/crates/thread_local': '[email protected]',
+ 'rust/crates/which': '[email protected]',
+ # more rust crate owners could be added later
}
+PROJ_REVIEWERS: ProjMapping = {
+ # define non-rust project reviewers here
+}
+
+# Combine all roject reviewers.
+PROJ_REVIEWERS.update(RUST_CRATE_OWNERS)
+
+# Estimated number of rust projects, not the actual number.
+# It is only used to make random distribution "fair" among RUST_REVIEWERS.
+# It should not be too small, to spread nicely to multiple reviewers.
+# It should be larger or equal to len(RUST_CRATES_OWNERS).
+NUM_RUST_PROJECTS = 90
+
# Reviewers for external/rust/crates projects not found in PROJ_REVIEWER.
-RUST_REVIEWERS: List[str] = [
- '[email protected]',
- '[email protected]',
- '[email protected]',
- '[email protected]',
- '[email protected]',
- '[email protected]',
-]
+# Each person has a quota, the number of projects to review.
+# Sum of these numbers should be greater or equal to NUM_RUST_PROJECTS
+# to avoid error cases in the creation of RUST_REVIEWER_LIST.
+RUST_REVIEWERS: Mapping[str, int] = {
+ '[email protected]': 15,
+ '[email protected]': 15,
+ '[email protected]': 15,
+ '[email protected]': 15,
+ '[email protected]': 15,
+ '[email protected]': 15,
+ # If a Rust reviewer needs to take a vacation, comment out the line,
+ # and distribute the quota to other reviewers.
+}
+
+
+def add_proj_count(projects: Mapping[str, float], reviewer: str, n: float):
+ """Add n to the number of projects owned by the reviewer."""
+ if reviewer in projects:
+ projects[reviewer] += n
+ else:
+ projects[reviewer] = n
+
+
+# Random Rust reviewers are selected from RUST_REVIEWER_LIST,
+# which is created from RUST_REVIEWERS and PROJ_REVIEWERS.
+# A person P in RUST_REVIEWERS will occur in the RUST_REVIEWER_LIST N times,
+# if N = RUST_REVIEWERS[P] - (number of projects owned by P in PROJ_REVIEWERS)
+# is greater than 0. N is rounded up by math.ceil.
+def create_rust_reviewer_list() -> List[str]:
+ """Create a list of duplicated reviewers for weighted random selection."""
+ # Count number of projects owned by each reviewer.
+ rust_reviewers = set(RUST_REVIEWERS.keys())
+ projects = {} # map from owner to number of owned projects
+ for value in PROJ_REVIEWERS.values():
+ if isinstance(value, str): # single reviewer for a project
+ add_proj_count(projects, value, 1)
+ continue
+ # multiple reviewers share one project, count only rust_reviewers
+ reviewers = set(filter(lambda x: x in rust_reviewers, value))
+ if reviewers:
+ count = 1.0 / len(reviewers) # shared among all reviewers
+ for name in reviewers:
+ add_proj_count(projects, name, count)
+ result = []
+ for (x, n) in RUST_REVIEWERS.items():
+ if x in projects: # reduce x's quota by the number of assigned ones
+ n = n - projects[x]
+ if n > 0:
+ result.extend([x] * math.ceil(n))
+ if result:
+ return result
+ # Something was wrong or quotas were too small so that nobody
+ # was selected from the RUST_REVIEWERS. Select everyone!!
+ return list(RUST_REVIEWERS.keys())
+
+
+RUST_REVIEWER_LIST: List[str] = create_rust_reviewer_list()
def find_reviewers(proj_path: str) -> str:
@@ -55,10 +155,10 @@
reviewers = PROJ_REVIEWERS[proj_path]
if isinstance(reviewers, List): # pick any one reviewer
return 'r=' + random.choice(reviewers)
- if isinstance(reviewers, Set): # add all reviewers
- return ','.join(map(lambda x: 'r=' + x, reviewers))
+ if isinstance(reviewers, Set): # add all reviewers in sorted order
+ return ','.join(map(lambda x: 'r=' + x, sorted(reviewers)))
# reviewers must be a string
return 'r=' + reviewers
if proj_path.startswith('rust/crates/'):
- return 'r=' + random.choice(RUST_REVIEWERS)
+ return 'r=' + random.choice(RUST_REVIEWER_LIST)
return ''
