Split discrete functionalities of ReportSizeDeltas.report_size_deltas() into separate functions

per1234 · per1234 · commit 30810a6a63b2 · 2020-09-15T04:39:10.000-07:00
The addition of support for sourcing the sketches reports from a local path resulted in the function containing code for two completely separate usages of the script.

Moving each flavor of code to a separate function improves readability, maintainability, and testability.
diff --git a/reportsizedeltas/reportsizedeltas.py b/reportsizedeltas/reportsizedeltas.py
@@ -88,73 +88,80 @@ def report_size_deltas(self):
         """Comment a report of memory usage change to pull request(s)."""
         if os.environ["GITHUB_EVENT_NAME"] == "pull_request":
             # The sketches reports will be in a local folder location specified by the user
-            sketches_reports_folder = pathlib.Path(os.environ["GITHUB_WORKSPACE"], self.sketches_reports_source_name)
-            sketches_reports = self.get_sketches_reports(artifact_folder_object=sketches_reports_folder)
-
-            if sketches_reports:
-                report = self.generate_report(sketches_reports=sketches_reports)
-
-                with open(file=os.environ["GITHUB_EVENT_PATH"]) as github_event_file:
-                    pr_number = json.load(github_event_file)["pull_request"]["number"]
-
-                self.comment_report(pr_number=pr_number, report_markdown=report)
-
+            self.report_size_deltas_from_local_reports()
         else:
             # The script is being run from a workflow triggered by something other than a PR
             # Scan the repository's pull requests and comment memory usage change reports where appropriate.
-            # Get the repository's pull requests
-            logger.debug("Getting PRs for " + self.repository_name)
-            page_number = 1
-            page_count = 1
-            while page_number <= page_count:
-                api_data = self.api_request(request="repos/" + self.repository_name + "/pulls",
-                                            page_number=page_number)
-                prs_data = api_data["json_data"]
-                for pr_data in prs_data:
-                    # Note: closed PRs are not listed in the API response
-                    pr_number = pr_data["number"]
-                    pr_head_sha = pr_data["head"]["sha"]
-                    print("::debug::Processing pull request number:", pr_number)
-                    # When a PR is locked, only collaborators may comment. The automatically generated GITHUB_TOKEN will
-                    # likely be used, which is owned by the github-actions bot, who doesn't have collaborator status. So
-                    # locking the thread would cause the job to fail.
-                    if pr_data["locked"]:
-                        print("::debug::PR locked, skipping")
-                        continue
+            self.report_size_deltas_from_workflow_artifacts()
 
-                    if self.report_exists(pr_number=pr_number,
-                                          pr_head_sha=pr_head_sha):
-                        # Go on to the next PR
-                        print("::debug::Report already exists")
-                        continue
+    def report_size_deltas_from_local_reports(self):
+        """Comment a report of memory usage change to the pull request."""
+        sketches_reports_folder = pathlib.Path(os.environ["GITHUB_WORKSPACE"], self.sketches_reports_source_name)
+        sketches_reports = self.get_sketches_reports(artifact_folder_object=sketches_reports_folder)
 
-                    artifact_download_url = self.get_artifact_download_url_for_sha(
-                        pr_user_login=pr_data["user"]["login"],
-                        pr_head_ref=pr_data["head"]["ref"],
-                        pr_head_sha=pr_head_sha)
-                    if artifact_download_url is None:
-                        # Go on to the next PR
-                        print("::debug::No sketches report artifact found")
-                        continue
+        if sketches_reports:
+            report = self.generate_report(sketches_reports=sketches_reports)
 
-                    artifact_folder_object = self.get_artifact(artifact_download_url=artifact_download_url)
+            with open(file=os.environ["GITHUB_EVENT_PATH"]) as github_event_file:
+                pr_number = json.load(github_event_file)["pull_request"]["number"]
 
-                    sketches_reports = self.get_sketches_reports(artifact_folder_object=artifact_folder_object)
+            self.comment_report(pr_number=pr_number, report_markdown=report)
 
