Unit tests for process_pr script

cms-sw · Jun 21, 2024 · abdd470 · abdd470
1 parent 53bf297
commit abdd470
Show file tree

Hide file tree

Showing 27 changed files with 540 additions and 285 deletions.
diff --git a/.github/workflows/test-process-pr.yaml 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/[email protected]
+        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
@@ -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
@@ -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<!-- bot cache: {\"commits\":{\"2a9454e30606b17e52000110972998326ce9e428\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1711538467},\"79752f053efecad55dde17732259737e621a1f3f\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1712828239},\"dae848e38b8e387d7283a3e35818121487d9d76b\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712829250},\"e4d069b76c464274bf6e7d7cf9bac2153ed9a903\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712819089}},\"emoji\":{\"2049242908\":\"+1\",\"2049536626\":\"+1\",\"2056736344\":\"+1\",\"2056739513\":\"+1\",\"2056740892\":\"+1\",\"2056796593\":\"+1\",\"2056801055\":\"+1\",\"2056820593\":\"+1\",\"2056903278\":\"+1\",\"2056930228\":\"+1\"},\"last_seen_sha\":\"dae848e38b8e387d7283a3e35818121487d9d76b\",\"signatures\":{\"2049242908\":\"2a9454e30606b17e52000110972998326ce9e428\"}} -->"
+    },
     {
         "type": "status",
         "data": {

diff --git 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<!-- bot cache: {\"commits\":{\"2a9454e30606b17e52000110972998326ce9e428\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1711538467},\"79752f053efecad55dde17732259737e621a1f3f\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1712828239},\"dae848e38b8e387d7283a3e35818121487d9d76b\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712829250},\"e4d069b76c464274bf6e7d7cf9bac2153ed9a903\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712819089}},\"emoji\":{\"2049242908\":\"+1\",\"2049536626\":\"+1\",\"2056736344\":\"+1\",\"2056739513\":\"+1\",\"2056740892\":\"+1\",\"2056796593\":\"+1\"},\"last_seen_sha\":\"dae848e38b8e387d7283a3e35818121487d9d76b\",\"signatures\":{\"2049242908\":\"2a9454e30606b17e52000110972998326ce9e428\"}} -->"
+    },
     {
         "type": "status",
         "data": {

diff --git 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<!-- bot cache: {\"commits\":{\"2a9454e30606b17e52000110972998326ce9e428\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":true,\"time\":1711538467},\"35f9a4c06b006029da40ed8858e0dae4abd52cb3\":{\"files\":[],\"squashed\":false,\"time\":1712829250},\"79752f053efecad55dde17732259737e621a1f3f\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":true,\"time\":1712828239},\"dae848e38b8e387d7283a3e35818121487d9d76b\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":true,\"time\":1712829250},\"e4d069b76c464274bf6e7d7cf9bac2153ed9a903\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":true,\"time\":1712819089}},\"emoji\":{\"2049242908\":\"+1\",\"2049536626\":\"+1\",\"2056736344\":\"+1\",\"2056739513\":\"+1\",\"2056740892\":\"+1\",\"2056796593\":\"+1\",\"2056801055\":\"+1\",\"2056820593\":\"+1\",\"2056903278\":\"+1\",\"2056930228\":\"+1\",\"2056934192\":\"+1\",\"2056935714\":\"+1\",\"2056946596\":\"-1\",\"2056966759\":\"+1\"},\"last_seen_sha\":\"35f9a4c06b006029da40ed8858e0dae4abd52cb3\",\"signatures\":{\"2049242908\":\"2a9454e30606b17e52000110972998326ce9e428\"}} -->"
-    },
-    {
-        "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"
-        }
     }
 ]
diff --git 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<!-- bot cache: {\"commits\":{\"2a9454e30606b17e52000110972998326ce9e428\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1711538467},\"79752f053efecad55dde17732259737e621a1f3f\":{\"files\":[\"Utilities/ReleaseScripts/test/BuildFile.xml\"],\"squashed\":false,\"time\":1712828239},\"dae848e38b8e387d7283a3e35818121487d9d76b\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712829250},\"e4d069b76c464274bf6e7d7cf9bac2153ed9a903\":{\"files\":[\"DQMServices/Components/test/dqmiofilecopy.sh\"],\"squashed\":false,\"time\":1712819089}},\"emoji\":{\"2049242908\":\"+1\",\"2049536626\":\"+1\",\"2056736344\":\"+1\",\"2056739513\":\"+1\",\"2056740892\":\"+1\",\"2056796593\":\"+1\",\"2056801055\":\"+1\",\"2056820593\":\"+1\",\"2056903278\":\"+1\",\"2056930228\":\"+1\",\"2056934192\":\"+1\"},\"last_seen_sha\":\"dae848e38b8e387d7283a3e35818121487d9d76b\",\"signatures\":{\"2049242908\":\"2a9454e30606b17e52000110972998326ce9e428\"}} -->"
+    },
     {
         "type": "status",
         "data": {

diff --git 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 <a href=\"http://cms-sw.github.io/cms-bot-cmssw-cmds.html\">here</a>\n"
+    },
     {
         "type": "status",
         "data": {