From 309127ac7a4fda6d8ccf2118aec33d659015b2ba Mon Sep 17 00:00:00 2001 From: driazati Date: Wed, 9 Feb 2022 19:54:25 -0800 Subject: [PATCH 1/2] [ci] Check more events before pinging reviewers This was missing some events before (reviews without comments, PR updated from a draft -> ready for review) so these were being ignored when finding the latest event. This PR adds them and restructures the code a bit to make it more clear what is happening for each PR. This addresses some of the issues from #9983 --- tests/scripts/ping_reviewers.py | 74 +++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 22 deletions(-) diff --git a/tests/scripts/ping_reviewers.py b/tests/scripts/ping_reviewers.py index 50e67824888c..5c881594cf30 100755 --- a/tests/scripts/ping_reviewers.py +++ b/tests/scripts/ping_reviewers.py @@ -32,6 +32,7 @@ def prs_query(user: str, repo: str, cursor: str = None): after = "" if cursor is not None: after = f', before:"{cursor}"' + time_keys = "createdAt\nupdatedAt\nlastEditedAt\npublishedAt" return f""" {{ repository(name: "{repo}", owner: "{user}") {{ @@ -43,27 +44,29 @@ def prs_query(user: str, repo: str, cursor: str = None): number url body + {time_keys} isDraft author {{ login }} reviews(last:100) {{ nodes {{ + {time_keys} + bodyText author {{ login }} comments(last:100) {{ nodes {{ - updatedAt + {time_keys} bodyText }} }} }} }} - publishedAt comments(last:100) {{ nodes {{ authorAssociation bodyText - updatedAt + {time_keys} author {{ login }} @@ -92,8 +95,25 @@ def find_reviewers(body: str) -> List[str]: def check_pr(pr, wait_time, now): - published_at = datetime.datetime.strptime(pr["publishedAt"], GIT_DATE_FORMAT) - last_action = published_at + last_action = None + + def update_last(new_time, description): + if isinstance(new_time, str): + new_time = datetime.datetime.strptime(new_time, GIT_DATE_FORMAT) + if new_time is None: + print(f" time not found: {description}") + return + nonlocal last_action + if last_action is None or new_time > last_action[0]: + last_action = (new_time, description) + + def check_obj(obj, name): + update_last(obj["publishedAt"], f"{name} publishedAt: {obj}") + update_last(obj["updatedAt"], f"{name} updatedAt: {obj}") + update_last(obj["lastEditedAt"], f"{name} lastEditedAt: {obj}") + update_last(obj["createdAt"], f"{name} lastEditedAt: {obj}") + + check_obj(pr, "pr") # GitHub counts comments left as part of a review separately than standalone # comments @@ -101,17 +121,16 @@ def check_pr(pr, wait_time, now): review_comments = [] for review in reviews: review_comments += review["comments"]["nodes"] + check_obj(review, "review") # Collate all comments comments = pr["comments"]["nodes"] + review_comments # Find the last date of any comment for comment in comments: - commented_at = datetime.datetime.strptime(comment["updatedAt"], GIT_DATE_FORMAT) - if commented_at > last_action: - last_action = commented_at + check_obj(comment, "comment") - time_since_last_action = now - last_action + time_since_last_action = now - last_action[0] # Find reviewers in the PR's body pr_body_reviewers = find_reviewers(pr["body"]) @@ -128,17 +147,19 @@ def check_pr(pr, wait_time, now): if time_since_last_action > wait_time: print( - " Pinging reviewers", + " Pinging reviewers", reviewers, "on", pr["url"], "since it has been", time_since_last_action, - "since anything happened on that PR", + f"since anything happened on that PR (last action: {last_action[1]})", ) return reviewers else: - print(f"Not pinging PR {pr['number']}") + print( + f" Not pinging PR {pr['number']} since it has been only {time_since_last_action} since the last action: {last_action[1]}" + ) return None @@ -206,18 +227,27 @@ def ping_reviewers(pr, reviewers): prs = r["data"]["repository"]["pullRequests"]["nodes"] # Don't look at draft PRs at all - prs = [pr for pr in prs if not pr["isDraft"]] - - # Don't look at super old PRs - prs = [pr for pr in prs if pr["number"] > cutoff_pr_number] - - # [slow rollout] - prs = [pr for pr in prs if pr["author"]["login"] in author_allowlist] - - print(f"Checking {len(prs)} PRs: {[pr['number'] for pr in prs]}") + prs_to_check = [] + for pr in prs: + if pr["isDraft"]: + print(f"Skipping #{pr['number']} since it's a draft") + elif pr["number"] <= cutoff_pr_number: + print( + f"Skipping #{pr['number']} since it's too old ({pr['number']} <= {cutoff_pr_number})" + ) + elif pr["author"]["login"] not in author_allowlist: + # [slow rollout] + print( + f"Skipping #{pr['number']} since author {pr['author']['login']} is not in allowlist: {author_allowlist}" + ) + else: + print(f"Checking #{pr['number']}, {author_allowlist}") + prs_to_check.append(pr) + + print(f"Summary: Checking {len(prs_to_check)} of {len(prs)} fetched") # Ping reviewers on each PR in the response if necessary - for pr in prs: + for pr in prs_to_check: print("Checking", pr["url"]) reviewers = check_pr(pr, wait_time, now) if reviewers is not None and not args.dry_run: From ef80da377c6b2d74d6c197ef318f03c1ef07a145 Mon Sep 17 00:00:00 2001 From: driazati Date: Thu, 10 Feb 2022 15:21:54 -0800 Subject: [PATCH 2/2] fix tests --- tests/python/unittest/test_ci.py | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/tests/python/unittest/test_ci.py b/tests/python/unittest/test_ci.py index f5183d79b768..4e0690cf0933 100644 --- a/tests/python/unittest/test_ci.py +++ b/tests/python/unittest/test_ci.py @@ -206,11 +206,20 @@ def run(pr, check): assert check in proc.stdout + def all_time_keys(time): + return { + "updatedAt": time, + "lastEditedAt": time, + "createdAt": time, + "publishedAt": time, + } + run( { "isDraft": True, + "number": 2, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) run( @@ -218,7 +227,7 @@ def run(pr, check): "isDraft": False, "number": 2, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) run( @@ -229,7 +238,7 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": {"nodes": []}, }, "Pinging reviewers ['someone'] on https://github.com/apache/tvm/pull/123", @@ -244,14 +253,14 @@ def run(pr, check): "isDraft": False, "author": {"login": "user2"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-19T17:54:19Z", "bodyText": "abc"}, + {**all_time_keys("2022-01-19T17:54:19Z"), "bodyText": "abc"}, ] }, }, - "Checking 0 PRs", + "Checking 0 of 1 fetched", ) # Old comment, ping @@ -263,10 +272,13 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-19T17:54:19Z", "bodyText": "abc"}, + { + **all_time_keys("2022-01-18T17:54:19Z"), + "bodyText": "abc", + }, ] }, }, @@ -282,10 +294,10 @@ def run(pr, check): "isDraft": False, "author": {"login": "user"}, "reviews": {"nodes": []}, - "publishedAt": "2022-01-18T17:54:19Z", + **all_time_keys("2022-01-18T17:54:19Z"), "comments": { "nodes": [ - {"updatedAt": "2022-01-27T17:54:19Z", "bodyText": "abc"}, + {**all_time_keys("2022-01-27T17:54:19Z"), "bodyText": "abc"}, ] }, },