-                    if sketches_reports:
-                        if sketches_reports[0][self.ReportKeys.commit_hash] != pr_head_sha:
-                            # The deltas report key uses the hash from the report, but the report_exists() comparison is
-                            # done using the hash provided by the API. If for some reason the two didn't match, it would
-                            # result in the deltas report being done over and over again.
-                            print("::warning::Report commit hash doesn't match PR's head commit hash, skipping")
-                            continue
+    def report_size_deltas_from_workflow_artifacts(self):
+        """Scan the repository's pull requests and comment memory usage change reports where appropriate."""
+        # Get the repository's pull requests
+        logger.debug("Getting PRs for " + self.repository_name)
+        page_number = 1
+        page_count = 1
+        while page_number <= page_count:
+            api_data = self.api_request(request="repos/" + self.repository_name + "/pulls",
+                                        page_number=page_number)
+            prs_data = api_data["json_data"]
+            for pr_data in prs_data:
+                # Note: closed PRs are not listed in the API response
+                pr_number = pr_data["number"]
+                pr_head_sha = pr_data["head"]["sha"]
+                print("::debug::Processing pull request number:", pr_number)
+                # When a PR is locked, only collaborators may comment. The automatically generated GITHUB_TOKEN will
+                # likely be used, which is owned by the github-actions bot, who doesn't have collaborator status. So
+                # locking the thread would cause the job to fail.
+                if pr_data["locked"]:
+                    print("::debug::PR locked, skipping")
+                    continue
+
+                if self.report_exists(pr_number=pr_number,
+                                      pr_head_sha=pr_head_sha):
+                    # Go on to the next PR
+                    print("::debug::Report already exists")
+                    continue
+
+                artifact_download_url = self.get_artifact_download_url_for_sha(
+                    pr_user_login=pr_data["user"]["login"],
+                    pr_head_ref=pr_data["head"]["ref"],
+                    pr_head_sha=pr_head_sha)
+                if artifact_download_url is None:
+                    # Go on to the next PR
+                    print("::debug::No sketches report artifact found")
+                    continue
+
+                artifact_folder_object = self.get_artifact(artifact_download_url=artifact_download_url)
+
+                sketches_reports = self.get_sketches_reports(artifact_folder_object=artifact_folder_object)
+
+                if sketches_reports:
+                    if sketches_reports[0][self.ReportKeys.commit_hash] != pr_head_sha:
+                        # The deltas report key uses the hash from the report, but the report_exists() comparison is
+                        # done using the hash provided by the API. If for some reason the two didn't match, it would
+                        # result in the deltas report being done over and over again.
+                        print("::warning::Report commit hash doesn't match PR's head commit hash, skipping")
+                        continue
 
-                        report = self.generate_report(sketches_reports=sketches_reports)
+                    report = self.generate_report(sketches_reports=sketches_reports)
 
-                        self.comment_report(pr_number=pr_number, report_markdown=report)
+                    self.comment_report(pr_number=pr_number, report_markdown=report)
 
