From 5e6d889332629ae232e21f0ae7ff7cef72af9ec8 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Mon, 10 Jun 2024 15:49:55 -0700 Subject: [PATCH 1/3] fix: issue counts incorrect when HIDE_TIME_TO_CLOSE is True Signed-off-by: Zack Koppert --- issue_metrics.py | 40 +++++---- test_issue_metrics.py | 175 +++++++++++++++++++++++++++++++++++++++- test_markdown_writer.py | 4 +- 3 files changed, 192 insertions(+), 27 deletions(-) diff --git a/issue_metrics.py b/issue_metrics.py index e5024a4..0d7645f 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -193,14 +193,12 @@ def get_per_issue_metrics( ) if env_vars.hide_time_to_answer is False: issue_with_metrics.time_to_answer = measure_time_to_answer(issue) - if env_vars.hide_time_to_close is False: - if issue["closedAt"]: - issue_with_metrics.time_to_close = measure_time_to_close( - None, issue - ) - num_issues_closed += 1 - else: - num_issues_open += 1 + if env_vars.hide_time_to_close is False and issue["closedAt"]: + issue_with_metrics.time_to_close = measure_time_to_close(None, issue) + if issue["closedAt"]: + num_issues_closed += 1 + else: + num_issues_open += 1 else: issue_with_metrics = IssueWithMetrics( issue.title, # type: ignore @@ -236,19 +234,19 @@ def get_per_issue_metrics( ) if labels and env_vars.hide_label_metrics is False: issue_with_metrics.label_metrics = get_label_metrics(issue, labels) - if env_vars.hide_time_to_close is False: - if issue.state == "closed": # type: ignore - if pull_request: - issue_with_metrics.time_to_close = measure_time_to_merge( - pull_request, ready_for_review_at - ) - else: - issue_with_metrics.time_to_close = measure_time_to_close( - issue, None - ) - num_issues_closed += 1 - elif issue.state == "open": # type: ignore - num_issues_open += 1 + if env_vars.hide_time_to_close is False and issue.state == "closed": # type: ignore + if pull_request: + issue_with_metrics.time_to_close = measure_time_to_merge( + pull_request, ready_for_review_at + ) + else: + issue_with_metrics.time_to_close = measure_time_to_close( + issue, None + ) + if issue.state == "closed": # type: ignore + num_issues_closed += 1 + elif issue.state == "open": # type: ignore + num_issues_open += 1 issues_with_metrics.append(issue_with_metrics) return issues_with_metrics, num_issues_open, num_issues_closed diff --git a/test_issue_metrics.py b/test_issue_metrics.py index 1cf6d1d..5059a88 100644 --- a/test_issue_metrics.py +++ b/test_issue_metrics.py @@ -175,10 +175,131 @@ class TestGetPerIssueMetrics(unittest.TestCase): @patch.dict( os.environ, - {"GH_TOKEN": "test_token", "SEARCH_QUERY": "is:issue is:open repo:user/repo"}, + { + "GH_TOKEN": "test_token", + "SEARCH_QUERY": "is:issue is:open repo:user/repo", + "HIDE_AUTHOR": "true", + "HIDE_LABEL_METRICS": "true", + "HIDE_TIME_TO_ANSWER": "true", + "HIDE_TIME_TO_CLOSE": "true", + "HIDE_TIME_TO_FIRST_RESPONSE": "true", + }, + ) + def test_get_per_issue_metrics_with_hide_envs(self): + """ + Test that the function correctly calculates the metrics for + a list of GitHub issues where HIDE_* envs are set true + """ + + # Create mock data + mock_issue1 = MagicMock( + title="Issue 1", + html_url="https://github.com/user/repo/issues/1", + author="alice", + state="open", + comments=1, + created_at="2023-01-01T00:00:00Z", + ) + + mock_comment1 = MagicMock() + mock_comment1.created_at = datetime.fromisoformat("2023-01-02T00:00:00Z") + mock_issue1.issue.comments.return_value = [mock_comment1] + mock_issue1.issue.pull_request_urls = None + + mock_issue2 = MagicMock( + title="Issue 2", + html_url="https://github.com/user/repo/issues/2", + author="bob", + state="closed", + comments=1, + created_at="2023-01-01T00:00:00Z", + closed_at="2023-01-04T00:00:00Z", + ) + + mock_comment2 = MagicMock() + mock_comment2.created_at = datetime.fromisoformat("2023-01-03T00:00:00Z") + mock_issue2.issue.comments.return_value = [mock_comment2] + mock_issue2.issue.pull_request_urls = None + + issues = [ + mock_issue1, + mock_issue2, + ] + + # Call the function and check the result + with unittest.mock.patch( # type:ignore + "issue_metrics.measure_time_to_first_response", + measure_time_to_first_response, + ), unittest.mock.patch( # type:ignore + "issue_metrics.measure_time_to_close", measure_time_to_close + ): + ( + result_issues_with_metrics, + result_num_issues_open, + result_num_issues_closed, + ) = get_per_issue_metrics( + issues, + env_vars=get_env_vars(test=True), + ) + expected_issues_with_metrics = [ + IssueWithMetrics( + "Issue 1", + "https://github.com/user/repo/issues/1", + "alice", + None, + None, + None, + None, + ), + IssueWithMetrics( + "Issue 2", + "https://github.com/user/repo/issues/2", + "bob", + None, + None, + None, + None, + ), + ] + expected_num_issues_open = 1 + expected_num_issues_closed = 1 + self.assertEqual(result_num_issues_open, expected_num_issues_open) + self.assertEqual(result_num_issues_closed, expected_num_issues_closed) + self.assertEqual( + result_issues_with_metrics[0].time_to_first_response, + expected_issues_with_metrics[0].time_to_first_response, + ) + self.assertEqual( + result_issues_with_metrics[0].time_to_close, + expected_issues_with_metrics[0].time_to_close, + ) + self.assertEqual( + result_issues_with_metrics[1].time_to_first_response, + expected_issues_with_metrics[1].time_to_first_response, + ) + self.assertEqual( + result_issues_with_metrics[1].time_to_close, + expected_issues_with_metrics[1].time_to_close, + ) + + @patch.dict( + os.environ, + { + "GH_TOKEN": "test_token", + "SEARCH_QUERY": "is:issue is:open repo:user/repo", + "HIDE_AUTHOR": "false", + "HIDE_LABEL_METRICS": "false", + "HIDE_TIME_TO_ANSWER": "false", + "HIDE_TIME_TO_CLOSE": "false", + "HIDE_TIME_TO_FIRST_RESPONSE": "false", + }, ) - def test_get_per_issue_metrics(self): - """Test that the function correctly calculates the metrics for a list of GitHub issues.""" + def test_get_per_issue_metrics_without_hide_envs(self): + """ + Test that the function correctly calculates the metrics for + a list of GitHub issues where HIDE_* envs are set false + """ + # Create mock data mock_issue1 = MagicMock( title="Issue 1", @@ -225,7 +346,10 @@ def test_get_per_issue_metrics(self): result_issues_with_metrics, result_num_issues_open, result_num_issues_closed, - ) = get_per_issue_metrics(issues, env_vars=get_env_vars(test=True)) + ) = get_per_issue_metrics( + issues, + env_vars=get_env_vars(test=True), + ) expected_issues_with_metrics = [ IssueWithMetrics( "Issue 1", @@ -337,6 +461,49 @@ def test_get_per_issue_metrics_with_discussion(self): self.assertEqual(metrics[0][1].time_to_close, timedelta(days=6)) self.assertEqual(metrics[0][1].time_to_first_response, timedelta(days=2)) + @patch.dict( + os.environ, + { + "GH_TOKEN": "test_token", + "SEARCH_QUERY": "is:issue is:open repo:user/repo", + "HIDE_AUTHOR": "true", + "HIDE_LABEL_METRICS": "true", + "HIDE_TIME_TO_ANSWER": "true", + "HIDE_TIME_TO_CLOSE": "true", + "HIDE_TIME_TO_FIRST_RESPONSE": "true", + }, + ) + def test_get_per_issue_metrics_with_discussion_with_hide_envs(self): + """ + Test that the function correctly calculates + the metrics for a list of GitHub issues with discussions + and HIDE_* env vars set to True + """ + + issues = [self.issue1, self.issue2] + metrics = get_per_issue_metrics( + issues, discussions=True, env_vars=get_env_vars(test=True) + ) + + # get_per_issue_metrics returns a tuple of + # (issues_with_metrics, num_issues_open, num_issues_closed) + self.assertEqual(len(metrics), 3) + + # Check that the metrics are correct, 0 issues open, 2 issues closed + self.assertEqual(metrics[1], 0) + self.assertEqual(metrics[2], 2) + + # Check that the issues_with_metrics has 2 issues in it + self.assertEqual(len(metrics[0]), 2) + + # Check that the issues_with_metrics has the correct metrics, + self.assertEqual(metrics[0][0].time_to_answer, None) + self.assertEqual(metrics[0][0].time_to_close, None) + self.assertEqual(metrics[0][0].time_to_first_response, None) + self.assertEqual(metrics[0][1].time_to_answer, None) + self.assertEqual(metrics[0][1].time_to_close, None) + self.assertEqual(metrics[0][1].time_to_first_response, None) + if __name__ == "__main__": unittest.main() diff --git a/test_markdown_writer.py b/test_markdown_writer.py index 053dc0a..3ebb692 100644 --- a/test_markdown_writer.py +++ b/test_markdown_writer.py @@ -297,7 +297,7 @@ def test_writes_markdown_file_with_non_hidden_columns_only(self): "label1": timedelta(days=1), } num_issues_opened = 2 - num_issues_closed = 1 + num_issues_closed = 2 num_mentor_count = 5 # Call the function @@ -323,7 +323,7 @@ def test_writes_markdown_file_with_non_hidden_columns_only(self): "| Metric | Count |\n" "| --- | ---: |\n" "| Number of items that remain open | 2 |\n" - "| Number of items closed | 1 |\n" + "| Number of items closed | 2 |\n" "| Number of most active mentors | 5 |\n" "| Total number of items created | 2 |\n\n" "| Title | URL | Author |\n" From a987f816c64cf8a7ca9265332d558a7cb4ca27b9 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 11 Jun 2024 08:59:44 -0700 Subject: [PATCH 2/3] chore: readable and pythonic logic Co-authored-by: jmeridth --- issue_metrics.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/issue_metrics.py b/issue_metrics.py index 0d7645f..60cc3ea 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -193,10 +193,10 @@ def get_per_issue_metrics( ) if env_vars.hide_time_to_answer is False: issue_with_metrics.time_to_answer = measure_time_to_answer(issue) - if env_vars.hide_time_to_close is False and issue["closedAt"]: - issue_with_metrics.time_to_close = measure_time_to_close(None, issue) if issue["closedAt"]: num_issues_closed += 1 + if not env_vars.hide_time_to_close: + issue_with_metrics.time_to_close = measure_time_to_close(None, issue) else: num_issues_open += 1 else: @@ -234,17 +234,17 @@ def get_per_issue_metrics( ) if labels and env_vars.hide_label_metrics is False: issue_with_metrics.label_metrics = get_label_metrics(issue, labels) - if env_vars.hide_time_to_close is False and issue.state == "closed": # type: ignore - if pull_request: - issue_with_metrics.time_to_close = measure_time_to_merge( - pull_request, ready_for_review_at - ) - else: - issue_with_metrics.time_to_close = measure_time_to_close( - issue, None - ) if issue.state == "closed": # type: ignore num_issues_closed += 1 + if not env_vars.hide_time_to_close: + if pull_request: + issue_with_metrics.time_to_close = measure_time_to_merge( + pull_request, ready_for_review_at + ) + else: + issue_with_metrics.time_to_close = measure_time_to_close( + issue, None + ) elif issue.state == "open": # type: ignore num_issues_open += 1 issues_with_metrics.append(issue_with_metrics) From 7e2cae419502993fd25c129335fa1f6240dbe65c Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Tue, 11 Jun 2024 09:27:24 -0700 Subject: [PATCH 3/3] chore: linting Signed-off-by: Zack Koppert --- issue_metrics.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/issue_metrics.py b/issue_metrics.py index 60cc3ea..dfe57f2 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -196,7 +196,9 @@ def get_per_issue_metrics( if issue["closedAt"]: num_issues_closed += 1 if not env_vars.hide_time_to_close: - issue_with_metrics.time_to_close = measure_time_to_close(None, issue) + issue_with_metrics.time_to_close = measure_time_to_close( + None, issue + ) else: num_issues_open += 1 else: