From a46a5f553c2c64dce31055a2b11be0662aed8ddb Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Fri, 15 Apr 2022 21:53:39 +0200 Subject: [PATCH 01/26] Allow to only comment on changes, failures, ... --- python/publish/__init__.py | 11 ++ python/publish/publisher.py | 67 +++++++-- python/publish/unittestresults.py | 1 + python/publish_unit_test_results.py | 5 +- python/test/test_action_script.py | 15 +- python/test/test_publisher.py | 226 +++++++++++++++++++++++++--- 6 files changed, 282 insertions(+), 43 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index a615fd7d..81249fa4 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -27,6 +27,17 @@ comment_mode_update ] +comment_condition_always = 'always' +comment_condition_changes = 'changes' +comment_condition_failures = 'test failures' +comment_condition_errors = 'test errors' +comment_conditions = [ + comment_condition_always, + comment_condition_changes, + comment_condition_failures, + comment_condition_errors +] + fail_on_mode_nothing = 'nothing' fail_on_mode_errors = 'errors' fail_on_mode_failures = 'test failures' diff --git a/python/publish/publisher.py b/python/publish/publisher.py index a17d9e16..68940bd0 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -14,6 +14,7 @@ from publish import hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, hide_comments_mode_off, \ comment_mode_off, comment_mode_create, comment_mode_update, digest_prefix, restrict_unicode_list, \ + comment_condition_always, comment_condition_changes, comment_condition_failures, comment_condition_errors, \ get_stats_from_digest, digest_header, get_short_summary, get_long_summary_md, \ get_long_summary_with_digest_md, get_error_annotations, get_case_annotations, \ get_all_tests_list_annotation, get_skipped_tests_list_annotation, get_all_tests_list, \ @@ -44,6 +45,7 @@ class Settings: check_name: str comment_title: str comment_mode: str + comment_condition: str job_summary: bool compare_earlier: bool pull_request_build: str @@ -56,6 +58,12 @@ class Settings: seconds_between_github_reads: float seconds_between_github_writes: float + def require_comment(self, data: 'PublishData'): + return (self.comment_condition == comment_condition_always or + self.comment_condition == comment_condition_changes and data.has_changes or + self.comment_condition == comment_condition_failures and (data.has_failures or data.has_errors) or + self.comment_condition == comment_condition_errors and data.has_errors) + @dataclasses.dataclass(frozen=True) class PublishData: @@ -121,6 +129,30 @@ def reduce(d: Dict[str, Any]) -> Dict[str, Any]: )) return data + @property + def has_changes(self): + return (self.stats_with_delta is None or + any([field.get('delta') for field in [self.stats_with_delta.files, + self.stats_with_delta.suites, + self.stats_with_delta.tests, + self.stats_with_delta.tests_succ, + self.stats_with_delta.tests_skip, + self.stats_with_delta.tests_fail, + self.stats_with_delta.tests_error, + self.stats_with_delta.runs, + self.stats_with_delta.runs_succ, + self.stats_with_delta.runs_skip, + self.stats_with_delta.runs_fail, + self.stats_with_delta.runs_error]])) + + @property + def has_failures(self): + return self.stats.tests_fail > 0 or self.stats.runs_fail > 0 + + @property + def has_errors(self): + return len(self.stats.errors) > 0 or self.stats.tests_error > 0 or self.stats.runs_error > 0 + class Publisher: @@ -136,26 +168,29 @@ def publish(self, cases: UnitTestCaseResults, conclusion: str): logger.info(f'publishing {conclusion} results for commit {self._settings.commit}') - check_run, before_check_run = self.publish_check(stats, cases, conclusion) + check_run, before_check_run, test_data = self.publish_check(stats, cases, conclusion) if self._settings.job_summary: self.publish_job_summary(self._settings.comment_title, stats, check_run, before_check_run) - if self._settings.comment_mode != comment_mode_off: - pulls = self.get_pulls(self._settings.commit) - if pulls: - for pull in pulls: - self.publish_comment(self._settings.comment_title, stats, pull, check_run, cases) - if self._settings.hide_comment_mode == hide_comments_mode_orphaned: - self.hide_orphaned_commit_comments(pull) - elif self._settings.hide_comment_mode == hide_comments_mode_all_but_latest: - self.hide_all_but_latest_comments(pull) - if self._settings.hide_comment_mode == hide_comments_mode_off: - logger.info('hide_comments disabled, not hiding any comments') + if self._settings.require_comment(test_data): + if self._settings.comment_mode != comment_mode_off: + pulls = self.get_pulls(self._settings.commit) + if pulls: + for pull in pulls: + self.publish_comment(self._settings.comment_title, stats, pull, check_run, cases) + if self._settings.hide_comment_mode == hide_comments_mode_orphaned: + self.hide_orphaned_commit_comments(pull) + elif self._settings.hide_comment_mode == hide_comments_mode_all_but_latest: + self.hide_all_but_latest_comments(pull) + if self._settings.hide_comment_mode == hide_comments_mode_off: + logger.info('hide_comments disabled, not hiding any comments') + else: + logger.info(f'there is no pull request for commit {self._settings.commit}') else: - logger.info(f'there is no pull request for commit {self._settings.commit}') + logger.info('comment_on_pr disabled, not commenting on any pull requests') else: - logger.info('comment_on_pr disabled, not commenting on any pull requests') + logger.info(f'No comment required as comment_condition is {self._settings.comment_condition}') def get_pulls(self, commit: str) -> List[PullRequest]: # totalCount calls the GitHub API just to get the total number @@ -280,7 +315,7 @@ def get_test_list_from_annotation(annotation: CheckRunAnnotation) -> Optional[Li def publish_check(self, stats: UnitTestRunResults, cases: UnitTestCaseResults, - conclusion: str) -> Tuple[CheckRun, Optional[CheckRun]]: + conclusion: str) -> Tuple[CheckRun, Optional[CheckRun], PublishData]: # get stats from earlier commits before_stats = None before_check_run = None @@ -335,7 +370,7 @@ def publish_check(self, logger.debug(f'updating check with {len(annotations)} more annotations') check_run.edit(output=output) logger.debug(f'updated check') - return check_run, before_check_run + return check_run, before_check_run, data def publish_json(self, data: PublishData): if self._settings.json_file: diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index 8bd54508..b45640c2 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -131,6 +131,7 @@ def without_cases(self): tests_errors=self.suite_errors, ) + @dataclass(frozen=True) class UnitTestResults(ParsedUnitTestResultsWithCommit): cases: int diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index b8d26ca2..7fa681ce 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -16,7 +16,8 @@ import publish.github_action from publish import hide_comments_modes, available_annotations, default_annotations, \ pull_request_build_modes, fail_on_modes, fail_on_mode_errors, fail_on_mode_failures, \ - comment_mode_off, comment_mode_update, comment_modes, punctuation_space + comment_mode_off, comment_mode_update, comment_modes, comment_condition_always, comment_conditions, \ + punctuation_space from publish.github_action import GithubAction from publish.junit import parse_junit_xml_files from publish.progress import progress_logger @@ -319,6 +320,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_name=check_name, comment_title=get_var('COMMENT_TITLE', options) or check_name, comment_mode=get_var('COMMENT_MODE', options) or (comment_mode_update if comment_on_pr else comment_mode_off), + comment_condition=get_var('COMMENT_CONDITION', options) or comment_condition_always, job_summary=get_bool_var('JOB_SUMMARY', options, default=True, gha=gha), compare_earlier=get_bool_var('COMPARE_TO_EARLIER_COMMIT', options, default=True, gha=gha), pull_request_build=get_var('PULL_REQUEST_BUILD', options) or 'merge', @@ -336,6 +338,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') check_var(settings.comment_mode, 'COMMENT_MODE', 'Commit mode', comment_modes) + check_var(settings.comment_condition, 'COMMENT_CONDITION', 'Commit condition', comment_conditions) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index 23a3b661..8c2d8e76 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -10,7 +10,7 @@ from publish import pull_request_build_mode_merge, fail_on_mode_failures, fail_on_mode_errors, \ fail_on_mode_nothing, comment_mode_off, comment_mode_create, comment_mode_update, \ - hide_comments_modes, pull_request_build_modes, punctuation_space + comment_condition_always, comment_conditions, hide_comments_modes, pull_request_build_modes, punctuation_space from publish.github_action import GithubAction from publish.unittestresults import ParsedUnitTestResults, ParseError from publish_unit_test_results import get_conclusion, get_commit_sha, get_var, \ @@ -146,6 +146,7 @@ def get_settings(token='token', check_name='check name', comment_title='title', comment_mode=comment_mode_create, + comment_condition=comment_condition_always, job_summary=True, compare_earlier=True, test_changes_limit=10, @@ -178,6 +179,7 @@ def get_settings(token='token', check_name=check_name, comment_title=comment_title, comment_mode=comment_mode, + comment_condition=comment_condition, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -329,6 +331,17 @@ def test_get_settings_comment_mode(self): self.do_test_get_settings(COMMENT_MODE='mode') self.assertEqual("Value 'mode' is not supported for variable COMMENT_MODE, expected: off, create new, update last", str(re.exception)) + def test_get_settings_comment_condition(self): + for cond in comment_conditions: + with self.subTest(condition=cond): + self.do_test_get_settings(COMMENT_CONDITION=cond, expected=self.get_settings(comment_condition=cond)) + + self.do_test_get_settings(COMMENT_CONDITION=None, expected=self.get_settings(comment_condition=comment_condition_always)) + + with self.assertRaises(RuntimeError) as re: + self.do_test_get_settings(COMMENT_CONDITION='condition') + self.assertEqual(f"Value 'condition' is not supported for variable COMMENT_CONDITION, expected: always, changes, test failures, test errors", str(re.exception)) + def test_get_settings_compare_to_earlier_commit(self): warning = 'Option compare_to_earlier_commit has to be boolean, so either "true" or "false": foo' self.do_test_get_settings(COMPARE_TO_EARLIER_COMMIT='false', expected=self.get_settings(compare_earlier=False)) diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 33294e5a..9f249d7f 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -16,6 +16,71 @@ errors = [ParseError('file', 'error', 1, 2)] +def create_unit_test_run_results(files=1, + errors: List[ParseError] = [], + suites=2, + duration=3, + tests=22, tests_succ=4, tests_skip=5, tests_fail=6, tests_error=7, + runs=38, runs_succ=8, runs_skip=9, runs_fail=10, runs_error=11) -> UnitTestRunResults: + return UnitTestRunResults( + files=files, + errors=list(errors), + suites=suites, + duration=duration, + tests=tests, tests_succ=tests_succ, tests_skip=tests_skip, tests_fail=tests_fail, tests_error=tests_error, + runs=runs, runs_succ=runs_succ, runs_skip=runs_skip, runs_fail=runs_fail, runs_error=runs_error, + commit='commit' + ) + + +def create_unit_test_run_delta_results(files_delta=-1, + errors=[], + suites_delta=-2, + duration_delta=-3, + tests_delta=-4, + tests_succ_delta=-5, + tests_skip_delta=-6, + tests_fail_delta=-7, + tests_error_delta=-8, + runs_delta=-9, + runs_succ_delta=-10, + runs_skip_delta=-11, + runs_fail_delta=-12, + runs_error_delta=-13) -> UnitTestRunDeltaResults: + return UnitTestRunDeltaResults( + files={'number': 1, 'delta': files_delta}, + errors=errors, + suites={'number': 2, 'delta': suites_delta}, + duration={'number': 3, 'delta': duration_delta}, + tests={'number': 4, 'delta': tests_delta}, tests_succ={'number': 5, 'delta': tests_succ_delta}, tests_skip={'number': 6, 'delta': tests_skip_delta}, tests_fail={'number': 7, 'delta': tests_fail_delta}, tests_error={'number': 8, 'delta': tests_error_delta}, + runs={'number': 9, 'delta': runs_delta}, runs_succ={'number': 10, 'delta': runs_succ_delta}, runs_skip={'number': 11, 'delta': runs_skip_delta}, runs_fail={'number': 12, 'delta': runs_fail_delta}, runs_error={'number': 13, 'delta': runs_error_delta}, + commit='commit', + reference_type='type', reference_commit='ref' + ) + + +def create_publish_data(stats: Optional[UnitTestRunResults] = create_unit_test_run_results(), + stats_with_delta: Optional[UnitTestRunDeltaResults] = create_unit_test_run_delta_results()) -> PublishData: + return PublishData( + title='title', + summary='summary', + conclusion='conclusion', + stats=stats, + stats_with_delta=stats_with_delta, + annotations=[Annotation( + path='path', + start_line=1, + end_line=2, + start_column=3, + end_column=4, + annotation_level='failure', + message='message', + title=f'Error processing result file', + raw_details='file' + )] + ) + + class TestPublisher(unittest.TestCase): @staticmethod @@ -44,6 +109,7 @@ def create_github_pr(repo: str, @staticmethod def create_settings(comment_mode=comment_mode_create, + comment_condition=comment_condition_always, job_summary=True, compare_earlier=True, hide_comment_mode=hide_comments_mode_off, @@ -75,6 +141,7 @@ def create_settings(comment_mode=comment_mode_create, check_name='Check Name', comment_title='Comment Title', comment_mode=comment_mode, + comment_condition=comment_condition, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -88,26 +155,7 @@ def create_settings(comment_mode=comment_mode_create, seconds_between_github_writes=2.5 ) - stats = UnitTestRunResults( - files=1, - errors=[], - suites=2, - duration=3, - - tests=22, - tests_succ=4, - tests_skip=5, - tests_fail=6, - tests_error=7, - - runs=38, - runs_succ=8, - runs_skip=9, - runs_fail=10, - runs_error=11, - - commit='commit' - ) + stats = create_unit_test_run_results() def create_mocks(self, repo_name: Optional[str] = None, @@ -235,7 +283,7 @@ def call_mocked_publish(settings: Settings, publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_pulls = mock.Mock(return_value=prs) - publisher.publish_check = mock.Mock(return_value=(cr, None)) + publisher.publish_check = mock.Mock(return_value=(cr, None, mock.MagicMock())) Publisher.publish(publisher, stats, cases, 'success') # return calls to mocked instance, except call to _logger @@ -282,6 +330,43 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) + @staticmethod + def create_publish_data_mock(has_changes=False, has_failures=False, has_errors=False): + data = mock.MagicMock() + data.has_changes = has_changes + data.has_failures = has_failures + data.has_errors = has_errors + return data + + def test_settings_require_comment_always(self): + settings = self.create_settings(comment_condition=comment_condition_always) + self.assertEqual(settings.require_comment(self.create_publish_data_mock()), True) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) + + def test_settings_require_comment_changes(self): + settings = self.create_settings(comment_condition=comment_condition_changes) + self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), True) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) + + def test_settings_require_comment_failures(self): + settings = self.create_settings(comment_condition=comment_condition_failures) + self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), True) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), True) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) + + def test_settings_require_comment_errors(self): + settings = self.create_settings(comment_condition=comment_condition_errors) + self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), False) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), True) + self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) + def test_publish_without_comment(self): settings = self.create_settings(comment_mode=comment_mode_off, hide_comment_mode=hide_comments_mode_off) mock_calls = self.call_mocked_publish(settings, prs=[object()]) @@ -343,6 +428,35 @@ def test_publish_with_comment_without_pr(self): self.assertEqual((settings.commit, ), args) self.assertEqual({}, kwargs) + def test_publish_with_comment_without_pr_with_require_comment(self): + # pretty much same as test_publish_with_comment_without_pr, except that we vary settings.require_comment + for require in [False, True]: + with self.subTest(require=require): + with mock.patch('publish.publisher.Settings.require_comment') as m: + m.return_value = require + + settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_comments_mode_off) + mock_calls = self.call_mocked_publish(settings, prs=[]) + + self.assertEqual(3 if require else 2, len(mock_calls)) + + (method, args, kwargs) = mock_calls[0] + self.assertEqual('publish_check', method) + self.assertEqual((self.stats, self.cases, 'success'), args) + self.assertEqual({}, kwargs) + + (method, args, kwargs) = mock_calls[1] + self.assertEqual('publish_job_summary', method) + self.assertEqual((settings.comment_title, self.stats, None, None), args) + self.assertEqual({}, kwargs) + + # we should only see a call for get_pulls if require_comment returns true + if require: + (method, args, kwargs) = mock_calls[2] + self.assertEqual('get_pulls', method) + self.assertEqual((settings.commit, ), args) + self.assertEqual({}, kwargs) + def test_publish_with_comment_without_hiding(self): pr = object() cr = object() @@ -1157,7 +1271,7 @@ def do_test_publish_check_without_base_stats(self, errors: List[ParseError], ann # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') + check_run, before_check_run, _ = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') repo.get_commit.assert_not_called() error_annotations = [get_error_annotation(error).to_dict() for error in errors] @@ -1236,7 +1350,7 @@ def do_test_publish_check_with_base_stats(self, errors: List[ParseError]): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') + check_run, before_check_run, _ = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') repo.get_commit.assert_called_once_with(earlier_commit) error_annotations = [get_error_annotation(error).to_dict() for error in errors] @@ -1303,7 +1417,7 @@ def test_publish_check_without_compare(self): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run = publisher.publish_check(self.stats, self.cases, 'conclusion') + check_run, before_check_run, _ = publisher.publish_check(self.stats, self.cases, 'conclusion') repo.get_commit.assert_not_called() create_check_run_kwargs = dict( @@ -1363,7 +1477,7 @@ def test_publish_check_with_multiple_annotation_pages(self): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run = publisher.publish_check(self.stats, cases, 'conclusion') + check_run, before_check_run, _ = publisher.publish_check(self.stats, cases, 'conclusion') repo.get_commit.assert_called_once_with(earlier_commit) # we expect a single call to create_check_run @@ -1769,6 +1883,68 @@ def test_publish_job_summary_with_before(self): 'Results for commit commit.\u2003± Comparison against earlier commit before.\n', ), args) self.assertEqual({}, kwargs) + def test_publish_data_has_changes(self): + def create_stats_with_delta(files_delta=0, + suites_delta=0, + duration_delta=0, + tests_delta=0, + tests_succ_delta=0, + tests_skip_delta=0, + tests_fail_delta=0, + tests_error_delta=0, + runs_delta=0, + runs_succ_delta=0, + runs_skip_delta=0, + runs_fail_delta=0, + runs_error_delta=0) -> UnitTestRunDeltaResults: + return create_unit_test_run_delta_results(files_delta=files_delta, suites_delta=suites_delta, duration_delta=duration_delta, + tests_delta=tests_delta, tests_succ_delta=tests_succ_delta, tests_skip_delta=tests_skip_delta, tests_fail_delta=tests_fail_delta, tests_error_delta=tests_error_delta, + runs_delta=runs_delta, runs_succ_delta=runs_succ_delta, runs_skip_delta=runs_skip_delta, runs_fail_delta=runs_fail_delta, runs_error_delta=runs_error_delta) + + for label, data, expected in [('no stats with deltas', create_publish_data(stats_with_delta=None), True), + ('no deltas', create_publish_data(stats_with_delta=create_stats_with_delta()), False), + ('files', create_publish_data(stats_with_delta=create_stats_with_delta(files_delta=1)), True), + ('suites', create_publish_data(stats_with_delta=create_stats_with_delta(suites_delta=1)), True), + ('duration', create_publish_data(stats_with_delta=create_stats_with_delta(duration_delta=1)), False), + ('tests', create_publish_data(stats_with_delta=create_stats_with_delta(tests_delta=1)), True), + ('tests succ', create_publish_data(stats_with_delta=create_stats_with_delta(tests_succ_delta=1)), True), + ('tests skip', create_publish_data(stats_with_delta=create_stats_with_delta(tests_skip_delta=1)), True), + ('tests fail', create_publish_data(stats_with_delta=create_stats_with_delta(tests_fail_delta=1)), True), + ('tests error', create_publish_data(stats_with_delta=create_stats_with_delta(tests_error_delta=1)), True), + ('runs', create_publish_data(stats_with_delta=create_stats_with_delta(runs_delta=1)), True), + ('runs succ', create_publish_data(stats_with_delta=create_stats_with_delta(runs_succ_delta=1)), True), + ('runs skip', create_publish_data(stats_with_delta=create_stats_with_delta(runs_skip_delta=1)), True), + ('runs fail', create_publish_data(stats_with_delta=create_stats_with_delta(runs_fail_delta=1)), True), + ('runs error', create_publish_data(stats_with_delta=create_stats_with_delta(runs_error_delta=1)), True)]: + with self.subTest(msg=label): + self.assertEqual(data.has_changes, expected, msg=label) + + def test_publish_data_has_failures(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, data, expected in [('no failures', create_publish_data(stats=create_stats()), False), + ('errors', create_publish_data(stats=create_stats(errors=errors)), False), + ('test failures', create_publish_data(stats=create_stats(tests_fail=1)), True), + ('test errors', create_publish_data(stats=create_stats(tests_error=1)), False), + ('runs failures', create_publish_data(stats=create_stats(runs_fail=1)), True), + ('runs errors', create_publish_data(stats=create_stats(runs_error=1)), False)]: + with self.subTest(msg=label): + self.assertEqual(data.has_failures, expected, msg=label) + + def test_publish_data_has_errors(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, data, expected in [('no errors', create_publish_data(stats=create_stats()), False), + ('errors', create_publish_data(stats=create_stats(errors=errors)), True), + ('test failures', create_publish_data(stats=create_stats(tests_fail=1)), False), + ('test errors', create_publish_data(stats=create_stats(tests_error=1)), True), + ('runs failures', create_publish_data(stats=create_stats(runs_fail=1)), False), + ('runs errors', create_publish_data(stats=create_stats(runs_error=1)), True)]: + with self.subTest(msg=label): + self.assertEqual(data.has_errors, expected, msg=label) + def test_publish_comment(self): settings = self.create_settings(event={'pull_request': {'base': {'sha': 'commit base'}}}, event_name='pull_request') base_commit = 'base-commit' From f1125fcfac6d9b920fb028a199af5db1d93e9693 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 17 Apr 2022 14:59:10 +0200 Subject: [PATCH 02/26] Add new option as 'comment_on' to action.yml --- .github/workflows/ci-cd.yml | 2 +- README.md | 1 + action.yml | 4 ++++ composite/action.yml | 5 +++++ python/publish_unit_test_results.py | 4 ++-- python/test/test_action_script.py | 8 ++++---- 6 files changed, 17 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index f0ad0aad..c9b90fb6 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -202,7 +202,7 @@ jobs: id: test-results if: always() run: | - docker run --workdir $GITHUB_WORKSPACE --rm -e INPUT_CHECK_NAME -e INPUT_FILES -e INPUT_TIME_UNIT -e INPUT_GITHUB_TOKEN -e INPUT_GITHUB_RETRIES -e INPUT_COMMIT -e INPUT_COMMENT_TITLE -e INPUT_FAIL_ON -e INPUT_REPORT_INDIVIDUAL_RUNS -e INPUT_DEDUPLICATE_CLASSES_BY_FILE_NAME -e INPUT_IGNORE_RUNS -e INPUT_HIDE_COMMENTS -e INPUT_COMMENT_ON_PR -e INPUT_COMMENT_MODE -e INPUT_COMPARE_TO_EARLIER_COMMIT -e INPUT_PULL_REQUEST_BUILD -e INPUT_EVENT_FILE -e INPUT_EVENT_NAME -e INPUT_TEST_CHANGES_LIMIT -e INPUT_CHECK_RUN_ANNOTATIONS -e INPUT_CHECK_RUN_ANNOTATIONS_BRANCH -e INPUT_SECONDS_BETWEEN_GITHUB_READS -e INPUT_SECONDS_BETWEEN_GITHUB_WRITES -e INPUT_JSON_FILE -e INPUT_JSON_THOUSANDS_SEPARATOR -e INPUT_JOB_SUMMARY -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e GITHUB_STEP_SUMMARY -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "$RUNNER_TEMP":"$RUNNER_TEMP" -v "$GITHUB_WORKSPACE":"$GITHUB_WORKSPACE" enricomi/publish-unit-test-result-action:latest + docker run --workdir $GITHUB_WORKSPACE --rm -e INPUT_CHECK_NAME -e INPUT_FILES -e INPUT_TIME_UNIT -e INPUT_GITHUB_TOKEN -e INPUT_GITHUB_RETRIES -e INPUT_COMMIT -e INPUT_COMMENT_TITLE -e INPUT_COMMENT_ON -e INPUT_FAIL_ON -e INPUT_REPORT_INDIVIDUAL_RUNS -e INPUT_DEDUPLICATE_CLASSES_BY_FILE_NAME -e INPUT_IGNORE_RUNS -e INPUT_HIDE_COMMENTS -e INPUT_COMMENT_ON_PR -e INPUT_COMMENT_MODE -e INPUT_COMPARE_TO_EARLIER_COMMIT -e INPUT_PULL_REQUEST_BUILD -e INPUT_EVENT_FILE -e INPUT_EVENT_NAME -e INPUT_TEST_CHANGES_LIMIT -e INPUT_CHECK_RUN_ANNOTATIONS -e INPUT_CHECK_RUN_ANNOTATIONS_BRANCH -e INPUT_SECONDS_BETWEEN_GITHUB_READS -e INPUT_SECONDS_BETWEEN_GITHUB_WRITES -e INPUT_JSON_FILE -e INPUT_JSON_THOUSANDS_SEPARATOR -e INPUT_JOB_SUMMARY -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e GITHUB_STEP_SUMMARY -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "$RUNNER_TEMP":"$RUNNER_TEMP" -v "$GITHUB_WORKSPACE":"$GITHUB_WORKSPACE" enricomi/publish-unit-test-result-action:latest env: INPUT_GITHUB_TOKEN: ${{ github.token }} INPUT_CHECK_NAME: Test Results (Docker Image) diff --git a/README.md b/README.md index 3b4f13e8..0c597e55 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,7 @@ The list of most notable options: |:-----|:-----:|:----------| |`time_unit`|`seconds`|Time values in the XML files have this unit. Supports `seconds` and `milliseconds`.| |`job_summary`|`true`| Set to `true`, the results are published as part of the [job summary page](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) of the workflow run.| +|`comment_on`|`always`|Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".| |`comment_mode`|`update last`|The action posts comments to a pull request that is associated with the commit. Set to `create new` to create a new comment on each commit, `update last` to create only one comment and update later on, `off` to not create pull request comments.| |`hide_comments`|`"all but latest"`|Configures which earlier comments in a pull request are hidden by the action:
`"orphaned commits"` - comments for removed commits
`"all but latest"` - all comments but the latest
`"off"` - no hiding| |`compare_to_earlier_commit`|`true`|Test results are compared to results of earlier commits to show changes:
`false` - disable comparison, `true` - compare across commits.'| diff --git a/action.yml b/action.yml index 9d4f090a..61282422 100644 --- a/action.yml +++ b/action.yml @@ -21,6 +21,10 @@ inputs: comment_title: description: 'Title of PR comments, defaults to value of check_name input' required: false + comment_on: + description: 'Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + default: 'always' + required: false fail_on: description: 'The created test result check run has failure state if any test fails or test errors occur. Never fails when set to "nothing", fails only on errors when set to "errors". Default is "test failures".' default: 'test failures' diff --git a/composite/action.yml b/composite/action.yml index dc171219..d1cece72 100644 --- a/composite/action.yml +++ b/composite/action.yml @@ -21,6 +21,10 @@ inputs: comment_title: description: 'Title of PR comments, defaults to value of check_name input' required: false + comment_on: + description: 'Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + default: 'always' + required: false fail_on: description: 'The created test result check run has failure state if any test fails or test errors occur. Never fails when set to "nothing", fails only on errors when set to "errors". Default is "test failures".' default: 'test failures' @@ -149,6 +153,7 @@ runs: COMMIT: ${{ inputs.commit }} CHECK_NAME: ${{ inputs.check_name }} COMMENT_TITLE: ${{ inputs.comment_title }} + COMMENT_ON: ${{ inputs.comment_on }} FAIL_ON: ${{ inputs.fail_on }} FILES: ${{ inputs.files }} TIME_UNIT: ${{ inputs.time_unit }} diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index 7fa681ce..d82b94a9 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -320,7 +320,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_name=check_name, comment_title=get_var('COMMENT_TITLE', options) or check_name, comment_mode=get_var('COMMENT_MODE', options) or (comment_mode_update if comment_on_pr else comment_mode_off), - comment_condition=get_var('COMMENT_CONDITION', options) or comment_condition_always, + comment_condition=get_var('COMMENT_ON', options) or comment_condition_always, job_summary=get_bool_var('JOB_SUMMARY', options, default=True, gha=gha), compare_earlier=get_bool_var('COMPARE_TO_EARLIER_COMMIT', options, default=True, gha=gha), pull_request_build=get_var('PULL_REQUEST_BUILD', options) or 'merge', @@ -338,7 +338,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') check_var(settings.comment_mode, 'COMMENT_MODE', 'Commit mode', comment_modes) - check_var(settings.comment_condition, 'COMMENT_CONDITION', 'Commit condition', comment_conditions) + check_var(settings.comment_condition, 'COMMENT_ON', 'Commit condition', comment_conditions) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index 8c2d8e76..b7176551 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -334,13 +334,13 @@ def test_get_settings_comment_mode(self): def test_get_settings_comment_condition(self): for cond in comment_conditions: with self.subTest(condition=cond): - self.do_test_get_settings(COMMENT_CONDITION=cond, expected=self.get_settings(comment_condition=cond)) + self.do_test_get_settings(COMMENT_ON=cond, expected=self.get_settings(comment_condition=cond)) - self.do_test_get_settings(COMMENT_CONDITION=None, expected=self.get_settings(comment_condition=comment_condition_always)) + self.do_test_get_settings(COMMENT_ON=None, expected=self.get_settings(comment_condition=comment_condition_always)) with self.assertRaises(RuntimeError) as re: - self.do_test_get_settings(COMMENT_CONDITION='condition') - self.assertEqual(f"Value 'condition' is not supported for variable COMMENT_CONDITION, expected: always, changes, test failures, test errors", str(re.exception)) + self.do_test_get_settings(COMMENT_ON='condition') + self.assertEqual(f"Value 'condition' is not supported for variable COMMENT_ON, expected: always, changes, test failures, test errors", str(re.exception)) def test_get_settings_compare_to_earlier_commit(self): warning = 'Option compare_to_earlier_commit has to be boolean, so either "true" or "false": foo' From 0f328a21e471836beddcdb747150c97dd03200bc Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 18 Apr 2022 15:54:49 +0100 Subject: [PATCH 03/26] Reworking changes and failure logic - Move require_comment from Settings into Publisher - Move has_changes into UnitTestRunDeltaResults - Add has_changes to SomeTestChanges - Update tests --- python/publish/__init__.py | 5 + python/publish/publisher.py | 81 +++---- python/publish/unittestresults.py | 15 ++ python/test/test_publish.py | 11 + python/test/test_publisher.py | 358 ++++++++++++---------------- python/test/test_unittestresults.py | 105 ++++++++ 6 files changed, 322 insertions(+), 253 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index 81249fa4..e9c71e09 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -91,6 +91,11 @@ def __init__(self, self._skipped_tests_before = set(skipped_tests_before) if skipped_tests_before is not None else None self._skipped_tests_current = set(skipped_tests_current) if skipped_tests_current is not None else None + @property + def has_changes(self) -> bool: + return (self.adds() is not None and self.removes() is not None and len(self.adds().union(self.removes())) > 0 or + self.skips() is not None and self.un_skips() is not None and len(self.skips().union(self.un_skips())) > 0) + def adds(self) -> Optional[Set[str]]: if self._all_tests_before is None or self._all_tests_current is None: return None diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 68940bd0..6adca58c 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -58,12 +58,6 @@ class Settings: seconds_between_github_reads: float seconds_between_github_writes: float - def require_comment(self, data: 'PublishData'): - return (self.comment_condition == comment_condition_always or - self.comment_condition == comment_condition_changes and data.has_changes or - self.comment_condition == comment_condition_failures and (data.has_failures or data.has_errors) or - self.comment_condition == comment_condition_errors and data.has_errors) - @dataclasses.dataclass(frozen=True) class PublishData: @@ -129,30 +123,6 @@ def reduce(d: Dict[str, Any]) -> Dict[str, Any]: )) return data - @property - def has_changes(self): - return (self.stats_with_delta is None or - any([field.get('delta') for field in [self.stats_with_delta.files, - self.stats_with_delta.suites, - self.stats_with_delta.tests, - self.stats_with_delta.tests_succ, - self.stats_with_delta.tests_skip, - self.stats_with_delta.tests_fail, - self.stats_with_delta.tests_error, - self.stats_with_delta.runs, - self.stats_with_delta.runs_succ, - self.stats_with_delta.runs_skip, - self.stats_with_delta.runs_fail, - self.stats_with_delta.runs_error]])) - - @property - def has_failures(self): - return self.stats.tests_fail > 0 or self.stats.runs_fail > 0 - - @property - def has_errors(self): - return len(self.stats.errors) > 0 or self.stats.tests_error > 0 or self.stats.runs_error > 0 - class Publisher: @@ -168,29 +138,26 @@ def publish(self, cases: UnitTestCaseResults, conclusion: str): logger.info(f'publishing {conclusion} results for commit {self._settings.commit}') - check_run, before_check_run, test_data = self.publish_check(stats, cases, conclusion) + check_run, before_check_run = self.publish_check(stats, cases, conclusion) if self._settings.job_summary: self.publish_job_summary(self._settings.comment_title, stats, check_run, before_check_run) - if self._settings.require_comment(test_data): - if self._settings.comment_mode != comment_mode_off: - pulls = self.get_pulls(self._settings.commit) - if pulls: - for pull in pulls: - self.publish_comment(self._settings.comment_title, stats, pull, check_run, cases) - if self._settings.hide_comment_mode == hide_comments_mode_orphaned: - self.hide_orphaned_commit_comments(pull) - elif self._settings.hide_comment_mode == hide_comments_mode_all_but_latest: - self.hide_all_but_latest_comments(pull) - if self._settings.hide_comment_mode == hide_comments_mode_off: - logger.info('hide_comments disabled, not hiding any comments') - else: - logger.info(f'there is no pull request for commit {self._settings.commit}') + if self._settings.comment_mode != comment_mode_off: + pulls = self.get_pulls(self._settings.commit) + if pulls: + for pull in pulls: + self.publish_comment(self._settings.comment_title, stats, pull, check_run, cases) + if self._settings.hide_comment_mode == hide_comments_mode_orphaned: + self.hide_orphaned_commit_comments(pull) + elif self._settings.hide_comment_mode == hide_comments_mode_all_but_latest: + self.hide_all_but_latest_comments(pull) + if self._settings.hide_comment_mode == hide_comments_mode_off: + logger.info('hide_comments disabled, not hiding any comments') else: - logger.info('comment_on_pr disabled, not commenting on any pull requests') + logger.info(f'there is no pull request for commit {self._settings.commit}') else: - logger.info(f'No comment required as comment_condition is {self._settings.comment_condition}') + logger.info('comment_on_pr disabled, not commenting on any pull requests') def get_pulls(self, commit: str) -> List[PullRequest]: # totalCount calls the GitHub API just to get the total number @@ -315,7 +282,7 @@ def get_test_list_from_annotation(annotation: CheckRunAnnotation) -> Optional[Li def publish_check(self, stats: UnitTestRunResults, cases: UnitTestCaseResults, - conclusion: str) -> Tuple[CheckRun, Optional[CheckRun], PublishData]: + conclusion: str) -> Tuple[CheckRun, Optional[CheckRun]]: # get stats from earlier commits before_stats = None before_check_run = None @@ -370,7 +337,7 @@ def publish_check(self, logger.debug(f'updating check with {len(annotations)} more annotations') check_run.edit(output=output) logger.debug(f'updated check') - return check_run, before_check_run, data + return check_run, before_check_run def publish_json(self, data: PublishData): if self._settings.json_file: @@ -455,7 +422,7 @@ def publish_comment(self, stats: UnitTestRunResults, pull_request: PullRequest, check_run: Optional[CheckRun] = None, - cases: Optional[UnitTestCaseResults] = None) -> PullRequest: + cases: Optional[UnitTestCaseResults] = None): # compare them with earlier stats base_check_run = None if self._settings.compare_earlier: @@ -478,6 +445,11 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) + # are we required to create a comment on this PR? + if not self.require_comment(stats, stats_with_delta, test_changes): + logger.info(f'No comment required as comment_on is {self._settings.comment_condition}') + return + details_url = check_run.html_url if check_run else None summary = get_long_summary_with_digest_md(stats_with_delta, stats, details_url, test_changes, self._settings.test_changes_limit) body = f'## {title}\n{summary}' @@ -488,7 +460,14 @@ def publish_comment(self, comment = pull_request.create_issue_comment(body) logger.info(f'created comment for pull request #{pull_request.number}: {comment.html_url}') - return pull_request + def require_comment(self, + stats: UnitTestRunResults, + stats_with_delta: UnitTestRunDeltaResults, + test_changes: SomeTestChanges) -> bool: + return (self._settings.comment_condition == comment_condition_always or + self._settings.comment_condition == comment_condition_changes and (stats_with_delta is None or stats_with_delta.has_changes or test_changes.has_changes) or + self._settings.comment_condition == comment_condition_failures and (stats.has_failures or stats.has_errors) or + self._settings.comment_condition == comment_condition_errors and stats.has_errors) def reuse_comment(self, pull: PullRequest, body: str) -> bool: # get comments of this pull request diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index b45640c2..cb84ff7f 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -168,6 +168,14 @@ class UnitTestRunResults: commit: str + @property + def has_failures(self): + return self.tests_fail > 0 or self.runs_fail > 0 + + @property + def has_errors(self): + return len(self.errors) > 0 or self.tests_error > 0 or self.runs_error > 0 + def with_errors(self, errors: List[ParseError]): return UnitTestRunResults( files=self.files, @@ -247,6 +255,13 @@ class UnitTestRunDeltaResults: def to_dict(self) -> Dict[str, Any]: return dataclasses.asdict(self) + @property + def has_changes(self): + return (any([field.get('delta') + for field in [self.files, self.suites, + self.tests, self.tests_succ, self.tests_skip, self.tests_fail, self.tests_error, + self.runs, self.runs_succ, self.runs_skip, self.runs_fail, self.runs_error]])) + UnitTestRunResultsOrDeltaResults = Union[UnitTestRunResults, UnitTestRunDeltaResults] diff --git a/python/test/test_publish.py b/python/test/test_publish.py index f9de1027..5dd08691 100644 --- a/python/test/test_publish.py +++ b/python/test/test_publish.py @@ -106,6 +106,17 @@ def test_test_changes_has_no_tests(self): self.assertEqual(SomeTestChanges(default, [], default, ['two']).has_no_tests(), True) self.assertEqual(SomeTestChanges(default, ['one'], default, ['two']).has_no_tests(), False) + def test_test_changes_has_changes(self): + for changes, expected in [(SomeTestChanges(None, None, None, None), False), + (SomeTestChanges([], [], [], []), False), + (SomeTestChanges(['one'], ['one'], ['two'], ['two']), False), + (SomeTestChanges(['one'], ['three'], ['two'], ['two']), True), + (SomeTestChanges(['one'], ['one'], ['two'], ['three']), True), + (SomeTestChanges(['one'], ['two'], ['two'], ['three']), True), + (SomeTestChanges(['one'], None, ['two'], None), False), + (SomeTestChanges(None, ['one'], None, ['two']), False)]: + self.assertEqual(changes.has_changes, expected, str(changes)) + def test_restrict_unicode(self): self.assertEqual(None, restrict_unicode(None)) self.assertEqual('', restrict_unicode('')) diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 9f249d7f..58d41591 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -1,3 +1,4 @@ +import itertools import os import tempfile import unittest @@ -16,71 +17,6 @@ errors = [ParseError('file', 'error', 1, 2)] -def create_unit_test_run_results(files=1, - errors: List[ParseError] = [], - suites=2, - duration=3, - tests=22, tests_succ=4, tests_skip=5, tests_fail=6, tests_error=7, - runs=38, runs_succ=8, runs_skip=9, runs_fail=10, runs_error=11) -> UnitTestRunResults: - return UnitTestRunResults( - files=files, - errors=list(errors), - suites=suites, - duration=duration, - tests=tests, tests_succ=tests_succ, tests_skip=tests_skip, tests_fail=tests_fail, tests_error=tests_error, - runs=runs, runs_succ=runs_succ, runs_skip=runs_skip, runs_fail=runs_fail, runs_error=runs_error, - commit='commit' - ) - - -def create_unit_test_run_delta_results(files_delta=-1, - errors=[], - suites_delta=-2, - duration_delta=-3, - tests_delta=-4, - tests_succ_delta=-5, - tests_skip_delta=-6, - tests_fail_delta=-7, - tests_error_delta=-8, - runs_delta=-9, - runs_succ_delta=-10, - runs_skip_delta=-11, - runs_fail_delta=-12, - runs_error_delta=-13) -> UnitTestRunDeltaResults: - return UnitTestRunDeltaResults( - files={'number': 1, 'delta': files_delta}, - errors=errors, - suites={'number': 2, 'delta': suites_delta}, - duration={'number': 3, 'delta': duration_delta}, - tests={'number': 4, 'delta': tests_delta}, tests_succ={'number': 5, 'delta': tests_succ_delta}, tests_skip={'number': 6, 'delta': tests_skip_delta}, tests_fail={'number': 7, 'delta': tests_fail_delta}, tests_error={'number': 8, 'delta': tests_error_delta}, - runs={'number': 9, 'delta': runs_delta}, runs_succ={'number': 10, 'delta': runs_succ_delta}, runs_skip={'number': 11, 'delta': runs_skip_delta}, runs_fail={'number': 12, 'delta': runs_fail_delta}, runs_error={'number': 13, 'delta': runs_error_delta}, - commit='commit', - reference_type='type', reference_commit='ref' - ) - - -def create_publish_data(stats: Optional[UnitTestRunResults] = create_unit_test_run_results(), - stats_with_delta: Optional[UnitTestRunDeltaResults] = create_unit_test_run_delta_results()) -> PublishData: - return PublishData( - title='title', - summary='summary', - conclusion='conclusion', - stats=stats, - stats_with_delta=stats_with_delta, - annotations=[Annotation( - path='path', - start_line=1, - end_line=2, - start_column=3, - end_column=4, - annotation_level='failure', - message='message', - title=f'Error processing result file', - raw_details='file' - )] - ) - - class TestPublisher(unittest.TestCase): @staticmethod @@ -155,7 +91,26 @@ def create_settings(comment_mode=comment_mode_create, seconds_between_github_writes=2.5 ) - stats = create_unit_test_run_results() + stats = UnitTestRunResults( + files=1, + errors=[], + suites=2, + duration=3, + + tests=22, + tests_succ=4, + tests_skip=5, + tests_fail=6, + tests_error=7, + + runs=38, + runs_succ=8, + runs_skip=9, + runs_fail=10, + runs_error=11, + + commit='commit' + ) def create_mocks(self, repo_name: Optional[str] = None, @@ -283,7 +238,7 @@ def call_mocked_publish(settings: Settings, publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_pulls = mock.Mock(return_value=prs) - publisher.publish_check = mock.Mock(return_value=(cr, None, mock.MagicMock())) + publisher.publish_check = mock.Mock(return_value=(cr, None)) Publisher.publish(publisher, stats, cases, 'success') # return calls to mocked instance, except call to _logger @@ -330,42 +285,102 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) - @staticmethod - def create_publish_data_mock(has_changes=False, has_failures=False, has_errors=False): - data = mock.MagicMock() - data.has_changes = has_changes - data.has_failures = has_failures - data.has_errors = has_errors - return data - - def test_settings_require_comment_always(self): - settings = self.create_settings(comment_condition=comment_condition_always) - self.assertEqual(settings.require_comment(self.create_publish_data_mock()), True) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) - - def test_settings_require_comment_changes(self): - settings = self.create_settings(comment_condition=comment_condition_changes) - self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), True) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) - - def test_settings_require_comment_failures(self): - settings = self.create_settings(comment_condition=comment_condition_failures) - self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), True) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), True) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) - - def test_settings_require_comment_errors(self): - settings = self.create_settings(comment_condition=comment_condition_errors) - self.assertEqual(settings.require_comment(self.create_publish_data_mock()), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True)), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_failures=True)), False) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_errors=True)), True) - self.assertEqual(settings.require_comment(self.create_publish_data_mock(has_changes=True, has_failures=True, has_errors=True)), True) + def do_test_require_comment(self, comment_condition, tests): + publisher = mock.MagicMock(Publisher) + publisher._settings = self.create_settings(comment_condition=comment_condition) + + for has_failures, has_errors, stats_has_changes, tests_has_changes, expected in tests: + with self.subTest(has_failures=has_failures, has_errors=has_errors, + stats_has_changes=stats_has_changes, tests_has_changes=tests_has_changes): + stats = mock.MagicMock(has_failures=has_failures, has_errors=has_errors) + stats_with_delta = mock.MagicMock(has_changes=stats_has_changes) + test_changes = mock.MagicMock(has_changes=tests_has_changes) + required = Publisher.require_comment(publisher, stats, stats_with_delta, test_changes) + self.assertEqual(required, expected) + + def test_require_comment_always(self): + self.do_test_require_comment(comment_condition_always, [ + # failures, errors, changes, test changes + (False, False, False, False, True), + (False, False, False, True, True), + (False, False, True, False, True), + (False, False, True, True, True), + (False, True, False, False, True), + (False, True, False, True, True), + (False, True, True, False, True), + (False, True, True, True, True), + (True, False, False, False, True), + (True, False, False, True, True), + (True, False, True, False, True), + (True, False, True, True, True), + (True, True, False, False, True), + (True, True, False, True, True), + (True, True, True, False, True), + (True, True, True, True, True) + ]) + + def test_require_comment_changes(self): + self.do_test_require_comment(comment_condition_changes, [ + # failures, errors, changes, test changes + (False, False, False, False, False), + (False, False, False, True, True), + (False, False, True, False, True), + (False, False, True, True, True), + (False, True, False, False, False), + (False, True, False, True, True), + (False, True, True, False, True), + (False, True, True, True, True), + (True, False, False, False, False), + (True, False, False, True, True), + (True, False, True, False, True), + (True, False, True, True, True), + (True, True, False, False, False), + (True, True, False, True, True), + (True, True, True, False, True), + (True, True, True, True, True) + ]) + + def test_require_comment_failures(self): + self.do_test_require_comment(comment_condition_failures, [ + # failures, errors, changes, test changes + (False, False, False, False, False), + (False, False, False, True, False), + (False, False, True, False, False), + (False, False, True, True, False), + (False, True, False, False, True), + (False, True, False, True, True), + (False, True, True, False, True), + (False, True, True, True, True), + (True, False, False, False, True), + (True, False, False, True, True), + (True, False, True, False, True), + (True, False, True, True, True), + (True, True, False, False, True), + (True, True, False, True, True), + (True, True, True, False, True), + (True, True, True, True, True) + ]) + + def test_require_comment_errors(self): + self.do_test_require_comment(comment_condition_errors, [ + # failures, errors, changes, test changes + (False, False, False, False, False), + (False, False, False, True, False), + (False, False, True, False, False), + (False, False, True, True, False), + (False, True, False, False, True), + (False, True, False, True, True), + (False, True, True, False, True), + (False, True, True, True, True), + (True, False, False, False, False), + (True, False, False, True, False), + (True, False, True, False, False), + (True, False, True, True, False), + (True, True, False, False, True), + (True, True, False, True, True), + (True, True, True, False, True), + (True, True, True, True, True) + ]) def test_publish_without_comment(self): settings = self.create_settings(comment_mode=comment_mode_off, hide_comment_mode=hide_comments_mode_off) @@ -428,35 +443,6 @@ def test_publish_with_comment_without_pr(self): self.assertEqual((settings.commit, ), args) self.assertEqual({}, kwargs) - def test_publish_with_comment_without_pr_with_require_comment(self): - # pretty much same as test_publish_with_comment_without_pr, except that we vary settings.require_comment - for require in [False, True]: - with self.subTest(require=require): - with mock.patch('publish.publisher.Settings.require_comment') as m: - m.return_value = require - - settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_comments_mode_off) - mock_calls = self.call_mocked_publish(settings, prs=[]) - - self.assertEqual(3 if require else 2, len(mock_calls)) - - (method, args, kwargs) = mock_calls[0] - self.assertEqual('publish_check', method) - self.assertEqual((self.stats, self.cases, 'success'), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[1] - self.assertEqual('publish_job_summary', method) - self.assertEqual((settings.comment_title, self.stats, None, None), args) - self.assertEqual({}, kwargs) - - # we should only see a call for get_pulls if require_comment returns true - if require: - (method, args, kwargs) = mock_calls[2] - self.assertEqual('get_pulls', method) - self.assertEqual((settings.commit, ), args) - self.assertEqual({}, kwargs) - def test_publish_with_comment_without_hiding(self): pr = object() cr = object() @@ -578,11 +564,12 @@ def test_publish_comment_compare_earlier(self): publisher.get_stats_delta = mock.Mock(return_value=bs) publisher.get_base_commit_sha = mock.Mock(return_value="base commit") publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.require_comment = mock.Mock(return_value=True) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(4, len(mock_calls)) + self.assertEqual(5, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -604,6 +591,9 @@ def test_publish_comment_compare_earlier(self): self.assertEqual((bcr, ), args) self.assertEqual({}, kwargs) + (method, args, kwargs) = mock_calls[4] + self.assertEqual('require_comment', method) + mock_calls = pr.mock_calls self.assertEqual(3, len(mock_calls)) @@ -645,6 +635,7 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): publisher.get_stats_from_check_run = mock.Mock(return_value=bs) publisher.get_stats_delta = mock.Mock(return_value=bs) publisher.get_base_commit_sha = mock.Mock(return_value="base commit") + publisher.require_comment = mock.Mock(return_value=True) # the earlier test cases with restricted unicode as they come from the check runs API publisher.get_test_lists_from_check_run = mock.Mock(return_value=( # before, these existed: test 𝒂, test 𝒃, skipped 𝒄, skipped 𝒅 @@ -659,7 +650,7 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): mock_calls = publisher.mock_calls - self.assertEqual(4, len(mock_calls)) + self.assertEqual(5, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -681,6 +672,9 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): self.assertEqual((bcr, ), args) self.assertEqual({}, kwargs) + (method, args, kwargs) = mock_calls[4] + self.assertEqual('require_comment', method) + mock_calls = pr.mock_calls self.assertEqual(3, len(mock_calls)) @@ -779,11 +773,12 @@ def test_publish_comment_compare_with_None(self): publisher.get_check_run = mock.Mock(return_value=None) publisher.get_base_commit_sha = mock.Mock(return_value=None) publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.require_comment = mock.Mock(return_value=True) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(3, len(mock_calls)) + self.assertEqual(4, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -800,6 +795,9 @@ def test_publish_comment_compare_with_None(self): self.assertEqual((None, ), args) self.assertEqual({}, kwargs) + (method, args, kwargs) = mock_calls[3] + self.assertEqual('require_comment', method) + mock_calls = pr.mock_calls self.assertEqual(3, len(mock_calls)) @@ -828,11 +826,12 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): publisher._settings = settings publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) publisher.reuse_comment = mock.Mock(return_value=one_exists) + publisher.require_comment = mock.Mock(return_value=True) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(2, len(mock_calls)) + self.assertEqual(3, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_test_lists_from_check_run', method) @@ -840,6 +839,9 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[1] + self.assertEqual('require_comment', method) + + (method, args, kwargs) = mock_calls[2] self.assertEqual('reuse_comment', method) self.assertEqual((pr, '## title\nbody'), args) self.assertEqual({}, kwargs) @@ -1271,7 +1273,7 @@ def do_test_publish_check_without_base_stats(self, errors: List[ParseError], ann # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run, _ = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') + check_run, before_check_run = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') repo.get_commit.assert_not_called() error_annotations = [get_error_annotation(error).to_dict() for error in errors] @@ -1350,7 +1352,7 @@ def do_test_publish_check_with_base_stats(self, errors: List[ParseError]): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run, _ = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') + check_run, before_check_run = publisher.publish_check(self.stats.with_errors(errors), self.cases, 'conclusion') repo.get_commit.assert_called_once_with(earlier_commit) error_annotations = [get_error_annotation(error).to_dict() for error in errors] @@ -1417,7 +1419,7 @@ def test_publish_check_without_compare(self): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run, _ = publisher.publish_check(self.stats, self.cases, 'conclusion') + check_run, before_check_run = publisher.publish_check(self.stats, self.cases, 'conclusion') repo.get_commit.assert_not_called() create_check_run_kwargs = dict( @@ -1477,7 +1479,7 @@ def test_publish_check_with_multiple_annotation_pages(self): # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): - check_run, before_check_run, _ = publisher.publish_check(self.stats, cases, 'conclusion') + check_run, before_check_run = publisher.publish_check(self.stats, cases, 'conclusion') repo.get_commit.assert_called_once_with(earlier_commit) # we expect a single call to create_check_run @@ -1883,68 +1885,6 @@ def test_publish_job_summary_with_before(self): 'Results for commit commit.\u2003± Comparison against earlier commit before.\n', ), args) self.assertEqual({}, kwargs) - def test_publish_data_has_changes(self): - def create_stats_with_delta(files_delta=0, - suites_delta=0, - duration_delta=0, - tests_delta=0, - tests_succ_delta=0, - tests_skip_delta=0, - tests_fail_delta=0, - tests_error_delta=0, - runs_delta=0, - runs_succ_delta=0, - runs_skip_delta=0, - runs_fail_delta=0, - runs_error_delta=0) -> UnitTestRunDeltaResults: - return create_unit_test_run_delta_results(files_delta=files_delta, suites_delta=suites_delta, duration_delta=duration_delta, - tests_delta=tests_delta, tests_succ_delta=tests_succ_delta, tests_skip_delta=tests_skip_delta, tests_fail_delta=tests_fail_delta, tests_error_delta=tests_error_delta, - runs_delta=runs_delta, runs_succ_delta=runs_succ_delta, runs_skip_delta=runs_skip_delta, runs_fail_delta=runs_fail_delta, runs_error_delta=runs_error_delta) - - for label, data, expected in [('no stats with deltas', create_publish_data(stats_with_delta=None), True), - ('no deltas', create_publish_data(stats_with_delta=create_stats_with_delta()), False), - ('files', create_publish_data(stats_with_delta=create_stats_with_delta(files_delta=1)), True), - ('suites', create_publish_data(stats_with_delta=create_stats_with_delta(suites_delta=1)), True), - ('duration', create_publish_data(stats_with_delta=create_stats_with_delta(duration_delta=1)), False), - ('tests', create_publish_data(stats_with_delta=create_stats_with_delta(tests_delta=1)), True), - ('tests succ', create_publish_data(stats_with_delta=create_stats_with_delta(tests_succ_delta=1)), True), - ('tests skip', create_publish_data(stats_with_delta=create_stats_with_delta(tests_skip_delta=1)), True), - ('tests fail', create_publish_data(stats_with_delta=create_stats_with_delta(tests_fail_delta=1)), True), - ('tests error', create_publish_data(stats_with_delta=create_stats_with_delta(tests_error_delta=1)), True), - ('runs', create_publish_data(stats_with_delta=create_stats_with_delta(runs_delta=1)), True), - ('runs succ', create_publish_data(stats_with_delta=create_stats_with_delta(runs_succ_delta=1)), True), - ('runs skip', create_publish_data(stats_with_delta=create_stats_with_delta(runs_skip_delta=1)), True), - ('runs fail', create_publish_data(stats_with_delta=create_stats_with_delta(runs_fail_delta=1)), True), - ('runs error', create_publish_data(stats_with_delta=create_stats_with_delta(runs_error_delta=1)), True)]: - with self.subTest(msg=label): - self.assertEqual(data.has_changes, expected, msg=label) - - def test_publish_data_has_failures(self): - def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: - return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) - - for label, data, expected in [('no failures', create_publish_data(stats=create_stats()), False), - ('errors', create_publish_data(stats=create_stats(errors=errors)), False), - ('test failures', create_publish_data(stats=create_stats(tests_fail=1)), True), - ('test errors', create_publish_data(stats=create_stats(tests_error=1)), False), - ('runs failures', create_publish_data(stats=create_stats(runs_fail=1)), True), - ('runs errors', create_publish_data(stats=create_stats(runs_error=1)), False)]: - with self.subTest(msg=label): - self.assertEqual(data.has_failures, expected, msg=label) - - def test_publish_data_has_errors(self): - def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: - return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) - - for label, data, expected in [('no errors', create_publish_data(stats=create_stats()), False), - ('errors', create_publish_data(stats=create_stats(errors=errors)), True), - ('test failures', create_publish_data(stats=create_stats(tests_fail=1)), False), - ('test errors', create_publish_data(stats=create_stats(tests_error=1)), True), - ('runs failures', create_publish_data(stats=create_stats(runs_fail=1)), False), - ('runs errors', create_publish_data(stats=create_stats(runs_error=1)), True)]: - with self.subTest(msg=label): - self.assertEqual(data.has_errors, expected, msg=label) - def test_publish_comment(self): settings = self.create_settings(event={'pull_request': {'base': {'sha': 'commit base'}}}, event_name='pull_request') base_commit = 'base-commit' @@ -1969,6 +1909,20 @@ def test_publish_comment(self): f'{expected_digest}' ) + def test_publish_comment_not_required(self): + # same as test_publish_comment but test_publish_comment returns False + with mock.patch('publish.publisher.Publisher.require_comment', return_value=False): + settings = self.create_settings(event={'pull_request': {'base': {'sha': 'commit base'}}}, event_name='pull_request') + base_commit = 'base-commit' + + gh, gha, req, repo, commit = self.create_mocks(digest=self.base_digest, check_names=[settings.check_name]) + pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) + publisher = Publisher(settings, gh, gha) + + publisher.publish_comment(settings.comment_title, self.stats, pr) + + pr.create_issue_comment.assert_not_called() + def test_publish_comment_without_base(self): settings = self.create_settings() diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index 99dfabe2..d179b88c 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -1,4 +1,5 @@ import dataclasses +from typing import List import unittest from xml.etree.ElementTree import ParseError as XmlParseError @@ -12,6 +13,49 @@ errors_dict = [dataclasses.asdict(e) for e in errors] +def create_unit_test_run_results(files=1, + errors: List[ParseError] = [], + suites=2, + duration=3, + tests=22, tests_succ=4, tests_skip=5, tests_fail=6, tests_error=7, + runs=38, runs_succ=8, runs_skip=9, runs_fail=10, runs_error=11) -> UnitTestRunResults: + return UnitTestRunResults( + files=files, + errors=list(errors), + suites=suites, + duration=duration, + tests=tests, tests_succ=tests_succ, tests_skip=tests_skip, tests_fail=tests_fail, tests_error=tests_error, + runs=runs, runs_succ=runs_succ, runs_skip=runs_skip, runs_fail=runs_fail, runs_error=runs_error, + commit='commit' + ) + + +def create_unit_test_run_delta_results(files_delta=-1, + errors=[], + suites_delta=-2, + duration_delta=-3, + tests_delta=-4, + tests_succ_delta=-5, + tests_skip_delta=-6, + tests_fail_delta=-7, + tests_error_delta=-8, + runs_delta=-9, + runs_succ_delta=-10, + runs_skip_delta=-11, + runs_fail_delta=-12, + runs_error_delta=-13) -> UnitTestRunDeltaResults: + return UnitTestRunDeltaResults( + files={'number': 1, 'delta': files_delta}, + errors=errors, + suites={'number': 2, 'delta': suites_delta}, + duration={'number': 3, 'delta': duration_delta}, + tests={'number': 4, 'delta': tests_delta}, tests_succ={'number': 5, 'delta': tests_succ_delta}, tests_skip={'number': 6, 'delta': tests_skip_delta}, tests_fail={'number': 7, 'delta': tests_fail_delta}, tests_error={'number': 8, 'delta': tests_error_delta}, + runs={'number': 9, 'delta': runs_delta}, runs_succ={'number': 10, 'delta': runs_succ_delta}, runs_skip={'number': 11, 'delta': runs_skip_delta}, runs_fail={'number': 12, 'delta': runs_fail_delta}, runs_error={'number': 13, 'delta': runs_error_delta}, + commit='commit', + reference_type='type', reference_commit='ref' + ) + + class TestUnitTestResults(unittest.TestCase): def test_parse_error_from_xml_parse_error(self): @@ -429,3 +473,64 @@ def test_get_stats_delta(self): reference_commit='ref', reference_type='type' )) + + def test_unit_test_run_delta_results_has_changes(self): + def create_stats_with_delta(files_delta=0, + suites_delta=0, + duration_delta=0, + tests_delta=0, + tests_succ_delta=0, + tests_skip_delta=0, + tests_fail_delta=0, + tests_error_delta=0, + runs_delta=0, + runs_succ_delta=0, + runs_skip_delta=0, + runs_fail_delta=0, + runs_error_delta=0) -> UnitTestRunDeltaResults: + return create_unit_test_run_delta_results(files_delta=files_delta, suites_delta=suites_delta, duration_delta=duration_delta, + tests_delta=tests_delta, tests_succ_delta=tests_succ_delta, tests_skip_delta=tests_skip_delta, tests_fail_delta=tests_fail_delta, tests_error_delta=tests_error_delta, + runs_delta=runs_delta, runs_succ_delta=runs_succ_delta, runs_skip_delta=runs_skip_delta, runs_fail_delta=runs_fail_delta, runs_error_delta=runs_error_delta) + + for label, stats, expected in [('no deltas', create_stats_with_delta(), False), + ('files', create_stats_with_delta(files_delta=1), True), + ('suites', create_stats_with_delta(suites_delta=1), True), + ('duration', create_stats_with_delta(duration_delta=1), False), + ('tests', create_stats_with_delta(tests_delta=1), True), + ('tests succ', create_stats_with_delta(tests_succ_delta=1), True), + ('tests skip', create_stats_with_delta(tests_skip_delta=1), True), + ('tests fail', create_stats_with_delta(tests_fail_delta=1), True), + ('tests error', create_stats_with_delta(tests_error_delta=1), True), + ('runs', create_stats_with_delta(runs_delta=1), True), + ('runs succ', create_stats_with_delta(runs_succ_delta=1), True), + ('runs skip', create_stats_with_delta(runs_skip_delta=1), True), + ('runs fail', create_stats_with_delta(runs_fail_delta=1), True), + ('runs error', create_stats_with_delta(runs_error_delta=1), True)]: + with self.subTest(msg=label): + self.assertEqual(stats.has_changes, expected, msg=label) + + def unit_test_run_results_has_failures(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, stats, expected in [('no failures', create_stats(), False), + ('errors', create_stats(errors=errors), False), + ('test failures', create_stats(tests_fail=1), True), + ('test errors', create_stats(tests_error=1), False), + ('runs failures', create_stats(runs_fail=1), True), + ('runs errors', create_stats(runs_error=1), False)]: + with self.subTest(msg=label): + self.assertEqual(stats.has_failures, expected, msg=label) + + def test_test_run_results_has_errors(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, stats, expected in [('no errors', create_stats(), False), + ('errors', create_stats(errors=errors), True), + ('test failures', create_stats(tests_fail=1), False), + ('test errors', create_stats(tests_error=1), True), + ('runs failures', create_stats(runs_fail=1), False), + ('runs errors', create_stats(runs_error=1), True)]: + with self.subTest(msg=label): + self.assertEqual(stats.has_errors, expected, msg=label) From 46404231b4f26af049fa363c94e350b62215196a Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 18 Apr 2022 16:06:26 +0100 Subject: [PATCH 04/26] Fix imports in test_publisher and test_publish --- python/test/test_publish.py | 14 +++++++++++++- python/test/test_publisher.py | 14 +++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/python/test/test_publish.py b/python/test/test_publish.py index 5dd08691..255a1b0d 100644 --- a/python/test/test_publish.py +++ b/python/test/test_publish.py @@ -2,10 +2,22 @@ import locale import pathlib import unittest +from collections import defaultdict +from typing import Any import mock -from publish import * +from publish import Annotation, UnitTestCaseResults, UnitTestRunResults, UnitTestRunDeltaResults, CaseMessages, \ + get_error_annotation, get_digest_from_stats, \ + all_tests_label_md, skipped_tests_label_md, failed_tests_label_md, passed_tests_label_md, test_errors_label_md, \ + duration_label_md, SomeTestChanges, abbreviate, abbreviate_bytes, get_test_name, get_formatted_digits, \ + get_magnitude, get_delta, as_short_commit, as_delta, as_stat_number, as_stat_duration, get_stats_from_digest, \ + digest_string, ungest_string, get_details_line_md, get_commit_line_md, restrict_unicode, \ + get_short_summary, get_short_summary_md, get_long_summary_md, get_long_summary_with_runs_md, \ + get_long_summary_without_runs_md, get_long_summary_with_digest_md, \ + get_test_changes_md, get_test_changes_list_md, get_test_changes_summary_md, \ + get_case_annotations, get_case_annotation, get_all_tests_list_annotation, \ + get_skipped_tests_list_annotation, get_case_messages, chunk_test_list from publish.junit import parse_junit_xml_files from publish.unittestresults import get_stats, UnitTestCase, ParseError from publish.unittestresults import get_test_results diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 58d41591..828b41a1 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -1,18 +1,26 @@ -import itertools +import json import os import tempfile import unittest from collections.abc import Collection from datetime import datetime, timezone +from typing import Optional, List, Mapping, Union, Any import github.CheckRun import mock from github import Github, GithubException -from publish import * +from publish import comment_mode_create, comment_mode_update, comment_mode_off, comment_condition_always, \ + comment_condition_changes, comment_condition_failures, comment_condition_errors, hide_comments_mode_off, \ + hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, Annotation, default_annotations, \ + get_error_annotation, digest_header, get_digest_from_stats, \ + all_tests_list, skipped_tests_list, none_list, \ + all_tests_label_md, skipped_tests_label_md, failed_tests_label_md, passed_tests_label_md, test_errors_label_md, \ + duration_label_md, pull_request_build_mode_merge, punctuation_space from publish.github_action import GithubAction from publish.publisher import Publisher, Settings, PublishData -from publish.unittestresults import UnitTestCase, ParseError +from publish.unittestresults import UnitTestCase, ParseError, UnitTestRunResults, UnitTestRunDeltaResults, \ + UnitTestCaseResults errors = [ParseError('file', 'error', 1, 2)] From f80f9632da902c287d6056def6eb4baad65c0ffc Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 18 Apr 2022 16:28:54 +0100 Subject: [PATCH 05/26] Fix some typos and debug settings --- python/publish_unit_test_results.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index d82b94a9..9c994ba8 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -337,8 +337,8 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.token, 'GITHUB_TOKEN', 'GitHub token') check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') - check_var(settings.comment_mode, 'COMMENT_MODE', 'Commit mode', comment_modes) - check_var(settings.comment_condition, 'COMMENT_ON', 'Commit condition', comment_conditions) + check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes) + check_var(settings.comment_condition, 'COMMENT_ON', 'Comment condition', comment_conditions) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) From 5d73a16f67da5590b407f70e45c147c9be118598 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Tue, 19 Apr 2022 20:36:07 +0100 Subject: [PATCH 06/26] Use latest action comment on PR for require_comment --- python/publish/publisher.py | 81 ++++++++-- python/test/test_publisher.py | 294 ++++++++++++++++++---------------- 2 files changed, 224 insertions(+), 151 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 6adca58c..a152a4f4 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -11,6 +11,7 @@ from github.CheckRun import CheckRun from github.CheckRunAnnotation import CheckRunAnnotation from github.PullRequest import PullRequest +from github.IssueComment import IssueComment from publish import hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, hide_comments_mode_off, \ comment_mode_off, comment_mode_create, comment_mode_update, digest_prefix, restrict_unicode_list, \ @@ -445,8 +446,15 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) + # we need fetch the latest comment if comment_condition != comment_condition_always + # or self._settings.comment_mode == comment_mode_update + latest_comment = None + if self._settings.comment_condition != comment_condition_always or self._settings.comment_mode == comment_mode_update: + latest_comment = self.get_latest_comment(pull_request) + latest_comment_body = latest_comment.body if latest_comment else None + # are we required to create a comment on this PR? - if not self.require_comment(stats, stats_with_delta, test_changes): + if not self.require_comment(stats, stats_with_delta, test_changes, latest_comment_body): logger.info(f'No comment required as comment_on is {self._settings.comment_condition}') return @@ -454,22 +462,64 @@ def publish_comment(self, summary = get_long_summary_with_digest_md(stats_with_delta, stats, details_url, test_changes, self._settings.test_changes_limit) body = f'## {title}\n{summary}' - # reuse existing commend when comment_mode == comment_mode_update - # if none exists or comment_mode != comment_mode_update, create new comment - if self._settings.comment_mode != comment_mode_update or not self.reuse_comment(pull_request, body): + # reuse existing comment when comment_mode == comment_mode_update, otherwise create new comment + if self._settings.comment_mode == comment_mode_update and latest_comment is not None: + self.reuse_comment(latest_comment, body) + logger.info(f'edited comment for pull request #{pull_request.number}: {latest_comment.html_url}') + else: comment = pull_request.create_issue_comment(body) logger.info(f'created comment for pull request #{pull_request.number}: {comment.html_url}') + @staticmethod + def comment_has_changes(comment_body: Optional[str]) -> bool: + if comment_body is None: + return False + end = comment_body.lower().find('results for commit') + if end < 0: + return False + comment_body = comment_body[:end] + + # remove links + comment_body = re.sub(r'\([^)]*\)', '', comment_body) + # replace ' ' with some non-whitespace string, it separates columns in the comment + comment_body = comment_body.replace(' ', '⋯') + + m = re.search(r'[-+](\s*[0-9]+)+\s+(([^0-9\s])|\n)', comment_body) + return m is not None + + @classmethod + def comment_has_failures(cls, comment_body: Optional[str]) -> bool: + return cls._comment_has(comment_body, r'\[:x:]') + + @classmethod + def comment_has_errors(cls, comment_body: Optional[str]) -> bool: + return cls._comment_has(comment_body, r'(\[:fire:]|errors)') + + @staticmethod + def _comment_has(comment_body: Optional[str], symbol: str) -> bool: + if comment_body is None: + return False + + end = comment_body.lower().find('results for commit') + if end < 0: + return False + comment_body = comment_body[:end] + + # we assume '00 ...' indicates multiple failures / errors (more than 99) + m = re.search(r'([0-9][0-9]|[1-9])\s' + symbol, comment_body) + return m is not None + def require_comment(self, stats: UnitTestRunResults, stats_with_delta: UnitTestRunDeltaResults, - test_changes: SomeTestChanges) -> bool: + test_changes: SomeTestChanges, + comment_body: Optional[str]) -> bool: return (self._settings.comment_condition == comment_condition_always or - self._settings.comment_condition == comment_condition_changes and (stats_with_delta is None or stats_with_delta.has_changes or test_changes.has_changes) or - self._settings.comment_condition == comment_condition_failures and (stats.has_failures or stats.has_errors) or - self._settings.comment_condition == comment_condition_errors and stats.has_errors) + self._settings.comment_condition == comment_condition_changes and (self.comment_has_changes(comment_body) or stats_with_delta is None or stats_with_delta.has_changes or test_changes.has_changes) or + self._settings.comment_condition == comment_condition_failures and (self.comment_has_failures(comment_body) or self.comment_has_errors(comment_body) or stats.has_failures or stats.has_errors) or + self._settings.comment_condition == comment_condition_errors and (self.comment_has_errors(comment_body) or stats.has_errors)) - def reuse_comment(self, pull: PullRequest, body: str) -> bool: + def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]: # get comments of this pull request comments = self.get_pull_request_comments(pull, order_by_updated=True) @@ -478,23 +528,22 @@ def reuse_comment(self, pull: PullRequest, body: str) -> bool: # if there is no such comment, stop here if len(comments) == 0: - return False + return None - # edit last comment + # fetch latest action comment comment_id = comments[-1].get("databaseId") + return pull.get_issue_comment(comment_id) + + def reuse_comment(self, comment: IssueComment, body: str): if ':recycle:' not in body: body = f'{body}\n:recycle: This comment has been updated with latest results.' try: - comment = pull.get_issue_comment(comment_id) comment.edit(body) - logger.info(f'edited comment for pull request #{pull.number}: {comment.html_url}') except Exception as e: - self._gha.warning(f'Failed to edit existing comment #{comment_id}') + self._gha.warning(f'Failed to edit existing comment #{comment.id}') logger.debug('editing existing comment failed', exc_info=e) - return True - def get_base_commit_sha(self, pull_request: PullRequest) -> Optional[str]: if self._settings.pull_request_build == pull_request_build_mode_merge: if self._settings.event: diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 828b41a1..b28ccc13 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -1,10 +1,12 @@ +import dataclasses import json import os +import re import tempfile import unittest from collections.abc import Collection from datetime import datetime, timezone -from typing import Optional, List, Mapping, Union, Any +from typing import Optional, List, Mapping, Union, Any, Callable import github.CheckRun import mock @@ -16,15 +18,26 @@ get_error_annotation, digest_header, get_digest_from_stats, \ all_tests_list, skipped_tests_list, none_list, \ all_tests_label_md, skipped_tests_label_md, failed_tests_label_md, passed_tests_label_md, test_errors_label_md, \ - duration_label_md, pull_request_build_mode_merge, punctuation_space + duration_label_md, pull_request_build_mode_merge, punctuation_space, get_long_summary_md from publish.github_action import GithubAction from publish.publisher import Publisher, Settings, PublishData from publish.unittestresults import UnitTestCase, ParseError, UnitTestRunResults, UnitTestRunDeltaResults, \ - UnitTestCaseResults + UnitTestRunResultsOrDeltaResults, UnitTestCaseResults, get_diff_value errors = [ParseError('file', 'error', 1, 2)] +@dataclasses.dataclass(frozen=True) +class CommentConditionTest: + comment_has_changes: bool + comment_has_failures: bool + comment_has_errors: bool + stats_has_changes: bool + stats_has_failures: bool + stats_has_errors: bool + tests_has_changes: bool + + class TestPublisher(unittest.TestCase): @staticmethod @@ -293,102 +306,135 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) - def do_test_require_comment(self, comment_condition, tests): + def test_re(self): + m = re.search(r'[-+](\s*[0-9]+)+\s+(([^0-9\s])|\n)', '- 1 \n') + self.assertIsNotNone(m) + + def test_comment_has_changes(self): + with self.subTest(comment=None): + self.assertEqual(Publisher.comment_has_changes(None), False) + + for mag in [1, 10, 100, 1000]: + kwargs = { + 'files': get_diff_value(1+mag, 1+mag), + 'errors': [], + 'suites': get_diff_value(2+mag, 2+mag), + 'duration': get_diff_value(3+mag, 3+mag, 'duration'), + + 'tests': get_diff_value(22+mag, 22+mag), + 'tests_succ': get_diff_value(4+mag, 4+mag), + 'tests_skip': get_diff_value(5+mag, 5+mag), + 'tests_fail': get_diff_value(6+mag, 6+mag), + 'tests_error': get_diff_value(7+mag, 7+mag), + + 'runs': get_diff_value(38+mag, 38+mag), + 'runs_succ': get_diff_value(8+mag, 8+mag), + 'runs_skip': get_diff_value(9+mag, 9+mag), + 'runs_fail': get_diff_value(10+mag, 10+mag), + 'runs_error': get_diff_value(11+mag, 11+mag), + + 'commit': 'commit', + 'reference_type': 'ref', + 'reference_commit': 'base' + } + + # comments with changes + for field in [None, 'files', 'suites', 'duration', + 'tests', 'tests_succ', 'tests_skip', 'tests_fail', 'tests_error', + 'runs', 'runs_succ', 'runs_skip', 'runs_fail', 'runs_error']: + for change in {-1, +1, mag, -mag}: + with self.subTest(field=field, magnitude=mag, change=change): + kwargs2 = {k: v if k != field else {k2: v2 + change if k2 == 'delta' else v2 + for k2, v2 in v.items()} + for k, v in kwargs.items()} + stats = UnitTestRunDeltaResults(**kwargs2) + summary = get_long_summary_md(stats, 'http://details') + comment = f'## title\n{summary}' + self.assertEqual(Publisher.comment_has_changes(comment), stats.has_changes, msg=comment) + + def test_comment_has_failures(self): + self.do_test_comment_has(lambda comment: Publisher.comment_has_failures(comment), lambda stats: stats.has_failures) + + def test_comment_has_errors(self): + self.do_test_comment_has(lambda comment: Publisher.comment_has_errors(comment), lambda stats: stats.has_errors) + + def do_test_comment_has(self, + actual: Callable[[str], bool], + expected: Callable[[UnitTestRunResultsOrDeltaResults], bool]): + with self.subTest(comment=None): + self.assertEqual(Publisher.comment_has_failures(None), False) + + kwargs = { + 'errors': [], + 'commit': 'commit' + } + fields = ['files', 'suites', 'duration', + 'tests', 'tests_succ', 'tests_skip', 'tests_fail', 'tests_error', + 'runs', 'runs_succ', 'runs_skip', 'runs_fail', 'runs_error'] + kwargs.update(**{field: 0 for field in fields}) + + for mag in [1, 10, 100, 1000]: + for field in fields + ['errors']: + with self.subTest(field=field, magnitude=mag): + kwargs2 = {k: mag if k == field and field != 'errors' else v for k, v in kwargs.items()} + stats = UnitTestRunResults(**kwargs2) + if field == 'errors': + stats = stats.with_errors(errors * mag) + summary = get_long_summary_md(stats, 'http://details') + comment = f'## title\n{summary}' + print(f'field={field} stats={stats} summary={summary}') + self.assertEqual(actual(comment), expected(stats), msg=comment) + + def do_test_require_comment(self, comment_condition, test_expectation: Callable[["CommentConditionTest"], bool]): + tests = [(test, test_expectation(test)) for test in self.comment_condition_tests] + publisher = mock.MagicMock(Publisher) publisher._settings = self.create_settings(comment_condition=comment_condition) - for has_failures, has_errors, stats_has_changes, tests_has_changes, expected in tests: - with self.subTest(has_failures=has_failures, has_errors=has_errors, - stats_has_changes=stats_has_changes, tests_has_changes=tests_has_changes): - stats = mock.MagicMock(has_failures=has_failures, has_errors=has_errors) - stats_with_delta = mock.MagicMock(has_changes=stats_has_changes) - test_changes = mock.MagicMock(has_changes=tests_has_changes) - required = Publisher.require_comment(publisher, stats, stats_with_delta, test_changes) + for test, expected in tests: + with self.subTest(test): + publisher.comment_has_changes = mock.Mock(return_value=test.comment_has_changes) + publisher.comment_has_failures = mock.Mock(return_value=test.comment_has_failures) + publisher.comment_has_errors = mock.Mock(return_value=test.comment_has_errors) + stats = mock.MagicMock(has_failures=test.stats_has_failures, has_errors=test.stats_has_errors) + stats_with_delta = mock.MagicMock(has_changes=test.stats_has_changes) + test_changes = mock.MagicMock(has_changes=test.tests_has_changes) + required = Publisher.require_comment(publisher, stats, stats_with_delta, test_changes, 'body') self.assertEqual(required, expected) + comment_condition_tests = [CommentConditionTest(comment_has_changes, comment_has_failures, comment_has_errors, + stat_has_changes, stat_has_failures, stat_has_errors, tests_has_changes) + for comment_has_changes in [False, True] + for comment_has_failures in [False, True] + for comment_has_errors in [False, True] + for stat_has_changes in [False, True] + for stat_has_failures in [False, True] + for stat_has_errors in [False, True] + for tests_has_changes in [False, True]] + def test_require_comment_always(self): - self.do_test_require_comment(comment_condition_always, [ - # failures, errors, changes, test changes - (False, False, False, False, True), - (False, False, False, True, True), - (False, False, True, False, True), - (False, False, True, True, True), - (False, True, False, False, True), - (False, True, False, True, True), - (False, True, True, False, True), - (False, True, True, True, True), - (True, False, False, False, True), - (True, False, False, True, True), - (True, False, True, False, True), - (True, False, True, True, True), - (True, True, False, False, True), - (True, True, False, True, True), - (True, True, True, False, True), - (True, True, True, True, True) - ]) + self.do_test_require_comment( + comment_condition_always, + lambda _: True + ) def test_require_comment_changes(self): - self.do_test_require_comment(comment_condition_changes, [ - # failures, errors, changes, test changes - (False, False, False, False, False), - (False, False, False, True, True), - (False, False, True, False, True), - (False, False, True, True, True), - (False, True, False, False, False), - (False, True, False, True, True), - (False, True, True, False, True), - (False, True, True, True, True), - (True, False, False, False, False), - (True, False, False, True, True), - (True, False, True, False, True), - (True, False, True, True, True), - (True, True, False, False, False), - (True, True, False, True, True), - (True, True, True, False, True), - (True, True, True, True, True) - ]) + self.do_test_require_comment( + comment_condition_changes, + lambda test: test.comment_has_changes or test.stats_has_changes or test.tests_has_changes + ) def test_require_comment_failures(self): - self.do_test_require_comment(comment_condition_failures, [ - # failures, errors, changes, test changes - (False, False, False, False, False), - (False, False, False, True, False), - (False, False, True, False, False), - (False, False, True, True, False), - (False, True, False, False, True), - (False, True, False, True, True), - (False, True, True, False, True), - (False, True, True, True, True), - (True, False, False, False, True), - (True, False, False, True, True), - (True, False, True, False, True), - (True, False, True, True, True), - (True, True, False, False, True), - (True, True, False, True, True), - (True, True, True, False, True), - (True, True, True, True, True) - ]) + self.do_test_require_comment( + comment_condition_failures, + lambda test: test.comment_has_failures or test.comment_has_errors or test.stats_has_failures or test.stats_has_errors + ) def test_require_comment_errors(self): - self.do_test_require_comment(comment_condition_errors, [ - # failures, errors, changes, test changes - (False, False, False, False, False), - (False, False, False, True, False), - (False, False, True, False, False), - (False, False, True, True, False), - (False, True, False, False, True), - (False, True, False, True, True), - (False, True, True, False, True), - (False, True, True, True, True), - (True, False, False, False, False), - (True, False, False, True, False), - (True, False, True, False, False), - (True, False, True, True, False), - (True, True, False, False, True), - (True, True, False, True, True), - (True, True, True, False, True), - (True, True, True, True, True) - ]) + self.do_test_require_comment( + comment_condition_errors, + lambda test: test.comment_has_errors or test.stats_has_errors + ) def test_publish_without_comment(self): settings = self.create_settings(comment_mode=comment_mode_off, hide_comment_mode=hide_comments_mode_off) @@ -827,19 +873,21 @@ def test_publish_comment_compare_with_None(self): def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): pr = mock.MagicMock() cr = mock.MagicMock() + lc = mock.MagicMock(body='latest comment') if one_exists else None stats = self.stats cases = UnitTestCaseResults(self.cases) settings = self.create_settings(comment_mode=comment_mode_update, compare_earlier=False) publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.get_latest_comment = mock.Mock(return_value=lc) publisher.reuse_comment = mock.Mock(return_value=one_exists) publisher.require_comment = mock.Mock(return_value=True) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(3, len(mock_calls)) + self.assertEqual(4 if one_exists else 3, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_test_lists_from_check_run', method) @@ -847,17 +895,28 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[1] - self.assertEqual('require_comment', method) + self.assertEqual('get_latest_comment', method) + self.assertEqual((pr, ), args) + self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[2] - self.assertEqual('reuse_comment', method) - self.assertEqual((pr, '## title\nbody'), args) - self.assertEqual({}, kwargs) + self.assertEqual('require_comment', method) + + if one_exists: + (method, args, kwargs) = mock_calls[3] + self.assertEqual('reuse_comment', method) + self.assertEqual((lc, '## title\nbody'), args) + self.assertEqual({}, kwargs) mock_calls = pr.mock_calls - self.assertEqual(0 if one_exists else 3, len(mock_calls)) + self.assertEqual(1 if one_exists else 3, len(mock_calls)) - if not one_exists: + if one_exists: + (method, args, kwargs) = mock_calls[0] + self.assertEqual('number.__str__', method) + self.assertEqual((), args) + self.assertEqual({}, kwargs) + else: (method, args, kwargs) = mock_calls[0] self.assertEqual('create_issue_comment', method) self.assertEqual(('## title\nbody', ), args) @@ -879,57 +938,22 @@ def test_publish_comment_with_reuse_comment_none_existing(self): def test_publish_comment_with_reuse_comment_one_existing(self): self.do_test_publish_comment_with_reuse_comment(one_exists=True) - def do_test_reuse_comment(self, - pull_request_comments: List[Any], - action_comments: List[Mapping[str, int]], - body='body', - expected_body='body\n:recycle: This comment has been updated with latest results.'): - pr = mock.MagicMock() + def do_test_reuse_comment(self, earlier_body: str, expected_body: str): comment = mock.MagicMock() - pr.get_issue_comment = mock.Mock(return_value=comment) - settings = self.create_settings(comment_mode=comment_mode_update, compare_earlier=False) publisher = mock.MagicMock(Publisher) - publisher._settings = settings - publisher.get_pull_request_comments = mock.Mock(return_value=pull_request_comments) - publisher.get_action_comments = mock.Mock(return_value=action_comments) - Publisher.reuse_comment(publisher, pr, body) - - mock_calls = publisher.mock_calls - self.assertEqual(2, len(mock_calls)) - - (method, args, kwargs) = mock_calls[0] - self.assertEqual('get_pull_request_comments', method) - self.assertEqual((pr, ), args) - self.assertEqual({'order_by_updated': True}, kwargs) - - (method, args, kwargs) = mock_calls[1] - self.assertEqual('get_action_comments', method) - self.assertEqual((pull_request_comments, ), args) - self.assertEqual({}, kwargs) - - if action_comments: - pr.get_issue_comment.assert_called_once_with(action_comments[-1].get('databaseId')) - comment.edit.assert_called_once_with(expected_body) - - def test_reuse_comment_non_existing(self): - self.do_test_reuse_comment(pull_request_comments=[1, 2, 3], action_comments=[]) - - def test_reuse_comment_one_existing(self): - self.do_test_reuse_comment(pull_request_comments=[1, 2, 3], action_comments=[{'databaseId': 1}]) + Publisher.reuse_comment(publisher, comment, earlier_body) - def test_reuse_comment_multiple_existing(self): - self.do_test_reuse_comment(pull_request_comments=[1, 2, 3], action_comments=[{'databaseId': 1}, {'databaseId': 2}, {'databaseId': 3}]) + comment.edit.assert_called_once_with(expected_body) + self.assertEqual(0, len(publisher.mock_calls)) def test_reuse_comment_existing_not_updated(self): # we do not expect the body to be extended by the recycle message - self.do_test_reuse_comment(pull_request_comments=[1, 2, 3], action_comments=[{'databaseId': 1}], - body='a new comment', + self.do_test_reuse_comment(earlier_body='a new comment', expected_body='a new comment\n:recycle: This comment has been updated with latest results.') def test_reuse_comment_existing_updated(self): # we do not expect the body to be extended by the recycle message - self.do_test_reuse_comment(pull_request_comments=[1, 2, 3], action_comments=[{'databaseId': 1}], - body='comment already updated\n:recycle: Has been updated', + self.do_test_reuse_comment(earlier_body='comment already updated\n:recycle: Has been updated', expected_body='comment already updated\n:recycle: Has been updated') def do_test_get_pulls(self, From 83540e193afd80a8ba610f774c95fb849c96a0de Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 19:09:59 +0200 Subject: [PATCH 07/26] Make require_comment use stats from earlier comment digest --- python/publish/publisher.py | 77 +++++--------- python/publish/unittestresults.py | 46 +++++++- python/test/test_publisher.py | 157 ++++++++-------------------- python/test/test_unittestresults.py | 157 +++++++++++++++++++--------- 4 files changed, 222 insertions(+), 215 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index a152a4f4..2d4574b8 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -23,7 +23,8 @@ Annotation, SomeTestChanges from publish import logger from publish.github_action import GithubAction -from publish.unittestresults import UnitTestCaseResults, UnitTestRunResults, UnitTestRunDeltaResults, get_stats_delta +from publish.unittestresults import UnitTestCaseResults, UnitTestRunResults, UnitTestRunDeltaResults, \ + UnitTestRunResultsOrDeltaResults, get_stats_delta @dataclass(frozen=True) @@ -266,6 +267,10 @@ def get_stats_from_check_run(check_run: CheckRun) -> Optional[UnitTestRunResults for line in summary.split('\n'): logger.debug(f'summary: {line}') + return Publisher.get_stats_from_summary_md(summary) + + @staticmethod + def get_stats_from_summary_md(summary: str) -> Optional[UnitTestRunResults]: pos = summary.index(digest_header) if digest_header in summary else None if pos: digest = summary[pos + len(digest_header):] @@ -454,8 +459,9 @@ def publish_comment(self, latest_comment_body = latest_comment.body if latest_comment else None # are we required to create a comment on this PR? - if not self.require_comment(stats, stats_with_delta, test_changes, latest_comment_body): - logger.info(f'No comment required as comment_on is {self._settings.comment_condition}') + earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None + if not self.require_comment(stats_with_delta, earlier_stats, test_changes): + logger.info(f'No comment required as comment_on condition {self._settings.comment_condition} is not met') return details_url = check_run.html_url if check_run else None @@ -470,54 +476,25 @@ def publish_comment(self, comment = pull_request.create_issue_comment(body) logger.info(f'created comment for pull request #{pull_request.number}: {comment.html_url}') - @staticmethod - def comment_has_changes(comment_body: Optional[str]) -> bool: - if comment_body is None: - return False - end = comment_body.lower().find('results for commit') - if end < 0: - return False - comment_body = comment_body[:end] - - # remove links - comment_body = re.sub(r'\([^)]*\)', '', comment_body) - # replace ' ' with some non-whitespace string, it separates columns in the comment - comment_body = comment_body.replace(' ', '⋯') - - m = re.search(r'[-+](\s*[0-9]+)+\s+(([^0-9\s])|\n)', comment_body) - return m is not None - - @classmethod - def comment_has_failures(cls, comment_body: Optional[str]) -> bool: - return cls._comment_has(comment_body, r'\[:x:]') - - @classmethod - def comment_has_errors(cls, comment_body: Optional[str]) -> bool: - return cls._comment_has(comment_body, r'(\[:fire:]|errors)') - - @staticmethod - def _comment_has(comment_body: Optional[str], symbol: str) -> bool: - if comment_body is None: - return False - - end = comment_body.lower().find('results for commit') - if end < 0: - return False - comment_body = comment_body[:end] - - # we assume '00 ...' indicates multiple failures / errors (more than 99) - m = re.search(r'([0-9][0-9]|[1-9])\s' + symbol, comment_body) - return m is not None - def require_comment(self, - stats: UnitTestRunResults, - stats_with_delta: UnitTestRunDeltaResults, - test_changes: SomeTestChanges, - comment_body: Optional[str]) -> bool: - return (self._settings.comment_condition == comment_condition_always or - self._settings.comment_condition == comment_condition_changes and (self.comment_has_changes(comment_body) or stats_with_delta is None or stats_with_delta.has_changes or test_changes.has_changes) or - self._settings.comment_condition == comment_condition_failures and (self.comment_has_failures(comment_body) or self.comment_has_errors(comment_body) or stats.has_failures or stats.has_errors) or - self._settings.comment_condition == comment_condition_errors and (self.comment_has_errors(comment_body) or stats.has_errors)) + stats: UnitTestRunResultsOrDeltaResults, + earlier_stats: Optional[UnitTestRunResults], + test_changes: SomeTestChanges) -> bool: + return (self._settings.comment_condition == comment_condition_always + or + self._settings.comment_condition == comment_condition_changes and ( + earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats) or + not stats.is_delta or stats.has_changes or + test_changes.has_changes) + or + self._settings.comment_condition == comment_condition_failures and ( + earlier_stats is not None and (earlier_stats.has_failures or earlier_stats.has_errors) or + stats.has_failures or stats.has_errors) + or + self._settings.comment_condition == comment_condition_errors and ( + earlier_stats is not None and earlier_stats.has_errors or + stats.has_errors) + ) def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]: # get comments of this pull request diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index cb84ff7f..4d1f2b65 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -168,6 +168,10 @@ class UnitTestRunResults: commit: str + @property + def is_delta(self) -> bool: + return False + @property def has_failures(self): return self.tests_fail > 0 or self.runs_fail > 0 @@ -225,6 +229,18 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': ) +def patch_ne_duration(orig_ne): + def ne_without_duration(self, other) -> bool: + other = dataclasses.replace(other, duration=self.duration) + return orig_ne(self, other) + + return ne_without_duration + + +# patch UnitTestRunResults.__ne__ to not include duration +UnitTestRunResults.__ne__ = patch_ne_duration(UnitTestRunResults.__ne__) + + Numeric = Mapping[str, int] @@ -252,16 +268,40 @@ class UnitTestRunDeltaResults: reference_type: str reference_commit: str - def to_dict(self) -> Dict[str, Any]: - return dataclasses.asdict(self) + @property + def is_delta(self) -> bool: + return True @property - def has_changes(self): + def has_changes(self) -> bool: return (any([field.get('delta') for field in [self.files, self.suites, self.tests, self.tests_succ, self.tests_skip, self.tests_fail, self.tests_error, self.runs, self.runs_succ, self.runs_skip, self.runs_fail, self.runs_error]])) + @property + def has_failures(self): + return self.tests_fail.get('number') > 0 or self.runs_fail.get('number') > 0 + + @property + def has_errors(self): + return len(self.errors) > 0 or self.tests_error.get('number') > 0 or self.runs_error.get('number') > 0 + + def to_dict(self) -> Dict[str, Any]: + return dataclasses.asdict(self) + + def without_delta(self) -> UnitTestRunResults: + def v(value: Numeric) -> int: + return value.get('number') + + def d(value: Numeric) -> int: + return value.get('duration') + + return UnitTestRunResults(files=v(self.files), errors=self.errors, suites=v(self.suites), duration=d(self.duration), + tests=v(self.tests), tests_succ=v(self.tests_succ), tests_skip=v(self.tests_skip), tests_fail=v(self.tests_fail), tests_error=v(self.tests_error), + runs=v(self.runs), runs_succ=v(self.runs_succ), runs_skip=v(self.runs_skip), runs_fail=v(self.runs_fail), runs_error=v(self.runs_error), + commit=self.commit) + UnitTestRunResultsOrDeltaResults = Union[UnitTestRunResults, UnitTestRunDeltaResults] diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index b28ccc13..49b3df02 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -1,7 +1,6 @@ import dataclasses import json import os -import re import tempfile import unittest from collections.abc import Collection @@ -18,24 +17,26 @@ get_error_annotation, digest_header, get_digest_from_stats, \ all_tests_list, skipped_tests_list, none_list, \ all_tests_label_md, skipped_tests_label_md, failed_tests_label_md, passed_tests_label_md, test_errors_label_md, \ - duration_label_md, pull_request_build_mode_merge, punctuation_space, get_long_summary_md + duration_label_md, pull_request_build_mode_merge, punctuation_space from publish.github_action import GithubAction from publish.publisher import Publisher, Settings, PublishData from publish.unittestresults import UnitTestCase, ParseError, UnitTestRunResults, UnitTestRunDeltaResults, \ - UnitTestRunResultsOrDeltaResults, UnitTestCaseResults, get_diff_value + UnitTestCaseResults errors = [ParseError('file', 'error', 1, 2)] @dataclasses.dataclass(frozen=True) class CommentConditionTest: - comment_has_changes: bool - comment_has_failures: bool - comment_has_errors: bool - stats_has_changes: bool - stats_has_failures: bool - stats_has_errors: bool - tests_has_changes: bool + earlier_is_none: bool + earlier_is_different: bool + earlier_has_failures: bool + earlier_has_errors: bool + # current_has_changes being None indicates it is not a UnitTestRunDeltaResults but UnitTestRunResults + current_has_changes: Optional[bool] + current_has_failures: bool + current_has_errors: bool + tests_have_changes: bool class TestPublisher(unittest.TestCase): @@ -306,85 +307,6 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) - def test_re(self): - m = re.search(r'[-+](\s*[0-9]+)+\s+(([^0-9\s])|\n)', '- 1 \n') - self.assertIsNotNone(m) - - def test_comment_has_changes(self): - with self.subTest(comment=None): - self.assertEqual(Publisher.comment_has_changes(None), False) - - for mag in [1, 10, 100, 1000]: - kwargs = { - 'files': get_diff_value(1+mag, 1+mag), - 'errors': [], - 'suites': get_diff_value(2+mag, 2+mag), - 'duration': get_diff_value(3+mag, 3+mag, 'duration'), - - 'tests': get_diff_value(22+mag, 22+mag), - 'tests_succ': get_diff_value(4+mag, 4+mag), - 'tests_skip': get_diff_value(5+mag, 5+mag), - 'tests_fail': get_diff_value(6+mag, 6+mag), - 'tests_error': get_diff_value(7+mag, 7+mag), - - 'runs': get_diff_value(38+mag, 38+mag), - 'runs_succ': get_diff_value(8+mag, 8+mag), - 'runs_skip': get_diff_value(9+mag, 9+mag), - 'runs_fail': get_diff_value(10+mag, 10+mag), - 'runs_error': get_diff_value(11+mag, 11+mag), - - 'commit': 'commit', - 'reference_type': 'ref', - 'reference_commit': 'base' - } - - # comments with changes - for field in [None, 'files', 'suites', 'duration', - 'tests', 'tests_succ', 'tests_skip', 'tests_fail', 'tests_error', - 'runs', 'runs_succ', 'runs_skip', 'runs_fail', 'runs_error']: - for change in {-1, +1, mag, -mag}: - with self.subTest(field=field, magnitude=mag, change=change): - kwargs2 = {k: v if k != field else {k2: v2 + change if k2 == 'delta' else v2 - for k2, v2 in v.items()} - for k, v in kwargs.items()} - stats = UnitTestRunDeltaResults(**kwargs2) - summary = get_long_summary_md(stats, 'http://details') - comment = f'## title\n{summary}' - self.assertEqual(Publisher.comment_has_changes(comment), stats.has_changes, msg=comment) - - def test_comment_has_failures(self): - self.do_test_comment_has(lambda comment: Publisher.comment_has_failures(comment), lambda stats: stats.has_failures) - - def test_comment_has_errors(self): - self.do_test_comment_has(lambda comment: Publisher.comment_has_errors(comment), lambda stats: stats.has_errors) - - def do_test_comment_has(self, - actual: Callable[[str], bool], - expected: Callable[[UnitTestRunResultsOrDeltaResults], bool]): - with self.subTest(comment=None): - self.assertEqual(Publisher.comment_has_failures(None), False) - - kwargs = { - 'errors': [], - 'commit': 'commit' - } - fields = ['files', 'suites', 'duration', - 'tests', 'tests_succ', 'tests_skip', 'tests_fail', 'tests_error', - 'runs', 'runs_succ', 'runs_skip', 'runs_fail', 'runs_error'] - kwargs.update(**{field: 0 for field in fields}) - - for mag in [1, 10, 100, 1000]: - for field in fields + ['errors']: - with self.subTest(field=field, magnitude=mag): - kwargs2 = {k: mag if k == field and field != 'errors' else v for k, v in kwargs.items()} - stats = UnitTestRunResults(**kwargs2) - if field == 'errors': - stats = stats.with_errors(errors * mag) - summary = get_long_summary_md(stats, 'http://details') - comment = f'## title\n{summary}' - print(f'field={field} stats={stats} summary={summary}') - self.assertEqual(actual(comment), expected(stats), msg=comment) - def do_test_require_comment(self, comment_condition, test_expectation: Callable[["CommentConditionTest"], bool]): tests = [(test, test_expectation(test)) for test in self.comment_condition_tests] @@ -393,24 +315,25 @@ def do_test_require_comment(self, comment_condition, test_expectation: Callable[ for test, expected in tests: with self.subTest(test): - publisher.comment_has_changes = mock.Mock(return_value=test.comment_has_changes) - publisher.comment_has_failures = mock.Mock(return_value=test.comment_has_failures) - publisher.comment_has_errors = mock.Mock(return_value=test.comment_has_errors) - stats = mock.MagicMock(has_failures=test.stats_has_failures, has_errors=test.stats_has_errors) - stats_with_delta = mock.MagicMock(has_changes=test.stats_has_changes) - test_changes = mock.MagicMock(has_changes=test.tests_has_changes) - required = Publisher.require_comment(publisher, stats, stats_with_delta, test_changes, 'body') + earlier = mock.MagicMock(__ne__=mock.Mock(return_value=test.earlier_is_different), has_failures=test.earlier_has_failures, has_errors=test.earlier_has_errors) if not test.earlier_is_none else None + current = mock.MagicMock(is_delta=test.current_has_changes is not None, has_changes=test.current_has_changes, has_failures=test.current_has_failures, has_errors=test.current_has_errors) + if current.is_delta: + current.without_delta = mock.Mock(return_value=current) + test_changes = mock.MagicMock(has_changes=test.tests_have_changes) + required = Publisher.require_comment(publisher, current, earlier, test_changes) self.assertEqual(required, expected) - comment_condition_tests = [CommentConditionTest(comment_has_changes, comment_has_failures, comment_has_errors, - stat_has_changes, stat_has_failures, stat_has_errors, tests_has_changes) - for comment_has_changes in [False, True] - for comment_has_failures in [False, True] - for comment_has_errors in [False, True] - for stat_has_changes in [False, True] - for stat_has_failures in [False, True] - for stat_has_errors in [False, True] - for tests_has_changes in [False, True]] + comment_condition_tests = [CommentConditionTest(earlier_is_none, earlier_is_different, earlier_has_failures, earlier_has_errors, + current_has_changes, current_has_failures, current_has_errors, + tests_have_changes) + for earlier_is_none in [False, True] + for earlier_is_different in [False, True] + for earlier_has_failures in [False, True] + for earlier_has_errors in [False, True] + for current_has_changes in [None, False, True] + for current_has_failures in [False, True] + for current_has_errors in [False, True] + for tests_have_changes in [False, True]] def test_require_comment_always(self): self.do_test_require_comment( @@ -421,19 +344,21 @@ def test_require_comment_always(self): def test_require_comment_changes(self): self.do_test_require_comment( comment_condition_changes, - lambda test: test.comment_has_changes or test.stats_has_changes or test.tests_has_changes + lambda test: not test.earlier_is_none and test.earlier_is_different or + test.current_has_changes is None or test.current_has_changes or + test.tests_have_changes ) def test_require_comment_failures(self): self.do_test_require_comment( comment_condition_failures, - lambda test: test.comment_has_failures or test.comment_has_errors or test.stats_has_failures or test.stats_has_errors + lambda test: not test.earlier_is_none and (test.earlier_has_failures or test.earlier_has_errors) or test.current_has_failures or test.current_has_errors ) def test_require_comment_errors(self): self.do_test_require_comment( comment_condition_errors, - lambda test: test.comment_has_errors or test.stats_has_errors + lambda test: not test.earlier_is_none and test.earlier_has_errors or test.current_has_errors ) def test_publish_without_comment(self): @@ -887,7 +812,7 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(4 if one_exists else 3, len(mock_calls)) + self.assertEqual(5 if one_exists else 3, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_test_lists_from_check_run', method) @@ -899,14 +824,22 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): self.assertEqual((pr, ), args) self.assertEqual({}, kwargs) - (method, args, kwargs) = mock_calls[2] - self.assertEqual('require_comment', method) - if one_exists: + (method, args, kwargs) = mock_calls[2] + self.assertEqual('get_stats_from_summary_md', method) + self.assertEqual(('latest comment', ), args) + self.assertEqual({}, kwargs) + (method, args, kwargs) = mock_calls[3] + self.assertEqual('require_comment', method) + + (method, args, kwargs) = mock_calls[4] self.assertEqual('reuse_comment', method) self.assertEqual((lc, '## title\nbody'), args) self.assertEqual({}, kwargs) + else: + (method, args, kwargs) = mock_calls[2] + self.assertEqual('require_comment', method) mock_calls = pr.mock_calls self.assertEqual(1 if one_exists else 3, len(mock_calls)) diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index d179b88c..d8d88561 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -1,6 +1,6 @@ +import unittest import dataclasses from typing import List -import unittest from xml.etree.ElementTree import ParseError as XmlParseError from publish.unittestresults import get_test_results, get_stats, get_stats_delta, \ @@ -30,27 +30,27 @@ def create_unit_test_run_results(files=1, ) -def create_unit_test_run_delta_results(files_delta=-1, +def create_unit_test_run_delta_results(files=1, files_delta=-1, errors=[], - suites_delta=-2, - duration_delta=-3, - tests_delta=-4, - tests_succ_delta=-5, - tests_skip_delta=-6, - tests_fail_delta=-7, - tests_error_delta=-8, - runs_delta=-9, - runs_succ_delta=-10, - runs_skip_delta=-11, - runs_fail_delta=-12, - runs_error_delta=-13) -> UnitTestRunDeltaResults: + suites=2, suites_delta=-2, + duration=3, duration_delta=-3, + tests=4, tests_delta=-4, + tests_succ=5, tests_succ_delta=-5, + tests_skip=6, tests_skip_delta=-6, + tests_fail=7, tests_fail_delta=-7, + tests_error=8, tests_error_delta=-8, + runs=9, runs_delta=-9, + runs_succ=10, runs_succ_delta=-10, + runs_skip=11, runs_skip_delta=-11, + runs_fail=12, runs_fail_delta=-12, + runs_error=13, runs_error_delta=-13) -> UnitTestRunDeltaResults: return UnitTestRunDeltaResults( - files={'number': 1, 'delta': files_delta}, + files={'number': files, 'delta': files_delta}, errors=errors, - suites={'number': 2, 'delta': suites_delta}, - duration={'number': 3, 'delta': duration_delta}, - tests={'number': 4, 'delta': tests_delta}, tests_succ={'number': 5, 'delta': tests_succ_delta}, tests_skip={'number': 6, 'delta': tests_skip_delta}, tests_fail={'number': 7, 'delta': tests_fail_delta}, tests_error={'number': 8, 'delta': tests_error_delta}, - runs={'number': 9, 'delta': runs_delta}, runs_succ={'number': 10, 'delta': runs_succ_delta}, runs_skip={'number': 11, 'delta': runs_skip_delta}, runs_fail={'number': 12, 'delta': runs_fail_delta}, runs_error={'number': 13, 'delta': runs_error_delta}, + suites={'number': suites, 'delta': suites_delta}, + duration={'duration': duration, 'delta': duration_delta}, + tests={'number': tests, 'delta': tests_delta}, tests_succ={'number': tests_succ, 'delta': tests_succ_delta}, tests_skip={'number': tests_skip, 'delta': tests_skip_delta}, tests_fail={'number': tests_fail, 'delta': tests_fail_delta}, tests_error={'number': tests_error, 'delta': tests_error_delta}, + runs={'number': runs, 'delta': runs_delta}, runs_succ={'number': runs_succ, 'delta': runs_succ_delta}, runs_skip={'number': runs_skip, 'delta': runs_skip_delta}, runs_fail={'number': runs_fail, 'delta': runs_fail_delta}, runs_error={'number': runs_error, 'delta': runs_error_delta}, commit='commit', reference_type='type', reference_commit='ref' ) @@ -474,6 +474,53 @@ def test_get_stats_delta(self): reference_type='type' )) + def test_test_run_results_ne(self): + stats = create_unit_test_run_results() + create_other = create_unit_test_run_results + for diff, other, expected in [('nothing', create_other(), False), + ('files', create_other(files=stats.files+1), True), + ('errors', create_other(errors=errors), True), + ('suites', create_other(suites=stats.suites+1), True), + ('duration', create_other(duration=stats.duration+1), False), + ('tests', create_other(tests=stats.tests+1), True), + ('test success', create_other(tests_succ=stats.tests_succ+1), True), + ('test skips', create_other(tests_skip=stats.tests_skip+1), True), + ('test failures', create_other(tests_fail=stats.tests_fail+1), True), + ('test errors', create_other(tests_error=stats.tests_error+1), True), + ('runs', create_other(runs=stats.runs+1), True), + ('runs success', create_other(runs_succ=stats.runs_succ+1), True), + ('runs skips', create_other(runs_skip=stats.runs_skip+1), True), + ('runs failures', create_other(runs_fail=stats.runs_fail+1), True), + ('runs errors', create_other(runs_error=stats.runs_error+1), True)]: + with self.subTest(different_in=diff): + self.assertEqual(stats != other, expected, msg=diff) + + def unit_test_run_results_has_failures(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, stats, expected in [('no failures', create_stats(), False), + ('errors', create_stats(errors=errors), False), + ('test failures', create_stats(tests_fail=1), True), + ('test errors', create_stats(tests_error=1), False), + ('runs failures', create_stats(runs_fail=1), True), + ('runs errors', create_stats(runs_error=1), False)]: + with self.subTest(msg=label): + self.assertEqual(stats.has_failures, expected, msg=label) + + def test_test_run_results_has_errors(self): + def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: + return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + + for label, stats, expected in [('no errors', create_stats(), False), + ('errors', create_stats(errors=errors), True), + ('test failures', create_stats(tests_fail=1), False), + ('test errors', create_stats(tests_error=1), True), + ('runs failures', create_stats(runs_fail=1), False), + ('runs errors', create_stats(runs_error=1), True)]: + with self.subTest(msg=label): + self.assertEqual(stats.has_errors, expected, msg=label) + def test_unit_test_run_delta_results_has_changes(self): def create_stats_with_delta(files_delta=0, suites_delta=0, @@ -493,44 +540,54 @@ def create_stats_with_delta(files_delta=0, runs_delta=runs_delta, runs_succ_delta=runs_succ_delta, runs_skip_delta=runs_skip_delta, runs_fail_delta=runs_fail_delta, runs_error_delta=runs_error_delta) for label, stats, expected in [('no deltas', create_stats_with_delta(), False), - ('files', create_stats_with_delta(files_delta=1), True), - ('suites', create_stats_with_delta(suites_delta=1), True), - ('duration', create_stats_with_delta(duration_delta=1), False), - ('tests', create_stats_with_delta(tests_delta=1), True), - ('tests succ', create_stats_with_delta(tests_succ_delta=1), True), - ('tests skip', create_stats_with_delta(tests_skip_delta=1), True), - ('tests fail', create_stats_with_delta(tests_fail_delta=1), True), - ('tests error', create_stats_with_delta(tests_error_delta=1), True), - ('runs', create_stats_with_delta(runs_delta=1), True), - ('runs succ', create_stats_with_delta(runs_succ_delta=1), True), - ('runs skip', create_stats_with_delta(runs_skip_delta=1), True), - ('runs fail', create_stats_with_delta(runs_fail_delta=1), True), - ('runs error', create_stats_with_delta(runs_error_delta=1), True)]: + ('files', create_stats_with_delta(files_delta=1), True), + ('suites', create_stats_with_delta(suites_delta=1), True), + ('duration', create_stats_with_delta(duration_delta=1), False), + ('tests', create_stats_with_delta(tests_delta=1), True), + ('tests succ', create_stats_with_delta(tests_succ_delta=1), True), + ('tests skip', create_stats_with_delta(tests_skip_delta=1), True), + ('tests fail', create_stats_with_delta(tests_fail_delta=1), True), + ('tests error', create_stats_with_delta(tests_error_delta=1), True), + ('runs', create_stats_with_delta(runs_delta=1), True), + ('runs succ', create_stats_with_delta(runs_succ_delta=1), True), + ('runs skip', create_stats_with_delta(runs_skip_delta=1), True), + ('runs fail', create_stats_with_delta(runs_fail_delta=1), True), + ('runs error', create_stats_with_delta(runs_error_delta=1), True)]: with self.subTest(msg=label): self.assertEqual(stats.has_changes, expected, msg=label) - def unit_test_run_results_has_failures(self): - def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: - return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + def unit_test_run_delta_results_has_failures(self): + def create_delta_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunDeltaResults: + return create_unit_test_run_delta_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) - for label, stats, expected in [('no failures', create_stats(), False), - ('errors', create_stats(errors=errors), False), - ('test failures', create_stats(tests_fail=1), True), - ('test errors', create_stats(tests_error=1), False), - ('runs failures', create_stats(runs_fail=1), True), - ('runs errors', create_stats(runs_error=1), False)]: + for label, stats, expected in [('no failures', create_delta_stats(), False), + ('errors', create_delta_stats(errors=errors), False), + ('test failures', create_delta_stats(tests_fail=1), True), + ('test errors', create_delta_stats(tests_error=1), False), + ('runs failures', create_delta_stats(runs_fail=1), True), + ('runs errors', create_delta_stats(runs_error=1), False)]: with self.subTest(msg=label): self.assertEqual(stats.has_failures, expected, msg=label) - def test_test_run_results_has_errors(self): - def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: - return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) + def test_test_run_delta_results_has_errors(self): + def create_delta_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunDeltaResults: + return create_unit_test_run_delta_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) - for label, stats, expected in [('no errors', create_stats(), False), - ('errors', create_stats(errors=errors), True), - ('test failures', create_stats(tests_fail=1), False), - ('test errors', create_stats(tests_error=1), True), - ('runs failures', create_stats(runs_fail=1), False), - ('runs errors', create_stats(runs_error=1), True)]: + for label, stats, expected in [('no errors', create_delta_stats(), False), + ('errors', create_delta_stats(errors=errors), True), + ('test failures', create_delta_stats(tests_fail=1), False), + ('test errors', create_delta_stats(tests_error=1), True), + ('runs failures', create_delta_stats(runs_fail=1), False), + ('runs errors', create_delta_stats(runs_error=1), True)]: with self.subTest(msg=label): self.assertEqual(stats.has_errors, expected, msg=label) + + def test_test_run_delta_results_without_delta(self): + with_deltas = create_unit_test_run_delta_results(files=1, errors=errors, suites=2, duration=3, + tests=4, tests_succ=5, tests_skip=6, tests_fail=7, tests_error=8, + runs=9, runs_succ=10, runs_skip=11, runs_fail=12, runs_error=13) + without_deltas = with_deltas.without_delta() + expected = create_unit_test_run_results(files=1, errors=errors, suites=2, duration=3, + tests=4, tests_succ=5, tests_skip=6, tests_fail=7, tests_error=8, + runs=9, runs_succ=10, runs_skip=11, runs_fail=12, runs_error=13) + self.assertEqual(expected, without_deltas) From 3efede11ed04da70db860ed249bf344e56ee0792 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 19:13:34 +0200 Subject: [PATCH 08/26] Remove unrelevant call assertions --- python/test/test_publisher.py | 63 +++++------------------------------ 1 file changed, 9 insertions(+), 54 deletions(-) diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 49b3df02..a16598b7 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -529,7 +529,7 @@ def test_publish_without_compare(self): self.assertEqual({}, kwargs) def test_publish_comment_compare_earlier(self): - pr = mock.MagicMock() + pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) cr = mock.MagicMock() bcr = mock.MagicMock() bs = UnitTestRunResults(1, [], 1, 1, 3, 1, 2, 0, 0, 3, 1, 2, 0, 0, 'commit') @@ -574,25 +574,15 @@ def test_publish_comment_compare_earlier(self): self.assertEqual('require_comment', method) mock_calls = pr.mock_calls - self.assertEqual(3, len(mock_calls)) + self.assertEqual(1, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('create_issue_comment', method) self.assertEqual(('## title\nbody', ), args) self.assertEqual({}, kwargs) - (method, args, kwargs) = mock_calls[1] - self.assertEqual('number.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[2] - self.assertEqual('create_issue_comment().html_url.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - def test_publish_comment_compare_earlier_with_restricted_unicode(self): - pr = mock.MagicMock() + pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) cr = mock.MagicMock(html_url='html://url') bcr = mock.MagicMock() bs = UnitTestRunResults(1, [], 1, 1, 3, 1, 2, 0, 0, 3, 1, 2, 0, 0, 'commit') @@ -655,7 +645,7 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): self.assertEqual('require_comment', method) mock_calls = pr.mock_calls - self.assertEqual(3, len(mock_calls)) + self.assertEqual(1, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('create_issue_comment', method) @@ -706,16 +696,6 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): f'{expected_digest}', ), args) self.assertEqual({}, kwargs) - (method, args, kwargs) = mock_calls[1] - self.assertEqual('number.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[2] - self.assertEqual('create_issue_comment().html_url.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - def test_publish_comment_compare_with_itself(self): pr = mock.MagicMock() cr = mock.MagicMock() @@ -742,7 +722,7 @@ def test_publish_comment_compare_with_itself(self): self.assertEqual(0, len(mock_calls)) def test_publish_comment_compare_with_None(self): - pr = mock.MagicMock() + pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) cr = mock.MagicMock() stats = self.stats cases = UnitTestCaseResults(self.cases) @@ -778,25 +758,15 @@ def test_publish_comment_compare_with_None(self): self.assertEqual('require_comment', method) mock_calls = pr.mock_calls - self.assertEqual(3, len(mock_calls)) + self.assertEqual(1, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('create_issue_comment', method) self.assertEqual(('## title\nbody', ), args) self.assertEqual({}, kwargs) - (method, args, kwargs) = mock_calls[1] - self.assertEqual('number.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[2] - self.assertEqual('create_issue_comment().html_url.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): - pr = mock.MagicMock() + pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) cr = mock.MagicMock() lc = mock.MagicMock(body='latest comment') if one_exists else None stats = self.stats @@ -842,29 +812,14 @@ def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): self.assertEqual('require_comment', method) mock_calls = pr.mock_calls - self.assertEqual(1 if one_exists else 3, len(mock_calls)) + self.assertEqual(0 if one_exists else 1, len(mock_calls)) - if one_exists: - (method, args, kwargs) = mock_calls[0] - self.assertEqual('number.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - else: + if not one_exists: (method, args, kwargs) = mock_calls[0] self.assertEqual('create_issue_comment', method) self.assertEqual(('## title\nbody', ), args) self.assertEqual({}, kwargs) - (method, args, kwargs) = mock_calls[1] - self.assertEqual('number.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[2] - self.assertEqual('create_issue_comment().html_url.__str__', method) - self.assertEqual((), args) - self.assertEqual({}, kwargs) - def test_publish_comment_with_reuse_comment_none_existing(self): self.do_test_publish_comment_with_reuse_comment(one_exists=False) From 9e4dba987e8e6cd4c05dd7894d83670030544c51 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 19:20:41 +0200 Subject: [PATCH 09/26] Rephrase comment_on description --- README.md | 2 +- action.yml | 2 +- composite/action.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0c597e55..f1fe24e0 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,7 @@ The list of most notable options: |:-----|:-----:|:----------| |`time_unit`|`seconds`|Time values in the XML files have this unit. Supports `seconds` and `milliseconds`.| |`job_summary`|`true`| Set to `true`, the results are published as part of the [job summary page](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) of the workflow run.| -|`comment_on`|`always`|Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".| +|`comment_on`|`always`|Create PR comments only in some cases: When there are `"changes"` in the number of tests, failures or runs, when `"test failures"` exist (includes errors), or when (only) `"test errors"` exist. Defaults to `"always"`.| |`comment_mode`|`update last`|The action posts comments to a pull request that is associated with the commit. Set to `create new` to create a new comment on each commit, `update last` to create only one comment and update later on, `off` to not create pull request comments.| |`hide_comments`|`"all but latest"`|Configures which earlier comments in a pull request are hidden by the action:
`"orphaned commits"` - comments for removed commits
`"all but latest"` - all comments but the latest
`"off"` - no hiding| |`compare_to_earlier_commit`|`true`|Test results are compared to results of earlier commits to show changes:
`false` - disable comparison, `true` - compare across commits.'| diff --git a/action.yml b/action.yml index 61282422..c14956f0 100644 --- a/action.yml +++ b/action.yml @@ -22,7 +22,7 @@ inputs: description: 'Title of PR comments, defaults to value of check_name input' required: false comment_on: - description: 'Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create PR comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' default: 'always' required: false fail_on: diff --git a/composite/action.yml b/composite/action.yml index d1cece72..482670a8 100644 --- a/composite/action.yml +++ b/composite/action.yml @@ -22,7 +22,7 @@ inputs: description: 'Title of PR comments, defaults to value of check_name input' required: false comment_on: - description: 'Create PR comments only in some cases: When number of tests, failures or runs "changes", when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create PR comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' default: 'always' required: false fail_on: From c1d7b746dcad7e8a07715300b9fbc6c5dd239163 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 19:26:02 +0200 Subject: [PATCH 10/26] Replace PR by pull request in README.md and action.yml --- README.md | 6 +++--- action.yml | 8 ++++---- composite/action.yml | 8 ++++---- python/publish/publisher.py | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index f1fe24e0..11f01bab 100644 --- a/README.md +++ b/README.md @@ -174,7 +174,7 @@ The list of most notable options: |:-----|:-----:|:----------| |`files`|`*.xml`|File patterns to select the test result XML files, e.g. `"test-results/**/*.xml"`. Use multiline string for multiple patterns. Supports `*`, `**`, `?`, `[]`. Excludes files when starting with `!`. | |`check_name`|`"Unit Test Results"`|An alternative name for the check result.| -|`comment_title`|same as `check_name`|An alternative name for the pull request comment.| +|`comment_title`|same as `check_name`|An alternative title for the pull request comment.|
Options related to Git and GitHub @@ -198,11 +198,11 @@ The list of most notable options: |:-----|:-----:|:----------| |`time_unit`|`seconds`|Time values in the XML files have this unit. Supports `seconds` and `milliseconds`.| |`job_summary`|`true`| Set to `true`, the results are published as part of the [job summary page](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) of the workflow run.| -|`comment_on`|`always`|Create PR comments only in some cases: When there are `"changes"` in the number of tests, failures or runs, when `"test failures"` exist (includes errors), or when (only) `"test errors"` exist. Defaults to `"always"`.| +|`comment_on`|`always`|Create pull request comments only in some cases: When there are `"changes"` in the number of tests, failures or runs, when `"test failures"` exist (includes errors), or when (only) `"test errors"` exist. Defaults to `"always"`.| |`comment_mode`|`update last`|The action posts comments to a pull request that is associated with the commit. Set to `create new` to create a new comment on each commit, `update last` to create only one comment and update later on, `off` to not create pull request comments.| |`hide_comments`|`"all but latest"`|Configures which earlier comments in a pull request are hidden by the action:
`"orphaned commits"` - comments for removed commits
`"all but latest"` - all comments but the latest
`"off"` - no hiding| |`compare_to_earlier_commit`|`true`|Test results are compared to results of earlier commits to show changes:
`false` - disable comparison, `true` - compare across commits.'| -|`test_changes_limit`|`10`|Limits the number of removed or skipped tests reported on PR comments. This report can be disabled with a value of `0`.| +|`test_changes_limit`|`10`|Limits the number of removed or skipped tests reported on pull request comments. This report can be disabled with a value of `0`.| |`report_individual_runs`|`false`|Individual runs of the same test may see different failures. Reports all individual failures when set `true`, and the first failure only otherwise.| |`deduplicate_classes_by_file_name`|`false`|De-duplicates classes with same name by their file name when set `true`, combines test results for those classes otherwise.| |`check_run_annotations`|`all tests, skipped tests`|Adds additional information to the check run. This is a comma-separated list of any of the following values:
`all tests` - list all found tests,
`skipped tests` - list all skipped tests
Set to `none` to add no extra annotations at all.| diff --git a/action.yml b/action.yml index c14956f0..92e3bc3f 100644 --- a/action.yml +++ b/action.yml @@ -19,10 +19,10 @@ inputs: default: 'Unit Test Results' required: false comment_title: - description: 'Title of PR comments, defaults to value of check_name input' + description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false comment_on: - description: 'Create PR comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create pull request comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' default: 'always' required: false fail_on: @@ -54,7 +54,7 @@ inputs: description: 'Deprecated, please use comment_mode instead!' required: false comment_mode: - description: 'Control PR comments: off - disable PR comments, create new - create comments on PRs, each time a new one, update last - create comment on PR, reuse an existing one' + description: 'Control pull request comments: off - disable pull request comments, create new - create comments on PRs, each time a new one, update last - create comment on pull request, reuse an existing one' default: 'update last' required: false job_summary: @@ -76,7 +76,7 @@ inputs: description: 'An alternative event name to use. Useful to replace a "workflow_run" event name with the actual source event name: github.event.workflow_run.event.' required: false test_changes_limit: - description: 'Limits the number of removed or skipped tests reported on PR comments. This report can be disabled with a value of 0. The default is 10.' + description: 'Limits the number of removed or skipped tests reported on pull request comments. This report can be disabled with a value of 0. The default is 10.' required: false check_run_annotations: description: 'Adds additional information to the check run. This is a comma-separated list of any of the following values: "all tests" - list all found tests, "skipped tests" - list all skipped tests. Set to "none" to add no extra annotations at all' diff --git a/composite/action.yml b/composite/action.yml index 482670a8..26401832 100644 --- a/composite/action.yml +++ b/composite/action.yml @@ -19,10 +19,10 @@ inputs: default: 'Unit Test Results' required: false comment_title: - description: 'Title of PR comments, defaults to value of check_name input' + description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false comment_on: - description: 'Create PR comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create pull request comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' default: 'always' required: false fail_on: @@ -54,7 +54,7 @@ inputs: description: 'Deprecated, please use comment_mode instead!' required: false comment_mode: - description: 'Control PR comments: off - disable PR comments, create new - create comments on PRs, each time a new one, update last - create comment on PR, reuse an existing one' + description: 'Control pull request comments: off - disable pull request comments, create new - create comments on PRs, each time a new one, update last - create comment on pull request, reuse an existing one' default: 'update last' required: false job_summary: @@ -76,7 +76,7 @@ inputs: description: 'An alternative event name to use. Useful to replace a "workflow_run" event name with the actual source event name: github.event.workflow_run.event.' required: false test_changes_limit: - description: 'Limits the number of removed or skipped tests reported on PR comments. This report can be disabled with a value of 0. The default is 10.' + description: 'Limits the number of removed or skipped tests reported on pull request comments. This report can be disabled with a value of 0. The default is 10.' required: false check_run_annotations: description: 'Adds additional information to the check run. This is a comma-separated list of any of the following values: "all tests" - list all found tests, "skipped tests" - list all skipped tests. Set to "none" to add no extra annotations at all' diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 2d4574b8..881facc5 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -451,7 +451,7 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) - # we need fetch the latest comment if comment_condition != comment_condition_always + # we need to fetch the latest comment if comment_condition != comment_condition_always # or self._settings.comment_mode == comment_mode_update latest_comment = None if self._settings.comment_condition != comment_condition_always or self._settings.comment_mode == comment_mode_update: From f0ff707c64df6596ba4aa1e939766be5fb113dce Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 20:13:55 +0200 Subject: [PATCH 11/26] Return int and not Optional[int] --- python/publish/unittestresults.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index 4d1f2b65..e92972e6 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -292,10 +292,10 @@ def to_dict(self) -> Dict[str, Any]: def without_delta(self) -> UnitTestRunResults: def v(value: Numeric) -> int: - return value.get('number') + return value['number'] def d(value: Numeric) -> int: - return value.get('duration') + return value['duration'] return UnitTestRunResults(files=v(self.files), errors=self.errors, suites=v(self.suites), duration=d(self.duration), tests=v(self.tests), tests_succ=v(self.tests_succ), tests_skip=v(self.tests_skip), tests_fail=v(self.tests_fail), tests_error=v(self.tests_error), From fb7510742245117ad07e9856cd865133670faf98 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 21:37:58 +0200 Subject: [PATCH 12/26] Turn comment_on into set, make failure and error distjoint --- README.md | 4 ++-- action.yml | 4 ++-- composite/action.yml | 4 ++-- python/publish/__init__.py | 4 ++-- python/publish/publisher.py | 22 +++++++++++----------- python/publish_unit_test_results.py | 7 +++++-- python/test/test_action_script.py | 29 +++++++++++++++++------------ python/test/test_publisher.py | 18 +++++++++++++----- 8 files changed, 54 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 11f01bab..129d3f71 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,7 @@ The list of most notable options: |:-----|:-----:|:----------| |`time_unit`|`seconds`|Time values in the XML files have this unit. Supports `seconds` and `milliseconds`.| |`job_summary`|`true`| Set to `true`, the results are published as part of the [job summary page](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) of the workflow run.| -|`comment_on`|`always`|Create pull request comments only in some cases: When there are `"changes"` in the number of tests, failures or runs, when `"test failures"` exist (includes errors), or when (only) `"test errors"` exist. Defaults to `"always"`.| +|`comment_on`|`always`|Create pull request comments only in some cases. This is a comma-separated list of any of the following values:
`changes` - when there are changes in the number of tests, failures or runs (compared to target branch)
`failures` - when test failures exist
`errors` - when test errors exist.
Example: `changes, failures, errors`.| |`comment_mode`|`update last`|The action posts comments to a pull request that is associated with the commit. Set to `create new` to create a new comment on each commit, `update last` to create only one comment and update later on, `off` to not create pull request comments.| |`hide_comments`|`"all but latest"`|Configures which earlier comments in a pull request are hidden by the action:
`"orphaned commits"` - comments for removed commits
`"all but latest"` - all comments but the latest
`"off"` - no hiding| |`compare_to_earlier_commit`|`true`|Test results are compared to results of earlier commits to show changes:
`false` - disable comparison, `true` - compare across commits.'| @@ -206,7 +206,7 @@ The list of most notable options: |`report_individual_runs`|`false`|Individual runs of the same test may see different failures. Reports all individual failures when set `true`, and the first failure only otherwise.| |`deduplicate_classes_by_file_name`|`false`|De-duplicates classes with same name by their file name when set `true`, combines test results for those classes otherwise.| |`check_run_annotations`|`all tests, skipped tests`|Adds additional information to the check run. This is a comma-separated list of any of the following values:
`all tests` - list all found tests,
`skipped tests` - list all skipped tests
Set to `none` to add no extra annotations at all.| -|`check_run_annotations_branch`|`event.repository.default_branch` or `"main, master"`|Adds check run annotations only on given branches. If not given, this defaults to the default branch of your repository, e.g. `main` or `master`. Comma separated list of branch names allowed, asterisk `"*"` matches all branches. Example: `main, master, branch_one`| +|`check_run_annotations_branch`|`event.repository.default_branch` or `"main, master"`|Adds check run annotations only on given branches. If not given, this defaults to the default branch of your repository, e.g. `main` or `master`. Comma separated list of branch names allowed, asterisk `"*"` matches all branches. Example: `main, master, branch_one`.| |`ignore_runs`|`false`|Does not process test run information by ignoring `` elements in the XML files, which is useful for very large XML files. This disables any check run annotations.| |`json_file`|no file|Results are written to this JSON file.| |`json_thousands_separator`|`" "`|Formatted numbers in JSON use this character to separate groups of thousands. Common values are "," or ".". Defaults to punctuation space (\u2008).| diff --git a/action.yml b/action.yml index 92e3bc3f..12ec251c 100644 --- a/action.yml +++ b/action.yml @@ -22,7 +22,7 @@ inputs: description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false comment_on: - description: 'Create pull request comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create pull request comments only in some cases. This is a comma-separated list of any of the following values: "changes" - when there are changes in the number of tests, failures or runs (compared to target branch), "failures` - when test failures exist, "errors" - when test errors exist. Example: "changes, failures, errors". Defaults to `always`.' default: 'always' required: false fail_on: @@ -83,7 +83,7 @@ inputs: default: 'all tests, skipped tests' required: false check_run_annotations_branch: - description: 'Adds check run annotations only on given branches. Comma separated list of branch names allowed, asterisk "*" matches all branches. Defaults to event.repository.default_branch or "main, master".' + description: 'Adds check run annotations only on given branches. Comma-separated list of branch names allowed, asterisk "*" matches all branches. Defaults to event.repository.default_branch or "main, master".' required: false seconds_between_github_reads: description: 'Sets the number of seconds the action waits between concurrent read requests to the GitHub API. This throttles the API usage to avoid abuse rate limits: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits' diff --git a/composite/action.yml b/composite/action.yml index 26401832..d06e94db 100644 --- a/composite/action.yml +++ b/composite/action.yml @@ -22,7 +22,7 @@ inputs: description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false comment_on: - description: 'Create pull request comments only in some cases: When there are "changes" in the number of tests, failures or runs, when "test failures" exist (includes errors), or when (only) "test errors" exist. Defaults to "always".' + description: 'Create pull request comments only in some cases. This is a comma-separated list of any of the following values: "changes" - when there are changes in the number of tests, failures or runs (compared to target branch), "failures` - when test failures exist, "errors" - when test errors exist. Example: "changes, failures, errors". Defaults to `always`.' default: 'always' required: false fail_on: @@ -83,7 +83,7 @@ inputs: default: 'all tests, skipped tests' required: false check_run_annotations_branch: - description: 'Adds check run annotations only on given branches. Comma separated list of branch names allowed, asterisk "*" matches all branches. Defaults to event.repository.default_branch or "main, master".' + description: 'Adds check run annotations only on given branches. Comma-separated list of branch names allowed, asterisk "*" matches all branches. Defaults to event.repository.default_branch or "main, master".' required: false seconds_between_github_reads: description: 'Sets the number of seconds the action waits between concurrent read requests to the GitHub API. This throttles the API usage to avoid abuse rate limits: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits' diff --git a/python/publish/__init__.py b/python/publish/__init__.py index e9c71e09..065182ad 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -29,8 +29,8 @@ comment_condition_always = 'always' comment_condition_changes = 'changes' -comment_condition_failures = 'test failures' -comment_condition_errors = 'test errors' +comment_condition_failures = 'failures' +comment_condition_errors = 'errors' comment_conditions = [ comment_condition_always, comment_condition_changes, diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 881facc5..49ae6123 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -4,7 +4,7 @@ import os import re from dataclasses import dataclass -from typing import List, Any, Optional, Tuple, Mapping, Dict, Union +from typing import List, Set, Any, Optional, Tuple, Mapping, Dict, Union from copy import deepcopy from github import Github, GithubException @@ -47,7 +47,7 @@ class Settings: check_name: str comment_title: str comment_mode: str - comment_condition: str + comment_conditions: Set[str] job_summary: bool compare_earlier: bool pull_request_build: str @@ -451,17 +451,17 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) - # we need to fetch the latest comment if comment_condition != comment_condition_always + # we need to fetch the latest comment if comment_condition_always not in comment_conditions # or self._settings.comment_mode == comment_mode_update latest_comment = None - if self._settings.comment_condition != comment_condition_always or self._settings.comment_mode == comment_mode_update: + if comment_condition_always not in self._settings.comment_conditions or self._settings.comment_mode == comment_mode_update: latest_comment = self.get_latest_comment(pull_request) latest_comment_body = latest_comment.body if latest_comment else None # are we required to create a comment on this PR? earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None if not self.require_comment(stats_with_delta, earlier_stats, test_changes): - logger.info(f'No comment required as comment_on condition {self._settings.comment_condition} is not met') + logger.info(f'No comment required as comment_on condition {self._settings.comment_conditions} is not met') return details_url = check_run.html_url if check_run else None @@ -480,18 +480,18 @@ def require_comment(self, stats: UnitTestRunResultsOrDeltaResults, earlier_stats: Optional[UnitTestRunResults], test_changes: SomeTestChanges) -> bool: - return (self._settings.comment_condition == comment_condition_always + return (comment_condition_always in self._settings.comment_conditions or - self._settings.comment_condition == comment_condition_changes and ( + comment_condition_changes in self._settings.comment_conditions and ( earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats) or not stats.is_delta or stats.has_changes or test_changes.has_changes) or - self._settings.comment_condition == comment_condition_failures and ( - earlier_stats is not None and (earlier_stats.has_failures or earlier_stats.has_errors) or - stats.has_failures or stats.has_errors) + comment_condition_failures in self._settings.comment_conditions and ( + earlier_stats is not None and earlier_stats.has_failures or + stats.has_failures) or - self._settings.comment_condition == comment_condition_errors and ( + comment_condition_errors in self._settings.comment_conditions and ( earlier_stats is not None and earlier_stats.has_errors or stats.has_errors) ) diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index 9c994ba8..2e628d75 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -288,6 +288,9 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: comment_on_pr = get_bool_var('COMMENT_ON_PR', options, default=True, gha=gha) annotations = get_annotations_config(options, event) + comment_on = get_var('COMMENT_ON', options) or comment_condition_always + comment_on = {comment_condition.strip() for comment_condition in comment_on.split(',')} + fail_on = get_var('FAIL_ON', options) or 'test failures' check_var(fail_on, 'FAIL_ON', 'Check fail mode', fail_on_modes) # here we decide that we want to fail on errors when we fail on test failures, like log level escalation @@ -320,7 +323,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_name=check_name, comment_title=get_var('COMMENT_TITLE', options) or check_name, comment_mode=get_var('COMMENT_MODE', options) or (comment_mode_update if comment_on_pr else comment_mode_off), - comment_condition=get_var('COMMENT_ON', options) or comment_condition_always, + comment_conditions=comment_on, job_summary=get_bool_var('JOB_SUMMARY', options, default=True, gha=gha), compare_earlier=get_bool_var('COMPARE_TO_EARLIER_COMMIT', options, default=True, gha=gha), pull_request_build=get_var('PULL_REQUEST_BUILD', options) or 'merge', @@ -338,7 +341,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes) - check_var(settings.comment_condition, 'COMMENT_ON', 'Comment condition', comment_conditions) + check_var(list(settings.comment_conditions), 'COMMENT_ON', 'Comment conditions', list(comment_conditions)) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index b7176551..704d72a0 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -5,12 +5,13 @@ import tempfile import unittest from typing import Optional, Union, List - +from itertools import combinations import mock from publish import pull_request_build_mode_merge, fail_on_mode_failures, fail_on_mode_errors, \ fail_on_mode_nothing, comment_mode_off, comment_mode_create, comment_mode_update, \ - comment_condition_always, comment_conditions, hide_comments_modes, pull_request_build_modes, punctuation_space + comment_condition_always, comment_condition_changes, comment_condition_failures, comment_condition_errors, \ + comment_conditions, hide_comments_modes, pull_request_build_modes, punctuation_space from publish.github_action import GithubAction from publish.unittestresults import ParsedUnitTestResults, ParseError from publish_unit_test_results import get_conclusion, get_commit_sha, get_var, \ @@ -146,7 +147,7 @@ def get_settings(token='token', check_name='check name', comment_title='title', comment_mode=comment_mode_create, - comment_condition=comment_condition_always, + comment_conditions={comment_condition_always}, job_summary=True, compare_earlier=True, test_changes_limit=10, @@ -179,7 +180,7 @@ def get_settings(token='token', check_name=check_name, comment_title=comment_title, comment_mode=comment_mode, - comment_condition=comment_condition, + comment_conditions=comment_conditions, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -332,15 +333,19 @@ def test_get_settings_comment_mode(self): self.assertEqual("Value 'mode' is not supported for variable COMMENT_MODE, expected: off, create new, update last", str(re.exception)) def test_get_settings_comment_condition(self): - for cond in comment_conditions: - with self.subTest(condition=cond): - self.do_test_get_settings(COMMENT_ON=cond, expected=self.get_settings(comment_condition=cond)) + with self.subTest(condition=None): + self.do_test_get_settings(COMMENT_ON=None, expected=self.get_settings(comment_conditions={comment_condition_always})) - self.do_test_get_settings(COMMENT_ON=None, expected=self.get_settings(comment_condition=comment_condition_always)) + for cond in [cond + for card in range(1, len(comment_conditions)+1) + for cond in combinations(comment_conditions, card)]: + with self.subTest(condition=', '.join(cond)): + self.do_test_get_settings(COMMENT_ON=', '.join(cond), expected=self.get_settings(comment_conditions=set(cond))) - with self.assertRaises(RuntimeError) as re: - self.do_test_get_settings(COMMENT_ON='condition') - self.assertEqual(f"Value 'condition' is not supported for variable COMMENT_ON, expected: always, changes, test failures, test errors", str(re.exception)) + with self.subTest(condition='unknown'): + with self.assertRaises(RuntimeError) as re: + self.do_test_get_settings(COMMENT_ON='condition') + self.assertEqual(f"Some values in 'condition' are not supported for variable COMMENT_ON, allowed: always, changes, failures, errors", str(re.exception)) def test_get_settings_compare_to_earlier_commit(self): warning = 'Option compare_to_earlier_commit has to be boolean, so either "true" or "false": foo' @@ -475,7 +480,7 @@ def do_test_get_settings(self, COMMIT='commit', # defaults to get_commit_sha(event, event_name) FILES='files', COMMENT_TITLE='title', # defaults to check name - COMMENT_MODE='create new', # true unless 'false' + COMMENT_MODE='create new', JOB_SUMMARY='true', HIDE_COMMENTS='off', # defaults to 'all but latest' REPORT_INDIVIDUAL_RUNS='true', # false unless 'true' diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index a16598b7..ce75f104 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -67,7 +67,7 @@ def create_github_pr(repo: str, @staticmethod def create_settings(comment_mode=comment_mode_create, - comment_condition=comment_condition_always, + comment_conditions={comment_condition_always}, job_summary=True, compare_earlier=True, hide_comment_mode=hide_comments_mode_off, @@ -99,7 +99,7 @@ def create_settings(comment_mode=comment_mode_create, check_name='Check Name', comment_title='Comment Title', comment_mode=comment_mode, - comment_condition=comment_condition, + comment_conditions=comment_conditions, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -307,11 +307,11 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) - def do_test_require_comment(self, comment_condition, test_expectation: Callable[["CommentConditionTest"], bool]): + def do_test_require_comment(self, comment_conditions, test_expectation: Callable[["CommentConditionTest"], bool]): tests = [(test, test_expectation(test)) for test in self.comment_condition_tests] publisher = mock.MagicMock(Publisher) - publisher._settings = self.create_settings(comment_condition=comment_condition) + publisher._settings = self.create_settings(comment_conditions=comment_conditions if isinstance(comment_conditions, set) else {comment_conditions}) for test, expected in tests: with self.subTest(test): @@ -352,7 +352,7 @@ def test_require_comment_changes(self): def test_require_comment_failures(self): self.do_test_require_comment( comment_condition_failures, - lambda test: not test.earlier_is_none and (test.earlier_has_failures or test.earlier_has_errors) or test.current_has_failures or test.current_has_errors + lambda test: not test.earlier_is_none and test.earlier_has_failures or test.current_has_failures ) def test_require_comment_errors(self): @@ -361,6 +361,14 @@ def test_require_comment_errors(self): lambda test: not test.earlier_is_none and test.earlier_has_errors or test.current_has_errors ) + def test_require_comment_changes_failures_and_errors(self): + self.do_test_require_comment( + {comment_condition_changes, comment_condition_failures, comment_condition_errors}, + lambda test: not test.earlier_is_none and (test.earlier_is_different or test.earlier_has_failures or test.earlier_has_errors) or + test.current_has_changes is None or test.current_has_changes or test.current_has_failures or test.current_has_errors or + test.tests_have_changes + ) + def test_publish_without_comment(self): settings = self.create_settings(comment_mode=comment_mode_off, hide_comment_mode=hide_comments_mode_off) mock_calls = self.call_mocked_publish(settings, prs=[object()]) From 9632dcf36276944ae4bb9bc069406972a86c199a Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 22:11:43 +0200 Subject: [PATCH 13/26] Exclude commit from stats comparison, improve info log --- python/publish/publisher.py | 2 +- python/publish/unittestresults.py | 11 ++++++----- python/test/test_unittestresults.py | 11 ++++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 49ae6123..59825175 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -461,7 +461,7 @@ def publish_comment(self, # are we required to create a comment on this PR? earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None if not self.require_comment(stats_with_delta, earlier_stats, test_changes): - logger.info(f'No comment required as comment_on condition {self._settings.comment_conditions} is not met') + logger.info(f'No comment required as comment_on condition {", ".join(self._settings.comment_conditions)} is not met') return details_url = check_run.html_url if check_run else None diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index e92972e6..c78ad614 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -229,16 +229,17 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': ) -def patch_ne_duration(orig_ne): - def ne_without_duration(self, other) -> bool: - other = dataclasses.replace(other, duration=self.duration) +def patch_ne(orig_ne): + def ne_without_duration_and_commit(self: UnitTestRunResults, other) -> bool: + if isinstance(other, UnitTestRunResults): + other = dataclasses.replace(other, duration=self.duration, commit=self.commit) return orig_ne(self, other) - return ne_without_duration + return ne_without_duration_and_commit # patch UnitTestRunResults.__ne__ to not include duration -UnitTestRunResults.__ne__ = patch_ne_duration(UnitTestRunResults.__ne__) +UnitTestRunResults.__ne__ = patch_ne(UnitTestRunResults.__ne__) Numeric = Mapping[str, int] diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index d8d88561..6f38d9c2 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -18,7 +18,8 @@ def create_unit_test_run_results(files=1, suites=2, duration=3, tests=22, tests_succ=4, tests_skip=5, tests_fail=6, tests_error=7, - runs=38, runs_succ=8, runs_skip=9, runs_fail=10, runs_error=11) -> UnitTestRunResults: + runs=38, runs_succ=8, runs_skip=9, runs_fail=10, runs_error=11, + commit='commit') -> UnitTestRunResults: return UnitTestRunResults( files=files, errors=list(errors), @@ -26,7 +27,7 @@ def create_unit_test_run_results(files=1, duration=duration, tests=tests, tests_succ=tests_succ, tests_skip=tests_skip, tests_fail=tests_fail, tests_error=tests_error, runs=runs, runs_succ=runs_succ, runs_skip=runs_skip, runs_fail=runs_fail, runs_error=runs_error, - commit='commit' + commit=commit ) @@ -491,10 +492,14 @@ def test_test_run_results_ne(self): ('runs success', create_other(runs_succ=stats.runs_succ+1), True), ('runs skips', create_other(runs_skip=stats.runs_skip+1), True), ('runs failures', create_other(runs_fail=stats.runs_fail+1), True), - ('runs errors', create_other(runs_error=stats.runs_error+1), True)]: + ('runs errors', create_other(runs_error=stats.runs_error+1), True), + ('commit', create_other(commit='other'), False)]: with self.subTest(different_in=diff): self.assertEqual(stats != other, expected, msg=diff) + with self.subTest(different_in='type'): + self.assertEqual(True, stats != object()) + def unit_test_run_results_has_failures(self): def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) From 2a10883c7117e6458d9790aaa024b5e7ea910dfe Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 22:52:38 +0200 Subject: [PATCH 14/26] Compare ParseErrors as dicts --- python/publish/unittestresults.py | 13 +++++++++++-- python/test/test_unittestresults.py | 9 +++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index c78ad614..23ea2e9e 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -150,7 +150,8 @@ class UnitTestResults(ParsedUnitTestResultsWithCommit): @dataclass(frozen=True) class UnitTestRunResults: files: int - errors: List[ParseError] + # when deserialized from dict, this is a list of dicts + errors: Union[List[ParseError], List[Mapping[str, Any]]] suites: int duration: int @@ -230,9 +231,17 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': def patch_ne(orig_ne): + def dict_errors(results: UnitTestRunResults) -> List[Mapping[str, Any]]: + errors = results.errors + if len(errors) and isinstance(errors[0], ParseError): + errors = [dataclasses.asdict(error) for error in errors] + return errors + def ne_without_duration_and_commit(self: UnitTestRunResults, other) -> bool: if isinstance(other, UnitTestRunResults): - other = dataclasses.replace(other, duration=self.duration, commit=self.commit) + other = dataclasses.replace(other, errors=dict_errors(other), duration=self.duration, commit=self.commit) + if self.errors: + self = dataclasses.replace(self, errors=dict_errors(self)) return orig_ne(self, other) return ne_without_duration_and_commit diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index 6f38d9c2..cd904952 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -500,6 +500,15 @@ def test_test_run_results_ne(self): with self.subTest(different_in='type'): self.assertEqual(True, stats != object()) + stats = create_unit_test_run_results(errors=errors) + with self.subTest(different_in='error instance'): + self.assertEqual(False, stats != create_unit_test_run_results(errors=[dataclasses.replace(error) for error in errors])) + + with self.subTest(different_in='error dict'): + other = create_unit_test_run_results(errors=errors_dict) + self.assertEqual(False, stats != other) + self.assertEqual(False, other != stats) + def unit_test_run_results_has_failures(self): def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) From b424ac74e5cef83e3b5ee06308b7fcee79a37a18 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 23:02:58 +0200 Subject: [PATCH 15/26] Ignore errors as they are not stored in the digest --- python/publish/unittestresults.py | 13 ++----------- python/test/test_unittestresults.py | 12 ++---------- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index 23ea2e9e..70b2608e 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -150,8 +150,7 @@ class UnitTestResults(ParsedUnitTestResultsWithCommit): @dataclass(frozen=True) class UnitTestRunResults: files: int - # when deserialized from dict, this is a list of dicts - errors: Union[List[ParseError], List[Mapping[str, Any]]] + errors: List[ParseError] suites: int duration: int @@ -231,17 +230,9 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': def patch_ne(orig_ne): - def dict_errors(results: UnitTestRunResults) -> List[Mapping[str, Any]]: - errors = results.errors - if len(errors) and isinstance(errors[0], ParseError): - errors = [dataclasses.asdict(error) for error in errors] - return errors - def ne_without_duration_and_commit(self: UnitTestRunResults, other) -> bool: if isinstance(other, UnitTestRunResults): - other = dataclasses.replace(other, errors=dict_errors(other), duration=self.duration, commit=self.commit) - if self.errors: - self = dataclasses.replace(self, errors=dict_errors(self)) + other = dataclasses.replace(other, errors=self.errors, duration=self.duration, commit=self.commit) return orig_ne(self, other) return ne_without_duration_and_commit diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index cd904952..16c0ffd9 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -127,6 +127,7 @@ def test_unit_test_run_results_to_dict(self): ) self.assertEqual(expected, actual) + # results from dicts usually do not contain errors def test_unit_test_run_results_from_dict(self): actual = UnitTestRunResults.from_dict(dict( files=1, errors=errors_dict, suites=2, duration=3, @@ -480,7 +481,7 @@ def test_test_run_results_ne(self): create_other = create_unit_test_run_results for diff, other, expected in [('nothing', create_other(), False), ('files', create_other(files=stats.files+1), True), - ('errors', create_other(errors=errors), True), + ('errors', create_other(errors=errors), False), ('suites', create_other(suites=stats.suites+1), True), ('duration', create_other(duration=stats.duration+1), False), ('tests', create_other(tests=stats.tests+1), True), @@ -500,15 +501,6 @@ def test_test_run_results_ne(self): with self.subTest(different_in='type'): self.assertEqual(True, stats != object()) - stats = create_unit_test_run_results(errors=errors) - with self.subTest(different_in='error instance'): - self.assertEqual(False, stats != create_unit_test_run_results(errors=[dataclasses.replace(error) for error in errors])) - - with self.subTest(different_in='error dict'): - other = create_unit_test_run_results(errors=errors_dict) - self.assertEqual(False, stats != other) - self.assertEqual(False, other != stats) - def unit_test_run_results_has_failures(self): def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) From 122ba46255a234e62d33320268c5dd6578533edb Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 23:17:42 +0200 Subject: [PATCH 16/26] Explain why comment is required --- python/publish/publisher.py | 56 +++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 59825175..4ea1a20f 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -480,21 +480,47 @@ def require_comment(self, stats: UnitTestRunResultsOrDeltaResults, earlier_stats: Optional[UnitTestRunResults], test_changes: SomeTestChanges) -> bool: - return (comment_condition_always in self._settings.comment_conditions - or - comment_condition_changes in self._settings.comment_conditions and ( - earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats) or - not stats.is_delta or stats.has_changes or - test_changes.has_changes) - or - comment_condition_failures in self._settings.comment_conditions and ( - earlier_stats is not None and earlier_stats.has_failures or - stats.has_failures) - or - comment_condition_errors in self._settings.comment_conditions and ( - earlier_stats is not None and earlier_stats.has_errors or - stats.has_errors) - ) + if comment_condition_always in self._settings.comment_conditions: + logger.debug(f'Comment required as condition contains {comment_condition_always}') + return True + + if comment_condition_changes in self._settings.comment_conditions: + if earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats): + logger.debug(f'Comment required as condition contains {comment_condition_changes} and stats different to earlier') + logger.debug(f'earlier: {earlier_stats}') + logger.debug(f'current: {stats}') + if stats.is_delta: + logger.debug(f'current without delta: {stats.without_delta()}') + return True + if not stats.is_delta: + logger.debug(f'Comment required as condition contains {comment_condition_changes} and no delta available') + return True + if stats.has_changes: + logger.debug(f'Comment required as condition contains {comment_condition_changes} and changes exist') + logger.debug(f'current: {stats}') + return True + if test_changes.has_changes: + logger.debug(f'Comment required as condition contains {comment_condition_changes} and tests changed') + logger.debug(f'tests: {test_changes}') + return True + + if comment_condition_failures in self._settings.comment_conditions: + if earlier_stats is not None and earlier_stats.has_failures: + logger.debug(f'Comment required as condition contains {comment_condition_failures} and earlier failures exist') + return True + if stats.has_failures: + logger.debug(f'Comment required as condition contains {comment_condition_failures} and failures exist') + return True + + if comment_condition_errors in self._settings.comment_conditions: + if earlier_stats is not None and earlier_stats.has_errors: + logger.debug(f'Comment required as condition contains {comment_condition_errors} and earlier errors exist') + return True + if stats.has_errors: + logger.debug(f'Comment required as condition contains {comment_condition_errors} and errors exist') + return True + + return False def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]: # get comments of this pull request From 49aad59592b5930c759d555cd81a0dab25daad90 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 23:26:38 +0200 Subject: [PATCH 17/26] Rename patched ne function --- python/publish/unittestresults.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index 70b2608e..23cd8ce5 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -230,12 +230,12 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': def patch_ne(orig_ne): - def ne_without_duration_and_commit(self: UnitTestRunResults, other) -> bool: + def patched_ne(self: UnitTestRunResults, other) -> bool: if isinstance(other, UnitTestRunResults): other = dataclasses.replace(other, errors=self.errors, duration=self.duration, commit=self.commit) return orig_ne(self, other) - return ne_without_duration_and_commit + return patched_ne # patch UnitTestRunResults.__ne__ to not include duration From 6fb32c721ceeb64b55c5936bdbe6ce9a19f2e345 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Sun, 12 Jun 2022 23:35:03 +0200 Subject: [PATCH 18/26] Minor changes to debug logging --- python/publish/publisher.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 4ea1a20f..c62a8b3c 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -486,21 +486,22 @@ def require_comment(self, if comment_condition_changes in self._settings.comment_conditions: if earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats): - logger.debug(f'Comment required as condition contains {comment_condition_changes} and stats different to earlier') + logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and stats different to earlier') logger.debug(f'earlier: {earlier_stats}') - logger.debug(f'current: {stats}') if stats.is_delta: - logger.debug(f'current without delta: {stats.without_delta()}') + logger.debug(f'current: {stats.without_delta()}') + else: + logger.debug(f'current: {stats}') return True if not stats.is_delta: - logger.debug(f'Comment required as condition contains {comment_condition_changes} and no delta available') + logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and no delta available') return True if stats.has_changes: - logger.debug(f'Comment required as condition contains {comment_condition_changes} and changes exist') + logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and changes exist') logger.debug(f'current: {stats}') return True if test_changes.has_changes: - logger.debug(f'Comment required as condition contains {comment_condition_changes} and tests changed') + logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and tests changed') logger.debug(f'tests: {test_changes}') return True From b64a311e0ea7dbc061534852e2d6743f3d310ea0 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 13 Jun 2022 00:20:18 +0200 Subject: [PATCH 19/26] Add newline after digest, detect end of digest --- python/publish/__init__.py | 2 +- python/publish/publisher.py | 9 +++++--- python/test/test_publish.py | 12 +++++------ python/test/test_publisher.py | 39 ++++++++++++++++++++++++----------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index 065182ad..a8cba084 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -717,7 +717,7 @@ def get_long_summary_with_digest_md(stats: UnitTestRunResultsOrDeltaResults, raise ValueError('stats must be UnitTestRunResults when no digest_stats is given') summary = get_long_summary_md(stats, details_url, test_changes, test_list_changes_limit) digest = get_digest_from_stats(stats if digest_stats is None else digest_stats) - return f'{summary}\n{digest_header}{digest}' + return f'{summary}\n{digest_header}{digest}\n' def get_case_messages(case_results: UnitTestCaseResults) -> CaseMessages: diff --git a/python/publish/publisher.py b/python/publish/publisher.py index c62a8b3c..80e75bc6 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -271,9 +271,12 @@ def get_stats_from_check_run(check_run: CheckRun) -> Optional[UnitTestRunResults @staticmethod def get_stats_from_summary_md(summary: str) -> Optional[UnitTestRunResults]: - pos = summary.index(digest_header) if digest_header in summary else None - if pos: - digest = summary[pos + len(digest_header):] + start = summary.index(digest_header) if digest_header in summary else None + if start: + digest = summary[start + len(digest_header):] + end = digest.index('\n') if '\n' in digest else None + if end: + digest = digest[:end] logger.debug(f'digest: {digest}') stats = get_stats_from_digest(digest) logger.debug(f'stats: {stats}') diff --git a/python/test/test_publish.py b/python/test/test_publish.py index 255a1b0d..7526c875 100644 --- a/python/test/test_publish.py +++ b/python/test/test_publish.py @@ -1014,7 +1014,7 @@ def test_get_long_summary_with_digest_md_with_single_run(self): 'H4sIAAAAAAAC/02MywqAIBQFfyVct+kd/UyEJVzKjKuuon/vZF' 'juzsyBOYWibbFiyIo8E9aTC1ACZs+TI7MDKyAO91x13KP1UkI0' 'v1jpgGg/oSbaILpPLMyGYXoY9nvsPTPNvfzXAiexwGlLGq3JAe' - 'K6buousrLZAAAA') + 'K6buousrLZAAAA\n') def test_get_long_summary_with_digest_md_with_multiple_runs(self): # makes gzipped digest deterministic @@ -1038,7 +1038,7 @@ def test_get_long_summary_with_digest_md_with_multiple_runs(self): 'H4sIAAAAAAAC/03MwQqDMBAE0F+RnD24aiv6M0VShaVqZJOciv' '/e0brR28wbmK8ZeRq86TLKM+Mjh6OUKO8ofWC3oFaoGMI+1Zpf' 'PloLeFzw4RXwTDD2PAGaBIOIE0gBkbjsf+0Z9Y6KBP87IoXzjk' - 'qF+51188wBRdP2A3NU1srcAAAA') + 'qF+51188wBRdP2A3NU1srcAAAA\n') def test_get_long_summary_with_digest_md_with_test_errors(self): # makes gzipped digest deterministic @@ -1062,7 +1062,7 @@ def test_get_long_summary_with_digest_md_with_test_errors(self): 'H4sIAAAAAAAC/0XOwQ6CMBAE0F8hPXtgEVT8GdMUSDYCJdv2ZP' 'x3psLW28zbZLIfM/E8BvOs6FKZkDj+SoMyJLGR/Yp6RcUh5lOr' '+RWSc4DuD2/eALcCk+UZcC8winiBPCCS1rzXn1HnqC5wzBEpnH' - 'PUKOgc5QedXxaOaJq+O+lMT3jdAAAA') + 'PUKOgc5QedXxaOaJq+O+lMT3jdAAAA\n') def test_get_long_summary_with_digest_md_with_parse_errors(self): # makes gzipped digest deterministic @@ -1086,7 +1086,7 @@ def test_get_long_summary_with_digest_md_with_parse_errors(self): 'H4sIAAAAAAAC/0XOwQ6CMBAE0F8hPXtgEVT8GdMUSDYCJdv2ZP' 'x3psLW28zbZLIfM/E8BvOs6FKZkDj+SoMyJLGR/Yp6RcUh5lOr' '+RWSc4DuD2/eALcCk+UZcC8winiBPCCS1rzXn1HnqC5wzBEpnH' - 'PUKOgc5QedXxaOaJq+O+lMT3jdAAAA') + 'PUKOgc5QedXxaOaJq+O+lMT3jdAAAA\n') def test_get_long_summary_with_digest_md_with_delta(self): # makes gzipped digest deterministic @@ -1115,7 +1115,7 @@ def test_get_long_summary_with_digest_md_with_delta(self): 'H4sIAAAAAAAC/02MywqAIBQFfyVct+kd/UyEJVzKjKuuon/vZF' 'juzsyBOYWibbFiyIo8E9aTC1ACZs+TI7MDKyAO91x13KP1UkI0' 'v1jpgGg/oSbaILpPLMyGYXoY9nvsPTPNvfzXAiexwGlLGq3JAe' - 'K6buousrLZAAAA') + 'K6buousrLZAAAA\n') def test_get_long_summary_with_digest_md_with_delta_and_parse_errors(self): # makes gzipped digest deterministic @@ -1144,7 +1144,7 @@ def test_get_long_summary_with_digest_md_with_delta_and_parse_errors(self): 'H4sIAAAAAAAC/02MywqAIBQFfyVct+kd/UyEJVzKjKuuon/vZF' 'juzsyBOYWibbFiyIo8E9aTC1ACZs+TI7MDKyAO91x13KP1UkI0' 'v1jpgGg/oSbaILpPLMyGYXoY9nvsPTPNvfzXAiexwGlLGq3JAe' - 'K6buousrLZAAAA') + 'K6buousrLZAAAA\n') def test_get_long_summary_with_digest_md_with_delta_results_only(self): with self.assertRaises(ValueError) as context: diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index ce75f104..2a92860a 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -17,11 +17,14 @@ get_error_annotation, digest_header, get_digest_from_stats, \ all_tests_list, skipped_tests_list, none_list, \ all_tests_label_md, skipped_tests_label_md, failed_tests_label_md, passed_tests_label_md, test_errors_label_md, \ - duration_label_md, pull_request_build_mode_merge, punctuation_space + duration_label_md, pull_request_build_mode_merge, punctuation_space, \ + get_long_summary_with_digest_md from publish.github_action import GithubAction from publish.publisher import Publisher, Settings, PublishData from publish.unittestresults import UnitTestCase, ParseError, UnitTestRunResults, UnitTestRunDeltaResults, \ UnitTestCaseResults +from test_unittestresults import create_unit_test_run_results + errors = [ParseError('file', 'error', 1, 2)] @@ -701,7 +704,7 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): '```\n' '
\n' '\n' - f'{expected_digest}', ), args) + f'{expected_digest}\n', ), args) self.assertEqual({}, kwargs) def test_publish_comment_compare_with_itself(self): @@ -997,6 +1000,18 @@ def test_get_check_run_from_list_many(self): name = runs[0].name self.do_test_get_check_run_from_list(runs, expected) + def test_get_stats_from_summary_md(self): + results = create_unit_test_run_results() + summary = get_long_summary_with_digest_md(results, results, 'http://url') + actual = Publisher.get_stats_from_summary_md(summary) + self.assertEqual(results, actual) + + def test_get_stats_from_summary_md_recycled(self): + summary = f'body\n\n{digest_header}H4sIAGpapmIC/1WMyw7CIBQFf6Vh7QK4FMGfMeQWEmJbDI9V479LI6DuzsxJ5iDOrzaR2wSXiaTi84ClRJN92CvSivXI5yX7vqeCWIX4iod/VsGGcMavf8LGGGILxrKfPaba7j3Ghvj0ROeWg86/NQzb5nMFIhCBgnbUzQAIVik+c6W1YU5KVPoqNF04teT1BvQuAoL9AAAA\n:recycle: This comment has been updated with latest results.' + actual = Publisher.get_stats_from_summary_md(summary) + self.assertIsNotNone(actual) + self.assertEqual(6, actual.tests) + @staticmethod def mock_check_run(name: str, status: str, started_at: datetime, summary: str) -> mock.Mock: run = mock.MagicMock(status=status, started_at=started_at, output=mock.MagicMock(summary=summary)) @@ -1236,7 +1251,7 @@ def do_test_publish_check_without_base_stats(self, errors: List[ParseError], ann 'H4sIAAAAAAAC/0WOSQqEMBBFryJZu+g4tK2XkRAVCoc0lWQl3t' '3vULqr9z48alUDTb1XTaLTRPlI4YQM0EU2gdwCzIEYwjllAq2P' '1sIUrxjpD1E+YjA0QXwf0TM7hqlgOC5HMP/dt/RevnK18F3THx' - 'FS08fz1s0zBZBc2w5zHdX73QAAAA=='.format(errors='{} errors\u2004\u2003'.format(len(errors)) if len(errors) > 0 else ''), + 'FS08fz1s0zBZBc2w5zHdX73QAAAA==\n'.format(errors='{} errors\u2004\u2003'.format(len(errors)) if len(errors) > 0 else ''), 'annotations': annotations } ) @@ -1302,7 +1317,7 @@ def do_test_publish_check_with_base_stats(self, errors: List[ParseError]): 'H4sIAAAAAAAC/0WOSQqEMBBFryJZu+g4tK2XkRAVCoc0lWQl3t' '3vULqr9z48alUDTb1XTaLTRPlI4YQM0EU2gdwCzIEYwjllAq2P' '1sIUrxjpD1E+YjA0QXwf0TM7hqlgOC5HMP/dt/RevnK18F3THx' - 'FS08fz1s0zBZBc2w5zHdX73QAAAA=='.format(errors='{} errors\u2004\u2003'.format(len(errors)) if len(errors) > 0 else ''), + 'FS08fz1s0zBZBc2w5zHdX73QAAAA==\n'.format(errors='{} errors\u2004\u2003'.format(len(errors)) if len(errors) > 0 else ''), 'annotations': error_annotations + [ {'path': 'test file', 'start_line': 0, 'end_line': 0, 'annotation_level': 'warning', 'message': 'result file', 'title': '1 out of 2 runs failed: test (class)', 'raw_details': 'content'}, {'path': 'test file', 'start_line': 0, 'end_line': 0, 'annotation_level': 'failure', 'message': 'result file', 'title': '1 out of 2 runs with error: test2 (class)', 'raw_details': 'error content'}, @@ -1366,7 +1381,7 @@ def test_publish_check_without_compare(self): '[test-results]:data:application/gzip;base64,H4sIAAAAAAAC/0WOSQqEMBBFryJ' 'Zu+g4tK2XkRAVCoc0lWQl3t3vULqr9z48alUDTb1XTaLTRPlI4YQM0EU2gdwCzIEYwjllAq' '2P1sIUrxjpD1E+YjA0QXwf0TM7hqlgOC5HMP/dt/RevnK18F3THxFS08fz1s0zBZBc2w5zH' - 'dX73QAAAA==', + 'dX73QAAAA==\n', 'annotations': [ {'path': 'test file', 'start_line': 0, 'end_line': 0, 'annotation_level': 'warning', 'message': 'result file', 'title': '1 out of 2 runs failed: test (class)', 'raw_details': 'content'}, {'path': 'test file', 'start_line': 0, 'end_line': 0, 'annotation_level': 'failure', 'message': 'result file', 'title': '1 out of 2 runs with error: test2 (class)', 'raw_details': 'error content'}, @@ -1428,7 +1443,7 @@ def test_publish_check_with_multiple_annotation_pages(self): 'H4sIAAAAAAAC/0WOSQqEMBBFryJZu+g4tK2XkRAVCoc0lWQl3t' '3vULqr9z48alUDTb1XTaLTRPlI4YQM0EU2gdwCzIEYwjllAq2P' '1sIUrxjpD1E+YjA0QXwf0TM7hqlgOC5HMP/dt/RevnK18F3THx' - 'FS08fz1s0zBZBc2w5zHdX73QAAAA==', + 'FS08fz1s0zBZBc2w5zHdX73QAAAA==\n', 'annotations': ([ {'path': 'test file', 'start_line': i, 'end_line': i, 'annotation_level': 'warning', 'message': 'result file', 'title': f'test{i} (class) failed', 'raw_details': f'content{i}'} # we expect the first 50 annotations in the create call @@ -1461,7 +1476,7 @@ def test_publish_check_with_multiple_annotation_pages(self): 'H4sIAAAAAAAC/0WOSQqEMBBFryJZu+g4tK2XkRAVCoc0lWQl3t' '3vULqr9z48alUDTb1XTaLTRPlI4YQM0EU2gdwCzIEYwjllAq2P' '1sIUrxjpD1E+YjA0QXwf0TM7hqlgOC5HMP/dt/RevnK18F3THx' - 'FS08fz1s0zBZBc2w5zHdX73QAAAA==', + 'FS08fz1s0zBZBc2w5zHdX73QAAAA==\n', 'annotations': ([ {'path': 'test file', 'start_line': i, 'end_line': i, 'annotation_level': 'warning', 'message': 'result file', 'title': f'test{i} (class) failed', 'raw_details': f'content{i}'} # for each edit we expect a batch of 50 annotations starting at start @@ -1834,7 +1849,7 @@ def test_publish_comment(self): '\n' 'Results for commit commit.\u2003± Comparison against base commit base.\n' '\n' - f'{expected_digest}' + f'{expected_digest}\n' ) def test_publish_comment_not_required(self): @@ -1875,7 +1890,7 @@ def test_publish_comment_without_base(self): '\n' 'Results for commit commit.\n' '\n' - f'{expected_digest}' + f'{expected_digest}\n' ) def test_publish_comment_without_compare(self): @@ -1899,7 +1914,7 @@ def test_publish_comment_without_compare(self): '\n' 'Results for commit commit.\n' '\n' - f'{expected_digest}' + f'{expected_digest}\n' ) def test_publish_comment_with_check_run_with_annotations(self): @@ -1926,7 +1941,7 @@ def test_publish_comment_with_check_run_with_annotations(self): '\n' 'Results for commit commit.\u2003± Comparison against base commit base.\n' '\n' - f'{expected_digest}' + f'{expected_digest}\n' ) def test_publish_comment_with_check_run_without_annotations(self): @@ -1955,7 +1970,7 @@ def test_publish_comment_with_check_run_without_annotations(self): '\n' 'Results for commit commit.\u2003± Comparison against base commit base.\n' '\n' - f'{expected_digest}' + f'{expected_digest}\n' ) def test_get_base_commit_sha_none_event(self): From 1cb1378cac350424233b7b7af86b894659578af3 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 13 Jun 2022 09:06:35 +0200 Subject: [PATCH 20/26] Remove SomeTestChanges from detecting changes --- python/publish/publisher.py | 12 +++++------- python/test/test_publisher.py | 16 +++++----------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 80e75bc6..684b2b57 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -463,7 +463,7 @@ def publish_comment(self, # are we required to create a comment on this PR? earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None - if not self.require_comment(stats_with_delta, earlier_stats, test_changes): + if not self.require_comment(stats_with_delta, earlier_stats): logger.info(f'No comment required as comment_on condition {", ".join(self._settings.comment_conditions)} is not met') return @@ -481,8 +481,10 @@ def publish_comment(self, def require_comment(self, stats: UnitTestRunResultsOrDeltaResults, - earlier_stats: Optional[UnitTestRunResults], - test_changes: SomeTestChanges) -> bool: + earlier_stats: Optional[UnitTestRunResults]) -> bool: + # SomeTestChanges.has_changes cannot be used here as changes between earlier comment + # and current results cannot be identified + if comment_condition_always in self._settings.comment_conditions: logger.debug(f'Comment required as condition contains {comment_condition_always}') return True @@ -503,10 +505,6 @@ def require_comment(self, logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and changes exist') logger.debug(f'current: {stats}') return True - if test_changes.has_changes: - logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and tests changed') - logger.debug(f'tests: {test_changes}') - return True if comment_condition_failures in self._settings.comment_conditions: if earlier_stats is not None and earlier_stats.has_failures: diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 2a92860a..c141be69 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -39,7 +39,6 @@ class CommentConditionTest: current_has_changes: Optional[bool] current_has_failures: bool current_has_errors: bool - tests_have_changes: bool class TestPublisher(unittest.TestCase): @@ -322,21 +321,18 @@ def do_test_require_comment(self, comment_conditions, test_expectation: Callable current = mock.MagicMock(is_delta=test.current_has_changes is not None, has_changes=test.current_has_changes, has_failures=test.current_has_failures, has_errors=test.current_has_errors) if current.is_delta: current.without_delta = mock.Mock(return_value=current) - test_changes = mock.MagicMock(has_changes=test.tests_have_changes) - required = Publisher.require_comment(publisher, current, earlier, test_changes) + required = Publisher.require_comment(publisher, current, earlier) self.assertEqual(required, expected) comment_condition_tests = [CommentConditionTest(earlier_is_none, earlier_is_different, earlier_has_failures, earlier_has_errors, - current_has_changes, current_has_failures, current_has_errors, - tests_have_changes) + current_has_changes, current_has_failures, current_has_errors) for earlier_is_none in [False, True] for earlier_is_different in [False, True] for earlier_has_failures in [False, True] for earlier_has_errors in [False, True] for current_has_changes in [None, False, True] for current_has_failures in [False, True] - for current_has_errors in [False, True] - for tests_have_changes in [False, True]] + for current_has_errors in [False, True]] def test_require_comment_always(self): self.do_test_require_comment( @@ -348,8 +344,7 @@ def test_require_comment_changes(self): self.do_test_require_comment( comment_condition_changes, lambda test: not test.earlier_is_none and test.earlier_is_different or - test.current_has_changes is None or test.current_has_changes or - test.tests_have_changes + test.current_has_changes is None or test.current_has_changes ) def test_require_comment_failures(self): @@ -368,8 +363,7 @@ def test_require_comment_changes_failures_and_errors(self): self.do_test_require_comment( {comment_condition_changes, comment_condition_failures, comment_condition_errors}, lambda test: not test.earlier_is_none and (test.earlier_is_different or test.earlier_has_failures or test.earlier_has_errors) or - test.current_has_changes is None or test.current_has_changes or test.current_has_failures or test.current_has_errors or - test.tests_have_changes + test.current_has_changes is None or test.current_has_changes or test.current_has_failures or test.current_has_errors ) def test_publish_without_comment(self): From b5f2e376e64ba6218aacfcf25f354ce74be3919c Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 13 Jun 2022 10:50:06 +0200 Subject: [PATCH 21/26] Improving info logging --- python/publish/publisher.py | 48 ++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 684b2b57..7987393c 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -139,7 +139,7 @@ def publish(self, stats: UnitTestRunResults, cases: UnitTestCaseResults, conclusion: str): - logger.info(f'publishing {conclusion} results for commit {self._settings.commit}') + logger.info(f'Publishing {conclusion} results for commit {self._settings.commit}') check_run, before_check_run = self.publish_check(stats, cases, conclusion) if self._settings.job_summary: @@ -155,11 +155,11 @@ def publish(self, elif self._settings.hide_comment_mode == hide_comments_mode_all_but_latest: self.hide_all_but_latest_comments(pull) if self._settings.hide_comment_mode == hide_comments_mode_off: - logger.info('hide_comments disabled, not hiding any comments') + logger.info('Hiding comments disabled (hide_comments)') else: - logger.info(f'there is no pull request for commit {self._settings.commit}') + logger.info(f'There is no pull request for commit {self._settings.commit}') else: - logger.info('comment_on_pr disabled, not commenting on any pull requests') + logger.info('Commenting on pull requests disabled (comment_on_pr)') def get_pulls(self, commit: str) -> List[PullRequest]: # totalCount calls the GitHub API just to get the total number @@ -341,7 +341,7 @@ def publish_check(self, status='completed', conclusion=conclusion, output=output) - logger.info(f'created check {check_run.html_url}') + logger.info(f'Created check {check_run.html_url}') else: logger.debug(f'updating check with {len(annotations)} more annotations') check_run.edit(output=output) @@ -375,7 +375,7 @@ def publish_job_summary(self, summary = get_long_summary_md(stats_with_delta, details_url) markdown = f'## {title}\n{summary}' self._gha.add_to_job_summary(markdown) - logger.info(f'created job summary') + logger.info(f'Created job summary') @staticmethod def get_test_lists_from_check_run(check_run: Optional[CheckRun]) -> Tuple[Optional[List[str]], Optional[List[str]]]: @@ -464,7 +464,7 @@ def publish_comment(self, # are we required to create a comment on this PR? earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None if not self.require_comment(stats_with_delta, earlier_stats): - logger.info(f'No comment required as comment_on condition {", ".join(self._settings.comment_conditions)} is not met') + logger.info(f'No comment required as comment_on condition "{", ".join(self._settings.comment_conditions)}" is not met') return details_url = check_run.html_url if check_run else None @@ -474,10 +474,10 @@ def publish_comment(self, # reuse existing comment when comment_mode == comment_mode_update, otherwise create new comment if self._settings.comment_mode == comment_mode_update and latest_comment is not None: self.reuse_comment(latest_comment, body) - logger.info(f'edited comment for pull request #{pull_request.number}: {latest_comment.html_url}') + logger.info(f'Edited comment for pull request #{pull_request.number}: {latest_comment.html_url}') else: comment = pull_request.create_issue_comment(body) - logger.info(f'created comment for pull request #{pull_request.number}: {comment.html_url}') + logger.info(f'Created comment for pull request #{pull_request.number}: {comment.html_url}') def require_comment(self, stats: UnitTestRunResultsOrDeltaResults, @@ -491,35 +491,39 @@ def require_comment(self, if comment_condition_changes in self._settings.comment_conditions: if earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats): - logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and stats different to earlier') + logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' + f'and stats are different to earlier comment') logger.debug(f'earlier: {earlier_stats}') - if stats.is_delta: - logger.debug(f'current: {stats.without_delta()}') - else: - logger.debug(f'current: {stats}') + logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') return True if not stats.is_delta: - logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and no delta available') + logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' + f'but no delta statistics to target branch available') return True if stats.has_changes: - logger.debug(f'Comment required as condition contains "{comment_condition_changes}" and changes exist') + logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' + f'and changes to target branch exist') logger.debug(f'current: {stats}') return True if comment_condition_failures in self._settings.comment_conditions: if earlier_stats is not None and earlier_stats.has_failures: - logger.debug(f'Comment required as condition contains {comment_condition_failures} and earlier failures exist') + logger.info(f'Comment required as condition contains {comment_condition_failures} ' + f'and failures exist in earlier comment') return True if stats.has_failures: - logger.debug(f'Comment required as condition contains {comment_condition_failures} and failures exist') + logger.info(f'Comment required as condition contains {comment_condition_failures} ' + f'and failures exist in current comment') return True if comment_condition_errors in self._settings.comment_conditions: if earlier_stats is not None and earlier_stats.has_errors: - logger.debug(f'Comment required as condition contains {comment_condition_errors} and earlier errors exist') + logger.info(f'Comment required as condition contains {comment_condition_errors} ' + f'and errors exist in earlier comment') return True if stats.has_errors: - logger.debug(f'Comment required as condition contains {comment_condition_errors} and errors exist') + logger.info(f'Comment required as condition contains {comment_condition_errors} ' + f'and errors exist in current comment') return True return False @@ -656,7 +660,7 @@ def hide_orphaned_commit_comments(self, pull: PullRequest) -> None: # hide all those comments for node_id, comment_commit_sha in comment_ids: - logger.info(f'hiding unit test result comment for commit {comment_commit_sha}') + logger.info(f'Hiding unit test result comment for commit {comment_commit_sha}') self.hide_comment(node_id) def hide_all_but_latest_comments(self, pull: PullRequest) -> None: @@ -673,5 +677,5 @@ def hide_all_but_latest_comments(self, pull: PullRequest) -> None: # hide all those comments for node_id in comment_ids: - logger.info(f'hiding unit test result comment {node_id}') + logger.info(f'Hiding unit test result comment {node_id}') self.hide_comment(node_id) From 8b5c71b51cb971b27ccb34647066cae171cac115 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Mon, 13 Jun 2022 12:58:41 +0200 Subject: [PATCH 22/26] Remove comment_on set, integrate into comment_mode, deprecate create and update --- python/publish/__init__.py | 29 ++++++++++---------- python/publish/publisher.py | 40 +++++++++++++--------------- python/publish_unit_test_results.py | 36 +++++++++++++++++-------- python/test/test_action_script.py | 41 ++++++++--------------------- python/test/test_publisher.py | 30 ++++++++------------- 5 files changed, 80 insertions(+), 96 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index a8cba084..b7fb3ae6 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -19,24 +19,23 @@ punctuation_space = ' ' comment_mode_off = 'off' -comment_mode_create = 'create new' -comment_mode_update = 'update last' +comment_mode_create = 'create new' # deprecated +comment_mode_update = 'update last' # deprecated +comment_mode_always = 'always' +comment_mode_changes = 'changes' +comment_mode_failures = 'failures' # includes comment_mode_errors +comment_mode_errors = 'errors' comment_modes = [ comment_mode_off, - comment_mode_create, - comment_mode_update -] - -comment_condition_always = 'always' -comment_condition_changes = 'changes' -comment_condition_failures = 'failures' -comment_condition_errors = 'errors' -comment_conditions = [ - comment_condition_always, - comment_condition_changes, - comment_condition_failures, - comment_condition_errors + comment_mode_always, + comment_mode_changes, + comment_mode_failures, + comment_mode_errors ] +comment_modes_deprecated = { + comment_mode_create: comment_mode_always, + comment_mode_update: comment_mode_always +} fail_on_mode_nothing = 'nothing' fail_on_mode_errors = 'errors' diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 7987393c..1869fc4e 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -15,7 +15,7 @@ from publish import hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, hide_comments_mode_off, \ comment_mode_off, comment_mode_create, comment_mode_update, digest_prefix, restrict_unicode_list, \ - comment_condition_always, comment_condition_changes, comment_condition_failures, comment_condition_errors, \ + comment_mode_always, comment_mode_changes, comment_mode_failures, comment_mode_errors, \ get_stats_from_digest, digest_header, get_short_summary, get_long_summary_md, \ get_long_summary_with_digest_md, get_error_annotations, get_case_annotations, \ get_all_tests_list_annotation, get_skipped_tests_list_annotation, get_all_tests_list, \ @@ -47,7 +47,6 @@ class Settings: check_name: str comment_title: str comment_mode: str - comment_conditions: Set[str] job_summary: bool compare_earlier: bool pull_request_build: str @@ -454,17 +453,16 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) - # we need to fetch the latest comment if comment_condition_always not in comment_conditions - # or self._settings.comment_mode == comment_mode_update + # we need to fetch the latest comment unless comment mode is off, always or deprecated create latest_comment = None - if comment_condition_always not in self._settings.comment_conditions or self._settings.comment_mode == comment_mode_update: + if self._settings.comment_mode not in [comment_mode_off, comment_mode_create, comment_mode_always]: latest_comment = self.get_latest_comment(pull_request) latest_comment_body = latest_comment.body if latest_comment else None # are we required to create a comment on this PR? earlier_stats = self.get_stats_from_summary_md(latest_comment_body) if latest_comment_body else None if not self.require_comment(stats_with_delta, earlier_stats): - logger.info(f'No comment required as comment_on condition "{", ".join(self._settings.comment_conditions)}" is not met') + logger.info(f'No pull request comment required as comment mode is {self._settings.comment_mode} (comment_mode)') return details_url = check_run.html_url if check_run else None @@ -485,44 +483,44 @@ def require_comment(self, # SomeTestChanges.has_changes cannot be used here as changes between earlier comment # and current results cannot be identified - if comment_condition_always in self._settings.comment_conditions: - logger.debug(f'Comment required as condition contains {comment_condition_always}') + if self._settings.comment_mode == comment_mode_always: + logger.debug(f'Comment required as comment mode is {self._settings.comment_mode}') return True - if comment_condition_changes in self._settings.comment_conditions: + if self._settings.comment_mode == comment_mode_changes: if earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats): - logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' - f'and stats are different to earlier comment') + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'and statistics are different to earlier comment') logger.debug(f'earlier: {earlier_stats}') logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') return True if not stats.is_delta: - logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' f'but no delta statistics to target branch available') return True if stats.has_changes: - logger.info(f'Comment required as condition contains "{comment_condition_changes}" ' + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' f'and changes to target branch exist') logger.debug(f'current: {stats}') return True - if comment_condition_failures in self._settings.comment_conditions: + if self._settings.comment_mode == comment_mode_failures: if earlier_stats is not None and earlier_stats.has_failures: - logger.info(f'Comment required as condition contains {comment_condition_failures} ' - f'and failures exist in earlier comment') + logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' + f'and failures existed in earlier comment') return True if stats.has_failures: - logger.info(f'Comment required as condition contains {comment_condition_failures} ' + logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' f'and failures exist in current comment') return True - if comment_condition_errors in self._settings.comment_conditions: + if self._settings.comment_mode in [comment_mode_failures, comment_mode_errors]: if earlier_stats is not None and earlier_stats.has_errors: - logger.info(f'Comment required as condition contains {comment_condition_errors} ' - f'and errors exist in earlier comment') + logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' + f'and errors existed in earlier comment') return True if stats.has_errors: - logger.info(f'Comment required as condition contains {comment_condition_errors} ' + logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' f'and errors exist in current comment') return True diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index 2e628d75..ed8a223b 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -6,7 +6,7 @@ from collections import defaultdict from datetime import datetime from glob import glob -from typing import List, Optional, Union, Tuple, Any +from typing import List, Optional, Union, Mapping, Tuple, Any import github import humanize @@ -16,8 +16,7 @@ import publish.github_action from publish import hide_comments_modes, available_annotations, default_annotations, \ pull_request_build_modes, fail_on_modes, fail_on_mode_errors, fail_on_mode_failures, \ - comment_mode_off, comment_mode_update, comment_modes, comment_condition_always, comment_conditions, \ - punctuation_space + comment_mode_off, comment_mode_always, comment_modes, comment_modes_deprecated, punctuation_space from publish.github_action import GithubAction from publish.junit import parse_junit_xml_files from publish.progress import progress_logger @@ -260,6 +259,24 @@ def deprecate_var(val: Optional[str], deprecated_var: str, replacement_var: str, gha.warning(message) +def available_values(values: List[str]) -> str: + values = [f'"{val}"' for val in values] + return f"{', '.join(values[:-1])} or {values[-1]}" + + +def deprecate_val(val: Optional[str], var: str, replacement_vals: Mapping[str, str], gha: Optional[GithubAction]): + if val in replacement_vals: + message = f'Value "{val}" for option {var.lower()} is deprecated!' + replacement = replacement_vals[val] + if replacement: + message = f'{message} Instead, use value "{replacement}".' + + if gha is None: + logger.debug(message) + else: + gha.warning(message) + + def is_float(text: str) -> bool: return re.match('^[+-]?(([0-9]*\\.[0-9]+)|([0-9]+(\\.[0-9]?)?))$', text) is not None @@ -288,9 +305,6 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: comment_on_pr = get_bool_var('COMMENT_ON_PR', options, default=True, gha=gha) annotations = get_annotations_config(options, event) - comment_on = get_var('COMMENT_ON', options) or comment_condition_always - comment_on = {comment_condition.strip() for comment_condition in comment_on.split(',')} - fail_on = get_var('FAIL_ON', options) or 'test failures' check_var(fail_on, 'FAIL_ON', 'Check fail mode', fail_on_modes) # here we decide that we want to fail on errors when we fail on test failures, like log level escalation @@ -322,8 +336,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: time_factor=time_factor, check_name=check_name, comment_title=get_var('COMMENT_TITLE', options) or check_name, - comment_mode=get_var('COMMENT_MODE', options) or (comment_mode_update if comment_on_pr else comment_mode_off), - comment_conditions=comment_on, + comment_mode=get_var('COMMENT_MODE', options) or (comment_mode_always if comment_on_pr else comment_mode_off), job_summary=get_bool_var('JOB_SUMMARY', options, default=True, gha=gha), compare_earlier=get_bool_var('COMPARE_TO_EARLIER_COMMIT', options, default=True, gha=gha), pull_request_build=get_var('PULL_REQUEST_BUILD', options) or 'merge', @@ -340,8 +353,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.token, 'GITHUB_TOKEN', 'GitHub token') check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') - check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes) - check_var(list(settings.comment_conditions), 'COMMENT_ON', 'Comment conditions', list(comment_conditions)) + check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes + list(comment_modes_deprecated.keys())) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) @@ -351,7 +363,9 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var_condition(settings.seconds_between_github_reads > 0, f'SECONDS_BETWEEN_GITHUB_READS must be a positive number: {seconds_between_github_reads}') check_var_condition(settings.seconds_between_github_writes > 0, f'SECONDS_BETWEEN_GITHUB_WRITES must be a positive number: {seconds_between_github_writes}') - deprecate_var(get_var('COMMENT_ON_PR', options) or None, 'COMMENT_ON_PR', 'Instead, use option "comment_mode" with values "off", "create new", or "update last".', gha) + deprecate_var(get_var('COMMENT_ON_PR', options) or None, 'COMMENT_ON_PR', + f'Instead, use option "comment_mode" with values {available_values(comment_modes)}.', gha) + deprecate_val(settings.comment_mode, 'COMMENT_MODE', comment_modes_deprecated, gha) return settings diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index 704d72a0..0aa36c04 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -5,13 +5,11 @@ import tempfile import unittest from typing import Optional, Union, List -from itertools import combinations import mock from publish import pull_request_build_mode_merge, fail_on_mode_failures, fail_on_mode_errors, \ - fail_on_mode_nothing, comment_mode_off, comment_mode_create, comment_mode_update, \ - comment_condition_always, comment_condition_changes, comment_condition_failures, comment_condition_errors, \ - comment_conditions, hide_comments_modes, pull_request_build_modes, punctuation_space + fail_on_mode_nothing, comment_modes, comment_mode_off, comment_mode_always, comment_mode_update, \ + hide_comments_modes, pull_request_build_modes, punctuation_space from publish.github_action import GithubAction from publish.unittestresults import ParsedUnitTestResults, ParseError from publish_unit_test_results import get_conclusion, get_commit_sha, get_var, \ @@ -146,8 +144,7 @@ def get_settings(token='token', time_factor=1.0, check_name='check name', comment_title='title', - comment_mode=comment_mode_create, - comment_conditions={comment_condition_always}, + comment_mode=comment_mode_always, job_summary=True, compare_earlier=True, test_changes_limit=10, @@ -180,7 +177,6 @@ def get_settings(token='token', check_name=check_name, comment_title=comment_title, comment_mode=comment_mode, - comment_conditions=comment_conditions, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -308,45 +304,30 @@ def test_get_settings_comment_title(self): self.do_test_get_settings(COMMENT_TITLE=None, CHECK_NAME='name', expected=self.get_settings(comment_title='name', check_name='name')) def test_get_settings_comment_on_pr(self): - default_comment_mode = comment_mode_update + default_comment_mode = comment_mode_always bool_warning = 'Option comment_on_pr has to be boolean, so either "true" or "false": foo' - depr_warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "create new", or "update last".' + depr_warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "failures" or "errors".' self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='false', expected=self.get_settings(comment_mode=comment_mode_off), warning=depr_warning) self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='False', expected=self.get_settings(comment_mode=comment_mode_off), warning=depr_warning) self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='true', expected=self.get_settings(comment_mode=default_comment_mode), warning=depr_warning) self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='True', expected=self.get_settings(comment_mode=default_comment_mode), warning=depr_warning) - self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='foo', expected=self.get_settings(comment_mode=comment_mode_update), warning=[bool_warning, depr_warning]) - self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_update)) + self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='foo', expected=self.get_settings(comment_mode=comment_mode_always), warning=[bool_warning, depr_warning]) + self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_always)) def test_get_settings_comment_mode(self): - warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "create new", or "update last".' - for mode in [comment_mode_off, comment_mode_create, comment_mode_update]: + warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "failures" or "errors".' + for mode in comment_modes: with self.subTest(mode=mode): self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=mode)) self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='true' if mode == comment_mode_off else 'false', expected=self.get_settings(comment_mode=mode), warning=warning) - self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_update)) + self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_always)) with self.assertRaises(RuntimeError) as re: self.do_test_get_settings(COMMENT_MODE='mode') self.assertEqual("Value 'mode' is not supported for variable COMMENT_MODE, expected: off, create new, update last", str(re.exception)) - def test_get_settings_comment_condition(self): - with self.subTest(condition=None): - self.do_test_get_settings(COMMENT_ON=None, expected=self.get_settings(comment_conditions={comment_condition_always})) - - for cond in [cond - for card in range(1, len(comment_conditions)+1) - for cond in combinations(comment_conditions, card)]: - with self.subTest(condition=', '.join(cond)): - self.do_test_get_settings(COMMENT_ON=', '.join(cond), expected=self.get_settings(comment_conditions=set(cond))) - - with self.subTest(condition='unknown'): - with self.assertRaises(RuntimeError) as re: - self.do_test_get_settings(COMMENT_ON='condition') - self.assertEqual(f"Some values in 'condition' are not supported for variable COMMENT_ON, allowed: always, changes, failures, errors", str(re.exception)) - def test_get_settings_compare_to_earlier_commit(self): warning = 'Option compare_to_earlier_commit has to be boolean, so either "true" or "false": foo' self.do_test_get_settings(COMPARE_TO_EARLIER_COMMIT='false', expected=self.get_settings(compare_earlier=False)) @@ -480,7 +461,7 @@ def do_test_get_settings(self, COMMIT='commit', # defaults to get_commit_sha(event, event_name) FILES='files', COMMENT_TITLE='title', # defaults to check name - COMMENT_MODE='create new', + COMMENT_MODE='always', JOB_SUMMARY='true', HIDE_COMMENTS='off', # defaults to 'all but latest' REPORT_INDIVIDUAL_RUNS='true', # false unless 'true' diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index c141be69..f19ea80b 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -11,8 +11,8 @@ import mock from github import Github, GithubException -from publish import comment_mode_create, comment_mode_update, comment_mode_off, comment_condition_always, \ - comment_condition_changes, comment_condition_failures, comment_condition_errors, hide_comments_mode_off, \ +from publish import comment_mode_create, comment_mode_update, comment_mode_off, comment_mode_always, \ + comment_mode_changes, comment_mode_failures, comment_mode_errors, hide_comments_mode_off, \ hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, Annotation, default_annotations, \ get_error_annotation, digest_header, get_digest_from_stats, \ all_tests_list, skipped_tests_list, none_list, \ @@ -68,8 +68,7 @@ def create_github_pr(repo: str, return pr @staticmethod - def create_settings(comment_mode=comment_mode_create, - comment_conditions={comment_condition_always}, + def create_settings(comment_mode=comment_mode_always, job_summary=True, compare_earlier=True, hide_comment_mode=hide_comments_mode_off, @@ -101,7 +100,6 @@ def create_settings(comment_mode=comment_mode_create, check_name='Check Name', comment_title='Comment Title', comment_mode=comment_mode, - comment_conditions=comment_conditions, job_summary=job_summary, compare_earlier=compare_earlier, pull_request_build=pull_request_build, @@ -309,11 +307,11 @@ def test_get_test_list_annotations_chunked_and_restricted_unicode(self): Annotation(path='.github', start_line=0, end_line=0, start_column=None, end_column=None, annotation_level='notice', message='There are 3 tests, see "Raw output" for the list of tests 3 to 3.', title='3 tests found (test 3 to 3)', raw_details='class ‑ test \\U0001d484') ], annotations) - def do_test_require_comment(self, comment_conditions, test_expectation: Callable[["CommentConditionTest"], bool]): + def do_test_require_comment(self, comment_mode, test_expectation: Callable[["CommentConditionTest"], bool]): tests = [(test, test_expectation(test)) for test in self.comment_condition_tests] publisher = mock.MagicMock(Publisher) - publisher._settings = self.create_settings(comment_conditions=comment_conditions if isinstance(comment_conditions, set) else {comment_conditions}) + publisher._settings = self.create_settings(comment_mode=comment_mode) for test, expected in tests: with self.subTest(test): @@ -336,36 +334,30 @@ def do_test_require_comment(self, comment_conditions, test_expectation: Callable def test_require_comment_always(self): self.do_test_require_comment( - comment_condition_always, + comment_mode_always, lambda _: True ) def test_require_comment_changes(self): self.do_test_require_comment( - comment_condition_changes, + comment_mode_changes, lambda test: not test.earlier_is_none and test.earlier_is_different or test.current_has_changes is None or test.current_has_changes ) def test_require_comment_failures(self): self.do_test_require_comment( - comment_condition_failures, - lambda test: not test.earlier_is_none and test.earlier_has_failures or test.current_has_failures + comment_mode_failures, + lambda test: not test.earlier_is_none and (test.earlier_has_failures or test.earlier_has_errors) or + (test.current_has_failures or test.current_has_errors) ) def test_require_comment_errors(self): self.do_test_require_comment( - comment_condition_errors, + comment_mode_errors, lambda test: not test.earlier_is_none and test.earlier_has_errors or test.current_has_errors ) - def test_require_comment_changes_failures_and_errors(self): - self.do_test_require_comment( - {comment_condition_changes, comment_condition_failures, comment_condition_errors}, - lambda test: not test.earlier_is_none and (test.earlier_is_different or test.earlier_has_failures or test.earlier_has_errors) or - test.current_has_changes is None or test.current_has_changes or test.current_has_failures or test.current_has_errors - ) - def test_publish_without_comment(self): settings = self.create_settings(comment_mode=comment_mode_off, hide_comment_mode=hide_comments_mode_off) mock_calls = self.call_mocked_publish(settings, prs=[object()]) From d025fe4c9bd79f79a6a9cfd64a4e9bf2b3693e6d Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Tue, 14 Jun 2022 11:27:44 +0200 Subject: [PATCH 23/26] Add comment mode failure and error changes --- python/publish/__init__.py | 4 ++ python/publish/publisher.py | 41 ++++++++++++++-- python/publish/unittestresults.py | 62 +++++++++++++++++-------- python/publish_unit_test_results.py | 9 ++-- python/test/test_action_script.py | 6 +-- python/test/test_publisher.py | 72 +++++++++++++++++++++++++++-- python/test/test_unittestresults.py | 53 ++++++++++++++++++--- 7 files changed, 208 insertions(+), 39 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index b7fb3ae6..767cd9a4 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -23,12 +23,16 @@ comment_mode_update = 'update last' # deprecated comment_mode_always = 'always' comment_mode_changes = 'changes' +comment_mode_changes_failures = 'changes in failures' # includes comment_mode_changes_errors +comment_mode_changes_errors = 'changes in errors' comment_mode_failures = 'failures' # includes comment_mode_errors comment_mode_errors = 'errors' comment_modes = [ comment_mode_off, comment_mode_always, comment_mode_changes, + comment_mode_changes_failures, + comment_mode_changes_errors, comment_mode_failures, comment_mode_errors ] diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 1869fc4e..872e80b6 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -15,7 +15,8 @@ from publish import hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, hide_comments_mode_off, \ comment_mode_off, comment_mode_create, comment_mode_update, digest_prefix, restrict_unicode_list, \ - comment_mode_always, comment_mode_changes, comment_mode_failures, comment_mode_errors, \ + comment_mode_always, comment_mode_changes, comment_mode_changes_failures, comment_mode_changes_errors, \ + comment_mode_failures, comment_mode_errors, \ get_stats_from_digest, digest_header, get_short_summary, get_long_summary_md, \ get_long_summary_with_digest_md, get_error_annotations, get_case_annotations, \ get_all_tests_list_annotation, get_skipped_tests_list_annotation, get_all_tests_list, \ @@ -483,12 +484,12 @@ def require_comment(self, # SomeTestChanges.has_changes cannot be used here as changes between earlier comment # and current results cannot be identified - if self._settings.comment_mode == comment_mode_always: + if self._settings.comment_mode in [comment_mode_always, comment_mode_create, comment_mode_update]: logger.debug(f'Comment required as comment mode is {self._settings.comment_mode}') return True if self._settings.comment_mode == comment_mode_changes: - if earlier_stats is not None and earlier_stats != (stats.without_delta() if stats.is_delta else stats): + if earlier_stats is not None and earlier_stats.is_different(stats): logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' f'and statistics are different to earlier comment') logger.debug(f'earlier: {earlier_stats}') @@ -504,6 +505,40 @@ def require_comment(self, logger.debug(f'current: {stats}') return True + if self._settings.comment_mode == comment_mode_changes_failures: + if earlier_stats is not None and earlier_stats.is_different_in_failures(stats): + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'and failure statistics are different to earlier comment') + logger.debug(f'earlier: {earlier_stats}') + logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') + return True + if not stats.is_delta: + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'but no delta statistics to target branch available') + return True + if stats.has_failure_changes: + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'and changes in failures to target branch exist') + logger.debug(f'current: {stats}') + return True + + if self._settings.comment_mode in [comment_mode_changes_failures, comment_mode_changes_errors]: + if earlier_stats is not None and earlier_stats.is_different_in_errors(stats): + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'and error statistics are different to earlier comment') + logger.debug(f'earlier: {earlier_stats}') + logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') + return True + if not stats.is_delta: + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'but no delta statistics to target branch available') + return True + if stats.has_error_changes: + logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' + f'and changes in errors to target branch exist') + logger.debug(f'current: {stats}') + return True + if self._settings.comment_mode == comment_mode_failures: if earlier_stats is not None and earlier_stats.has_failures: logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' diff --git a/python/publish/unittestresults.py b/python/publish/unittestresults.py index 23cd8ce5..335e6b22 100644 --- a/python/publish/unittestresults.py +++ b/python/publish/unittestresults.py @@ -1,7 +1,7 @@ from collections import defaultdict import dataclasses from dataclasses import dataclass -from typing import Optional, List, Mapping, Any, Union, Dict +from typing import Optional, List, Mapping, Any, Union, Dict, Callable from xml.etree.ElementTree import ParseError as XmlParseError @@ -180,6 +180,34 @@ def has_failures(self): def has_errors(self): return len(self.errors) > 0 or self.tests_error > 0 or self.runs_error > 0 + @staticmethod + def _change_fields(results: 'UnitTestRunResults') -> List[int]: + return [results.files, results.suites, + results.tests, results.tests_succ, results.tests_skip, results.tests_fail, results.tests_error, + results.runs, results.runs_succ, results.runs_skip, results.runs_fail, results.runs_error] + + @staticmethod + def _failure_fields(results: 'UnitTestRunResults') -> List[int]: + return [results.tests_fail, results.runs_fail] + + @staticmethod + def _error_fields(results: 'UnitTestRunResults') -> List[int]: + return [results.tests_error, results.runs_error] + + def is_different(self, + other: 'UnitTestRunResultsOrDeltaResults', + fields_func: Callable[['UnitTestRunResults'], List[int]] = _change_fields.__func__): + if other.is_delta: + other = other.without_delta() + + return any([left != right for left, right in zip(fields_func(self), fields_func(other))]) + + def is_different_in_failures(self, other: 'UnitTestRunResultsOrDeltaResults'): + return self.is_different(other, self._failure_fields) + + def is_different_in_errors(self, other: 'UnitTestRunResultsOrDeltaResults'): + return self.is_different(other, self._error_fields) + def with_errors(self, errors: List[ParseError]): return UnitTestRunResults( files=self.files, @@ -229,19 +257,6 @@ def from_dict(values: Mapping[str, Any]) -> 'UnitTestRunResults': ) -def patch_ne(orig_ne): - def patched_ne(self: UnitTestRunResults, other) -> bool: - if isinstance(other, UnitTestRunResults): - other = dataclasses.replace(other, errors=self.errors, duration=self.duration, commit=self.commit) - return orig_ne(self, other) - - return patched_ne - - -# patch UnitTestRunResults.__ne__ to not include duration -UnitTestRunResults.__ne__ = patch_ne(UnitTestRunResults.__ne__) - - Numeric = Mapping[str, int] @@ -273,12 +288,23 @@ class UnitTestRunDeltaResults: def is_delta(self) -> bool: return True + @staticmethod + def _has_changes(fields: List[Numeric]) -> bool: + return any([field.get('delta') for field in fields]) + @property def has_changes(self) -> bool: - return (any([field.get('delta') - for field in [self.files, self.suites, - self.tests, self.tests_succ, self.tests_skip, self.tests_fail, self.tests_error, - self.runs, self.runs_succ, self.runs_skip, self.runs_fail, self.runs_error]])) + return self._has_changes([self.files, self.suites, + self.tests, self.tests_succ, self.tests_skip, self.tests_fail, self.tests_error, + self.runs, self.runs_succ, self.runs_skip, self.runs_fail, self.runs_error]) + + @property + def has_failure_changes(self) -> bool: + return self._has_changes([self.tests_fail, self.runs_fail]) + + @property + def has_error_changes(self) -> bool: + return self._has_changes([self.tests_error, self.runs_error]) @property def has_failures(self): diff --git a/python/publish_unit_test_results.py b/python/publish_unit_test_results.py index ed8a223b..ee5949dc 100644 --- a/python/publish_unit_test_results.py +++ b/python/publish_unit_test_results.py @@ -228,17 +228,18 @@ def get_bool_var(name: str, options: dict, default: bool, gha: Optional[GithubAc def check_var(var: Union[Optional[str], List[str]], name: str, label: str, - allowed_values: Optional[List[str]] = None) -> None: + allowed_values: Optional[List[str]] = None, + deprecated_values: Optional[List[str]] = None) -> None: if var is None: raise RuntimeError(f'{label} must be provided via action input or environment variable {name}') if allowed_values: if isinstance(var, str): - if var not in allowed_values: + if var not in allowed_values + (deprecated_values or []): raise RuntimeError(f"Value '{var}' is not supported for variable {name}, " f"expected: {', '.join(allowed_values)}") if isinstance(var, list): - if any([v not in allowed_values for v in var]): + if any([v not in allowed_values + (deprecated_values or []) for v in var]): raise RuntimeError(f"Some values in '{', '.join(var)}' " f"are not supported for variable {name}, " f"allowed: {', '.join(allowed_values)}") @@ -353,7 +354,7 @@ def get_settings(options: dict, gha: Optional[GithubAction] = None) -> Settings: check_var(settings.token, 'GITHUB_TOKEN', 'GitHub token') check_var(settings.repo, 'GITHUB_REPOSITORY', 'GitHub repository') check_var(settings.commit, 'COMMIT, GITHUB_SHA or event file', 'Commit SHA') - check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes + list(comment_modes_deprecated.keys())) + check_var(settings.comment_mode, 'COMMENT_MODE', 'Comment mode', comment_modes, list(comment_modes_deprecated.keys())) check_var(settings.pull_request_build, 'PULL_REQUEST_BUILD', 'Pull Request build', pull_request_build_modes) check_var(settings.hide_comment_mode, 'HIDE_COMMENTS', 'Hide comments mode', hide_comments_modes) check_var(settings.check_run_annotation, 'CHECK_RUN_ANNOTATIONS', 'Check run annotations', available_annotations) diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index 0aa36c04..bf09dc3d 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -306,7 +306,7 @@ def test_get_settings_comment_title(self): def test_get_settings_comment_on_pr(self): default_comment_mode = comment_mode_always bool_warning = 'Option comment_on_pr has to be boolean, so either "true" or "false": foo' - depr_warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "failures" or "errors".' + depr_warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "changes in failures", "changes in errors", "failures" or "errors".' self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='false', expected=self.get_settings(comment_mode=comment_mode_off), warning=depr_warning) self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR='False', expected=self.get_settings(comment_mode=comment_mode_off), warning=depr_warning) @@ -316,7 +316,7 @@ def test_get_settings_comment_on_pr(self): self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_always)) def test_get_settings_comment_mode(self): - warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "failures" or "errors".' + warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "changes in failures", "changes in errors", "failures" or "errors".' for mode in comment_modes: with self.subTest(mode=mode): self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=mode)) @@ -326,7 +326,7 @@ def test_get_settings_comment_mode(self): with self.assertRaises(RuntimeError) as re: self.do_test_get_settings(COMMENT_MODE='mode') - self.assertEqual("Value 'mode' is not supported for variable COMMENT_MODE, expected: off, create new, update last", str(re.exception)) + self.assertEqual("Value 'mode' is not supported for variable COMMENT_MODE, expected: off, always, changes, changes in failures, changes in errors, failures, errors", str(re.exception)) def test_get_settings_compare_to_earlier_commit(self): warning = 'Option compare_to_earlier_commit has to be boolean, so either "true" or "false": foo' diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index f19ea80b..6e693119 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -1,6 +1,8 @@ import dataclasses import json import os +import pathlib +import sys import tempfile import unittest from collections.abc import Collection @@ -12,7 +14,8 @@ from github import Github, GithubException from publish import comment_mode_create, comment_mode_update, comment_mode_off, comment_mode_always, \ - comment_mode_changes, comment_mode_failures, comment_mode_errors, hide_comments_mode_off, \ + comment_mode_changes, comment_mode_changes_failures, comment_mode_changes_errors, \ + comment_mode_failures, comment_mode_errors, hide_comments_mode_off, \ hide_comments_mode_orphaned, hide_comments_mode_all_but_latest, Annotation, default_annotations, \ get_error_annotation, digest_header, get_digest_from_stats, \ all_tests_list, skipped_tests_list, none_list, \ @@ -23,6 +26,9 @@ from publish.publisher import Publisher, Settings, PublishData from publish.unittestresults import UnitTestCase, ParseError, UnitTestRunResults, UnitTestRunDeltaResults, \ UnitTestCaseResults + +sys.path.append(str(pathlib.Path(__file__).resolve().parent)) + from test_unittestresults import create_unit_test_run_results @@ -33,10 +39,14 @@ class CommentConditionTest: earlier_is_none: bool earlier_is_different: bool + earlier_is_different_in_failures: bool + earlier_is_different_in_errors: bool earlier_has_failures: bool earlier_has_errors: bool # current_has_changes being None indicates it is not a UnitTestRunDeltaResults but UnitTestRunResults current_has_changes: Optional[bool] + current_has_failure_changes: bool + current_has_error_changes: bool current_has_failures: bool current_has_errors: bool @@ -315,23 +325,61 @@ def do_test_require_comment(self, comment_mode, test_expectation: Callable[["Com for test, expected in tests: with self.subTest(test): - earlier = mock.MagicMock(__ne__=mock.Mock(return_value=test.earlier_is_different), has_failures=test.earlier_has_failures, has_errors=test.earlier_has_errors) if not test.earlier_is_none else None - current = mock.MagicMock(is_delta=test.current_has_changes is not None, has_changes=test.current_has_changes, has_failures=test.current_has_failures, has_errors=test.current_has_errors) + earlier = mock.MagicMock( + is_different=mock.Mock(return_value=test.earlier_is_different), + is_different_in_failures=mock.Mock(return_value=test.earlier_is_different_in_failures), + is_different_in_errors=mock.Mock(return_value=test.earlier_is_different_in_errors), + has_failures=test.earlier_has_failures, + has_errors=test.earlier_has_errors + ) if not test.earlier_is_none else None + current = mock.MagicMock( + is_delta=test.current_has_changes is not None, + has_changes=test.current_has_changes, + has_failure_changes=test.current_has_failure_changes, + has_error_changes=test.current_has_error_changes, + has_failures=test.current_has_failures, + has_errors=test.current_has_errors) if current.is_delta: current.without_delta = mock.Mock(return_value=current) required = Publisher.require_comment(publisher, current, earlier) self.assertEqual(required, expected) - comment_condition_tests = [CommentConditionTest(earlier_is_none, earlier_is_different, earlier_has_failures, earlier_has_errors, - current_has_changes, current_has_failures, current_has_errors) + comment_condition_tests = [CommentConditionTest(earlier_is_none, + earlier_is_different, earlier_is_different_in_failures, earlier_is_different_in_errors, + earlier_has_failures, earlier_has_errors, + current_has_changes, current_has_failure_changes, current_has_error_changes, + current_has_failures, current_has_errors) for earlier_is_none in [False, True] for earlier_is_different in [False, True] + for earlier_is_different_in_failures in ([False, True] if not earlier_is_different else [True]) + for earlier_is_different_in_errors in ([False, True] if not earlier_is_different else [True]) for earlier_has_failures in [False, True] for earlier_has_errors in [False, True] + for current_has_changes in [None, False, True] + for current_has_failure_changes in ([False, True] if not current_has_changes else [True]) + for current_has_error_changes in ([False, True] if not current_has_changes else [True]) for current_has_failures in [False, True] for current_has_errors in [False, True]] + def test_require_comment_off(self): + self.do_test_require_comment( + comment_mode_off, + lambda _: False + ) + + def test_require_comment_create(self): + self.do_test_require_comment( + comment_mode_create, + lambda _: True + ) + + def test_require_comment_update(self): + self.do_test_require_comment( + comment_mode_update, + lambda _: True + ) + def test_require_comment_always(self): self.do_test_require_comment( comment_mode_always, @@ -345,6 +393,20 @@ def test_require_comment_changes(self): test.current_has_changes is None or test.current_has_changes ) + def test_require_comment_changes_failures(self): + self.do_test_require_comment( + comment_mode_changes_failures, + lambda test: not test.earlier_is_none and (test.earlier_is_different_in_failures or test.earlier_is_different_in_errors) or + test.current_has_changes is None or test.current_has_failure_changes or test.current_has_error_changes + ) + + def test_require_comment_changes_errors(self): + self.do_test_require_comment( + comment_mode_changes_errors, + lambda test: not test.earlier_is_none and test.earlier_is_different_in_errors or + test.current_has_changes is None or test.current_has_error_changes + ) + def test_require_comment_failures(self): self.do_test_require_comment( comment_mode_failures, diff --git a/python/test/test_unittestresults.py b/python/test/test_unittestresults.py index 16c0ffd9..8bb58653 100644 --- a/python/test/test_unittestresults.py +++ b/python/test/test_unittestresults.py @@ -476,7 +476,7 @@ def test_get_stats_delta(self): reference_type='type' )) - def test_test_run_results_ne(self): + def test_unit_test_run_results_is_different(self): stats = create_unit_test_run_results() create_other = create_unit_test_run_results for diff, other, expected in [('nothing', create_other(), False), @@ -496,12 +496,53 @@ def test_test_run_results_ne(self): ('runs errors', create_other(runs_error=stats.runs_error+1), True), ('commit', create_other(commit='other'), False)]: with self.subTest(different_in=diff): - self.assertEqual(stats != other, expected, msg=diff) + self.assertEqual(expected, stats.is_different(other), msg=diff) - with self.subTest(different_in='type'): - self.assertEqual(True, stats != object()) + def test_unit_test_run_results_is_different_in_failures(self): + stats = create_unit_test_run_results() + create_other = create_unit_test_run_results + for diff, other, expected in [('nothing', create_other(), False), + ('files', create_other(files=stats.files+1), False), + ('errors', create_other(errors=errors), False), + ('suites', create_other(suites=stats.suites+1), False), + ('duration', create_other(duration=stats.duration+1), False), + ('tests', create_other(tests=stats.tests+1), False), + ('test success', create_other(tests_succ=stats.tests_succ+1), False), + ('test skips', create_other(tests_skip=stats.tests_skip+1), False), + ('test failures', create_other(tests_fail=stats.tests_fail+1), True), + ('test errors', create_other(tests_error=stats.tests_error+1), False), + ('runs', create_other(runs=stats.runs+1), False), + ('runs success', create_other(runs_succ=stats.runs_succ+1), False), + ('runs skips', create_other(runs_skip=stats.runs_skip+1), False), + ('runs failures', create_other(runs_fail=stats.runs_fail+1), True), + ('runs errors', create_other(runs_error=stats.runs_error+1), False), + ('commit', create_other(commit='other'), False)]: + with self.subTest(different_in=diff): + self.assertEqual(expected, stats.is_different_in_failures(other), msg=diff) + + def test_unit_test_run_results_is_different_in_errors(self): + stats = create_unit_test_run_results() + create_other = create_unit_test_run_results + for diff, other, expected in [('nothing', create_other(), False), + ('files', create_other(files=stats.files+1), False), + ('errors', create_other(errors=errors), False), + ('suites', create_other(suites=stats.suites+1), False), + ('duration', create_other(duration=stats.duration+1), False), + ('tests', create_other(tests=stats.tests+1), False), + ('test success', create_other(tests_succ=stats.tests_succ+1), False), + ('test skips', create_other(tests_skip=stats.tests_skip+1), False), + ('test failures', create_other(tests_fail=stats.tests_fail+1), False), + ('test errors', create_other(tests_error=stats.tests_error+1), True), + ('runs', create_other(runs=stats.runs+1), False), + ('runs success', create_other(runs_succ=stats.runs_succ+1), False), + ('runs skips', create_other(runs_skip=stats.runs_skip+1), False), + ('runs failures', create_other(runs_fail=stats.runs_fail+1), False), + ('runs errors', create_other(runs_error=stats.runs_error+1), True), + ('commit', create_other(commit='other'), False)]: + with self.subTest(different_in=diff): + self.assertEqual(expected, stats.is_different_in_errors(other), msg=diff) - def unit_test_run_results_has_failures(self): + def test_unit_test_run_results_has_failures(self): def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) @@ -514,7 +555,7 @@ def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error with self.subTest(msg=label): self.assertEqual(stats.has_failures, expected, msg=label) - def test_test_run_results_has_errors(self): + def test_unit_test_run_results_has_errors(self): def create_stats(errors=[], tests_fail=0, tests_error=0, runs_fail=0, runs_error=0) -> UnitTestRunResults: return create_unit_test_run_results(errors=errors, tests_fail=tests_fail, tests_error=tests_error, runs_fail=runs_fail, runs_error=runs_error) From 44b1ee093c15b371dd7880db789d6e564853290b Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Tue, 14 Jun 2022 23:42:16 +0200 Subject: [PATCH 24/26] Reduce complexity of require_comment --- python/publish/publisher.py | 100 +++++++++++++++++------------------- 1 file changed, 48 insertions(+), 52 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 872e80b6..2a6c412a 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -4,7 +4,7 @@ import os import re from dataclasses import dataclass -from typing import List, Set, Any, Optional, Tuple, Mapping, Dict, Union +from typing import List, Set, Any, Optional, Tuple, Mapping, Dict, Union, Callable from copy import deepcopy from github import Github, GithubException @@ -488,27 +488,18 @@ def require_comment(self, logger.debug(f'Comment required as comment mode is {self._settings.comment_mode}') return True - if self._settings.comment_mode == comment_mode_changes: - if earlier_stats is not None and earlier_stats.is_different(stats): - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and statistics are different to earlier comment') - logger.debug(f'earlier: {earlier_stats}') - logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') - return True - if not stats.is_delta: - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'but no delta statistics to target branch available') - return True - if stats.has_changes: - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and changes to target branch exist') - logger.debug(f'current: {stats}') - return True + # helper method to detect if changes require a comment + def do_changes_require_comment(earlier_stats_is_different_to: Optional[Callable[[UnitTestRunResultsOrDeltaResults], bool]], + stats_has_changes: bool, + flavour: str = '') -> bool: + in_flavour = '' + if flavour: + flavour = f'{flavour} ' + in_flavour = f'in {flavour}' - if self._settings.comment_mode == comment_mode_changes_failures: - if earlier_stats is not None and earlier_stats.is_different_in_failures(stats): + if earlier_stats is not None and earlier_stats_is_different_to(stats): logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and failure statistics are different to earlier comment') + f'and {flavour}statistics are different to earlier comment') logger.debug(f'earlier: {earlier_stats}') logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') return True @@ -516,48 +507,53 @@ def require_comment(self, logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' f'but no delta statistics to target branch available') return True - if stats.has_failure_changes: + if stats_has_changes: logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and changes in failures to target branch exist') + f'and changes {in_flavour} to target branch exist') logger.debug(f'current: {stats}') return True + return False - if self._settings.comment_mode in [comment_mode_changes_failures, comment_mode_changes_errors]: - if earlier_stats is not None and earlier_stats.is_different_in_errors(stats): - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and error statistics are different to earlier comment') - logger.debug(f'earlier: {earlier_stats}') - logger.debug(f'current: {stats.without_delta() if stats.is_delta else stats}') - return True - if not stats.is_delta: - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'but no delta statistics to target branch available') - return True - if stats.has_error_changes: - logger.info(f'Comment required as comment mode is "{self._settings.comment_mode}" ' - f'and changes in errors to target branch exist') - logger.debug(f'current: {stats}') - return True + if self._settings.comment_mode == comment_mode_changes and \ + do_changes_require_comment(earlier_stats.is_different if earlier_stats else None, + stats.has_changes): + return True - if self._settings.comment_mode == comment_mode_failures: - if earlier_stats is not None and earlier_stats.has_failures: - logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' - f'and failures existed in earlier comment') - return True - if stats.has_failures: - logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' - f'and failures exist in current comment') - return True + if self._settings.comment_mode == comment_mode_changes_failures and \ + do_changes_require_comment(earlier_stats.is_different_in_failures if earlier_stats else None, + stats.has_failure_changes, + 'failures'): + return True + + if self._settings.comment_mode in [comment_mode_changes_failures, comment_mode_changes_errors] and \ + do_changes_require_comment(earlier_stats.is_different_in_errors if earlier_stats else None, + stats.has_error_changes, + 'errors'): + return True - if self._settings.comment_mode in [comment_mode_failures, comment_mode_errors]: - if earlier_stats is not None and earlier_stats.has_errors: + # helper method to detect if stats require a comment + def do_stats_require_comment(earlier_stats_require: Optional[bool], stats_require: bool, flavour: str) -> bool: + if earlier_stats is not None and earlier_stats_require: logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' - f'and errors existed in earlier comment') + f'and {flavour} existed in earlier comment') return True - if stats.has_errors: + if stats_require: logger.info(f'Comment required as comment mode is {self._settings.comment_mode} ' - f'and errors exist in current comment') + f'and {flavour} exist in current comment') return True + return False + + if self._settings.comment_mode == comment_mode_failures and \ + do_stats_require_comment(earlier_stats.has_failures if earlier_stats else None, + stats.has_failures, + 'failures'): + return True + + if self._settings.comment_mode in [comment_mode_failures, comment_mode_errors] and \ + do_stats_require_comment(earlier_stats.has_errors if earlier_stats else None, + stats.has_errors, + 'errors'): + return True return False From f3ad965e4ca95c34f7b65c7582421a330d8bf423 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Wed, 15 Jun 2022 00:05:40 +0200 Subject: [PATCH 25/26] Remove comment_on from action.yml and README.md --- .github/workflows/ci-cd.yml | 2 +- README.md | 5 ++--- action.yml | 8 ++------ composite/action.yml | 11 +++-------- python/test/test_action_script.py | 16 +++++++++++++--- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index c9b90fb6..f0ad0aad 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -202,7 +202,7 @@ jobs: id: test-results if: always() run: | - docker run --workdir $GITHUB_WORKSPACE --rm -e INPUT_CHECK_NAME -e INPUT_FILES -e INPUT_TIME_UNIT -e INPUT_GITHUB_TOKEN -e INPUT_GITHUB_RETRIES -e INPUT_COMMIT -e INPUT_COMMENT_TITLE -e INPUT_COMMENT_ON -e INPUT_FAIL_ON -e INPUT_REPORT_INDIVIDUAL_RUNS -e INPUT_DEDUPLICATE_CLASSES_BY_FILE_NAME -e INPUT_IGNORE_RUNS -e INPUT_HIDE_COMMENTS -e INPUT_COMMENT_ON_PR -e INPUT_COMMENT_MODE -e INPUT_COMPARE_TO_EARLIER_COMMIT -e INPUT_PULL_REQUEST_BUILD -e INPUT_EVENT_FILE -e INPUT_EVENT_NAME -e INPUT_TEST_CHANGES_LIMIT -e INPUT_CHECK_RUN_ANNOTATIONS -e INPUT_CHECK_RUN_ANNOTATIONS_BRANCH -e INPUT_SECONDS_BETWEEN_GITHUB_READS -e INPUT_SECONDS_BETWEEN_GITHUB_WRITES -e INPUT_JSON_FILE -e INPUT_JSON_THOUSANDS_SEPARATOR -e INPUT_JOB_SUMMARY -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e GITHUB_STEP_SUMMARY -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "$RUNNER_TEMP":"$RUNNER_TEMP" -v "$GITHUB_WORKSPACE":"$GITHUB_WORKSPACE" enricomi/publish-unit-test-result-action:latest + docker run --workdir $GITHUB_WORKSPACE --rm -e INPUT_CHECK_NAME -e INPUT_FILES -e INPUT_TIME_UNIT -e INPUT_GITHUB_TOKEN -e INPUT_GITHUB_RETRIES -e INPUT_COMMIT -e INPUT_COMMENT_TITLE -e INPUT_FAIL_ON -e INPUT_REPORT_INDIVIDUAL_RUNS -e INPUT_DEDUPLICATE_CLASSES_BY_FILE_NAME -e INPUT_IGNORE_RUNS -e INPUT_HIDE_COMMENTS -e INPUT_COMMENT_ON_PR -e INPUT_COMMENT_MODE -e INPUT_COMPARE_TO_EARLIER_COMMIT -e INPUT_PULL_REQUEST_BUILD -e INPUT_EVENT_FILE -e INPUT_EVENT_NAME -e INPUT_TEST_CHANGES_LIMIT -e INPUT_CHECK_RUN_ANNOTATIONS -e INPUT_CHECK_RUN_ANNOTATIONS_BRANCH -e INPUT_SECONDS_BETWEEN_GITHUB_READS -e INPUT_SECONDS_BETWEEN_GITHUB_WRITES -e INPUT_JSON_FILE -e INPUT_JSON_THOUSANDS_SEPARATOR -e INPUT_JOB_SUMMARY -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e GITHUB_STEP_SUMMARY -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "$RUNNER_TEMP":"$RUNNER_TEMP" -v "$GITHUB_WORKSPACE":"$GITHUB_WORKSPACE" enricomi/publish-unit-test-result-action:latest env: INPUT_GITHUB_TOKEN: ${{ github.token }} INPUT_CHECK_NAME: Test Results (Docker Image) diff --git a/README.md b/README.md index 129d3f71..31c98fad 100644 --- a/README.md +++ b/README.md @@ -175,6 +175,8 @@ The list of most notable options: |`files`|`*.xml`|File patterns to select the test result XML files, e.g. `"test-results/**/*.xml"`. Use multiline string for multiple patterns. Supports `*`, `**`, `?`, `[]`. Excludes files when starting with `!`. | |`check_name`|`"Unit Test Results"`|An alternative name for the check result.| |`comment_title`|same as `check_name`|An alternative title for the pull request comment.| +|`comment_mode`|`always`|The action posts comments to pull requests that are associated with the commit. Set to:
`always` - always comment
`changes` - comment only when changes to the target branch exist
`failures` - when failures or errors exist
`errors` - when (only) errors exist
`off` - to not create pull request comments
With `changes in failures` and `changes in errors`, it only comments when changes in the number of failures and errors exist, respectively.| +|`ignore_runs`|`false`|Does not process test run information by ignoring `` elements in the XML files, which is useful for very large XML files. This disables any check run annotations.|
Options related to Git and GitHub @@ -198,8 +200,6 @@ The list of most notable options: |:-----|:-----:|:----------| |`time_unit`|`seconds`|Time values in the XML files have this unit. Supports `seconds` and `milliseconds`.| |`job_summary`|`true`| Set to `true`, the results are published as part of the [job summary page](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) of the workflow run.| -|`comment_on`|`always`|Create pull request comments only in some cases. This is a comma-separated list of any of the following values:
`changes` - when there are changes in the number of tests, failures or runs (compared to target branch)
`failures` - when test failures exist
`errors` - when test errors exist.
Example: `changes, failures, errors`.| -|`comment_mode`|`update last`|The action posts comments to a pull request that is associated with the commit. Set to `create new` to create a new comment on each commit, `update last` to create only one comment and update later on, `off` to not create pull request comments.| |`hide_comments`|`"all but latest"`|Configures which earlier comments in a pull request are hidden by the action:
`"orphaned commits"` - comments for removed commits
`"all but latest"` - all comments but the latest
`"off"` - no hiding| |`compare_to_earlier_commit`|`true`|Test results are compared to results of earlier commits to show changes:
`false` - disable comparison, `true` - compare across commits.'| |`test_changes_limit`|`10`|Limits the number of removed or skipped tests reported on pull request comments. This report can be disabled with a value of `0`.| @@ -207,7 +207,6 @@ The list of most notable options: |`deduplicate_classes_by_file_name`|`false`|De-duplicates classes with same name by their file name when set `true`, combines test results for those classes otherwise.| |`check_run_annotations`|`all tests, skipped tests`|Adds additional information to the check run. This is a comma-separated list of any of the following values:
`all tests` - list all found tests,
`skipped tests` - list all skipped tests
Set to `none` to add no extra annotations at all.| |`check_run_annotations_branch`|`event.repository.default_branch` or `"main, master"`|Adds check run annotations only on given branches. If not given, this defaults to the default branch of your repository, e.g. `main` or `master`. Comma separated list of branch names allowed, asterisk `"*"` matches all branches. Example: `main, master, branch_one`.| -|`ignore_runs`|`false`|Does not process test run information by ignoring `` elements in the XML files, which is useful for very large XML files. This disables any check run annotations.| |`json_file`|no file|Results are written to this JSON file.| |`json_thousands_separator`|`" "`|Formatted numbers in JSON use this character to separate groups of thousands. Common values are "," or ".". Defaults to punctuation space (\u2008).| |`fail_on`|`"test failures"`|Configures the state of the created test result check run. With `"test failures"` it fails if any test fails or test errors occur. It never fails when set to `"nothing"`, and fails only on errors when set to `"errors"`.| diff --git a/action.yml b/action.yml index 12ec251c..a17e0406 100644 --- a/action.yml +++ b/action.yml @@ -21,8 +21,8 @@ inputs: comment_title: description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false - comment_on: - description: 'Create pull request comments only in some cases. This is a comma-separated list of any of the following values: "changes" - when there are changes in the number of tests, failures or runs (compared to target branch), "failures` - when test failures exist, "errors" - when test errors exist. Example: "changes, failures, errors". Defaults to `always`.' + comment_mode: + description: 'The action posts comments to pull requests that are associated with the commit. Set to: "always" - always comment, "changes" - comment only when changes to the target branch exist, "failures" - when failures or errors exist, "errors" - when (only) errors exist, "off" - to not create pull request comments. With "changes in failures" and "changes in errors", it only comments when changes in the number of failures and errors exist, respectively' default: 'always' required: false fail_on: @@ -53,10 +53,6 @@ inputs: comment_on_pr: description: 'Deprecated, please use comment_mode instead!' required: false - comment_mode: - description: 'Control pull request comments: off - disable pull request comments, create new - create comments on PRs, each time a new one, update last - create comment on pull request, reuse an existing one' - default: 'update last' - required: false job_summary: description: 'Set to `true`, the results are published as part of the job summary page of the workflow run' default: 'true' diff --git a/composite/action.yml b/composite/action.yml index d06e94db..b0d9f10c 100644 --- a/composite/action.yml +++ b/composite/action.yml @@ -21,8 +21,8 @@ inputs: comment_title: description: 'An alternative title for the pull request comment. Defaults to value of check_name input' required: false - comment_on: - description: 'Create pull request comments only in some cases. This is a comma-separated list of any of the following values: "changes" - when there are changes in the number of tests, failures or runs (compared to target branch), "failures` - when test failures exist, "errors" - when test errors exist. Example: "changes, failures, errors". Defaults to `always`.' + comment_mode: + description: 'The action posts comments to pull requests that are associated with the commit. Set to: "always" - always comment, "changes" - comment only when changes to the target branch exist, "failures" - when failures or errors exist, "errors" - when (only) errors exist, "off" - to not create pull request comments. With "changes in failures" and "changes in errors", it only comments when changes in the number of failures and errors exist, respectively' default: 'always' required: false fail_on: @@ -53,10 +53,6 @@ inputs: comment_on_pr: description: 'Deprecated, please use comment_mode instead!' required: false - comment_mode: - description: 'Control pull request comments: off - disable pull request comments, create new - create comments on PRs, each time a new one, update last - create comment on pull request, reuse an existing one' - default: 'update last' - required: false job_summary: description: 'Set to `true`, the results are published as part of the job summary page of the workflow run' default: 'true' @@ -153,7 +149,7 @@ runs: COMMIT: ${{ inputs.commit }} CHECK_NAME: ${{ inputs.check_name }} COMMENT_TITLE: ${{ inputs.comment_title }} - COMMENT_ON: ${{ inputs.comment_on }} + COMMENT_MODE: ${{ inputs.comment_mode }} FAIL_ON: ${{ inputs.fail_on }} FILES: ${{ inputs.files }} TIME_UNIT: ${{ inputs.time_unit }} @@ -162,7 +158,6 @@ runs: IGNORE_RUNS: ${{ inputs.ignore_runs }} HIDE_COMMENTS: ${{ inputs.hide_comments }} COMMENT_ON_PR: ${{ inputs.comment_on_pr }} - COMMENT_MODE: ${{ inputs.comment_mode }} COMPARE_TO_EARLIER_COMMIT: ${{ inputs.compare_to_earlier_commit }} PULL_REQUEST_BUILD: ${{ inputs.pull_request_build }} EVENT_FILE: ${{ inputs.event_file }} diff --git a/python/test/test_action_script.py b/python/test/test_action_script.py index bf09dc3d..538ef5e1 100644 --- a/python/test/test_action_script.py +++ b/python/test/test_action_script.py @@ -8,7 +8,7 @@ import mock from publish import pull_request_build_mode_merge, fail_on_mode_failures, fail_on_mode_errors, \ - fail_on_mode_nothing, comment_modes, comment_mode_off, comment_mode_always, comment_mode_update, \ + fail_on_mode_nothing, comment_modes, comment_modes_deprecated, comment_mode_off, comment_mode_always, \ hide_comments_modes, pull_request_build_modes, punctuation_space from publish.github_action import GithubAction from publish.unittestresults import ParsedUnitTestResults, ParseError @@ -316,11 +316,21 @@ def test_get_settings_comment_on_pr(self): self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_always)) def test_get_settings_comment_mode(self): - warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "changes in failures", "changes in errors", "failures" or "errors".' + comment_on_pr_warning = 'Option comment_on_pr is deprecated! Instead, use option "comment_mode" with values "off", "always", "changes", "changes in failures", "changes in errors", "failures" or "errors".' for mode in comment_modes: with self.subTest(mode=mode): self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=mode)) - self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='true' if mode == comment_mode_off else 'false', expected=self.get_settings(comment_mode=mode), warning=warning) + # comment_on_pr is ignored when comment_mode is given + self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='true', expected=self.get_settings(comment_mode=mode), warning=comment_on_pr_warning) + self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='false', expected=self.get_settings(comment_mode=mode), warning=comment_on_pr_warning) + + for mode in comment_modes_deprecated: + deprecated_warning = f'Value "{mode}" for option comment_mode is deprecated! Instead, use value "always".' + with self.subTest(mode=mode): + self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=mode), warning=deprecated_warning) + # comment_on_pr is ignored when comment_mode is given + self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='true', expected=self.get_settings(comment_mode=mode), warning=[comment_on_pr_warning, deprecated_warning]) + self.do_test_get_settings(COMMENT_MODE=mode, COMMENT_ON_PR='false', expected=self.get_settings(comment_mode=mode), warning=[comment_on_pr_warning, deprecated_warning]) self.do_test_get_settings(COMMENT_MODE=None, COMMENT_ON_PR=None, expected=self.get_settings(comment_mode=comment_mode_always)) From f94f45c8554ee63cd3ed05f425d1e36582196361 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Wed, 15 Jun 2022 21:56:13 +0200 Subject: [PATCH 26/26] Adjust tests for new comment modes --- python/publish/publisher.py | 14 ++-- python/test/test_publisher.py | 151 +++++++++++++++++++--------------- 2 files changed, 93 insertions(+), 72 deletions(-) diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 2a6c412a..e299a52c 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -454,9 +454,9 @@ def publish_comment(self, all_tests, skipped_tests = restrict_unicode_list(all_tests), restrict_unicode_list(skipped_tests) test_changes = SomeTestChanges(before_all_tests, all_tests, before_skipped_tests, skipped_tests) - # we need to fetch the latest comment unless comment mode is off, always or deprecated create + # we need to fetch the latest comment unless comment mode is off (this method would have been called) or deprecated create latest_comment = None - if self._settings.comment_mode not in [comment_mode_off, comment_mode_create, comment_mode_always]: + if self._settings.comment_mode not in [comment_mode_off, comment_mode_create]: latest_comment = self.get_latest_comment(pull_request) latest_comment_body = latest_comment.body if latest_comment else None @@ -470,13 +470,13 @@ def publish_comment(self, summary = get_long_summary_with_digest_md(stats_with_delta, stats, details_url, test_changes, self._settings.test_changes_limit) body = f'## {title}\n{summary}' - # reuse existing comment when comment_mode == comment_mode_update, otherwise create new comment - if self._settings.comment_mode == comment_mode_update and latest_comment is not None: - self.reuse_comment(latest_comment, body) - logger.info(f'Edited comment for pull request #{pull_request.number}: {latest_comment.html_url}') - else: + # only create new comment when comment_mode == comment_mode_create, or no comment exists + if self._settings.comment_mode == comment_mode_create or latest_comment is None: comment = pull_request.create_issue_comment(body) logger.info(f'Created comment for pull request #{pull_request.number}: {comment.html_url}') + else: + self.reuse_comment(latest_comment, body) + logger.info(f'Edited comment for pull request #{pull_request.number}: {latest_comment.html_url}') def require_comment(self, stats: UnitTestRunResultsOrDeltaResults, diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 6e693119..c956c9c8 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -461,7 +461,7 @@ def test_publish_without_comment_with_hiding(self): self.assertEqual({}, kwargs) def test_publish_with_comment_without_pr(self): - settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_comments_mode_off) + settings = self.create_settings(hide_comment_mode=hide_comments_mode_off) mock_calls = self.call_mocked_publish(settings, prs=[]) self.assertEqual(3, len(mock_calls)) @@ -484,7 +484,7 @@ def test_publish_with_comment_without_pr(self): def test_publish_with_comment_without_hiding(self): pr = object() cr = object() - settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_comments_mode_off) + settings = self.create_settings(hide_comment_mode=hide_comments_mode_off) mock_calls = self.call_mocked_publish(settings, prs=[pr], cr=cr) self.assertEqual(4, len(mock_calls)) @@ -512,7 +512,7 @@ def test_publish_with_comment_without_hiding(self): def do_test_publish_with_comment_with_hide(self, hide_mode: str, hide_method: str): pr = object() cr = object() - settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_mode) + settings = self.create_settings(hide_comment_mode=hide_mode) mock_calls = self.call_mocked_publish(settings, prs=[pr], cr=cr) self.assertEqual(5, len(mock_calls)) @@ -557,7 +557,7 @@ def test_publish_with_comment_hide_orphaned(self): def test_publish_without_compare(self): pr = object() cr = object() - settings = self.create_settings(comment_mode=comment_mode_create, hide_comment_mode=hide_comments_mode_all_but_latest, compare_earlier=False) + settings = self.create_settings(hide_comment_mode=hide_comments_mode_all_but_latest, compare_earlier=False) mock_calls = self.call_mocked_publish(settings, prs=[pr], cr=cr) self.assertEqual(5, len(mock_calls)) @@ -594,7 +594,7 @@ def test_publish_comment_compare_earlier(self): bs = UnitTestRunResults(1, [], 1, 1, 3, 1, 2, 0, 0, 3, 1, 2, 0, 0, 'commit') stats = self.stats cases = UnitTestCaseResults(self.cases) - settings = self.create_settings(comment_mode=comment_mode_create, compare_earlier=True) + settings = self.create_settings(compare_earlier=True) publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_check_run = mock.Mock(return_value=bcr) @@ -603,11 +603,12 @@ def test_publish_comment_compare_earlier(self): publisher.get_base_commit_sha = mock.Mock(return_value="base commit") publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) publisher.require_comment = mock.Mock(return_value=True) + publisher.get_latest_comment = mock.Mock(return_value=None) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(5, len(mock_calls)) + self.assertEqual(6, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -630,6 +631,9 @@ def test_publish_comment_compare_earlier(self): self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[4] + self.assertEqual('get_latest_comment', method) + + (method, args, kwargs) = mock_calls[5] self.assertEqual('require_comment', method) mock_calls = pr.mock_calls @@ -656,13 +660,14 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): ((None, 'class', 'test 𝒇'), {'success': [None]}), # added test 𝒇 ]) - settings = self.create_settings(comment_mode=comment_mode_create, compare_earlier=True) + settings = self.create_settings(compare_earlier=True) publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_check_run = mock.Mock(return_value=bcr) publisher.get_stats_from_check_run = mock.Mock(return_value=bs) publisher.get_stats_delta = mock.Mock(return_value=bs) publisher.get_base_commit_sha = mock.Mock(return_value="base commit") + publisher.get_latest_comment = mock.Mock(return_value=None) publisher.require_comment = mock.Mock(return_value=True) # the earlier test cases with restricted unicode as they come from the check runs API publisher.get_test_lists_from_check_run = mock.Mock(return_value=( @@ -678,7 +683,7 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): mock_calls = publisher.mock_calls - self.assertEqual(5, len(mock_calls)) + self.assertEqual(6, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -701,6 +706,9 @@ def test_publish_comment_compare_earlier_with_restricted_unicode(self): self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[4] + self.assertEqual('get_latest_comment', method) + + (method, args, kwargs) = mock_calls[5] self.assertEqual('require_comment', method) mock_calls = pr.mock_calls @@ -760,12 +768,13 @@ def test_publish_comment_compare_with_itself(self): cr = mock.MagicMock() stats = self.stats cases = UnitTestCaseResults(self.cases) - settings = self.create_settings(comment_mode=comment_mode_create, compare_earlier=True) + settings = self.create_settings(compare_earlier=True) publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_check_run = mock.Mock(return_value=None) publisher.get_base_commit_sha = mock.Mock(return_value=stats.commit) publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.get_latest_comment = mock.Mock(return_value=None) with mock.patch('publish.publisher.get_long_summary_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls @@ -785,18 +794,19 @@ def test_publish_comment_compare_with_None(self): cr = mock.MagicMock() stats = self.stats cases = UnitTestCaseResults(self.cases) - settings = self.create_settings(comment_mode=comment_mode_create, compare_earlier=True) + settings = self.create_settings(compare_earlier=True) publisher = mock.MagicMock(Publisher) publisher._settings = settings publisher.get_check_run = mock.Mock(return_value=None) publisher.get_base_commit_sha = mock.Mock(return_value=None) publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.get_latest_comment = mock.Mock(return_value=None) publisher.require_comment = mock.Mock(return_value=True) with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) mock_calls = publisher.mock_calls - self.assertEqual(4, len(mock_calls)) + self.assertEqual(5, len(mock_calls)) (method, args, kwargs) = mock_calls[0] self.assertEqual('get_base_commit_sha', method) @@ -814,6 +824,9 @@ def test_publish_comment_compare_with_None(self): self.assertEqual({}, kwargs) (method, args, kwargs) = mock_calls[3] + self.assertEqual('get_latest_comment', method) + + (method, args, kwargs) = mock_calls[4] self.assertEqual('require_comment', method) mock_calls = pr.mock_calls @@ -825,59 +838,61 @@ def test_publish_comment_compare_with_None(self): self.assertEqual({}, kwargs) def do_test_publish_comment_with_reuse_comment(self, one_exists: bool): - pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) - cr = mock.MagicMock() - lc = mock.MagicMock(body='latest comment') if one_exists else None - stats = self.stats - cases = UnitTestCaseResults(self.cases) - settings = self.create_settings(comment_mode=comment_mode_update, compare_earlier=False) - publisher = mock.MagicMock(Publisher) - publisher._settings = settings - publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) - publisher.get_latest_comment = mock.Mock(return_value=lc) - publisher.reuse_comment = mock.Mock(return_value=one_exists) - publisher.require_comment = mock.Mock(return_value=True) - with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): - Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) - mock_calls = publisher.mock_calls - - self.assertEqual(5 if one_exists else 3, len(mock_calls)) - - (method, args, kwargs) = mock_calls[0] - self.assertEqual('get_test_lists_from_check_run', method) - self.assertEqual((None, ), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[1] - self.assertEqual('get_latest_comment', method) - self.assertEqual((pr, ), args) - self.assertEqual({}, kwargs) - - if one_exists: - (method, args, kwargs) = mock_calls[2] - self.assertEqual('get_stats_from_summary_md', method) - self.assertEqual(('latest comment', ), args) - self.assertEqual({}, kwargs) - - (method, args, kwargs) = mock_calls[3] - self.assertEqual('require_comment', method) - - (method, args, kwargs) = mock_calls[4] - self.assertEqual('reuse_comment', method) - self.assertEqual((lc, '## title\nbody'), args) - self.assertEqual({}, kwargs) - else: - (method, args, kwargs) = mock_calls[2] - self.assertEqual('require_comment', method) - - mock_calls = pr.mock_calls - self.assertEqual(0 if one_exists else 1, len(mock_calls)) - - if not one_exists: - (method, args, kwargs) = mock_calls[0] - self.assertEqual('create_issue_comment', method) - self.assertEqual(('## title\nbody', ), args) - self.assertEqual({}, kwargs) + for mode in [comment_mode_update, comment_mode_always]: + with self.subTest(mode=mode): + pr = mock.MagicMock(number="1234", create_issue_comment=mock.Mock(return_value=mock.MagicMock())) + cr = mock.MagicMock() + lc = mock.MagicMock(body='latest comment') if one_exists else None + stats = self.stats + cases = UnitTestCaseResults(self.cases) + settings = self.create_settings(comment_mode=mode, compare_earlier=False) + publisher = mock.MagicMock(Publisher) + publisher._settings = settings + publisher.get_test_lists_from_check_run = mock.Mock(return_value=(None, None)) + publisher.get_latest_comment = mock.Mock(return_value=lc) + publisher.reuse_comment = mock.Mock(return_value=one_exists) + publisher.require_comment = mock.Mock(return_value=True) + with mock.patch('publish.publisher.get_long_summary_with_digest_md', return_value='body'): + Publisher.publish_comment(publisher, 'title', stats, pr, cr, cases) + mock_calls = publisher.mock_calls + + self.assertEqual(5 if one_exists else 3, len(mock_calls)) + + (method, args, kwargs) = mock_calls[0] + self.assertEqual('get_test_lists_from_check_run', method) + self.assertEqual((None, ), args) + self.assertEqual({}, kwargs) + + (method, args, kwargs) = mock_calls[1] + self.assertEqual('get_latest_comment', method) + self.assertEqual((pr, ), args) + self.assertEqual({}, kwargs) + + if one_exists: + (method, args, kwargs) = mock_calls[2] + self.assertEqual('get_stats_from_summary_md', method) + self.assertEqual(('latest comment', ), args) + self.assertEqual({}, kwargs) + + (method, args, kwargs) = mock_calls[3] + self.assertEqual('require_comment', method) + + (method, args, kwargs) = mock_calls[4] + self.assertEqual('reuse_comment', method) + self.assertEqual((lc, '## title\nbody'), args) + self.assertEqual({}, kwargs) + else: + (method, args, kwargs) = mock_calls[2] + self.assertEqual('require_comment', method) + + mock_calls = pr.mock_calls + self.assertEqual(0 if one_exists else 1, len(mock_calls)) + + if not one_exists: + (method, args, kwargs) = mock_calls[0] + self.assertEqual('create_issue_comment', method) + self.assertEqual(('## title\nbody', ), args) + self.assertEqual({}, kwargs) def test_publish_comment_with_reuse_comment_none_existing(self): self.do_test_publish_comment_with_reuse_comment(one_exists=False) @@ -1883,6 +1898,7 @@ def test_publish_comment(self): gh, gha, req, repo, commit = self.create_mocks(digest=self.base_digest, check_names=[settings.check_name]) pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): @@ -1901,7 +1917,7 @@ def test_publish_comment(self): ) def test_publish_comment_not_required(self): - # same as test_publish_comment but test_publish_comment returns False + # same as test_publish_comment but require_comment returns False with mock.patch('publish.publisher.Publisher.require_comment', return_value=False): settings = self.create_settings(event={'pull_request': {'base': {'sha': 'commit base'}}}, event_name='pull_request') base_commit = 'base-commit' @@ -1909,6 +1925,7 @@ def test_publish_comment_not_required(self): gh, gha, req, repo, commit = self.create_mocks(digest=self.base_digest, check_names=[settings.check_name]) pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) publisher.publish_comment(settings.comment_title, self.stats, pr) @@ -1920,6 +1937,7 @@ def test_publish_comment_without_base(self): gh, gha, req, repo, commit = self.create_mocks(digest=self.base_digest, check_names=[settings.check_name]) pr = self.create_github_pr(settings.repo) publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) compare = mock.MagicMock() compare.merge_base_commit.sha = None @@ -1948,6 +1966,7 @@ def test_publish_comment_without_compare(self): gh, gha, req, repo, commit = self.create_mocks(digest=self.base_digest, check_names=[settings.check_name]) pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): @@ -1973,6 +1992,7 @@ def test_publish_comment_with_check_run_with_annotations(self): pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) cr = mock.MagicMock(html_url='http://check-run.url') publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) # makes gzipped digest deterministic with mock.patch('gzip.time.time', return_value=0): @@ -2000,6 +2020,7 @@ def test_publish_comment_with_check_run_without_annotations(self): pr = self.create_github_pr(settings.repo, base_commit_sha=base_commit) cr = mock.MagicMock(html_url='http://check-run.url') publisher = Publisher(settings, gh, gha) + publisher.get_latest_comment = mock.Mock(return_value=None) stats = dict(self.stats.to_dict()) stats.update(tests_fail=0, tests_error=0, runs_fail=0, runs_error=0)