-                page_number += 1
-                page_count = api_data["page_count"]
+            page_number += 1
+            page_count = api_data["page_count"]
 
     def report_exists(self, pr_number, pr_head_sha):
         """Return whether a report has already been commented to the pull request thread for the latest workflow run
diff --git a/reportsizedeltas/tests/test_reportsizedeltas.py b/reportsizedeltas/tests/test_reportsizedeltas.py
@@ -177,14 +177,35 @@ def test_set_verbosity():
     reportsizedeltas.set_verbosity(enable_verbosity=False)
 
 
-def test_report_size_deltas_pull_request(mocker, monkeypatch):
+# noinspection PyUnresolvedReferences
+def test_report_size_deltas(mocker, monkeypatch):
+    mocker.patch("reportsizedeltas.ReportSizeDeltas.report_size_deltas_from_local_reports", autospec=True)
+    mocker.patch("reportsizedeltas.ReportSizeDeltas.report_size_deltas_from_workflow_artifacts", autospec=True)
+
+    report_size_deltas = get_reportsizedeltas_object()
+
+    # Test triggered by pull_request event
+    monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
+    report_size_deltas.report_size_deltas()
+    # noinspection PyUnresolvedReferences
+    report_size_deltas.report_size_deltas_from_local_reports.assert_called_once()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts.assert_not_called()
+
+    # Test triggered by other than pull_request event
+    mocker.resetall()
+    monkeypatch.setenv("GITHUB_EVENT_NAME", "schedule")
+    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_local_reports.assert_not_called()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts.assert_called_once()
+
+
+def test_report_size_deltas_from_local_reports(mocker, monkeypatch):
     sketches_reports_source_name = "golden-sketches-reports-source-path"
     github_workspace = "golden-github-workspace"
     sketches_reports_folder = pathlib.Path(github_workspace, sketches_reports_source_name)
     sketches_reports = unittest.mock.sentinel.sketches_reports
     report = "golden report"
 
-    monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
     monkeypatch.setenv("GITHUB_WORKSPACE", github_workspace)
     monkeypatch.setenv("GITHUB_EVENT_PATH",
                        str(test_data_path.joinpath("test_report_size_deltas_pull_request", "githubevent.json")))
@@ -197,22 +218,22 @@ def test_report_size_deltas_pull_request(mocker, monkeypatch):
 
     # Test handling of no sketches reports data available
     reportsizedeltas.ReportSizeDeltas.get_sketches_reports.return_value = None
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_local_reports()
 
     report_size_deltas.comment_report.assert_not_called()
 
     # Test report data available
     mocker.resetall()
     reportsizedeltas.ReportSizeDeltas.get_sketches_reports.return_value = sketches_reports
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_local_reports()
 
     report_size_deltas.get_sketches_reports.assert_called_once_with(report_size_deltas,
                                                                     artifact_folder_object=sketches_reports_folder)
     report_size_deltas.generate_report.assert_called_once_with(report_size_deltas, sketches_reports=sketches_reports)
     report_size_deltas.comment_report.assert_called_once_with(report_size_deltas, pr_number=42, report_markdown=report)
 
 
-def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
+def test_report_size_deltas_from_workflow_artifacts(mocker):
     artifact_download_url = "test_artifact_download_url"
     artifact_folder_object = "test_artifact_folder_object"
     pr_head_sha = "pr-head-sha"
@@ -221,8 +242,6 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     json_data = [{"number": 1, "locked": True, "head": {"sha": pr_head_sha, "ref": "asdf"}, "user": {"login": "1234"}},
                  {"number": 2, "locked": False, "head": {"sha": pr_head_sha, "ref": "asdf"}, "user": {"login": "1234"}}]
 
-    monkeypatch.setenv("GITHUB_EVENT_NAME", "schedule")
-
     report_size_deltas = get_reportsizedeltas_object()
 
     mocker.patch("reportsizedeltas.ReportSizeDeltas.api_request",
@@ -242,7 +261,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     # Test handling of locked PR
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_size_deltas.comment_report.assert_called_once_with(report_size_deltas, pr_number=2, report_markdown=report)
 
@@ -252,7 +271,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     reportsizedeltas.ReportSizeDeltas.report_exists.return_value = True
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_size_deltas.comment_report.assert_not_called()
 
@@ -261,7 +280,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     reportsizedeltas.ReportSizeDeltas.get_artifact_download_url_for_sha.return_value = None
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_size_deltas.comment_report.assert_not_called()
 
@@ -270,7 +289,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     reportsizedeltas.ReportSizeDeltas.get_sketches_reports.return_value = None
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_size_deltas.comment_report.assert_not_called()
 
@@ -281,7 +300,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
 
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_size_deltas.comment_report.assert_not_called()
 
@@ -290,7 +309,7 @@ def test_report_size_deltas_not_pull_request(mocker, monkeypatch):
     reportsizedeltas.ReportSizeDeltas.get_sketches_reports.return_value = sketches_reports
     mocker.resetall()
 
-    report_size_deltas.report_size_deltas()
+    report_size_deltas.report_size_deltas_from_workflow_artifacts()
 
     report_exists_calls = []
     get_artifact_download_url_for_sha_calls = []
