diff --git a/.github/workflows/test-process-pr.yaml b/.github/workflows/test-process-pr.yaml new file mode 100644 index 000000000000..1e8bfa093fa5 --- /dev/null +++ b/.github/workflows/test-process-pr.yaml @@ -0,0 +1,30 @@ +name: Test changes to process_pr.py + +on: + push: + paths: + - process_pr.py + +jobs: + build: + runs-on: ubuntu-20.04 + strategy: + matrix: + python-version: ["3.6", "3.9", "3.10", "3.11"] + + steps: + - uses: MathRobin/timezone-action@v1.1 + with: + timezoneLinux: 'Europe/Paris' + - uses: actions/checkout@v3 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v4 + with: + python-version: ${{ matrix.python-version }} + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r tests/test-requirements.txt + - name: Test with pytest + run: | + pytest -v -s tests/test_process_pr.py --auth_with_token diff --git a/process_pr.py b/process_pr.py index 24245335b11e..c24ef4640f71 100644 --- a/process_pr.py +++ b/process_pr.py @@ -1,3 +1,5 @@ +import copy + from categories import ( CMSSW_L2, CMSSW_L1, @@ -43,6 +45,7 @@ from _py2with3compatibility import run_cmd from json import dumps, dump, load, loads import yaml +import sys try: from yaml import CLoader as Loader, CDumper as Dumper @@ -237,7 +240,7 @@ def read_bot_cache(data): res = loads_maybe_decompress(data) for k, v in BOT_CACHE_TEMPLATE.items(): if k not in res: - res[k] = v + res[k] = copy.deepcopy(v) collect_commit_cache(res) return res @@ -439,6 +442,21 @@ def modify_comment(comment, match, replace, dryRun): return 0 +# github_utils.set_issue_emoji -> https://github.com/PyGithub/PyGithub/blob/v1.56/github/Issue.py#L569 +# github_utils.set_comment_emoji -> https://github.com/PyGithub/PyGithub/blob/v1.56/github/IssueComment.py#L149 +# github_utils.delete_issue_emoji, github_utils.delete_comment_emoji -> https://github.com/PyGithub/PyGithub/blob/v1.56/github/Reaction.py#L71 +def set_emoji(comment, emoji, reset_other): + if reset_other: + for e in comment.get_reactions(): + login = e.user.login.encode("ascii", "ignore") + if sys.version_info[0] == 3: + login = login.decode() + if login == GH_USER and e.content != emoji: + e.delete() + + comment.create_reaction(emoji) + + def set_comment_emoji_cache(dryRun, bot_cache, comment, repository, emoji="+1", reset_other=True): if dryRun: return @@ -448,10 +466,8 @@ def set_comment_emoji_cache(dryRun, bot_cache, comment, repository, emoji="+1", or (bot_cache["emoji"][comment_id] != emoji) or (comment.reactions[emoji] == 0) ): - if "Issue.Issue" in str(type(comment)): - set_issue_emoji(comment.number, repository, emoji=emoji, reset_other=reset_other) - else: - set_comment_emoji(comment.id, repository, emoji=emoji, reset_other=reset_other) + + set_emoji(comment, emoji, reset_other) bot_cache["emoji"][comment_id] = emoji return @@ -462,11 +478,9 @@ def has_user_emoji(bot_cache, comment, repository, emoji, user): if (comment_id in bot_cache["emoji"]) and (comment.reactions[emoji] > 0): e = bot_cache["emoji"][comment_id] else: - emojis = None - if "Issue.Issue" in str(type(comment)): - emojis = get_issue_emojis(comment.number, repository) - else: - emojis = get_comment_emojis(comment.id, repository) + # github_utils.get_issue_emojis -> https://github.com/PyGithub/PyGithub/blob/v1.56/github/Issue.py#L556 + # github_utils.get_comment_emojis -> https://github.com/PyGithub/PyGithub/blob/v1.56/github/IssueComment.py#L135 + emojis = comment.get_reactions() for x in emojis: if x["user"]["login"].encode("ascii", "ignore").decode() == user: e = x["content"] @@ -831,10 +845,25 @@ def add_nonblocking_labels(chg_files, extra_labels): return +# TODO: remove once we update pygithub +def get_commit_files(repo_, commit): + return (x["filename"] for x in get_commit(repo_.full_name, commit.sha)["files"]) + + +def on_labels_changed(added_labels, removed_labels): + # Placeholder function replaced during testing + pass + + def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=False): global L2_DATA if (not force) and ignore_issue(repo_config, repo, issue): return + + if "pytest" in sys.modules: + test_mode = True + else: + test_mode = False gh_user_char = "@" if not notify_user(issue): gh_user_char = "" @@ -1171,7 +1200,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F # Make sure bot cache has the needed keys for k, v in BOT_CACHE_TEMPLATE.items(): if k not in bot_cache: - bot_cache[k] = v + bot_cache[k] = copy.deepcopy(v) for comment in all_comments: ack_comment = comment @@ -1531,7 +1560,9 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F "{2}, to re-enable processing of this PR, you can write `+commit-count` in a comment. Thanks.".format( pr.commits, TOO_MANY_COMMITS_WARN_THRESHOLD, - ", ".join([gh_user_char + name for name in CMSSW_ISSUES_TRACKERS]), + ", ".join( + sorted([gh_user_char + name for name in CMSSW_ISSUES_TRACKERS]) + ), ) ) else: @@ -1628,7 +1659,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F bot_cache["commits"][commit.sha]["files"] = [] else: bot_cache["commits"][commit.sha]["files"] = sorted( - x["filename"] for x in get_commit(repo.full_name, commit.sha)["files"] + get_commit_files(repo, commit) ) elif len(commit.parents) > 1: @@ -1929,7 +1960,10 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F + "/pr-result" ) print("PR Result:", url) - e, o = run_cmd("curl -k -s -L --max-time 60 %s" % url) + if test_mode: + e, o = "", "ook" + else: + e, o = run_cmd("curl -k -s -L --max-time 60 %s" % url) if e: print(o) raise Exception("System-error: unable to get PR result") @@ -2101,6 +2135,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F print("Blockers:", blockers) print("Changed Labels: added", labels - old_labels, "removed", old_labels - labels) + on_labels_changed(labels - old_labels, old_labels - labels) if old_labels == labels: print("Labels unchanged.") elif not dryRunOrig: @@ -2130,7 +2165,7 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F backport_msg = "" if backport_pr_num: backport_msg = "%s%s\n" % (BACKPORT_STR, backport_pr_num) - l2s = ", ".join([gh_user_char + name for name in CMSSW_ISSUES_TRACKERS]) + l2s = ", ".join(gh_user_char + name for name in sorted(CMSSW_ISSUES_TRACKERS)) issueMessage = format( "%(msgPrefix)s %(gh_user_char)s%(user)s.\n\n" "%(l2s)s can you please review it and eventually sign/assign?" @@ -2306,23 +2341,28 @@ def process_pr(repo_config, gh, repo, issue, dryRun, cmsbuild_user=None, force=F print("DRY-RUN: not posting comment", messageFullySigned) unsigned = [commit_sha for (commit_sha, v) in list(signatures.items()) if v == "pending"] - missing_notifications = [ - gh_user_char + name - for name, l2_categories in list(CMSSW_L2.items()) - for signature in signing_categories - if signature in l2_categories and signature in unsigned and signature not in ["orp"] - ] + missing_notifications = sorted( + list( + set( + gh_user_char + name + for name, l2_categories in list(CMSSW_L2.items()) + for signature in signing_categories + if signature in l2_categories + and signature in unsigned + and signature not in ["orp"] + ) + ) + ) - missing_notifications = set(missing_notifications) # Construct message for the watchers watchersMsg = "" if watchers: watchersMsg = format( "%(watchers)s this is something you requested to" " watch as well.\n", - watchers=", ".join(watchers), + watchers=", ".join(sorted(watchers)), ) # Construct message for the release managers. - managers = ", ".join([gh_user_char + x for x in releaseManagers]) + managers = ", ".join([gh_user_char + x for x in sorted(releaseManagers)]) releaseManagersMsg = "" if releaseManagers: diff --git a/tests/PRActionData/TestProcessPr.test_abort.json b/tests/PRActionData/TestProcessPr.test_abort.json index 67af22412aa6..5a0c691d6979 100644 --- a/tests/PRActionData/TestProcessPr.test_abort.json +++ b/tests/PRActionData/TestProcessPr.test_abort.json @@ -41,7 +41,8 @@ "2056796593": "+1", "2056801055": "+1", "2056820593": "+1", - "2056903278": "+1" + "2056903278": "+1", + "2056930228": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -132,6 +133,10 @@ "context": "bot/17/jenkins" } }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_assign.json b/tests/PRActionData/TestProcessPr.test_assign.json index 8582a519670e..512d9bb2bb6f 100644 --- a/tests/PRActionData/TestProcessPr.test_assign.json +++ b/tests/PRActionData/TestProcessPr.test_assign.json @@ -37,7 +37,8 @@ "2049536626": "+1", "2056736344": "+1", "2056739513": "+1", - "2056740892": "+1" + "2056740892": "+1", + "2056796593": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -92,6 +93,10 @@ "type": "remove-label", "data": [] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_clean_squash.json b/tests/PRActionData/TestProcessPr.test_clean_squash.json index 8d933df5c724..27d49f1c71ff 100644 --- a/tests/PRActionData/TestProcessPr.test_clean_squash.json +++ b/tests/PRActionData/TestProcessPr.test_clean_squash.json @@ -131,16 +131,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "35f9a4c06b006029da40ed8858e0dae4abd52cb3", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [ @@ -175,36 +165,8 @@ "context": "cms/17/code-checks" } }, - { - "type": "status", - "data": { - "commit": "35f9a4c06b006029da40ed8858e0dae4abd52cb3", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2056820593", - "description": "2056820593:{\"PULL_REQUESTS\": \"cms-sw/cms-bot#2134\", \"SKIP_TESTS\": \"header,static\"}", - "context": "bot/17/test_parameters" - } - }, - { - "type": "emoji", - "data": [ - 2056820593, - "+1", - true - ] - }, { "type": "edit-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "35f9a4c06b006029da40ed8858e0dae4abd52cb3", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2056966759", - "description": "Comment by iarspider at 2024-04-15 14:14:24 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_close.json b/tests/PRActionData/TestProcessPr.test_close.json index dc869e19a25c..30a05265dbae 100644 --- a/tests/PRActionData/TestProcessPr.test_close.json +++ b/tests/PRActionData/TestProcessPr.test_close.json @@ -42,7 +42,8 @@ "2056801055": "+1", "2056820593": "+1", "2056903278": "+1", - "2056930228": "+1" + "2056930228": "+1", + "2056934192": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -110,6 +111,10 @@ "type": "close", "data": null }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_code_check_approved.json b/tests/PRActionData/TestProcessPr.test_code_check_approved.json index df407db0822b..edd9cebff424 100644 --- a/tests/PRActionData/TestProcessPr.test_code_check_approved.json +++ b/tests/PRActionData/TestProcessPr.test_code_check_approved.json @@ -48,6 +48,10 @@ "context": "cms/17/code-checks" } }, + { + "type": "create-comment", + "data": "A new Pull Request was created by @iarspider for master.\n\nIt involves the following packages:\n\n- Utilities/ReleaseScripts (**core**)\n\n\n@Dr15Jones, @iarspider, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.\n@wddgit this is something you requested to watch as well.\n@iarspider you are the release manager for this.\n\ncms-bot commands are listed here\n" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_dirty_squash.json b/tests/PRActionData/TestProcessPr.test_dirty_squash.json index fe4f83f7558c..388dcb68a6d5 100644 --- a/tests/PRActionData/TestProcessPr.test_dirty_squash.json +++ b/tests/PRActionData/TestProcessPr.test_dirty_squash.json @@ -138,16 +138,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [] @@ -178,36 +168,8 @@ "context": "cms/17/code-checks" } }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2056820593", - "description": "2056820593:{\"PULL_REQUESTS\": \"cms-sw/cms-bot#2134\", \"SKIP_TESTS\": \"header,static\"}", - "context": "bot/17/test_parameters" - } - }, - { - "type": "emoji", - "data": [ - 2056820593, - "+1", - true - ] - }, { "type": "edit-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2056966759", - "description": "Comment by iarspider at 2024-04-15 14:14:24 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_hold.json b/tests/PRActionData/TestProcessPr.test_hold.json index 1713a8ce5824..75967d51a503 100644 --- a/tests/PRActionData/TestProcessPr.test_hold.json +++ b/tests/PRActionData/TestProcessPr.test_hold.json @@ -35,7 +35,8 @@ "emoji": { "2049242908": "+1", "2049536626": "+1", - "2056736344": "+1" + "2056736344": "+1", + "2056739513": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -73,6 +74,10 @@ "type": "remove-label", "data": [] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_invalid_type.json b/tests/PRActionData/TestProcessPr.test_invalid_type.json index 55b17a40d264..a3de1e8b069b 100644 --- a/tests/PRActionData/TestProcessPr.test_invalid_type.json +++ b/tests/PRActionData/TestProcessPr.test_invalid_type.json @@ -44,7 +44,8 @@ "2056903278": "+1", "2056930228": "+1", "2056934192": "+1", - "2056935714": "+1" + "2056935714": "+1", + "2056946596": "-1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -124,6 +125,10 @@ "type": "remove-label", "data": [] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_many_commits_ok.json b/tests/PRActionData/TestProcessPr.test_many_commits_ok.json index 63911a358c21..419832b48a8c 100644 --- a/tests/PRActionData/TestProcessPr.test_many_commits_ok.json +++ b/tests/PRActionData/TestProcessPr.test_many_commits_ok.json @@ -7,16 +7,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "fd99bd146e87a00eb90eccef12f487ecc8f4c496", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/18/jenkins" - } - }, { "type": "add-label", "data": [ @@ -56,16 +46,6 @@ }, { "type": "create-comment", - "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "fd99bd146e87a00eb90eccef12f487ecc8f4c496", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/18#issuecomment-2058816831", - "description": "Comment by iarspider at 2024-04-16 10:59:44 UTC processed.", - "context": "bot/18/ack" - } + "data": "cms-bot internal usage" } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_new_pr.json b/tests/PRActionData/TestProcessPr.test_new_pr.json index 23dfd9e4753d..f29ad4eb6a91 100644 --- a/tests/PRActionData/TestProcessPr.test_new_pr.json +++ b/tests/PRActionData/TestProcessPr.test_new_pr.json @@ -6,16 +6,6 @@ "title": "CMSSW_14_1_X" } }, - { - "type": "status", - "data": { - "commit": "2a9454e30606b17e52000110972998326ce9e428", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [ @@ -55,15 +45,5 @@ { "type": "create-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "2a9454e30606b17e52000110972998326ce9e428", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17", - "description": "Comment by iarspider at 2024-03-27 12:22:45 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_partial_reset.json b/tests/PRActionData/TestProcessPr.test_partial_reset.json index e98b97e36b43..250fb6aabc5c 100644 --- a/tests/PRActionData/TestProcessPr.test_partial_reset.json +++ b/tests/PRActionData/TestProcessPr.test_partial_reset.json @@ -35,16 +35,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "e4d069b76c464274bf6e7d7cf9bac2153ed9a903", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [ @@ -85,15 +75,5 @@ { "type": "edit-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "e4d069b76c464274bf6e7d7cf9bac2153ed9a903", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2049247192", - "description": "Comment by iarspider at 2024-04-11 09:03:19 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_reopen.json b/tests/PRActionData/TestProcessPr.test_reopen.json index b2878525a48e..430c32382e78 100644 --- a/tests/PRActionData/TestProcessPr.test_reopen.json +++ b/tests/PRActionData/TestProcessPr.test_reopen.json @@ -43,7 +43,8 @@ "2056820593": "+1", "2056903278": "+1", "2056930228": "+1", - "2056934192": "+1" + "2056934192": "+1", + "2056935714": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -119,6 +120,10 @@ "type": "open", "data": null }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_reset_signature.json b/tests/PRActionData/TestProcessPr.test_reset_signature.json index 54c5a13ebb23..231c16726926 100644 --- a/tests/PRActionData/TestProcessPr.test_reset_signature.json +++ b/tests/PRActionData/TestProcessPr.test_reset_signature.json @@ -42,16 +42,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "79752f053efecad55dde17732259737e621a1f3f", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [ @@ -89,15 +79,5 @@ { "type": "edit-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "79752f053efecad55dde17732259737e621a1f3f", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2049247192", - "description": "Comment by iarspider at 2024-04-11 09:03:19 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_revert.json b/tests/PRActionData/TestProcessPr.test_revert.json index 26488cc26579..b3aea4cacaff 100644 --- a/tests/PRActionData/TestProcessPr.test_revert.json +++ b/tests/PRActionData/TestProcessPr.test_revert.json @@ -49,16 +49,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "dae848e38b8e387d7283a3e35818121487d9d76b", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [] @@ -94,15 +84,5 @@ { "type": "edit-comment", "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "dae848e38b8e387d7283a3e35818121487d9d76b", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2049247192", - "description": "Comment by iarspider at 2024-04-11 09:03:19 UTC processed.", - "context": "bot/17/ack" - } } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_run_test_params.json b/tests/PRActionData/TestProcessPr.test_run_test_params.json index 543a9212e480..3ebc060870c1 100644 --- a/tests/PRActionData/TestProcessPr.test_run_test_params.json +++ b/tests/PRActionData/TestProcessPr.test_run_test_params.json @@ -40,7 +40,8 @@ "2056740892": "+1", "2056796593": "+1", "2056801055": "+1", - "2056820593": "+1" + "2056820593": "+1", + "2056903278": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -135,6 +136,10 @@ true ] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_sign_core.json b/tests/PRActionData/TestProcessPr.test_sign_core.json index 11c5285c14fb..84244bdd6bac 100644 --- a/tests/PRActionData/TestProcessPr.test_sign_core.json +++ b/tests/PRActionData/TestProcessPr.test_sign_core.json @@ -11,7 +11,9 @@ "time": 1711538467 } }, - "emoji": {}, + "emoji": { + "2049242908": "+1" + }, "last_seen_sha": "2a9454e30606b17e52000110972998326ce9e428", "signatures": { "2049242908": "2a9454e30606b17e52000110972998326ce9e428" @@ -46,7 +48,7 @@ }, { "type": "edit-comment", - "data": "cms-bot internal usage" + "data": "cms-bot internal usage" }, { "type": "status", diff --git a/tests/PRActionData/TestProcessPr.test_sign_reject.json b/tests/PRActionData/TestProcessPr.test_sign_reject.json index 451d542922fd..1a86088ecfd5 100644 --- a/tests/PRActionData/TestProcessPr.test_sign_reject.json +++ b/tests/PRActionData/TestProcessPr.test_sign_reject.json @@ -58,7 +58,8 @@ "2056934192": "+1", "2056935714": "+1", "2056946596": "-1", - "2056966759": "+1" + "2056966759": "+1", + "2058758464": "+1" }, "last_seen_sha": "1d7419a436293c0337ca346fe868cb50cfdedc18", "signatures": { @@ -147,16 +148,6 @@ true ] }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "pending", - "target_url": null, - "description": "Waiting for authorized user to issue the test command.", - "context": "bot/17/jenkins" - } - }, { "type": "add-label", "data": [ @@ -169,36 +160,8 @@ "core-pending" ] }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2056820593", - "description": "2056820593:{\"PULL_REQUESTS\": \"cms-sw/cms-bot#2134\", \"SKIP_TESTS\": \"header,static\"}", - "context": "bot/17/test_parameters" - } - }, - { - "type": "emoji", - "data": [ - 2056820593, - "+1", - true - ] - }, { "type": "edit-comment", - "data": "cms-bot internal usage" - }, - { - "type": "status", - "data": { - "commit": "1d7419a436293c0337ca346fe868cb50cfdedc18", - "state": "success", - "target_url": "https://github.com/iarspider-cmssw/cmssw/pull/17#issuecomment-2058758464", - "description": "Comment by iarspider at 2024-04-16 10:27:25 UTC processed.", - "context": "bot/17/ack" - } + "data": "cms-bot internal usage" } ] \ No newline at end of file diff --git a/tests/PRActionData/TestProcessPr.test_start_tests.json b/tests/PRActionData/TestProcessPr.test_start_tests.json index 9946dbfc7c8f..e0b8ba29ce7a 100644 --- a/tests/PRActionData/TestProcessPr.test_start_tests.json +++ b/tests/PRActionData/TestProcessPr.test_start_tests.json @@ -33,7 +33,8 @@ } }, "emoji": { - "2049242908": "+1" + "2049242908": "+1", + "2049536626": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -110,6 +111,14 @@ "context": "cms/17/code-checks" } }, + { + "type": "create-comment", + "data": "Pull request #17 was updated. @Dr15Jones, @makortel, @smuzaffar can you please check and sign again.\n" + }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_test_params.json b/tests/PRActionData/TestProcessPr.test_test_params.json index 76b731d03624..2ff5f2d0ffc7 100644 --- a/tests/PRActionData/TestProcessPr.test_test_params.json +++ b/tests/PRActionData/TestProcessPr.test_test_params.json @@ -39,7 +39,8 @@ "2056739513": "+1", "2056740892": "+1", "2056796593": "+1", - "2056801055": "+1" + "2056801055": "+1", + "2056820593": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -115,6 +116,10 @@ true ] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_tests_passed.json b/tests/PRActionData/TestProcessPr.test_tests_passed.json index e85e533ce7dd..f48c3585c1d6 100644 --- a/tests/PRActionData/TestProcessPr.test_tests_passed.json +++ b/tests/PRActionData/TestProcessPr.test_tests_passed.json @@ -34,7 +34,8 @@ }, "emoji": { "2049242908": "+1", - "2049536626": "+1" + "2049536626": "+1", + "2056736344": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -99,6 +100,10 @@ true ] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_unassign.json b/tests/PRActionData/TestProcessPr.test_unassign.json index d544c84393c0..34ea5045bd17 100644 --- a/tests/PRActionData/TestProcessPr.test_unassign.json +++ b/tests/PRActionData/TestProcessPr.test_unassign.json @@ -38,7 +38,8 @@ "2056736344": "+1", "2056739513": "+1", "2056740892": "+1", - "2056796593": "+1" + "2056796593": "+1", + "2056801055": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -96,6 +97,10 @@ "db-pending" ] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_unhold.json b/tests/PRActionData/TestProcessPr.test_unhold.json index ac5f59c464d0..5440353da65a 100644 --- a/tests/PRActionData/TestProcessPr.test_unhold.json +++ b/tests/PRActionData/TestProcessPr.test_unhold.json @@ -36,7 +36,8 @@ "2049242908": "+1", "2049536626": "+1", "2056736344": "+1", - "2056739513": "+1" + "2056739513": "+1", + "2056740892": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -78,6 +79,10 @@ "hold" ] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/PRActionData/TestProcessPr.test_valid_type.json b/tests/PRActionData/TestProcessPr.test_valid_type.json index 1d1722e02c03..378ad635269a 100644 --- a/tests/PRActionData/TestProcessPr.test_valid_type.json +++ b/tests/PRActionData/TestProcessPr.test_valid_type.json @@ -45,7 +45,8 @@ "2056930228": "+1", "2056934192": "+1", "2056935714": "+1", - "2056946596": "-1" + "2056946596": "-1", + "2056966759": "+1" }, "last_seen_sha": "dae848e38b8e387d7283a3e35818121487d9d76b", "signatures": { @@ -135,6 +136,10 @@ "type": "remove-label", "data": [] }, + { + "type": "edit-comment", + "data": "cms-bot internal usage" + }, { "type": "status", "data": { diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/test_process_pr.py b/tests/test_process_pr.py index e8645d115b91..4038b1b8d6c8 100644 --- a/tests/test_process_pr.py +++ b/tests/test_process_pr.py @@ -1,29 +1,340 @@ +import functools import importlib import json import os +import os.path import sys import traceback +from unittest.mock import patch import pytest +import github from . import Framework from .Framework import readLine -from github.PaginatedList import PaginatedList +actions = [] +record_actions = False + + +# Utility function for recording calls and optionally calling the original function +def hook_and_call_original(hook, original_function, call_original, self, *args, **kwargs): + + if call_original: + res = original_function(self, *args, **kwargs) + else: + res = None + + res = hook(self, *args, **kwargs, res=res) + + return res + + +def on_issuecomment_edit(self, *args, **kwargs): + kwargs.pop("res") + assert len(kwargs) == 0, "IssueComment.edit signature has changed" + assert len(args) == 1, "IssueComment.edit signature has changed" + + body = args[0] + actions.append({"type": "edit-comment", "data": body}) + print("DRY RUN: Updating existing comment with text") + print(body.encode("ascii", "ignore").decode()) + + +def on_issuecomment_delete(self, *args, **kwargs): + kwargs.pop("res") + assert len(kwargs) == 0, "IssueComment.delete signature has changed" + assert len(args) == 0, "IssueComment.delete signature has changed" + actions.append({"type": "delete-comment", "data": None}) + + +def on_issue_create_comment(self, *args, **kwargs): + kwargs.pop("res") + assert len(kwargs) == 0, "Issue.create_comment signature has changed" + assert len(args) == 1, "Issue.create_comment signature has changed" + + body = args[0] + actions.append({"type": "create-comment", "data": body}) + print("DRY RUN: Creating comment with text") + print(body.encode("ascii", "ignore").decode()) + + +def on_issue_edit(self, *args, **kwargs): + if kwargs.get("state") == "closed": + actions.append({"type": "close", "data": None}) + + if kwargs.get("state") == "open": + actions.append({"type": "open", "data": None}) + + if kwargs.get("milestone", github.GithubObject.NotSet) != github.GithubObject.NotSet: + milestone = kwargs["milestone"] + actions.append( + { + "type": "update-milestone", + "data": {"id": milestone.number, "title": milestone.title}, + } + ) + + +def on_commit_create_status(self, *args, **kwargs): + assert len(args) == 1, "Commit.Commit.create_status signature has chagned" + + state = args[0] + target_url = kwargs.get("target_url") + description = kwargs.get("description") + context = kwargs.get("context") + + actions.append( + { + "type": "status", + "data": { + "commit": self.sha, + "state": state, + "target_url": target_url, + "description": description, + "context": context, + }, + } + ) + + print( + "DRY RUN: set commit status state={0}, target_url={1}, description={2}, context={3}".format( + state, target_url, description, context + ) + ) + + +# TODO: remove once we update pygithub +def get_commit_files(commit): + # noinspection PyProtectedMember + return github.PaginatedList.PaginatedList( + github.File.File, + commit._requester, + commit.url, + {}, + None, + "files", + ) + + +# TODO: remove once we update pygithub +def get_commit_files_pygithub(*args, **kwargs): + kwargs.pop("res") + assert len(args) == 2, "Signature of process_pr.get_commit_files changed" + assert len(kwargs) == 0, "Signature of process_pr.get_commit_files changed" + commit = args[1] + return (x.filename for x in get_commit_files(commit)) + + +def on_read_bot_cache(*args, **kwargs): + assert "res" in kwargs + res = kwargs.pop("res") + assert len(args) == 1, "Signature of process_pr.read_bot_cache changed" + assert len(kwargs) == 0, "Signature of process_pr.read_bot_cache changed" + actions.append({"type": "load-bot-cache", "data": res}) + return res + + +def on_create_property_file(*args, **kwargs): + kwargs.pop("res") + assert len(args) == 3, "Signature of process_pr.create_property_file changed" + assert len(kwargs) == 0, "Signature of process_pr.create_property_file changed" + + out_file_name = args[0] + parameters = args[1] + + actions.append( + {"type": "create-property-file", "data": {"filename": out_file_name, "data": parameters}} + ) + + +def on_set_comment_emoji_cache(*args, **kwargs): + kwargs.pop("res") + assert len(args) == 4 + assert len(kwargs) <= 2 + comment = args[2] + + actions.append( + { + "type": "emoji", + "data": (comment.id, kwargs.get("emoji", "+1"), kwargs.get("reset_other", True)), + } + ) + + +def on_labels_changed(*args, **kwargs): + kwargs.pop("res") + assert len(args) == 2 + assert len(kwargs) == 0 + + added_labels = args[0] + removed_labels = args[1] + + actions.append({"type": "add-label", "data": sorted(list(added_labels))}) + actions.append({"type": "remove-label", "data": sorted(list(removed_labels))}) class TestProcessPr(Framework.TestCase): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.process_pr_module = None + self.prId = -1 + + def setUpHooks(self): + self.patchers = [] + self.calls = {} + self.original_functions = {} + + functions_to_patch = [ + { + "module_path": "github.IssueComment", + "class_name": "IssueComment", + "function_name": "edit", + "hook_function": on_issuecomment_edit, + "call_original": False, + }, + { + "module_path": "github.IssueComment", + "class_name": "IssueComment", + "function_name": "delete", + "hook_function": on_issuecomment_delete, + "call_original": False, + }, + { + "module_path": "github.Issue", + "class_name": "Issue", + "function_name": "create_comment", + "hook_function": on_issue_create_comment, + "call_original": False, + }, + { + "module_path": "github.Issue", + "class_name": "Issue", + "function_name": "edit", + "hook_function": on_issue_edit, + "call_original": False, + }, + { + "module_path": "github.Commit", + "class_name": "Commit", + "function_name": "create_status", + "hook_function": on_commit_create_status, + "call_original": False, + }, + { + "module_path": "process_pr", + "class_name": None, + "function_name": "get_commit_files", + "hook_function": get_commit_files_pygithub, + "call_original": False, + }, + { + "module_path": "process_pr", + "class_name": None, + "function_name": "create_property_file", + "hook_function": on_create_property_file, + "call_original": True, + }, + # Make this function no-op + { + "module_path": "process_pr", + "class_name": None, + "function_name": "set_emoji", + "hook_function": lambda *args, **kwargs: None, + "call_original": False, + }, + # on_read_bot_cache + { + "module_path": "process_pr", + "class_name": None, + "function_name": "read_bot_cache", + "hook_function": on_read_bot_cache, + "call_original": True, + }, + # Actual handling of emoji cache + { + "module_path": "process_pr", + "class_name": None, + "function_name": "set_comment_emoji_cache", + "hook_function": on_set_comment_emoji_cache, + "call_original": True, + }, + { + "module_path": "process_pr", + "class_name": None, + "function_name": "on_labels_changed", + "hook_function": on_labels_changed, + "call_original": False, + }, + ] + + for func in functions_to_patch: + module = __import__(func["module_path"], fromlist=func["class_name"]) + if func["class_name"] is not None: + original_class = getattr(module, func["class_name"]) + original_function = getattr(original_class, func["function_name"]) + fn_to_patch = "{}.{}.{}".format( + func["module_path"], func["class_name"], func["function_name"] + ) + else: + original_function = getattr(module, func["function_name"]) + fn_to_patch = "{}.{}".format(func["module_path"], func["function_name"]) + + side_effect = functools.partial( + hook_and_call_original, + func["hook_function"], + original_function, + func["call_original"], + ) + + self.original_functions[fn_to_patch] = original_function + + patcher = patch(fn_to_patch, side_effect=side_effect, autospec=True) + self.patchers.append(patcher) + + # Apply the patch + patcher.start() + + def tearDown(self): + for patcher in self.patchers: + patcher.stop() + + self.process_pr = None + self.process_pr_module = None + @staticmethod def compareActions(res_, expected_): - res = {json.dumps(x, sort_keys=True) for x in res_} - expected = {json.dumps(x, sort_keys=True) for x in expected_} + def normalize_data(data): + if isinstance(data, dict): + return frozenset((key, normalize_data(value)) for key, value in data.items()) + elif isinstance(data, list): + return tuple(normalize_data(item) for item in data) + else: + return data + + def normalize_action(action): + return (action["type"], normalize_data(action["data"])) + + def denormalize_data(data): + if isinstance(data, frozenset): + return {key: denormalize_data(value) for key, value in data} + elif isinstance(data, tuple): + return [denormalize_data(item) for item in data] + else: + return data + + def denormalize_action(normalized_action): + return {"type": normalized_action[0], "data": denormalize_data(normalized_action[1])} + + res = set([normalize_action(action) for action in res_]) + expected = set([normalize_action(action) for action in expected_]) if res.symmetric_difference(expected): for itm in res - expected: - print("New action", itm) + print("New action", denormalize_action(itm)) for itm in expected - res: - print("Missing action", itm) + print("Missing action", denormalize_action(itm)) pytest.fail("Actions mismatch") @@ -55,20 +366,16 @@ def __openEventFile(self, mode): def __closeEventReplayFileIfNeeded(self): if self.__eventFile is not None: if ( - not self.recordMode + not record_actions ): # pragma no branch (Branch useful only when recording new tests, not used during automated tests) self.assertEqual(readLine(self.__eventFile), "") self.__eventFile.close() - def setUp(self): - super().setUp() - - self.__eventFileName = "" - self.__eventFile = None - - self.actionDataFolder = "PRActionData" - if not os.path.exists(self.actionDataFolder): - os.mkdir(self.actionDataFolder) + @classmethod + def setUpClass(cls): + cls.actionDataFolder = "PRActionData" + if not os.path.exists(cls.actionDataFolder): + os.mkdir(cls.actionDataFolder) repo_config_dir = os.path.abspath( os.path.join(os.path.dirname(__file__), "..", "repos", "iarspider_cmssw", "cmssw") @@ -78,6 +385,7 @@ def setUp(self): sys.path.insert(0, repo_config_dir) if "repo_config" in sys.modules: + print("Reloading repo_config...") importlib.reload(sys.modules["repo_config"]) importlib.reload(sys.modules["milestones"]) importlib.reload(sys.modules["releases"]) @@ -85,35 +393,50 @@ def setUp(self): else: importlib.import_module("repo_config") - self.repo_config = sys.modules["repo_config"] - assert "iarspider_cmssw" in self.repo_config.__file__ + cls.repo_config = sys.modules["repo_config"] + assert "iarspider_cmssw" in cls.repo_config.__file__ - self.process_pr = importlib.import_module("process_pr").process_pr + def setUp(self): + super().setUp() - def runTest(self, prId=17): + self.__eventFileName = "" + self.__eventFile = None + + self.setUpHooks() + if "process_pr" not in sys.modules: + importlib.import_module("process_pr") + + self.process_pr_module = sys.modules["process_pr"] + self.process_pr = self.process_pr_module.process_pr + + global actions + actions = [] + + def runTest(self, prId=17, testName=""): repo = self.g.get_repo("iarspider-cmssw/cmssw") issue = repo.get_issue(prId) - if self.recordMode: + if record_actions: self.__openEventFile("w") self.replayData = None else: f = self.__openEventFile("r") self.replayData = json.load(f) - self.processPrData = self.process_pr( + self.process_pr( self.repo_config, self.g, repo, issue, - True, + False, self.repo_config.CMSBUILD_USER, ) + self.processPrData = actions self.checkOrSaveTest() self.__closeEventReplayFileIfNeeded() def checkOrSaveTest(self): - if self.recordMode: + if record_actions: json.dump(self.processPrData, self.__eventFile, indent=4) else: TestProcessPr.compareActions(self.processPrData, self.replayData) @@ -131,9 +454,9 @@ def mark_tests( comparision=True, ): repo = self.g.get_repo("iarspider-cmssw/cmssw") - pr = repo.get_pull(prId) + pr = repo.get_pull(self.prId) commit = pr.get_commits().reversed[0] - prefix = "cms/" + str(prId) + "/" + prefix = "cms/" + str(self.prId) + "/" if queue.endswith("_X"): queue = queue.rstrip("_X") prefix += (queue + "/" if queue else "") + arch @@ -176,7 +499,7 @@ def mark_tests( ) def test_new_pr(self): - self.runTest() + self.runTest(testName="new_pr") def test_code_check_approved(self): self.runTest() @@ -252,13 +575,13 @@ def test_sign_reject(self): self.runTest() def test_many_commits_warn(self): - self.runTest(18) + self.runTest(prId=18) def test_many_commits_ok(self): - self.runTest(18) + self.runTest(prId=18) def test_too_many_commits(self): - self.runTest(18) + self.runTest(prId=18) # Not yet implemented # def test_future_commit(self):