From eb03c066a4db13b5c58735a3e10ec9b92f8ac93e Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Thu, 9 Nov 2023 16:01:55 +0100 Subject: [PATCH] Handle incomplete / none JSON elements --- python/publish/__init__.py | 20 +++++++++++++++++++- python/publish/publisher.py | 21 ++++++++------------- python/test/test_publish.py | 18 +++++++++++++++++- python/test/test_publisher.py | 29 +++++++++++++++++++++++++++-- 4 files changed, 71 insertions(+), 17 deletions(-) diff --git a/python/publish/__init__.py b/python/publish/__init__.py index 8b1d15e4..7bb04106 100644 --- a/python/publish/__init__.py +++ b/python/publish/__init__.py @@ -5,7 +5,7 @@ import re from collections import defaultdict from dataclasses import dataclass -from typing import List, Any, Union, Optional, Tuple, Mapping, Iterator, Set, Iterable +from typing import List, Any, Union, Optional, Tuple, Mapping, Iterator, Set, Iterable, Dict from publish.unittestresults import Numeric, UnitTestSuite, UnitTestCaseResults, UnitTestRunResults, \ UnitTestRunDeltaResults, UnitTestRunResultsOrDeltaResults, ParseError @@ -151,6 +151,24 @@ def removed_skips(self) -> Optional[Set[str]]: return skipped_before.intersection(removed) +def get_json_path(json: Dict[str, Any], path: Union[str, List[str]]) -> Any: + if isinstance(path, str): + path = path.split('.') + + if path[0] not in json: + return None + + elem = json[path[0]] + + if len(path) > 1: + if isinstance(elem, dict): + return get_json_path(elem, path[1:]) + else: + return None + else: + return elem + + def utf8_character_length(c: int) -> int: if c >= 0x00010000: return 4 diff --git a/python/publish/publisher.py b/python/publish/publisher.py index 5fdfb4e8..16869d79 100644 --- a/python/publish/publisher.py +++ b/python/publish/publisher.py @@ -13,7 +13,7 @@ from github.PullRequest import PullRequest from github.IssueComment import IssueComment -from publish import __version__, comment_mode_off, digest_prefix, restrict_unicode_list, \ +from publish import __version__, get_json_path, comment_mode_off, digest_prefix, restrict_unicode_list, \ 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, \ @@ -206,7 +206,7 @@ def publish(self, check_run = None before_check_run = None if self._settings.compare_earlier: - before_commit_sha = self._settings.event.get('before') + before_commit_sha = get_json_path(self._settings.event, 'before') logger.debug(f'comparing against before={before_commit_sha}') before_check_run = self.get_check_run(before_commit_sha) else: @@ -227,8 +227,8 @@ def publish(self, logger.info('Commenting on pull requests disabled') def get_pull_from_event(self) -> Optional[PullRequest]: - number = self._settings.event.get('pull_request', {}).get('number') - repo = self._settings.event.get('pull_request', {}).get('base', {}).get('repo', {}).get('full_name') + number = get_json_path(self._settings.event, 'pull_request.number') + repo = get_json_path(self._settings.event, 'pull_request.base.repo.full_name') if number is None or repo is None or repo != self._settings.repo: return None @@ -390,7 +390,7 @@ def publish_check(self, before_stats = None before_check_run = None if self._settings.compare_earlier: - before_commit_sha = self._settings.event.get('before') + before_commit_sha = get_json_path(self._settings.event, 'before') logger.debug(f'comparing against before={before_commit_sha}') before_check_run = self.get_check_run(before_commit_sha) before_stats = self.get_stats_from_check_run(before_check_run) if before_check_run is not None else None @@ -686,7 +686,7 @@ def get_base_commit_sha(self, pull_request: PullRequest) -> Optional[str]: if self._settings.event: # for pull request events we take the other parent of the merge commit (base) if self._settings.event_name == 'pull_request': - return self._settings.event.get('pull_request', {}).get('base', {}).get('sha') + return get_json_path(self._settings.event, 'pull_request.base.sha') # for workflow run events we should take the same as for pull request events, # but we have no way to figure out the actual merge commit and its parents # we do not take the base sha from pull_request as it is not immutable @@ -728,18 +728,13 @@ def get_pull_request_comments(self, pull: PullRequest, order_by_updated: bool) - "POST", self._settings.graphql_url, input=query ) - return data \ - .get('data', {}) \ - .get('repository', {}) \ - .get('pullRequest', {}) \ - .get('comments', {}) \ - .get('nodes') + return get_json_path(data, 'data.repository.pullRequest.comments.nodes') def get_action_comments(self, comments: List[Mapping[str, Any]], is_minimized: Optional[bool] = False): comment_body_start = f'## {self._settings.comment_title}\n' comment_body_indicators = ['\nresults for commit ', '\nResults for commit '] return list([comment for comment in comments - if comment.get('author', {}).get('login') == self._settings.actor + if get_json_path(comment, 'author.login') == self._settings.actor and (is_minimized is None or comment.get('isMinimized') == is_minimized) and comment.get('body', '').startswith(comment_body_start) and any(indicator in comment.get('body', '') for indicator in comment_body_indicators)]) diff --git a/python/test/test_publish.py b/python/test/test_publish.py index f49f0b2a..9001f3c1 100644 --- a/python/test/test_publish.py +++ b/python/test/test_publish.py @@ -5,7 +5,7 @@ import mock from publish import __version__, Annotation, UnitTestSuite, UnitTestRunResults, UnitTestRunDeltaResults, CaseMessages, \ - get_error_annotation, get_digest_from_stats, \ + get_json_path, 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, \ @@ -29,6 +29,22 @@ class PublishTest(unittest.TestCase): old_locale = None details = [UnitTestSuite('suite', 7, 3, 2, 1, 'std-out', 'std-err')] + def test_get_json_path(self): + detail = {'a': 'A', 'b': 'B', 'c': ['d'], 'e': {}, 'f': None} + json = {'id': 1, 'name': 'Name', 'detail': detail} + + self.assertEqual(None, get_json_path(json, 'not there')) + self.assertEqual(1, get_json_path(json, 'id')) + self.assertEqual('Name', get_json_path(json, 'name')) + self.assertEqual(detail, get_json_path(json, 'detail')) + self.assertEqual('A', get_json_path(json, 'detail.a')) + self.assertEqual(None, get_json_path(json, 'detail.a.g')) + self.assertEqual(['d'], get_json_path(json, 'detail.c')) + self.assertEqual({}, get_json_path(json, 'detail.e')) + self.assertEqual(None, get_json_path(json, 'detail.e.g')) + self.assertEqual(None, get_json_path(json, 'detail.f')) + self.assertEqual(None, get_json_path(json, 'detail.f.g')) + def test_test_changes(self): changes = SomeTestChanges(['removed-test', 'removed-skip', 'remain-test', 'remain-skip', 'skip', 'unskip'], ['remain-test', 'remain-skip', 'skip', 'unskip', 'add-test', 'add-skip'], diff --git a/python/test/test_publisher.py b/python/test/test_publisher.py index 2306c398..fba4f0fc 100644 --- a/python/test/test_publisher.py +++ b/python/test/test_publisher.py @@ -13,7 +13,7 @@ import mock from github import Github, GithubException -from publish import __version__, comment_mode_off, comment_mode_always, \ +from publish import __version__, get_json_path, comment_mode_off, comment_mode_always, \ comment_mode_changes, comment_mode_changes_failures, comment_mode_changes_errors, \ comment_mode_failures, comment_mode_errors, Annotation, default_annotations, \ get_error_annotation, digest_header, get_digest_from_stats, \ @@ -888,6 +888,22 @@ def test_get_pull_from_event(self): actual = publisher.get_pull_from_event() self.assertIs(actual, pr) repo.get_pull.assert_called_once_with(1234) + repo.get_pull.reset_mock() + + # test with none in pull request + for event in [ + {}, + {'pull_request': None}, + {'pull_request': {'number': 1234, 'base': None}}, + {'pull_request': {'number': 1234, 'base': {'repo': None}}}, + {'pull_request': {'number': 1234, 'base': {'repo': {}}}}, + ]: + settings = self.create_settings(event=event) + publisher = Publisher(settings, gh, gha) + + actual = publisher.get_pull_from_event() + self.assertIsNone(actual) + repo.get_pull.assert_not_called() def do_test_get_pulls(self, settings: Settings, @@ -911,7 +927,7 @@ def do_test_get_pulls(self, else: gh.search_issues.assert_not_called() if event_pull_request is not None and \ - settings.repo == settings.event.get('pull_request', {}).get('base', {}).get('repo', {}).get('full_name'): + settings.repo == get_json_path(settings.event, 'pull_request.base.repo.full_name'): repo.get_pull.assert_called_once_with(event_pull_request.number) commit.get_pulls.assert_not_called() else: @@ -2621,6 +2637,15 @@ def test_get_pull_request_comments_order_updated(self): 'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n', 'isMinimized': False }, + # malformed comments + { + 'id': 'comment nine', + 'author': None, + }, + { + 'id': 'comment ten', + 'author': {}, + }, ] def test_get_action_comments(self):