From e9ca4928c939ffaf6a6dd20bd27b7e38a9ebebc9 Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Wed, 19 Feb 2025 14:06:51 +0200 Subject: [PATCH 1/9] Raise custom CheckError for missing virtual environment and add CheckError exception class --- grader/checks/abstract_check.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/grader/checks/abstract_check.py b/grader/checks/abstract_check.py index 1e3a5a0..7f040d9 100644 --- a/grader/checks/abstract_check.py +++ b/grader/checks/abstract_check.py @@ -32,7 +32,7 @@ def run(self) -> float: :rtype: float """ if self._is_venv_required and not self.is_running_within_venv(): - raise RuntimeError("Virtual environment is required for this check") + raise CheckError("Virtual environment is required for this check") logger.log(VERBOSE, "Running %s", self.name) @@ -65,3 +65,9 @@ def is_running_within_venv() -> bool: :rtype: bool """ return VirtualEnvironment.is_initialized + + +class CheckError(Exception): + """Custom exception for check errors.""" + + pass From 051a5ec893a928cbae0b5c757f7d9d5071434d3e Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Wed, 19 Feb 2025 14:43:08 +0200 Subject: [PATCH 2/9] Remove unnecessary pass statement from CheckError exception class --- grader/checks/abstract_check.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/grader/checks/abstract_check.py b/grader/checks/abstract_check.py index 7f040d9..56d87ee 100644 --- a/grader/checks/abstract_check.py +++ b/grader/checks/abstract_check.py @@ -69,5 +69,3 @@ def is_running_within_venv() -> bool: class CheckError(Exception): """Custom exception for check errors.""" - - pass From a0b5fc3e9cf7284c91123918651345bbf8788e5f Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Wed, 19 Feb 2025 15:15:16 +0200 Subject: [PATCH 3/9] Add error handling for coverage check and report failures --- grader/checks/coverage_check.py | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/grader/checks/coverage_check.py b/grader/checks/coverage_check.py index 10822c7..d1501f8 100644 --- a/grader/checks/coverage_check.py +++ b/grader/checks/coverage_check.py @@ -5,7 +5,7 @@ import logging import os -from grader.checks.abstract_check import AbstractCheck +from grader.checks.abstract_check import AbstractCheck, CheckError from grader.utils.constants import ( COVERAGE_PATH, COVERAGE_RUN_ARGS, @@ -58,6 +58,10 @@ def __translate_score(self, coverage_score: float) -> float: :param coverage_score: The score from pylint to be translated :return: The translated score """ + + if self._max_points == -1: + raise CheckError("Max points for coverage check is set to -1") + step = 100 / (self._max_points + 1) steps = [i * step for i in range(self._max_points + 2)] @@ -75,7 +79,11 @@ def __coverage_run(self): """ command = [self.__coverage_full_path] + COVERAGE_RUN_ARGS + COVERAGE_RUN_PYTEST_ARGS + [self._project_root] - output = run(command, current_directory=self._project_root) + try: + output = run(command, current_directory=self._project_root) + except (OSError, ValueError) as e: + logger.error("Coverage run failed: %s", e) + raise CheckError("Coverage run failed") from e if output.returncode != 0: logger.error("Coverage run failed") @@ -89,11 +97,19 @@ def __coverage_report(self): """ source_files = find_all_source_files(self._project_root) - command = [self.__coverage_full_path] + COVERAGE_REPORT_ARGS_NO_FORMAT + source_files - output = run(command, current_directory=self._project_root) - - command = [self.__coverage_full_path] + COVERAGE_REPORT_ARGS + source_files - output = run(command, current_directory=self._project_root) + try: + command = [self.__coverage_full_path] + COVERAGE_REPORT_ARGS_NO_FORMAT + source_files + output = run(command, current_directory=self._project_root) + except (OSError, ValueError) as e: + logger.error("Coverage report (no format) failed: %s", e) + raise CheckError("Coverage report (no format) failed") from e + + try: + command = [self.__coverage_full_path] + COVERAGE_REPORT_ARGS + source_files + output = run(command, current_directory=self._project_root) + except (OSError, ValueError) as e: + logger.error("Coverage report (with format) failed: %s", e) + raise CheckError("Coverage report (with format) failed") from e if output.returncode != 0: logger.error("Coverage report failed") From 83a3a0d3c77f344df1e99c9bad443bd4739086ac Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Thu, 20 Feb 2025 18:27:23 +0200 Subject: [PATCH 4/9] Raise CheckError for failed coverage run and report generation --- grader/checks/coverage_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grader/checks/coverage_check.py b/grader/checks/coverage_check.py index d1501f8..6c1b5ab 100644 --- a/grader/checks/coverage_check.py +++ b/grader/checks/coverage_check.py @@ -41,12 +41,12 @@ def run(self) -> float: is_coverage_run_okay = self.__coverage_run() if not is_coverage_run_okay: - return 0.0 + raise CheckError("Coverage run failed") coverage_report_result = self.__coverage_report() if coverage_report_result is None: - return 0.0 + raise CheckError("Coverage report generation failed") return self.__translate_score(coverage_report_result) From 7c626c1ce2654eeee6f81a40acc112b9c4c94113 Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Thu, 20 Feb 2025 18:28:53 +0200 Subject: [PATCH 5/9] Refactor coverage check to raise CheckError on failure instead of returning boolean --- grader/checks/coverage_check.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/grader/checks/coverage_check.py b/grader/checks/coverage_check.py index 6c1b5ab..042acc1 100644 --- a/grader/checks/coverage_check.py +++ b/grader/checks/coverage_check.py @@ -38,10 +38,7 @@ def run(self) -> float: """ super().run() - is_coverage_run_okay = self.__coverage_run() - - if not is_coverage_run_okay: - raise CheckError("Coverage run failed") + self.__coverage_run() coverage_report_result = self.__coverage_report() @@ -87,9 +84,7 @@ def __coverage_run(self): if output.returncode != 0: logger.error("Coverage run failed") - return False - - return True + raise CheckError("Coverage run failed") def __coverage_report(self): """ @@ -113,6 +108,6 @@ def __coverage_report(self): if output.returncode != 0: logger.error("Coverage report failed") - return None + raise CheckError("Coverage report failed") return int(output.stdout) From c96b4d4cab2ffc14bf9e8a4c7c409d4bee85c3d6 Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Thu, 20 Feb 2025 18:43:15 +0200 Subject: [PATCH 6/9] Add error handling and raise CheckError in PylintCheck for file finding and execution failures --- grader/checks/pylint_check.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/grader/checks/pylint_check.py b/grader/checks/pylint_check.py index aa9f93c..2282f0c 100644 --- a/grader/checks/pylint_check.py +++ b/grader/checks/pylint_check.py @@ -12,7 +12,7 @@ import grader.utils.constants as const from grader.utils import process -from grader.checks.abstract_check import AbstractCheck +from grader.checks.abstract_check import AbstractCheck, CheckError from grader.utils.files import find_all_python_files logger = logging.getLogger("grader") @@ -38,7 +38,11 @@ def run(self) -> float: """ super().run() - pylint_args = find_all_python_files(self._project_root) + try: + pylint_args = find_all_python_files(self._project_root) + except OSError as error: + logger.error("Error while finding python files: %s", error) + raise CheckError("Error while finding python files") from error logger.debug("Running pylint check on files: %s", pylint_args) pylintrc_path = const.PYLINTRC @@ -47,10 +51,14 @@ def run(self) -> float: pylint_args.extend(["--rcfile", pylintrc_path]) command = [os.path.join(self._project_root, const.PYLINT_PATH)] + pylint_args - results = process.run(command, current_directory=self._project_root) + try: + results = process.run(command, current_directory=self._project_root) + except (OSError, ValueError) as error: + logger.error("Error while running pylint: %s", error) + raise CheckError("Error while running pylint") from error if results.returncode != 0: - return 0 + raise CheckError("Pylint check failed") pylint_score = self.__get_pylint_score(results.stdout) @@ -65,6 +73,9 @@ def __translate_score(self, pylint_score: float) -> float: :param pylint_score: The score from pylint to be translated :return: The translated score """ + if self._max_points == -1: + raise CheckError("Max points for pylint check is set to -1") + step = self.__pylint_max_score / (self._max_points + 1) steps = [i * step for i in range(self._max_points + 2)] @@ -94,10 +105,10 @@ def __get_pylint_score(self, pylint_output: str) -> float: return float(score) case None: logger.error("Pylint score not found") - return 0.0 + raise CheckError("Pylint score not found") logger.error("Pylint score not found") - return 0.0 + raise CheckError("Pylint score not found") class PylintCustomReporter(TextReporter): From b4078796a9a0f38e66174cbc8b0504097e41c70e Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Thu, 20 Feb 2025 18:47:22 +0200 Subject: [PATCH 7/9] Add error handling and raise CheckError in TypeHintsCheck for file finding, mypy execution, and report reading failures --- grader/checks/type_hints_check.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/grader/checks/type_hints_check.py b/grader/checks/type_hints_check.py index e94b88f..891365e 100644 --- a/grader/checks/type_hints_check.py +++ b/grader/checks/type_hints_check.py @@ -5,7 +5,7 @@ import logging -from grader.checks.abstract_check import AbstractCheck +from grader.checks.abstract_check import AbstractCheck, CheckError from grader.utils.constants import MYPY_TYPE_HINT_CONFIG, REPORTS_TEMP_DIR, MYPY_LINE_COUNT_REPORT from grader.utils import files from grader.utils import process @@ -41,19 +41,27 @@ def run(self) -> float: super().run() # Gather all files - all_source_files = files.find_all_source_files(self._project_root) + try: + all_source_files = files.find_all_source_files(self._project_root) + except OSError as error: + logger.error("Error while finding python files: %s", error) + raise CheckError("Error while finding python files") from error # Run mypy on all files command = [self.__mypy_binary] + self.__mypy_arguments + all_source_files - _ = process.run(command) + try: + _ = process.run(command) + except (OSError, ValueError) as error: + logger.error("Error while running mypy: %s", error) + raise CheckError("Error while running mypy") from error # Read mypy linecount report try: with open(MYPY_LINE_COUNT_REPORT, "r", encoding="utf-8") as report_file: report = report_file.readline().strip().split() - except FileNotFoundError: + except FileNotFoundError as error: logger.error("Mypy linecount report not found") - return 0.0 + raise CheckError("Mypy linecount report not found") from error # Fancy way to get the needed values - I need the 3rd and 4th values, out of 5 total *_, lines_with_type_annotations, lines_total, _ = report @@ -73,6 +81,9 @@ def __translate_score(self, mypy_score: float) -> float: :param pylint_score: The score from pylint to be translated :return: The translated score """ + if self._max_points == -1: + raise CheckError("Max points for type hints check is set to -1") + step = self.__mypy_max_score / (self._max_points + 1) steps = [i * step for i in range(self._max_points + 2)] From 8d5cff388cb32edf9d20fae56eff30d632c38755 Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Thu, 20 Feb 2025 18:48:24 +0200 Subject: [PATCH 8/9] Add error handling for check execution failures and log errors --- main.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index d0a766e..6c72f4a 100644 --- a/main.py +++ b/main.py @@ -6,6 +6,7 @@ import sys import grader.utils.constants as const +from grader.checks.abstract_check import CheckError from grader.checks.checks_factory import create_checks from grader.utils.cli import get_args from grader.utils.config import load_config @@ -40,12 +41,22 @@ non_venv_checks, venv_checks = create_checks(config, project_root) for check in non_venv_checks: - check_score = check.run() + try: + check_score = check.run() + except CheckError as error: + logger.error("Check failed: %s", error) + check_score = 0.0 + scores.append((check.name, check_score, check.max_points)) with VirtualEnvironment(project_root) as venv: for check in venv_checks: - check_score = check.run() + try: + check_score = check.run() + except CheckError as error: + logger.error("Check failed: %s", error) + check_score = 0.0 + scores.append((check.name, check_score, check.max_points)) for name, score, max_score in scores: From 26fa2ee0ef050e86fd07f9d9d31e1c089a258047 Mon Sep 17 00:00:00 2001 From: Lyuboslav Karev Date: Fri, 21 Feb 2025 10:54:21 +0200 Subject: [PATCH 9/9] Update tests to raise CheckError for coverage and type hints checks on failure --- tests/test_abstract_check.py | 4 ++-- tests/test_coverage_check.py | 11 ++++++----- tests/test_type_hints_check.py | 5 +++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/test_abstract_check.py b/tests/test_abstract_check.py index 0dfa450..502d4ab 100644 --- a/tests/test_abstract_check.py +++ b/tests/test_abstract_check.py @@ -4,7 +4,7 @@ import unittest -from grader.checks.abstract_check import AbstractCheck +from grader.checks.abstract_check import AbstractCheck, CheckError class TestAbstractCheck(unittest.TestCase): @@ -29,5 +29,5 @@ def run(self) -> float: check = DummyCheck("dummy", 1, "dummy", is_venv_requred=True) # Act & Assert - with self.assertRaises(RuntimeError): + with self.assertRaises(CheckError): check.run() diff --git a/tests/test_coverage_check.py b/tests/test_coverage_check.py index 015da82..aba1f06 100644 --- a/tests/test_coverage_check.py +++ b/tests/test_coverage_check.py @@ -6,6 +6,7 @@ from subprocess import CompletedProcess from unittest.mock import patch, MagicMock +from grader.checks.abstract_check import CheckError from grader.checks.coverage_check import CoverageCheck @@ -25,18 +26,18 @@ def setUp(self): @patch("subprocess.run") def test_01_coverage_run_fail(self, mocked_run: MagicMock): """ - Test that a failed coverage run logs an error and returns a score of 0.0. + Test that a failed coverage run logs an error and raises an exception. """ # Arrange mocked_run.return_value = CompletedProcess(args=["coverage", "run"], returncode=1) # Act with self.assertLogs("grader", level="ERROR") as log: - result = self.coverage_check.run() + with self.assertRaises(CheckError): + self.coverage_check.run() is_message_logged = "ERROR:grader:Coverage run failed" in log.output # Assert - self.assertEqual(0.0, result) self.assertTrue(is_message_logged) @patch("subprocess.run") @@ -55,11 +56,11 @@ def mocked_run_side_effect(*args, **kwargs): mocked_run.side_effect = mocked_run_side_effect # Act with self.assertLogs("grader", level="ERROR") as log: - result = self.coverage_check.run() + with self.assertRaises(CheckError): + self.coverage_check.run() is_message_logged = "ERROR:grader:Coverage report failed" in log.output # Assert - self.assertEqual(0.0, result) self.assertTrue(is_message_logged) @patch("grader.checks.coverage_check.CoverageCheck._CoverageCheck__coverage_run") diff --git a/tests/test_type_hints_check.py b/tests/test_type_hints_check.py index 1b89351..520fae7 100644 --- a/tests/test_type_hints_check.py +++ b/tests/test_type_hints_check.py @@ -9,6 +9,7 @@ from unittest.mock import patch, MagicMock import grader.utils.constants as const +from grader.checks.abstract_check import CheckError from grader.checks.type_hints_check import TypeHintsCheck @@ -71,11 +72,11 @@ def test_03_mypy_report_missing(self, mocked_run: MagicMock): # Act with self.assertLogs(level="ERROR") as cm: - result = self.type_hints_check.run() + with self.assertRaises(CheckError): + self.type_hints_check.run() is_error_logged = any("Mypy linecount report not found" in log for log in cm.output) # Assert - self.assertEqual(result, 0.0) self.assertTrue(is_error_logged) @patch("grader.utils.process.run")