-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Output customizations: REPORT_TITLE and OUTPUT_FILE #160
Changes from all commits
1693462
ae1df11
5c5cd84
8b227f0
98d9842
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,7 +216,16 @@ def test_write_to_markdown_no_issues(self): | |
"""Test that write_to_markdown writes the correct markdown file when no issues are found.""" | ||
# Call the function with no issues | ||
with patch("builtins.open", mock_open()) as mock_open_file: | ||
write_to_markdown(None, None, None, None, None, None, None) | ||
# write_to_markdown(None, None, None, None, None, None, None) | ||
write_to_markdown( | ||
issues_with_metrics=None, | ||
average_time_to_first_response=None, | ||
average_time_to_close=None, | ||
average_time_to_answer=None, | ||
average_time_in_labels=None, | ||
num_issues_opened=None, | ||
num_issues_closed=None, | ||
) | ||
|
||
# Check that the file was written correctly | ||
expected_output = [ | ||
|
@@ -324,3 +333,106 @@ def test_writes_markdown_file_with_non_hidden_columns_only(self): | |
) | ||
self.assertEqual(content, expected_content) | ||
os.remove("issue_metrics.md") | ||
|
||
|
||
class TestWriteToMarkdownWithOutputCustomization(unittest.TestCase): | ||
"""Test the write_to_markdown function with the REPORT_TITLE environment variables set.""" | ||
|
||
def setUp(self): | ||
os.environ["REPORT_TITLE"] = "My Custom Report Title" | ||
|
||
def tearDown(self): | ||
os.environ.pop("REPORT_TITLE") | ||
|
||
def test_writes_markdown_file_with_output_customization(self): | ||
""" | ||
Test that write_to_markdown writes the correct | ||
markdown file. | ||
""" | ||
|
||
# Create mock data | ||
issues_with_metrics = [ | ||
IssueWithMetrics( | ||
"Issue 1", | ||
"https://github.com/user/repo/issues/1", | ||
"alice", | ||
timedelta(days=1), | ||
timedelta(days=2), | ||
timedelta(days=3), | ||
{"bug": timedelta(days=1)}, | ||
), | ||
IssueWithMetrics( | ||
"feat| Issue 2", # title contains a vertical bar | ||
"https://github.com/user/repo/issues/2", | ||
"bob", | ||
timedelta(days=3), | ||
timedelta(days=4), | ||
timedelta(days=5), | ||
{"bug": timedelta(days=2)}, | ||
), | ||
] | ||
average_time_to_first_response = { | ||
"avg": timedelta(days=2), | ||
"med": timedelta(days=2), | ||
"90p": timedelta(days=2), | ||
} | ||
average_time_to_close = { | ||
"avg": timedelta(days=3), | ||
"med": timedelta(days=3), | ||
"90p": timedelta(days=3), | ||
} | ||
average_time_to_answer = { | ||
"avg": timedelta(days=4), | ||
"med": timedelta(days=4), | ||
"90p": timedelta(days=4), | ||
} | ||
average_time_in_labels = { | ||
"avg": {"bug": "1 day, 12:00:00"}, | ||
"med": {"bug": "1 day, 12:00:00"}, | ||
"90p": {"bug": "1 day, 12:00:00"}, | ||
} | ||
|
||
num_issues_opened = 2 | ||
num_issues_closed = 1 | ||
|
||
# Call the function | ||
write_to_markdown( | ||
issues_with_metrics=issues_with_metrics, | ||
average_time_to_first_response=average_time_to_first_response, | ||
average_time_to_close=average_time_to_close, | ||
average_time_to_answer=average_time_to_answer, | ||
average_time_in_labels=average_time_in_labels, | ||
num_issues_opened=num_issues_opened, | ||
num_issues_closed=num_issues_closed, | ||
labels=["bug"], | ||
report_title="My Custom Report Title", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zkoppert I applied the suggested changes. That means moving the default value for the report title into the write_to_markdown() signature. It does lead to a better code structure overall, I think. However one issue remains in this area: Btw the same issue exists in the testing logic for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think overall I am a bit unclear about the intended logic of using ENV vars in the code. Some ENV vars are read in various functions in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we would consolidate all env variables being read from the get_env_vars() function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this PR it would be good to follow that pattern. We can open a separate PR to get everything using the proper |
||
) | ||
|
||
# Check that the function writes the correct markdown file | ||
with open("issue_metrics.md", "r", encoding="utf-8") as file: | ||
content = file.read() | ||
expected_content = ( | ||
"# My Custom Report Title\n\n" | ||
"| Metric | Average | Median | 90th percentile |\n" | ||
"| --- | --- | --- | ---: |\n" | ||
"| Time to first response | 2 days, 0:00:00 | 2 days, 0:00:00 | 2 days, 0:00:00 |\n" | ||
"| Time to close | 3 days, 0:00:00 | 3 days, 0:00:00 | 3 days, 0:00:00 |\n" | ||
"| Time to answer | 4 days, 0:00:00 | 4 days, 0:00:00 | 4 days, 0:00:00 |\n" | ||
"| Time spent in bug | 1 day, 12:00:00 | 1 day, 12:00:00 | 1 day, 12:00:00 |\n" | ||
"\n" | ||
"| Metric | Count |\n" | ||
"| --- | ---: |\n" | ||
"| Number of items that remain open | 2 |\n" | ||
"| Number of items closed | 1 |\n" | ||
"| Total number of items created | 2 |\n\n" | ||
"| Title | URL | Author | Time to first response | Time to close |" | ||
" Time to answer | Time spent in bug |\n" | ||
"| --- | --- | --- | --- | --- | --- | --- |\n" | ||
"| Issue 1 | https://github.com/user/repo/issues/1 | alice | 1 day, 0:00:00 | " | ||
"2 days, 0:00:00 | 3 days, 0:00:00 | 1 day, 0:00:00 |\n" | ||
"| feat| Issue 2 | https://github.com/user/repo/issues/2 | bob | 3 days, 0:00:00 | " | ||
"4 days, 0:00:00 | 5 days, 0:00:00 | 2 days, 0:00:00 |\n\n" | ||
"_This report was generated with the [Issue Metrics Action](https://github.com/github/issue-metrics)_\n" | ||
) | ||
self.assertEqual(content, expected_content) | ||
os.remove("issue_metrics.md") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zkoppert I found these traces in the documentation.
Apparently at some point in the past, a file object was passed to the
write_to_markdown()
.Do you happen to know anything about this?
As I am looking to add the feature to set a custom OUTPUT_FILE, I don't want to go down an implementation path that might have been already tried in the past (and did not work). :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a link to the commit where I removed that unused bit of documentation. GitHub did not give me a straightforward way to get there from my comment above.
5c5cd84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that was aspirational. I don't think we actually wrote the code to support that.