From e1b3bf48a5b439d834e8d886c11f2402f60824c9 Mon Sep 17 00:00:00 2001 From: Michael Pruitt Date: Wed, 17 Jan 2024 16:22:29 -0600 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Ruth Netser Co-authored-by: Miles Dunn --- cli/objects/failure_rule.py | 5 +- cli/objects/job.py | 8 +- tests/unittests/conftest.py | 241 ++++++++---------- ...ch_objects_failure_rule_matches_failure.py | 2 +- .../objects/fixtures/test_fixtures.py | 15 +- ...ould_not_cause_failure_for_ignored_step.py | 4 +- 6 files changed, 120 insertions(+), 155 deletions(-) diff --git a/cli/objects/failure_rule.py b/cli/objects/failure_rule.py index 2de4d269..41339aae 100644 --- a/cli/objects/failure_rule.py +++ b/cli/objects/failure_rule.py @@ -192,8 +192,5 @@ def matches_failure(self, failure: Failure) -> bool: return ( hasattr(self, "step") and fnmatch.fnmatch(failure.step, self.step) - and ( - (failure.failure_type == self.failure_type) - or self.failure_type == "all" - ) + and failure.failure_type in (self.failure_type, "all") ) diff --git a/cli/objects/job.py b/cli/objects/job.py index 96e4e7ff..1619a30f 100644 --- a/cli/objects/job.py +++ b/cli/objects/job.py @@ -14,7 +14,6 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -import itertools import json import os from typing import Any @@ -324,13 +323,12 @@ def _find_failures(self, logs_dir: str, junit_dir: str) -> Optional[list[Failure unique_steps_with_failures = set() # Combine lists into one list - for failure in itertools.chain(test_failures, pod_failures): + for failure in test_failures + pod_failures: if failure.step not in unique_steps_with_failures: - unique_steps_with_failures.update([failure.step]) + unique_steps_with_failures.add(failure.step) if self.firewatch_config.failure_rules: for rule in self.firewatch_config.failure_rules: - if rule.matches_failure(failure) and rule.ignore: - failure.ignore = True + failure.ignore = rule.matches_failure(failure) and rule.ignore failures_list.append(failure) if len(failures_list) > 0: diff --git a/tests/unittests/conftest.py b/tests/unittests/conftest.py index c4fc8876..64c1c3fe 100644 --- a/tests/unittests/conftest.py +++ b/tests/unittests/conftest.py @@ -26,7 +26,7 @@ from cli.objects.job import Job from cli.report import Report -_logger = simple_logger.logger.get_logger(__name__) +LOGGER = simple_logger.logger.get_logger(__name__) BUILD_ID_ENV_VAR = "BUILD_ID" FIREWATCH_DEFAULT_JIRA_PROJECT_ENV_VAR = "FIREWATCH_DEFAULT_JIRA_PROJECT" @@ -54,6 +54,8 @@ ["junit_install.xml", "junit_install_status.xml", "junit_symptoms.xml"], ] +DEFAULT_MOCK_ISSUE_KEY = "LPTOCPCI-MOCK" + @pytest.fixture def jira(jira_config_path): @@ -85,18 +87,18 @@ def job(firewatch_config, build_id): def patch_jira(monkeypatch): @dataclass class MockIssue: - key: str = "LPTOCPCI-MOCK" + key: str = DEFAULT_MOCK_ISSUE_KEY def create_jira_issue(*args, **kwargs): - _logger.info("Patching Report.create_jira_issue") - _logger.info( + LOGGER.info("Patching Report.create_jira_issue") + LOGGER.info( f"Attempted call Report.create_issue with the following keywords: \n{pprint.pformat(kwargs)}", ) return MockIssue() def add_duplicate_comment(*args, **kwargs): - _logger.info("Patching Report.add_duplicate_comment") - _logger.info( + LOGGER.info("Patching Report.add_duplicate_comment") + LOGGER.info( f"Attempted to call Report.add_duplicate_comment with the following keywords: \n{pprint.pformat(kwargs)}", ) return @@ -128,12 +130,12 @@ def __post_init__(self): cap = CapJira() def create_jira_issue(*args, **kwargs): - _logger.info("Patching Report.create_jira_issue") + LOGGER.info("Patching Report.create_jira_issue") cap.create_jira_issue.append(CapInputs(args, kwargs)) return MockIssue() def add_duplicate_comment(*args, **kwargs): - _logger.info("Patching Report.add_duplicate_comment") + LOGGER.info("Patching Report.add_duplicate_comment") cap.add_duplicate_comment.append(CapInputs(args, kwargs)) monkeypatch.setattr(Report, "create_jira_issue", create_jira_issue) @@ -143,7 +145,7 @@ def add_duplicate_comment(*args, **kwargs): @pytest.fixture def patch_job_log_dir(monkeypatch, job_log_dir): - _logger.info("Patching Job log dir path") + LOGGER.info("Patching Job log dir path") def _download_logs(*args, **kwargs): return job_log_dir.as_posix() @@ -153,7 +155,7 @@ def _download_logs(*args, **kwargs): @pytest.fixture def patch_job_junit_dir(monkeypatch, job_junit_dir): - _logger.info("Patching Job junit dir path") + LOGGER.info("Patching Job junit dir path") def _download_junit(*args, **kwargs): return job_junit_dir.as_posix() @@ -161,12 +163,6 @@ def _download_junit(*args, **kwargs): monkeypatch.setattr(Job, "_download_junit", _download_junit) -@pytest.fixture -def assert_job_dir_exists(job_dir): - job_dir.mkdir(exist_ok=True, parents=True) - assert job_dir.is_dir() - - @pytest.fixture def fake_log_secret_path(job_log_dir): yield job_log_dir.joinpath("fake-step/fake_build_log.txt") @@ -178,10 +174,78 @@ def fake_junit_secret_path(job_junit_dir): @pytest.fixture -def assert_jira_config_file_exists(jira_config_path): - if not jira_config_path.is_file(): - jira_config_path.parent.mkdir(exist_ok=True, parents=True) - jira_config_path.write_text( +def firewatch_config_json(monkeypatch): + config_json = json.dumps( + { + "failure_rules": [ + { + "step": "exact-step-name", + "failure_type": "pod_failure", + "classification": "Infrastructure", + "jira_project": "!default", + "jira_component": ["some-component"], + "jira_assignee": "some-user@redhat.com", + "jira_security_level": "Restricted", + }, + { + "step": "*partial-name*", + "failure_type": "all", + "classification": "Misc.", + "jira_project": "OTHER", + "jira_component": ["component-1", "component-2", "!default"], + "jira_priority": "major", + "group": {"name": "some-group", "priority": 1}, + }, + { + "step": "*ends-with-this", + "failure_type": "test_failure", + "classification": "Test failures", + "jira_epic": "!default", + "jira_additional_labels": [ + "test-label-1", + "test-label-2", + "!default", + ], + "group": {"name": "some-group", "priority": 2}, + }, + { + "step": "*ignore*", + "failure_type": "test_failure", + "classification": "NONE", + "jira_project": "NONE", + "ignore": "true", + }, + { + "step": "affects-version", + "failure_type": "all", + "classification": "Affects Version", + "jira_project": "TEST", + "jira_epic": "!default", + "jira_affects_version": "4.14", + "jira_assignee": "!default", + }, + { + "step": "affects-version", + "failure_type": "all", + "classification": "Affects Version", + "jira_project": "TEST", + "jira_epic": "!default", + "jira_affects_version": "4.14", + "jira_assignee": "!default", + }, + ], + }, + ) + monkeypatch.setenv(FIREWATCH_CONFIG_ENV_VAR, config_json) + yield config_json + + +@pytest.fixture +def jira_config_path(tmp_path): + config_path = tmp_path.joinpath("jira_config.json") + if not config_path.is_file(): + config_path.parent.mkdir(exist_ok=True, parents=True) + config_path.write_text( json.dumps( { "token": os.getenv(JIRA_TOKEN_ENV_VAR), @@ -193,87 +257,7 @@ def assert_jira_config_file_exists(jira_config_path): }, ), ) - assert jira_config_path.is_file() - - -@pytest.fixture -def assert_firewatch_config_in_env(monkeypatch): - monkeypatch.setenv( - FIREWATCH_CONFIG_ENV_VAR, - json.dumps( - { - "failure_rules": [ - { - "step": "exact-step-name", - "failure_type": "pod_failure", - "classification": "Infrastructure", - "jira_project": "!default", - "jira_component": ["some-component"], - "jira_assignee": "some-user@redhat.com", - "jira_security_level": "Restricted", - }, - { - "step": "*partial-name*", - "failure_type": "all", - "classification": "Misc.", - "jira_project": "OTHER", - "jira_component": ["component-1", "component-2", "!default"], - "jira_priority": "major", - "group": {"name": "some-group", "priority": 1}, - }, - { - "step": "*ends-with-this", - "failure_type": "test_failure", - "classification": "Test failures", - "jira_epic": "!default", - "jira_additional_labels": [ - "test-label-1", - "test-label-2", - "!default", - ], - "group": {"name": "some-group", "priority": 2}, - }, - { - "step": "*ignore*", - "failure_type": "test_failure", - "classification": "NONE", - "jira_project": "NONE", - "ignore": "true", - }, - { - "step": "affects-version", - "failure_type": "all", - "classification": "Affects Version", - "jira_project": "TEST", - "jira_epic": "!default", - "jira_affects_version": "4.14", - "jira_assignee": "!default", - }, - { - "step": "affects-version", - "failure_type": "all", - "classification": "Affects Version", - "jira_project": "TEST", - "jira_epic": "!default", - "jira_affects_version": "4.14", - "jira_assignee": "!default", - }, - ], - }, - ), - ) - assert os.getenv(FIREWATCH_CONFIG_ENV_VAR) - - -@pytest.fixture -def assert_artifact_dir_exists(artifact_dir): - artifact_dir.mkdir(exist_ok=True, parents=True) - assert artifact_dir.is_dir() - - -@pytest.fixture -def jira_config_path(tmp_path): - yield tmp_path.joinpath("jira_config.json") + yield config_path @pytest.fixture(params=JOB_STEP_DIRS_TO_TEST) @@ -307,14 +291,11 @@ def job_dir_junit_artifact_paths(request, job_junit_step_dirs): @pytest.fixture -def assert_artifact_dir_in_env(monkeypatch, artifact_dir): - monkeypatch.setenv(ARTIFACT_DIR_ENV_VAR, artifact_dir.as_posix()) - assert os.getenv(ARTIFACT_DIR_ENV_VAR) - - -@pytest.fixture -def artifact_dir(tmp_path): - yield tmp_path / "artifacts" +def artifact_dir(monkeypatch, tmp_path): + path = tmp_path / "artifacts" + monkeypatch.setenv(ARTIFACT_DIR_ENV_VAR, path.as_posix()) + path.mkdir(exist_ok=True, parents=True) + yield path @pytest.fixture @@ -323,38 +304,28 @@ def job_dir(tmp_path, build_id): @pytest.fixture(params=BUILD_IDS_TO_TEST, ids=BUILD_IDS_TO_TEST) -def build_id(request): - yield request.param +def build_id(monkeypatch, request): + param = request.param + monkeypatch.setenv(BUILD_ID_ENV_VAR, param) + yield param @pytest.fixture(params=DEFAULT_JIRA_PROJECTS_TO_TEST, ids=DEFAULT_JIRA_PROJECTS_TO_TEST) -def default_jira_project(request): - yield request.param +def default_jira_project(monkeypatch, request): + param = request.param + monkeypatch.setenv(FIREWATCH_DEFAULT_JIRA_PROJECT_ENV_VAR, param) + yield param @pytest.fixture(params=DEFAULT_JIRA_EPICS_TO_TEST, ids=DEFAULT_JIRA_EPICS_TO_TEST) -def default_jira_epic(request): - yield request.param - - -@pytest.fixture -def assert_build_id_in_env(build_id, monkeypatch): - monkeypatch.setenv(BUILD_ID_ENV_VAR, build_id) - assert os.getenv(BUILD_ID_ENV_VAR) - - -@pytest.fixture -def assert_default_jira_project_in_env(default_jira_project, monkeypatch): - monkeypatch.setenv(FIREWATCH_DEFAULT_JIRA_PROJECT_ENV_VAR, default_jira_project) - assert os.getenv(FIREWATCH_DEFAULT_JIRA_PROJECT_ENV_VAR) - - -@pytest.fixture -def assert_default_jira_epic_in_env(default_jira_epic, monkeypatch): - monkeypatch.setenv(FIREWATCH_DEFAULT_JIRA_EPIC_ENV_VAR, default_jira_epic) - assert os.getenv(FIREWATCH_DEFAULT_JIRA_EPIC_ENV_VAR) +def default_jira_epic(monkeypatch, request): + param = request.param + monkeypatch.setenv(FIREWATCH_DEFAULT_JIRA_EPIC_ENV_VAR, param) + yield param @pytest.fixture -def assert_jira_token_in_env(): - assert os.getenv(JIRA_TOKEN_ENV_VAR) +def jira_token(): + token = os.getenv(JIRA_TOKEN_ENV_VAR) + assert token + yield token diff --git a/tests/unittests/objects/failure_rule/test_firewatch_objects_failure_rule_matches_failure.py b/tests/unittests/objects/failure_rule/test_firewatch_objects_failure_rule_matches_failure.py index ae108a4b..b2505bc6 100644 --- a/tests/unittests/objects/failure_rule/test_firewatch_objects_failure_rule_matches_failure.py +++ b/tests/unittests/objects/failure_rule/test_firewatch_objects_failure_rule_matches_failure.py @@ -20,7 +20,7 @@ @pytest.fixture(autouse=True) -def setup_tests(assert_default_jira_project_in_env): +def setup_tests(default_jira_project): ... diff --git a/tests/unittests/objects/fixtures/test_fixtures.py b/tests/unittests/objects/fixtures/test_fixtures.py index c7211d6d..dad00023 100644 --- a/tests/unittests/objects/fixtures/test_fixtures.py +++ b/tests/unittests/objects/fixtures/test_fixtures.py @@ -22,16 +22,15 @@ @pytest.fixture(autouse=True) def setup_test_environment( - assert_jira_token_in_env, - assert_jira_config_file_exists, - assert_default_jira_project_in_env, - assert_firewatch_config_in_env, + jira_token, + jira_config_path, + default_jira_project, + firewatch_config_json, patch_job_junit_dir, patch_job_log_dir, - assert_build_id_in_env, - assert_artifact_dir_exists, - assert_artifact_dir_in_env, - assert_job_dir_exists, + build_id, + artifact_dir, + job_dir, ): ... diff --git a/tests/unittests/objects/job/test_fail_with_test_failures_should_not_cause_failure_for_ignored_step.py b/tests/unittests/objects/job/test_fail_with_test_failures_should_not_cause_failure_for_ignored_step.py index 00659955..5a5c59f7 100644 --- a/tests/unittests/objects/job/test_fail_with_test_failures_should_not_cause_failure_for_ignored_step.py +++ b/tests/unittests/objects/job/test_fail_with_test_failures_should_not_cause_failure_for_ignored_step.py @@ -25,8 +25,8 @@ @pytest.fixture(autouse=True) def setup_tests( monkeypatch, - assert_jira_config_file_exists, - assert_default_jira_project_in_env, + jira_config_path, + default_jira_project, patch_job_log_dir, patch_job_junit_dir, ):