Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add COMMENT_TITLE env var for issue comment title #33

Merged
merged 7 commits into from
Oct 27, 2020
Merged

Add COMMENT_TITLE env var for issue comment title #33

merged 7 commits into from
Oct 27, 2020

Conversation

mas-wtag
Copy link
Contributor

@mas-wtag mas-wtag commented Oct 21, 2020 •
edited by EnricoMi
Loading

This PR allows hard coded issue comment title Unit Test Results to be changed when COMMENT_TITLE env var is defined.

@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   0s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

results for commit 3df6353 ± comparison against base commit de7f7f0

@EnricoMi
Copy link
Owner

Thanks for this PR. I was wondering if that comment title should be configurable independently from the check name, maybe through a COMMENT_TITLE env var. The check run name might make sense in the Actions tab but less in a PR comment and vice versa. What do you think?

@mas-wtag
Copy link
Contributor Author

Hi @EnricoMi , thanks for taking a look to the PR.

I was wondering if that comment title should be configurable independently from the check name, maybe through a COMMENT_TITLE env var.

  • Sure, I also think that would be a good way forward. I'll make the necessary adjustments in code.

@EnricoMi EnricoMi changed the title use CHECK_NAME env var for issue comment title Add COMMIT_NAME env var for issue comment title Oct 26, 2020
@EnricoMi EnricoMi changed the title Add COMMIT_NAME env var for issue comment title Add COMMIT_TITLE env var for issue comment title Oct 26, 2020
@EnricoMi EnricoMi changed the title Add COMMIT_TITLE env var for issue comment title Add COMMENT_TITLE env var for issue comment title Oct 26, 2020
@EnricoMi EnricoMi mentioned this pull request Oct 26, 2020
Closed
@EnricoMi
Copy link
Owner

Issue #42 made me think the comment title should use CHECK_NAME if no COMMENT_TITLE is given, otherwise COMMENT_TITLE. This allows to use different title and check run name if needed, or the same if sufficient.

@EnricoMi EnricoMi mentioned this pull request Oct 26, 2020
Merged
@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   0s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

results for commit b81988f ± comparison against base commit de7f7f0

@mas-wtag
Copy link
Contributor Author

Issue #42 made me think the comment title should use CHECK_NAME if no COMMENT_TITLE is given, otherwise COMMENT_TITLE. This allows to use different title and check run name if needed, or the same if sufficient.

@EnricoMi please have a look and let me know if it's all good 👍

EnricoMi
EnricoMi requested changes Oct 27, 2020
View reviewed changes
Copy link
Owner

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor changes, looks good.

The README.md also needs to mention the new option. Please add it to the example in https://github.com/EnricoMi/publish-unit-test-result-action/blob/master/README.md#using-this-action and put a short description in a new paragraph below check_name and mention that it defaults to check_names value.

publish_unit_test_results.py Outdated
@@ -693,7 +693,7 @@ def hide_comment(comment_node_id) -> bool:
comments = list([comment for comment in comments
if comment.get('author', {}).get('login') == 'github-actions'
and comment.get('isMinimized') is False
and comment.get('body', '').startswith('## Unit Test Results\n')
and comment.get('body', '').startswith('## {}\n'.format(check_name))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should read issue_comment_title, rather than check_name, because this looks at the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch :)

publish_unit_test_results.py Outdated
@@ -751,7 +751,7 @@ def main(token: str, event: dict, repo: str, commit: str, files_glob: str, check
stats = get_stats(results)

# publish the delta stats
publish(token, event, repo, commit, stats, results['case_results'], check_name, report_individual_runs)
publish(token, event, repo, commit, stats, results['case_results'], issue_comment_title, check_name, report_individual_runs)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

publish_unit_test_results.py Outdated
@@ -735,7 +735,7 @@ def write_stats_file(stats, filename) -> None:
f.write(json.dumps(stats))


def main(token: str, event: dict, repo: str, commit: str, files_glob: str, check_name: str,
def main(token: str, event: dict, repo: str, commit: str, files_glob: str, issue_comment_title: str, check_name: str,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this comment_title throughout the file? The title inside publish_comment is fine. And can check_name come first, then comment_title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, i just thought to name it differently since it shadows the name from the outer scope

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I am going to refactor that code soon, no shadowing then.

@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   0s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

results for commit 92d2bee ± comparison against base commit de7f7f0

@EnricoMi EnricoMi mentioned this pull request Oct 27, 2020
Closed
@EnricoMi
Copy link
Owner

EnricoMi commented Oct 27, 2020 •
edited
Loading

All looks good, but one unit test fails, not in your PR #33, but when I pulled your forked branch into my repo for testing, the same commit suddenly fails (#45). I will care about that later.

Please fix get_short_summary not knowing check_name when called form a unit test (because it is defined in __main__):
https://github.com/EnricoMi/publish-unit-test-result-action/actions/runs/331044367

The default argument should be provided to get_short_summary as an argument.

You can run the unit tests locally with

cd test
PYTHONPATH=.. python -m pytest

@EnricoMi
Copy link
Owner

The action works as expected:

@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   0s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

results for commit c1c4b51 ± comparison against base commit de7f7f0

@EnricoMi EnricoMi merged commit 1f19dc5 into EnricoMi:master Oct 27, 2020
@EnricoMi
Copy link
Owner

Thanks for the contribution!

@github-actions

This comment has been minimized.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EnricoMi EnricoMi EnricoMi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mas-wtag @EnricoMi