From 22ab288d753fde6aeb663025870da596f8314ca5 Mon Sep 17 00:00:00 2001 From: Ivan Razumov Date: Mon, 6 Nov 2023 09:06:22 +0100 Subject: [PATCH 1/5] Allow manual override to PR test results --- githublabels.py | 5 +++ process_pr.py | 81 ++++++++++++++++++++++++++++++------------------- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/githublabels.py b/githublabels.py index 2aff0df90f95..6d7c7869ed76 100644 --- a/githublabels.py +++ b/githublabels.py @@ -42,6 +42,8 @@ def get_dpg_pog(): continue TYPE_COMMANDS[lab] = [LABEL_COLORS["doc"], lab, "mtype"] +TEST_IGNORE_REASON = ["manual-override", "ib-failure", "external-failure"] + COMMON_LABELS = { "tests-started": LABEL_COLORS["hold"], "fully-signed": LABEL_COLORS["approved"], @@ -60,6 +62,9 @@ def get_dpg_pog(): for lab in TYPE_COMMANDS: COMMON_LABELS[lab] = TYPE_COMMANDS[lab][0] +for reason in TEST_IGNORE_REASON: + COMMON_LABELS["tests-" + reason] = LABEL_COLORS["info"] + COMPARISON_LABELS = { "comparison-notrun": "bfe5bf", "comparison-available": LABEL_TYPES["approved"], diff --git a/process_pr.py b/process_pr.py index 96821a717994..97702c0c5a73 100644 --- a/process_pr.py +++ b/process_pr.py @@ -22,7 +22,7 @@ CREATE_REPO, ) from cms_static import BACKPORT_STR, GH_CMSSW_ORGANIZATION, CMSBOT_NO_NOTIFY_MSG -from githublabels import TYPE_COMMANDS +from githublabels import TYPE_COMMANDS, TEST_IGNORE_REASON from repo_config import GH_REPO_ORGANIZATION import re, time from datetime import datetime @@ -121,6 +121,10 @@ def format(s, **kwds): REGEX_TEST_ABORT = re.compile( "^\s*((@|)cmsbuild\s*[,]*\s+|)(please\s*[,]*\s+|)abort(\s+test|)$", re.I ) +REGEX_TEST_IGNORE = re.compile( + r"^\s*(?:(?:@|)cmsbuild\s*[,]*\s+|)(?:please\s*[,]*\s+|)ignore\s+tests-rejected\s+(?:with|)([a-z -]+)$", + re.I, +) TEST_WAIT_GAP = 720 ALL_CHECK_FUNCTIONS = None EXTRA_RELVALS_TESTS = ["threading", "gpu", "high-stats", "nano"] @@ -1110,6 +1114,36 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F if m.group(2): code_check_apply_patch = True + # Check L2 signoff for users in this PR signing categories + if [x for x in commenter_categories if x in signing_categories]: + ctype = "" + selected_cats = [] + if re.match("^([+]1|approve[d]?|sign|signed)$", first_line, re.I): + ctype = "+1" + selected_cats = commenter_categories + elif re.match("^([-]1|reject|rejected)$", first_line, re.I): + ctype = "-1" + selected_cats = commenter_categories + elif re.match("^[+-][a-z][a-z0-9-]+$", first_line, re.I): + category_name = first_line[1:].lower() + if category_name in commenter_categories: + ctype = first_line[0] + "1" + selected_cats = [category_name] + if ctype == "+1": + for sign in selected_cats: + signatures[sign] = "approved" + if (test_comment is None) and ( + (repository in auto_test_repo) or ("*" in auto_test_repo) + ): + test_comment = comment + if sign == "orp": + mustClose = False + elif ctype == "-1": + for sign in selected_cats: + signatures[sign] = "rejected" + if sign == "orp": + mustClose = False + # Ignore all other messages which are before last commit. if issue.pull_request and (comment.created_at < last_commit_date): continue @@ -1237,37 +1271,16 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F abort_test = comment test_comment = None signatures["tests"] = "pending" + elif REGEX_TEST_IGNORE.match(first_line) and (signatures["tests"] == "rejected"): + reason = REGEX_TEST_IGNORE.match(first_line)[1].strip() + if reason not in TEST_IGNORE_REASON: + print("Invalid ignore reason:", reason) + set_comment_emoji(comment.id, repository, "-1") + reason = "" - # Check L2 signoff for users in this PR signing categories - if [x for x in commenter_categories if x in signing_categories]: - ctype = "" - selected_cats = [] - if re.match("^([+]1|approve[d]?|sign|signed)$", first_line, re.I): - ctype = "+1" - selected_cats = commenter_categories - elif re.match("^([-]1|reject|rejected)$", first_line, re.I): - ctype = "-1" - selected_cats = commenter_categories - elif re.match("^[+-][a-z][a-z0-9-]+$", first_line, re.I): - category_name = first_line[1:].lower() - if category_name in commenter_categories: - ctype = first_line[0] + "1" - selected_cats = [category_name] - if ctype == "+1": - for sign in selected_cats: - signatures[sign] = "approved" - if (test_comment is None) and ( - (repository in auto_test_repo) or ("*" in auto_test_repo) - ): - test_comment = comment - if sign == "orp": - mustClose = False - elif ctype == "-1": - for sign in selected_cats: - signatures[sign] = "rejected" - if sign == "orp": - mustClose = False - continue + if reason: + signatures["tests"] = reason + set_comment_emoji(comment.id, repository) # end of parsing comments section @@ -1559,6 +1572,10 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F xlabs = ["backport", "urgent", "backport-ok", "compilation-warnings"] for lab in TYPE_COMMANDS: xlabs.append(lab) + + if set(labels).intersection(set("tests-" + x for x in TEST_IGNORE_REASON)): + labels.append("tests-approved") + missingApprovals = [ x for x in labels @@ -1732,6 +1749,8 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F requiresTestMessage = " (tests are also fine)" elif "tests-rejected" in labels: requiresTestMessage = " (but tests are reportedly failing)" + elif labels.intersection(set("tests-" + x for x in TEST_IGNORE_REASON)): + requiresTestMessage = " (test failures were overridden)" autoMergeMsg = "" if ( From 615c2f909b4234f2be0d7ef4d284203544070f18 Mon Sep 17 00:00:00 2001 From: Ivan Razumov Date: Mon, 6 Nov 2023 10:51:46 +0100 Subject: [PATCH 2/5] Changes from code review --- process_pr.py | 78 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 29 deletions(-) diff --git a/process_pr.py b/process_pr.py index 97702c0c5a73..1ae31191296a 100644 --- a/process_pr.py +++ b/process_pr.py @@ -1114,35 +1114,25 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F if m.group(2): code_check_apply_patch = True - # Check L2 signoff for users in this PR signing categories - if [x for x in commenter_categories if x in signing_categories]: - ctype = "" - selected_cats = [] - if re.match("^([+]1|approve[d]?|sign|signed)$", first_line, re.I): - ctype = "+1" - selected_cats = commenter_categories - elif re.match("^([-]1|reject|rejected)$", first_line, re.I): - ctype = "-1" - selected_cats = commenter_categories - elif re.match("^[+-][a-z][a-z0-9-]+$", first_line, re.I): - category_name = first_line[1:].lower() - if category_name in commenter_categories: - ctype = first_line[0] + "1" - selected_cats = [category_name] - if ctype == "+1": - for sign in selected_cats: - signatures[sign] = "approved" - if (test_comment is None) and ( - (repository in auto_test_repo) or ("*" in auto_test_repo) - ): - test_comment = comment - if sign == "orp": - mustClose = False - elif ctype == "-1": - for sign in selected_cats: - signatures[sign] = "rejected" - if sign == "orp": - mustClose = False + # Set "tests" signature state earlier + if commenter == cmsbuild_user: + if not issue.pull_request and not push_test_issue: + continue + + if re.match(FAILED_TESTS_MSG, first_line) or re.match( + IGNORING_TESTS_MSG, first_line + ): + signatures["tests"] = "pending" + elif re.match(TRIGERING_TESTS_MSG, first_line) or re.match( + TRIGERING_TESTS_MSG1, first_line + ): + signatures["tests"] = "started" + elif "+1" in first_line: + signatures["tests"] = "approved" + elif "-1" in first_line: + signatures["tests"] = "rejected" + else: + signatures["tests"] = "pending" # Ignore all other messages which are before last commit. if issue.pull_request and (comment.created_at < last_commit_date): @@ -1282,6 +1272,36 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F signatures["tests"] = reason set_comment_emoji(comment.id, repository) + # Check L2 signoff for users in this PR signing categories + if [x for x in commenter_categories if x in signing_categories]: + ctype = "" + selected_cats = [] + if re.match("^([+]1|approve[d]?|sign|signed)$", first_line, re.I): + ctype = "+1" + selected_cats = commenter_categories + elif re.match("^([-]1|reject|rejected)$", first_line, re.I): + ctype = "-1" + selected_cats = commenter_categories + elif re.match("^[+-][a-z][a-z0-9-]+$", first_line, re.I): + category_name = first_line[1:].lower() + if category_name in commenter_categories: + ctype = first_line[0] + "1" + selected_cats = [category_name] + if ctype == "+1": + for sign in selected_cats: + signatures[sign] = "approved" + if (test_comment is None) and ( + (repository in auto_test_repo) or ("*" in auto_test_repo) + ): + test_comment = comment + if sign == "orp": + mustClose = False + elif ctype == "-1": + for sign in selected_cats: + signatures[sign] = "rejected" + if sign == "orp": + mustClose = False + # end of parsing comments section if push_test_issue: From c98551e7736ff0c5e2722d821d861522f17392be Mon Sep 17 00:00:00 2001 From: Ivan Razumov Date: Mon, 6 Nov 2023 10:54:59 +0100 Subject: [PATCH 3/5] Code-style --- process_pr.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/process_pr.py b/process_pr.py index 1ae31191296a..f103045822e8 100644 --- a/process_pr.py +++ b/process_pr.py @@ -1119,9 +1119,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F if not issue.pull_request and not push_test_issue: continue - if re.match(FAILED_TESTS_MSG, first_line) or re.match( - IGNORING_TESTS_MSG, first_line - ): + if re.match(FAILED_TESTS_MSG, first_line) or re.match(IGNORING_TESTS_MSG, first_line): signatures["tests"] = "pending" elif re.match(TRIGERING_TESTS_MSG, first_line) or re.match( TRIGERING_TESTS_MSG1, first_line From ff41a6536c621196cca6769e02559dfc09885d46 Mon Sep 17 00:00:00 2001 From: Ivan Razumov Date: Mon, 6 Nov 2023 11:26:49 +0100 Subject: [PATCH 4/5] Always process request to override failures, but delay changing labels --- process_pr.py | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/process_pr.py b/process_pr.py index f103045822e8..e34e9a9bb163 100644 --- a/process_pr.py +++ b/process_pr.py @@ -970,6 +970,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F test_params_msg = "" test_params_comment = None code_check_apply_patch = False + override_tests_failure = None # start of parsing comments section for c in issue.get_comments(): @@ -1114,24 +1115,6 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F if m.group(2): code_check_apply_patch = True - # Set "tests" signature state earlier - if commenter == cmsbuild_user: - if not issue.pull_request and not push_test_issue: - continue - - if re.match(FAILED_TESTS_MSG, first_line) or re.match(IGNORING_TESTS_MSG, first_line): - signatures["tests"] = "pending" - elif re.match(TRIGERING_TESTS_MSG, first_line) or re.match( - TRIGERING_TESTS_MSG1, first_line - ): - signatures["tests"] = "started" - elif "+1" in first_line: - signatures["tests"] = "approved" - elif "-1" in first_line: - signatures["tests"] = "rejected" - else: - signatures["tests"] = "pending" - # Ignore all other messages which are before last commit. if issue.pull_request and (comment.created_at < last_commit_date): continue @@ -1259,7 +1242,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F abort_test = comment test_comment = None signatures["tests"] = "pending" - elif REGEX_TEST_IGNORE.match(first_line) and (signatures["tests"] == "rejected"): + elif REGEX_TEST_IGNORE.match(first_line): reason = REGEX_TEST_IGNORE.match(first_line)[1].strip() if reason not in TEST_IGNORE_REASON: print("Invalid ignore reason:", reason) @@ -1267,7 +1250,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F reason = "" if reason: - signatures["tests"] = reason + override_tests_failure = reason set_comment_emoji(comment.id, repository) # Check L2 signoff for users in this PR signing categories @@ -1523,7 +1506,6 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F print( "DryRun: Setting status Waiting for authorized user to issue the test command." ) - # Labels coming from signature. labels = [] for cat in signing_categories: @@ -1532,6 +1514,13 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F l = cat + "-" + signatures[cat] labels.append(l) + # Process "ignore tests failure" + if override_tests_failure and signatures["tests"] == "rejected": + labels.remove("tests-rejected") + labels.append("tests-approved") + labels.append("tests-" + override_tests_failure) + signatures["tests"] = "approved" + if not issue.pull_request and len(signing_categories) == 0: labels.append("pending-assignment") if is_hold: From 7dd4b9395aa86d6f9a7ff0836257199476953e94 Mon Sep 17 00:00:00 2001 From: Ivan Razumov Date: Tue, 7 Nov 2023 11:05:27 +0100 Subject: [PATCH 5/5] Changes from review --- process_pr.py | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/process_pr.py b/process_pr.py index e34e9a9bb163..9a3867ceecb9 100644 --- a/process_pr.py +++ b/process_pr.py @@ -1246,12 +1246,14 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F reason = REGEX_TEST_IGNORE.match(first_line)[1].strip() if reason not in TEST_IGNORE_REASON: print("Invalid ignore reason:", reason) - set_comment_emoji(comment.id, repository, "-1") + if not dryRun: + set_comment_emoji(comment.id, repository, "-1") reason = "" if reason: override_tests_failure = reason - set_comment_emoji(comment.id, repository) + if not dryRun: + set_comment_emoji(comment.id, repository) # Check L2 signoff for users in this PR signing categories if [x for x in commenter_categories if x in signing_categories]: @@ -1508,19 +1510,18 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F ) # Labels coming from signature. labels = [] + + # Process "ignore tests failure" + if override_tests_failure and signatures["tests"] == "rejected": + signatures["tests"] = "approved" + labels.append("tests-" + override_tests_failure) + for cat in signing_categories: l = cat + "-pending" if cat in signatures: l = cat + "-" + signatures[cat] labels.append(l) - # Process "ignore tests failure" - if override_tests_failure and signatures["tests"] == "rejected": - labels.remove("tests-rejected") - labels.append("tests-approved") - labels.append("tests-" + override_tests_failure) - signatures["tests"] = "approved" - if not issue.pull_request and len(signing_categories) == 0: labels.append("pending-assignment") if is_hold: @@ -1580,9 +1581,6 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F for lab in TYPE_COMMANDS: xlabs.append(lab) - if set(labels).intersection(set("tests-" + x for x in TEST_IGNORE_REASON)): - labels.append("tests-approved") - missingApprovals = [ x for x in labels @@ -1752,12 +1750,12 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F # Do not complain about tests requiresTestMessage = " after it passes the integration tests" - if "tests-approved" in labels: + if labels.intersection(set("tests-" + x for x in TEST_IGNORE_REASON)): + requiresTestMessage = " (test failures were overridden)" + elif "tests-approved" in labels: requiresTestMessage = " (tests are also fine)" elif "tests-rejected" in labels: requiresTestMessage = " (but tests are reportedly failing)" - elif labels.intersection(set("tests-" + x for x in TEST_IGNORE_REASON)): - requiresTestMessage = " (test failures were overridden)" autoMergeMsg = "" if (