From d82dea45424d2021cfe054dfced8016c9f85be45 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Fri, 22 Aug 2025 10:05:33 +0100 Subject: [PATCH 1/7] Replace RuntimeErrors with custom exceptions Swap all RuntimeError exceptions for custom fab exceptions using a class hierarchy derived from RuntimeError to minimise distruptive changes. --- source/fab/artefacts.py | 4 +- source/fab/errors.py | 344 ++++++++++++++++++ source/fab/steps/__init__.py | 7 +- source/fab/steps/archive_objects.py | 12 +- source/fab/steps/compile_c.py | 5 +- source/fab/steps/compile_fortran.py | 9 +- source/fab/steps/find_source_files.py | 3 +- source/fab/steps/grab/prebuild.py | 3 +- source/fab/steps/grab/svn.py | 5 +- source/fab/steps/preprocess.py | 8 +- source/fab/steps/psyclone.py | 6 +- source/fab/tools/compiler.py | 18 +- source/fab/tools/compiler_wrapper.py | 11 +- source/fab/tools/linker.py | 3 +- source/fab/tools/psyclone.py | 26 +- source/fab/tools/tool.py | 19 +- source/fab/tools/tool_box.py | 3 +- source/fab/tools/tool_repository.py | 31 +- source/fab/tools/versioning.py | 4 +- .../unit_tests/steps/test_archive_objects.py | 8 +- tests/unit_tests/steps/test_compile_c.py | 7 +- .../unit_tests/steps/test_compile_fortran.py | 12 +- tests/unit_tests/steps/test_preprocess.py | 7 +- tests/unit_tests/test_artefacts.py | 5 +- tests/unit_tests/test_cui_arguments.py | 2 +- tests/unit_tests/test_errors.py | 182 +++++++++ tests/unit_tests/tools/test_compiler.py | 66 ++-- .../unit_tests/tools/test_compiler_wrapper.py | 13 +- tests/unit_tests/tools/test_linker.py | 6 +- tests/unit_tests/tools/test_psyclone.py | 26 +- tests/unit_tests/tools/test_tool.py | 26 +- tests/unit_tests/tools/test_tool_box.py | 4 +- .../unit_tests/tools/test_tool_repository.py | 47 +-- tests/unit_tests/tools/test_versioning.py | 12 +- 34 files changed, 760 insertions(+), 184 deletions(-) create mode 100644 source/fab/errors.py create mode 100644 tests/unit_tests/test_errors.py diff --git a/source/fab/artefacts.py b/source/fab/artefacts.py index 4391117d5..5fa79acf7 100644 --- a/source/fab/artefacts.py +++ b/source/fab/artefacts.py @@ -129,8 +129,8 @@ def replace(self, artefact: Union[str, ArtefactSet], art_set = self[artefact] if not isinstance(art_set, set): - raise RuntimeError(f"Replacing artefacts in dictionary " - f"'{artefact}' is not supported.") + name = artefact if isinstance(artefact, str) else artefact.name + raise ValueError(f"{name} is not mutable") art_set.difference_update(set(remove_files)) art_set.update(add_files) diff --git a/source/fab/errors.py b/source/fab/errors.py new file mode 100644 index 000000000..1dab3a48f --- /dev/null +++ b/source/fab/errors.py @@ -0,0 +1,344 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## +""" +Custom exception classes designed to replace generic RuntimeError +exceptions originally used in fab. +""" + +from pathlib import Path +from typing import List, Optional, TYPE_CHECKING, Union + +if TYPE_CHECKING: + from fab.tools.category import Category + from fab.tools.tool import Tool + + +class FabError(RuntimeError): + """Base class for all fab specific exceptions. + + :param message: reason for the exception + """ + + def __init__(self, message: str) -> None: + self.message = message + + def __str__(self) -> str: + return self.message + + +class FabToolError(FabError): + """Base class for fab tool and toolbox exceptions. + + :param tool: name of the current tool or category + :param message: reason for the exception + """ + + def __init__(self, tool: Union["Category", "Tool", str], message: str) -> None: + self.tool = tool + + # Check for name attributes rather than using isintance + # because Category and Tool ahve issues with circular + # dependencies + if hasattr(tool, "name"): + self.name = tool.name + elif hasattr(tool, "_name"): + self.name = tool._name + else: + self.name = tool + super().__init__(f"[{self.name}] {message}") + + +class FabToolMismatch(FabToolError): + """Tool and category mismatch. + + Error when a tool category does not match the expected setting. + + :param tool: name of the current tool + :param category: name of the current category + :param expectedd: name of the correct category + """ + + def __init__( + self, + tool: Union["Category", "Tool", str], + category: Union["Category", "Tool", type, str], + expected: str, + ) -> None: + self.category = category + self.expected = expected + + super().__init__(tool, f"got type {category} instead of {expected}") + + +class FabToolInvalidVersion(FabToolError): + """Version format problem. + + Error when version information cannot be extracted from a specific + tool. Where a version pattern is available, report this as part + of the error. + + :param tool: name of the current tool + :param value: output from the query command + :param expected: optional format of version string + """ + + def __init__( + self, + tool: Union["Category", "Tool", str], + value: str, + expected: Optional[str] = None, + ) -> None: + self.value = value + self.expected = expected + + message = f"invalid version {repr(self.value)}" + if expected is not None: + message += f" should be {repr(expected)}" + + super().__init__(tool, message) + + +class FabToolPsycloneAPI(FabToolError): + """PSyclone API and target problem. + + Error when the specified PSyclone API, which can be empty to + indicate DSL mode, and the associated target do not match. + + :param api: the current API or empty for DSL mode + :param target: the name of the target + :param present: optionally whether the target is present or + absent. Used to format the error message. Defaults to False. + """ + + def __init__( + self, api: Union[str, None], target: str, present: Optional[bool] = False + ) -> None: + self.target = target + self.present = present + self.api = api + + message = "called " + if api: + message += f"with {api} API " + else: + message += "without API " + if present: + message += "and with " + else: + message += "but not with " + message += f"{target}" + + super().__init__("psyclone", message) + + +class FabToolNotAvailable(FabToolError): + """An unavailable tool has been requested. + + Error where a tool which is not available in a particular suite of + tools has been requested. + + :param tool: name of the current tool + :param suite: optional name of the current tool suite + """ + + def __init__( + self, tool: Union["Category", "Tool", str], suite: Optional[str] = None + ) -> None: + message = "not available" + if suite: + message += f" in suite {suite}" + super().__init__(tool, message) + + +class FabToolInvalidSetting(FabToolError): + """An invalid tool setting has been requested. + + Error where an invalid setting, e.g. MPI, has been requested for a + particular tool. + + :param setting_type: name of the invalid setting + :param tool: the tool to which setting applies + :param additional: optional additional information + """ + + # FIXME: improve these here and in tool_repository + + def __init__( + self, + setting_type: str, + tool: Union["Category", "Tool", str], + additional: Optional[str] = None, + ) -> None: + self.setting_type = setting_type + + message = f"invalid {setting_type}" + if additional: + message += f" {additional}" + + super().__init__(tool, message) + + +class FabUnknownLibraryError(FabError): + """An unknown library has been requested. + + Error where an library which is not known to the current Linker + instance is requested. + + :param library: the name of the unknown library + """ + + def __init__(self, library: str) -> None: + self.library = library + super().__init__(f"unknown library {library}") + + +class FabCommandError(FabError): + """An error was encountered running a subcommand. + + Error where a subcommand run by the fab framework returned with a + non-zero exit code. The exit code plus any data from the stdout + and stderr streams are retained to allow them to be passed back up + the calling stack. + + :param command: the command being run + :param code: the command return code from the OS + :param output: output stream from the command + :param error: error stream from the command + """ + + def __init__( + self, + command: str, + code: int, + output: Union[str, bytes, None], + error: Union[str, bytes, None], + cwd: Optional[Union[str, Path]] = None, + ) -> None: + if isinstance(command, list): + self.command: str = " ".join(command) + else: + self.command = str(command) + self.code = int(code) + self.output = self._decode(output) + self.error = self._decode(error) + self.cwd = cwd + super().__init__(f"command {repr(self.command)} returned {code}") + + def _decode(self, value: Union[str, bytes, None]) -> str: + """Convert from bytes to a string as necessary.""" + if value is None: + return "" + if isinstance(value, bytes): + return value.decode() + return value + + +class FabCommandNotFound(FabError): + """Target command could not be found by subprocess. + + Error where the target command passed to a subprocess call could + not be found. + + :param command: the target command. For clarity, only the first + item is used in the error message but the entire command is + preserved for inspection by the caller. + """ + + def __init__(self, command: str) -> None: + self.command = command + if isinstance(command, list): + self.target: str = command[0] + elif isinstance(command, str): + self.target = command.split()[0] + else: + raise ValueError(f"invalid command: {command}") + + super().__init__(f"unable to execute {self.target}") + + +class FabMultiCommandError(FabError): + """Unpack multiple exceptions into a single one. + + Error which combines all potential exceptions raised by a + multiprocessing section into a single exception class for + subsequent inspection. + + This feature is is required because versions of python prior to + 3.11 do not support ExceptionGroups. + + :param errors: a list ot exceptions + :param label: an identifier for the multiprocessing section + """ + + def __init__( + self, errors: List[Union[str, Exception]], label: Optional[str] = None + ) -> None: + self.errors = errors + self.label = label or "during multiprocessing" + + message = f"{len(errors)} exception" + message += " " if len(errors) == 1 else "s " + message += f"{self.label}" + + super().__init__(message) + + +class FabSourceError(FabError): + """Base class for source code management exceptions.""" + + +class FabSourceNoFilesError(FabSourceError): + """No source files were found. + + Error where no source files have been once any filtering rules + have been applied. + + """ + + def __init__(self) -> None: + super().__init__("no source files found after filtering") + + +class FabSourceMergeError(FabSourceError): + """Version control merge has failed. + + Error where the underlying version control system has failed to + automatically merge source code changes, e.g. because of a source + conflict that requires manual resolution. + + :param tool: name of the version control system + :param reason: reason/error output from the version control system + indicating why the merge failed + :param revision: optional name of the specific revision being + targeted + """ + + def __init__(self, tool: str, reason: str, revision: Optional[str] = None) -> None: + self.tool = tool + self.reason = reason + + message = f"[{tool}] merge " + if revision: + message += f"of {revision} " + message += f"failed: {reason}" + + super().__init__(message) + + +class FabSourceFetchError(FabSourceError): + """An attempt to fetch source files has failed. + + Error where a specific set of files could not be fetched, + e.g. from a location containing prebuild files. + + :params source: location of the source files + :param reason: reason for the failure + """ + + def __init__(self, source: str, reason: str) -> None: + self.source = source + self.reason = reason + super().__init__(f"could not fetch {source}: {reason}") diff --git a/source/fab/steps/__init__.py b/source/fab/steps/__init__.py index 160d3ed9a..f45040dc5 100644 --- a/source/fab/steps/__init__.py +++ b/source/fab/steps/__init__.py @@ -11,6 +11,8 @@ from fab.metrics import send_metric from fab.util import by_type, TimerLogger +from fab.errors import FabMultiCommandError + from functools import wraps @@ -96,7 +98,4 @@ def check_for_errors(results: Iterable[Union[str, Exception]], exceptions = list(by_type(results, Exception)) if exceptions: - formatted_errors = "\n\n".join(map(str, exceptions)) - raise RuntimeError( - f"{formatted_errors}\n\n{len(exceptions)} error(s) found {caller_label}" - ) + raise FabMultiCommandError(exceptions, caller_label) diff --git a/source/fab/steps/archive_objects.py b/source/fab/steps/archive_objects.py index 13a64ecef..a24a2418d 100644 --- a/source/fab/steps/archive_objects.py +++ b/source/fab/steps/archive_objects.py @@ -19,6 +19,8 @@ from fab.tools import Ar, Category from fab.artefacts import ArtefactsGetter, CollectionGetter +from fab.errors import FabToolMismatch, FabCommandError + logger = logging.getLogger(__name__) DEFAULT_SOURCE_GETTER = CollectionGetter(ArtefactSet.OBJECT_FILES) @@ -105,8 +107,7 @@ def archive_objects(config: BuildConfig, source_getter = source or DEFAULT_SOURCE_GETTER ar = config.tool_box[Category.AR] if not isinstance(ar, Ar): - raise RuntimeError(f"Unexpected tool '{ar.name}' of type " - f"'{type(ar)}' instead of Ar") + raise FabToolMismatch(ar.name, type(ar), "Ar") output_fpath = str(output_fpath) if output_fpath else None target_objects = source_getter(config.artefact_store) @@ -130,9 +131,8 @@ def archive_objects(config: BuildConfig, log_or_dot(logger, f"CreateObjectArchive running archiver for " f"'{output_fpath}'.") - try: - ar.create(output_fpath, sorted(objects)) - except RuntimeError as err: - raise RuntimeError(f"error creating object archive:\n{err}") from err + + # Allow command errors to propagate up to the caller + ar.create(output_fpath, sorted(objects)) config.artefact_store.update_dict(output_collection, output_fpath, root) diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index a059b4345..16c584cec 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -21,6 +21,8 @@ from fab.tools import Category, Compiler, Flags from fab.util import CompiledFile, log_or_dot, Timer, by_type +from fab.errors import FabToolMismatch + logger = logging.getLogger(__name__) DEFAULT_SOURCE_GETTER = FilterBuildTrees(suffix='.c') @@ -123,8 +125,7 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): config = mp_payload.config compiler = config.tool_box[Category.C_COMPILER] if compiler.category != Category.C_COMPILER: - raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " - f"'{compiler.category}' instead of CCompiler") + raise FabToolMismatch(compiler.name, compiler.category, "CCompiler") # Tool box returns a Tool, in order to make mypy happy, we need # to cast it to be a Compiler. compiler = cast(Compiler, compiler) diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index a3dc97ea3..9c7e0e683 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -25,6 +25,8 @@ from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) +from fab.errors import FabToolMismatch + logger = logging.getLogger(__name__) DEFAULT_SOURCE_GETTER = FilterBuildTrees(suffix=['.f', '.f90']) @@ -133,8 +135,7 @@ def handle_compiler_args(config: BuildConfig, common_flags=None, # Command line tools are sometimes specified with flags attached. compiler = config.tool_box[Category.FORTRAN_COMPILER] if compiler.category != Category.FORTRAN_COMPILER: - raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " - f"'{compiler.category}' instead of FortranCompiler") + raise FabToolMismatch(compiler.name, compiler.category, "FortranCompiler") # The ToolBox returns a Tool. In order to make mypy happy, we need to # cast this to become a Compiler. compiler = cast(Compiler, compiler) @@ -264,9 +265,7 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ compiler = config.tool_box.get_tool(Category.FORTRAN_COMPILER, config.mpi) if compiler.category != Category.FORTRAN_COMPILER: - raise RuntimeError(f"Unexpected tool '{compiler.name}' of " - f"category '{compiler.category}' instead of " - f"FortranCompiler") + raise FabToolMismatch(compiler.name, compiler.category, "FortranCompiler") # The ToolBox returns a Tool, but we need to tell mypy that # this is a Compiler compiler = cast(Compiler, compiler) diff --git a/source/fab/steps/find_source_files.py b/source/fab/steps/find_source_files.py index 52b03e0a2..0e3e28dd0 100644 --- a/source/fab/steps/find_source_files.py +++ b/source/fab/steps/find_source_files.py @@ -13,6 +13,7 @@ from fab.artefacts import ArtefactSet from fab.steps import step from fab.util import file_walk +from fab.errors import FabSourceNoFilesError logger = logging.getLogger(__name__) @@ -144,7 +145,7 @@ def find_source_files(config, source_root=None, logger.debug(f"excluding {fpath}") if not filtered_fpaths: - raise RuntimeError("no source files found after filtering") + raise FabSourceNoFilesError() config.artefact_store.add(output_collection, filtered_fpaths) diff --git a/source/fab/steps/grab/prebuild.py b/source/fab/steps/grab/prebuild.py index 75ad8ff5b..611acdaaa 100644 --- a/source/fab/steps/grab/prebuild.py +++ b/source/fab/steps/grab/prebuild.py @@ -6,6 +6,7 @@ from fab.steps import step from fab.steps.grab import logger from fab.tools import Category +from fab.errors import FabSourceFetchError @step @@ -28,4 +29,4 @@ def grab_pre_build(config, path, allow_fail=False): msg = f"could not grab pre-build '{path}':\n{err}" logger.warning(msg) if not allow_fail: - raise RuntimeError(msg) from err + raise FabSourceFetchError(path, err) from err diff --git a/source/fab/steps/grab/svn.py b/source/fab/steps/grab/svn.py index b49c4652f..5c37e1660 100644 --- a/source/fab/steps/grab/svn.py +++ b/source/fab/steps/grab/svn.py @@ -15,6 +15,7 @@ from fab.steps import step from fab.tools import Category, Versioning +from fab.errors import FabSourceMergeError def _get_revision(src, revision=None) -> Tuple[str, Union[str, None]]: @@ -124,6 +125,6 @@ def check_conflict(tool: Versioning, dst: Union[str, Path]): for element in entry: if (element.tag == 'wc-status' and element.attrib['item'] == 'conflicted'): - raise RuntimeError(f'{tool} merge encountered a ' - f'conflict:\n{xml_str}') + raise FabSourceMergeError("svn", xml_str) + return False diff --git a/source/fab/steps/preprocess.py b/source/fab/steps/preprocess.py index 8698a1eff..c8122eb16 100644 --- a/source/fab/steps/preprocess.py +++ b/source/fab/steps/preprocess.py @@ -22,6 +22,8 @@ from fab.util import (log_or_dot_finish, input_to_output_fpath, log_or_dot, suffix_filter, Timer, by_type) +from fab.errors import FabToolMismatch + logger = logging.getLogger(__name__) @@ -150,8 +152,7 @@ def preprocess_fortran(config: BuildConfig, source: Optional[ArtefactsGetter] = fpp = config.tool_box[Category.FORTRAN_PREPROCESSOR] if not isinstance(fpp, CppFortran): - raise RuntimeError(f"Unexpected tool '{fpp.name}' of type " - f"'{type(fpp)}' instead of CppFortran") + raise FabToolMismatch(fpp.name, type(fpp), "CppFortran") try: common_flags = kwargs.pop('common_flags') @@ -223,8 +224,7 @@ def preprocess_c(config: BuildConfig, source_files = source_getter(config.artefact_store) cpp = config.tool_box[Category.C_PREPROCESSOR] if not isinstance(cpp, Cpp): - raise RuntimeError(f"Unexpected tool '{cpp.name}' of type " - f"'{type(cpp)}' instead of Cpp") + raise FabToolMismatch(cpp.name, type(cpp), "Cpp") pre_processor( config, diff --git a/source/fab/steps/psyclone.py b/source/fab/steps/psyclone.py index 33ec0208b..815b414a3 100644 --- a/source/fab/steps/psyclone.py +++ b/source/fab/steps/psyclone.py @@ -29,6 +29,8 @@ file_walk, TimerLogger, string_checksum, suffix_filter, by_type, log_or_dot_finish) +from fab.errors import FabToolMismatch + logger = logging.getLogger(__name__) @@ -303,8 +305,8 @@ def do_one_file(arg: Tuple[Path, MpCommonArgs]): config = mp_payload.config psyclone = config.tool_box[Category.PSYCLONE] if not isinstance(psyclone, Psyclone): - raise RuntimeError(f"Unexpected tool '{psyclone.name}' of type " - f"'{type(psyclone)}' instead of Psyclone") + raise FabToolMismatch(psyclone.name, type(psyclone), "Psyclone") + try: transformation_script = mp_payload.transformation_script logger.info(f"running psyclone on '{x90_file}'.") diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 1a3d1c4fe..806714709 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -17,6 +17,7 @@ from fab.tools.category import Category from fab.tools.flags import Flags from fab.tools.tool import CompilerSuiteTool +from fab.errors import FabToolInvalidVersion, FabToolError if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -220,8 +221,8 @@ def get_version(self) -> Tuple[int, ...]: # of the string, otherwise the $ would not match the end of line matches = re.search(self._version_regex, output, re.MULTILINE) if not matches: - raise RuntimeError(f"Unexpected version output format for " - f"compiler '{self.name}': {output}") + raise FabToolInvalidVersion(self.name, output) + version_string = matches.groups()[0] # Expect the version to be dot-separated integers. try: @@ -229,15 +230,13 @@ def get_version(self) -> Tuple[int, ...]: version = cast(Tuple[int], tuple(int(x) for x in version_string.split('.'))) except ValueError as err: - raise RuntimeError(f"Unexpected version output format for " - f"compiler '{self.name}'. Should be numeric " - f": {version_string}") from err + raise FabToolInvalidVersion(self.name, version_string, + "") from err # Expect at least 2 integer components, i.e. major.minor[.patch, ...] if len(version) < 2: - raise RuntimeError(f"Unexpected version output format for " - f"compiler '{self.name}'. Should have at least " - f"two parts, : {version_string}") + raise FabToolInvalidVersion(self.name, version_string, + "should have at least two parts, ") self.logger.info( f'Found compiler version for {self.name} = {version_string}') @@ -259,8 +258,7 @@ def run_version_command( try: return self.run(version_command, capture_output=True) except RuntimeError as err: - raise RuntimeError(f"Error asking for version of compiler " - f"'{self.name}'") from err + raise FabToolError(self.name, "unable to get compiler version") from err def get_version_string(self) -> str: """ diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index d5c56f926..1d255efe8 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -14,6 +14,8 @@ from fab.tools.category import Category from fab.tools.compiler import Compiler, FortranCompiler from fab.tools.flags import Flags +from fab.errors import FabToolError + if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -68,8 +70,7 @@ def has_syntax_only(self) -> bool: if self._compiler.category == Category.FORTRAN_COMPILER: return cast(FortranCompiler, self._compiler).has_syntax_only - raise RuntimeError(f"Compiler '{self._compiler.name}' has " - f"no has_syntax_only.") + raise FabToolError(self._compiler.name, "no syntax-only feature") def get_flags(self, profile: Optional[str] = None) -> List[str]: ''':returns: the ProfileFlags for the given profile, combined @@ -90,8 +91,7 @@ def set_module_output_path(self, path: Path): ''' if self._compiler.category != Category.FORTRAN_COMPILER: - raise RuntimeError(f"Compiler '{self._compiler.name}' has no " - f"'set_module_output_path' function.") + raise FabToolError(self._compiler.name, "no module output path feature") cast(FortranCompiler, self._compiler).set_module_output_path(path) def get_all_commandline_options( @@ -141,8 +141,7 @@ def get_all_commandline_options( else: # It's not valid to specify syntax_only for a non-Fortran compiler if syntax_only is not None: - raise RuntimeError(f"Syntax-only cannot be used with compiler " - f"'{self.name}'.") + raise FabToolError(self._compiler.name, "syntax-only is Fortran-specific") flags = self._compiler.get_all_commandline_options( config, input_file, output_file, add_flags=add_flags) diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 03f58503f..cb6b00736 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -17,6 +17,7 @@ from fab.tools.compiler import Compiler from fab.tools.flags import ProfileFlags from fab.tools.tool import CompilerSuiteTool +from fab.errors import FabUnknownLibraryError if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -138,7 +139,7 @@ def get_lib_flags(self, lib: str) -> List[str]: # another linker, return the result from the wrapped linker if self._linker: return self._linker.get_lib_flags(lib) - raise RuntimeError(f"Unknown library name: '{lib}'") from err + raise FabUnknownLibraryError(lib) from err def add_lib_flags(self, lib: str, flags: List[str], silent_replace: bool = False): diff --git a/source/fab/tools/psyclone.py b/source/fab/tools/psyclone.py index 6e4adbbfc..091f522d9 100644 --- a/source/fab/tools/psyclone.py +++ b/source/fab/tools/psyclone.py @@ -14,6 +14,7 @@ from fab.tools.category import Category from fab.tools.tool import Tool +from fab.errors import FabToolPsycloneAPI, FabToolNotAvailable if TYPE_CHECKING: # TODO 314: see if this circular dependency can be broken @@ -90,7 +91,7 @@ def process(self, ''' if not self.is_available: - raise RuntimeError("PSyclone is not available.") + raise FabToolNotAvailable("psyclone") # Convert the old style API nemo to be empty if api and api.lower() == "nemo": @@ -100,24 +101,23 @@ def process(self, # API specified, we need both psy- and alg-file, but not # transformed file. if not psy_file: - raise RuntimeError(f"PSyclone called with api '{api}', but " - f"no psy_file is specified.") + raise FabToolPsycloneAPI(api, "psy_file") + if not alg_file: - raise RuntimeError(f"PSyclone called with api '{api}', but " - f"no alg_file is specified.") + raise FabToolPsycloneAPI(api, "alg_file") + if transformed_file: - raise RuntimeError(f"PSyclone called with api '{api}' and " - f"transformed_file.") + raise FabToolPsycloneAPI(api, "transformed_file", True) + else: if psy_file: - raise RuntimeError("PSyclone called without api, but " - "psy_file is specified.") + raise FabToolPsycloneAPI(api, "psy_file", True) + if alg_file: - raise RuntimeError("PSyclone called without api, but " - "alg_file is specified.") + raise FabToolPsycloneAPI(api, "alg_file", True) + if not transformed_file: - raise RuntimeError("PSyclone called without api, but " - "transformed_file is not specified.") + raise FabToolPsycloneAPI(api, "transformed_file") parameters: List[Union[str, Path]] = [] # If an api is defined in this call (or in the constructor) add it diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index a12773cc4..0ddfe3c29 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -20,6 +20,7 @@ from fab.tools.category import Category from fab.tools.flags import ProfileFlags +from fab.errors import FabToolNotAvailable, FabCommandError, FabCommandNotFound class Tool: @@ -193,23 +194,19 @@ def run(self, # is available or not. Testing for `False` only means this `run` # function can be used to test if a tool is available. if self._is_available is False: - raise RuntimeError(f"Tool '{self.name}' is not available to run " - f"'{command}'.") + raise FabToolNotAvailable(self.name) self._logger.debug(f'run_command: {" ".join(command)}') try: res = subprocess.run(command, capture_output=capture_output, env=env, cwd=cwd, check=False) except FileNotFoundError as err: - raise RuntimeError("Unable to execute command: " - + str(command)) from err + raise FabCommandNotFound(command) from err if res.returncode != 0: - msg = (f'Command failed with return code {res.returncode}:\n' - f'{command}') - if res.stdout: - msg += f'\n{res.stdout.decode()}' - if res.stderr: - msg += f'\n{res.stderr.decode()}' - raise RuntimeError(msg) + raise FabCommandError(command, + res.returncode, + res.stdout, + res.stderr, + cwd) if capture_output: return res.stdout.decode() return "" diff --git a/source/fab/tools/tool_box.py b/source/fab/tools/tool_box.py index 4dd95aa53..5c47acbfe 100644 --- a/source/fab/tools/tool_box.py +++ b/source/fab/tools/tool_box.py @@ -12,6 +12,7 @@ from fab.tools.category import Category from fab.tools.tool import Tool +from fab.errors import FabToolNotAvailable class ToolBox: @@ -37,7 +38,7 @@ def add_tool(self, tool: Tool, :raises RuntimeError: if the tool to be added is not available. ''' if not tool.is_available: - raise RuntimeError(f"Tool '{tool}' is not available.") + raise FabToolNotAvailable(tool) if tool.category in self._all_tools and not silent_replace: warnings.warn(f"Replacing existing tool " diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 3def7b7f4..8315e84ae 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -26,6 +26,8 @@ Gcc, Gfortran, Icc, Icx, Ifort, Ifx, Nvc, Nvfortran, Psyclone, Rsync, Shell) +from fab.errors import FabToolInvalidSetting, FabToolNotAvailable + class ToolRepository(dict): '''This class implements the tool repository. It stores a list of @@ -218,8 +220,7 @@ def set_default_compiler_suite(self, suite: str): self[category] = sorted(self[category], key=lambda x: x.suite != suite) if len(self[category]) > 0 and self[category][0].suite != suite: - raise RuntimeError(f"Cannot find '{category}' " - f"in the suite '{suite}'.") + raise FabToolNotAvailable(category, suite) def get_default(self, category: Category, mpi: Optional[bool] = None, @@ -243,8 +244,7 @@ def get_default(self, category: Category, ''' if not isinstance(category, Category): - raise RuntimeError(f"Invalid category type " - f"'{type(category).__name__}'.") + raise FabToolInvalidSetting("category type", type(category).__name__) # If not a compiler or linker, return the first tool if not category.is_compiler and category != Category.LINKER: @@ -252,16 +252,14 @@ def get_default(self, category: Category, if tool.is_available: return tool tool_names = ",".join(i.name for i in self[category]) - raise RuntimeError(f"Can't find available '{category}' tool. " - f"Tools are '{tool_names}'.") + raise FabToolInvalidSetting("category", category, + f"where tool names are {tool_names}") if not isinstance(mpi, bool): - raise RuntimeError(f"Invalid or missing mpi specification " - f"for '{category}'.") + raise FabToolInvalidSetting("MPI setting", category) if not isinstance(openmp, bool): - raise RuntimeError(f"Invalid or missing openmp specification " - f"for '{category}'.") + raise FabToolInvalidSetting("OpenMP setting", category) for tool in self[category]: # If OpenMP is request, but the tool does not support openmp, @@ -276,12 +274,11 @@ def get_default(self, category: Category, # that seems to be an unlikely scenario. if mpi: if openmp: - raise RuntimeError(f"Could not find '{category}' that " - f"supports MPI and OpenMP.") - raise RuntimeError(f"Could not find '{category}' that " - f"supports MPI.") + raise FabToolInvalidSetting("MPI and OpenMP setting", category) + + raise FabToolInvalidSetting("MPI setting", category) if openmp: - raise RuntimeError(f"Could not find '{category}' that " - f"supports OpenMP.") - raise RuntimeError(f"Could not find any '{category}'.") + raise FabToolInvalidSetting("OpenMP setting", category) + + raise FabToolInvalidSetting("category match", category) diff --git a/source/fab/tools/versioning.py b/source/fab/tools/versioning.py index 410cc250f..de12bc747 100644 --- a/source/fab/tools/versioning.py +++ b/source/fab/tools/versioning.py @@ -12,6 +12,7 @@ from fab.tools.category import Category from fab.tools.tool import Tool +from fab.errors import FabSourceMergeError class Versioning(Tool, ABC): @@ -108,8 +109,7 @@ def merge(self, dst: Union[str, Path], self.run(['merge', 'FETCH_HEAD'], cwd=dst, capture_output=False) except RuntimeError as err: self.run(['merge', '--abort'], cwd=dst, capture_output=False) - raise RuntimeError(f"Error merging {revision}. " - f"Merge aborted.\n{err}") from err + raise FabSourceMergeError("git", str(err), revision) from err # ============================================================================= diff --git a/tests/unit_tests/steps/test_archive_objects.py b/tests/unit_tests/steps/test_archive_objects.py index 843da2ef5..b06d901c0 100644 --- a/tests/unit_tests/steps/test_archive_objects.py +++ b/tests/unit_tests/steps/test_archive_objects.py @@ -19,6 +19,8 @@ from fab.steps.archive_objects import archive_objects from fab.tools import Category, ToolRepository +from fab.errors import FabToolMismatch + class TestArchiveObjects: """ @@ -116,6 +118,6 @@ def test_incorrect_tool(self, stub_tool_box, monkeypatch): with raises(RuntimeError) as err: archive_objects(config=config, output_fpath=config.build_output / 'mylib.a') - assert str(err.value) == ("Unexpected tool 'some C compiler' of type " - "'' " - "instead of Ar") + assert isinstance(err.value, FabToolMismatch) + assert str(err.value) == ("[some C compiler] got type " + " instead of Ar") diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py index 7b4399637..dbe042f83 100644 --- a/tests/unit_tests/steps/test_compile_c.py +++ b/tests/unit_tests/steps/test_compile_c.py @@ -20,6 +20,8 @@ from fab.tools.flags import Flags from fab.tools.tool_box import ToolBox +from fab.errors import FabToolMismatch + @fixture(scope='function') def content(tmp_path: Path, stub_tool_box: ToolBox): @@ -62,8 +64,9 @@ def test_compile_c_wrong_compiler(content, fake_process: FakeProcess) -> None: mp_common_args = Mock(config=config) with raises(RuntimeError) as err: _compile_file((Mock(), mp_common_args)) - assert str(err.value) == ("Unexpected tool 'some C compiler' of category " - "'FORTRAN_COMPILER' instead of CCompiler") + assert isinstance(err.value, FabToolMismatch) + assert str(err.value) == ("[some C compiler] got type " + "FORTRAN_COMPILER instead of CCompiler") # This is more of an integration test than a unit test diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py index a991b97dd..2f55ce6d7 100644 --- a/tests/unit_tests/steps/test_compile_fortran.py +++ b/tests/unit_tests/steps/test_compile_fortran.py @@ -18,6 +18,8 @@ from fab.tools.tool_box import ToolBox from fab.util import CompiledFile +from fab.errors import FabToolMismatch + @fixture(scope='function') def analysed_files(): @@ -60,15 +62,17 @@ def test_compile_cc_wrong_compiler(stub_tool_box, mp_common_args = Mock(config=config) with raises(RuntimeError) as err: process_file((Mock(), mp_common_args)) + assert isinstance(err.value, FabToolMismatch) assert str(err.value) \ - == "Unexpected tool 'some Fortran compiler' of category " \ - + "'C_COMPILER' instead of FortranCompiler" + == "[some Fortran compiler] got type " \ + + "C_COMPILER instead of FortranCompiler" with raises(RuntimeError) as err: handle_compiler_args(config) + assert isinstance(err.value, FabToolMismatch) assert str(err.value) \ - == "Unexpected tool 'some Fortran compiler' of category " \ - + "'C_COMPILER' instead of FortranCompiler" + == "[some Fortran compiler] got type " \ + + "C_COMPILER instead of FortranCompiler" class TestCompilePass: diff --git a/tests/unit_tests/steps/test_preprocess.py b/tests/unit_tests/steps/test_preprocess.py index bd5786d20..47b2c4d9b 100644 --- a/tests/unit_tests/steps/test_preprocess.py +++ b/tests/unit_tests/steps/test_preprocess.py @@ -13,6 +13,8 @@ from fab.tools.category import Category from fab.tools.tool_box import ToolBox +from fab.errors import FabToolMismatch + class Test_preprocess_fortran: @@ -68,5 +70,6 @@ def test_wrong_exe(self, tmp_path: Path, config = BuildConfig('proj', tool_box, fab_workspace=tmp_path) with raises(RuntimeError) as err: preprocess_fortran(config=config) - assert str(err.value) == "Unexpected tool 'cpp' of type '' instead of CppFortran" + assert isinstance(err.value, FabToolMismatch) + assert str(err.value) == "[cpp] got type instead of CppFortran" diff --git a/tests/unit_tests/test_artefacts.py b/tests/unit_tests/test_artefacts.py index b253362cb..a12b5a690 100644 --- a/tests/unit_tests/test_artefacts.py +++ b/tests/unit_tests/test_artefacts.py @@ -84,11 +84,10 @@ def test_artefact_store_replace() -> None: Path("c")]) # Test the behaviour for dictionaries - with pytest.raises(RuntimeError) as err: + with pytest.raises(ValueError) as err: artefact_store.replace(ArtefactSet.OBJECT_FILES, remove_files=[Path("a")], add_files=["c"]) - assert ("Replacing artefacts in dictionary 'ArtefactSet.OBJECT_FILES' " - "is not supported" in str(err.value)) + assert str(err.value) == "OBJECT_FILES is not mutable" def test_artefacts_getter(): diff --git a/tests/unit_tests/test_cui_arguments.py b/tests/unit_tests/test_cui_arguments.py index 8e86cf211..6a95dca98 100644 --- a/tests/unit_tests/test_cui_arguments.py +++ b/tests/unit_tests/test_cui_arguments.py @@ -101,7 +101,7 @@ def test_nonexistent_user_file(self, fs: FakeFilesystem, capsys): assert exc.value.code == 2 captured = capsys.readouterr() - assert "error: fab file does not exist" in captured.err + assert "fab file does not exist" in captured.err def test_user_file_missing_arg(self, capsys): """Check missing file name triggers an error.""" diff --git a/tests/unit_tests/test_errors.py b/tests/unit_tests/test_errors.py new file mode 100644 index 000000000..b15aa9d6e --- /dev/null +++ b/tests/unit_tests/test_errors.py @@ -0,0 +1,182 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## +""" +Unit tests for custom fab exceptions. +""" + +import pytest +from unittest.mock import Mock, PropertyMock + +from fab.errors import ( + FabError, + FabToolError, + FabToolMismatch, + FabToolInvalidVersion, + FabToolPsycloneAPI, + FabToolNotAvailable, + FabToolInvalidSetting, + FabCommandError, + FabCommandNotFound, + FabMultiCommandError, + FabSourceNoFilesError, + FabSourceMergeError, + FabSourceFetchError, + FabUnknownLibraryError, +) + +from fab.tools.category import Category +from fab.tools.tool import Tool + + +class TestErrors: + """Basic tests for the FabError class hierarchy.""" + + def test_base(self): + """Test the base Fab error class.""" + + err = FabError("test message") + assert str(err) == "test message" + + def test_unknown_library(self): + """Test unknown library errors.""" + + err = FabUnknownLibraryError("mylib") + assert str(err) == "unknown library mylib" + + +class TestToolErrors: + """Test the FabToolError hierarchy.""" + + def test_tool_string(self): + """Test the base FabToolError class.""" + + err = FabToolError("cc", "compiler message") + assert str(err) == "[cc] compiler message" + + # Mock defines an internal name property, so special handling + # is required to reset the value to support testing + tool = Mock(_name="tool cc") + del tool.name + err = FabToolError(tool, "compiler message") + assert str(err) == "[tool cc] compiler message" + + category = Mock() + del category._name + type(category).name = PropertyMock(return_value="category cc") + err = FabToolError(category, "compiler message") + assert str(err) == "[category cc] compiler message" + + def test_mismatch(self): + """Test tool type mismatch class.""" + + err = FabToolMismatch("cc", "CCompiler", "Ar") + assert str(err) == "[cc] got type CCompiler instead of Ar" + + def test_invalid_version(self): + """Test invalid version class.""" + + err = FabToolInvalidVersion("cc", "abc") + assert str(err) == "[cc] invalid version 'abc'" + + err = FabToolInvalidVersion("cc", "abc", "VV.NN") + assert str(err) == "[cc] invalid version 'abc' should be 'VV.NN'" + + def test_psyclone_api(self): + """Test PSyclone API class.""" + + err = FabToolPsycloneAPI(None, "alg_file") + assert str(err) == "[psyclone] called without API but not with alg_file" + + err = FabToolPsycloneAPI("nemo", "alg_file") + assert str(err) == "[psyclone] called with nemo API but not with alg_file" + + err = FabToolPsycloneAPI("nemo", "alg_file", present=True) + assert str(err) == "[psyclone] called with nemo API and with alg_file" + + def test_not_available(self): + """Test tool not available class.""" + + err = FabToolNotAvailable("psyclone") + assert str(err) == "[psyclone] not available" + + err = FabToolNotAvailable("gfortran", "GCC") + assert str(err) == "[gfortran] not available in suite GCC" + + def test_invalid_setting(self): + """Test invalid setting class.""" + + err = FabToolInvalidSetting("category", "compiler") + assert str(err) == "[compiler] invalid category" + + err = FabToolInvalidSetting("category", "compiler", "nosuch") + assert str(err) == "[compiler] invalid category nosuch" + + +class TestCommandErrors: + """Test various command errors.""" + + def test_command(self): + """Test FabCommandError in various configurations.""" + + err = FabCommandError(["ls", "-l", "/nosuch"], 1, b"", b"ls: cannot", "/") + assert str(err) == "command 'ls -l /nosuch' returned 1" + + err = FabCommandError("ls -l /nosuch", 1, None, "ls: cannot", "/") + assert str(err) == "command 'ls -l /nosuch' returned 1" + + def test_not_found(self): + """Test command not found errors.""" + + err = FabCommandNotFound(["ls", "-l"]) + assert str(err) == "unable to execute ls" + + err = FabCommandNotFound("ls -l") + assert str(err) == "unable to execute ls" + + with pytest.raises(ValueError) as exc: + FabCommandNotFound({"a": 1}) + assert "invalid command" in str(exc.value) + + def test_multi(self): + """Test multiprocessing command errors.""" + + err = FabMultiCommandError([ValueError("invalid value")]) + assert str(err) == "1 exception during multiprocessing" + + err = FabMultiCommandError( + [ValueError("invalid value"), TypeError("invalid type")] + ) + assert str(err) == "2 exceptions during multiprocessing" + + err = FabMultiCommandError( + [ValueError("invalid value"), TypeError("invalid type")], "during psyclone" + ) + assert str(err) == "2 exceptions during psyclone" + + +class TestSourceErrors: + """Test the source errors hierarchy.""" + + def test_no_files(self): + """Test lack of source files.""" + + err = FabSourceNoFilesError() + assert str(err) == "no source files found after filtering" + + def test_merge(self): + """Test merge errors.""" + + err = FabSourceMergeError("git", "conflicting source files") + assert str(err) == "[git] merge failed: conflicting source files" + + err = FabSourceMergeError("git", "conflicting source files", "vn1.1") + assert str(err) == "[git] merge of vn1.1 failed: conflicting source files" + + def test_fetch(self): + """Test fetch errors.""" + + err = FabSourceFetchError("/my/dir1", "no such directory") + assert str(err) == "could not fetch /my/dir1: no such directory" diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 13b3827d5..33481fc14 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -22,6 +22,8 @@ Icx, Ifx, Nvc, Nvfortran) +from fab.errors import FabToolInvalidVersion, FabToolError + from tests.conftest import arg_list, call_list @@ -104,7 +106,7 @@ def test_compiler_check_available_runtime_error(): ''' Check the compiler is not available when get_version raises an error. ''' cc = Gcc() - with mock.patch.object(cc, "get_version", side_effect=RuntimeError("")): + with mock.patch.object(cc, "get_version", side_effect=FabToolInvalidVersion("cc", "")): assert not cc.check_available() @@ -131,10 +133,10 @@ def test_compiler_hash_compiler_error(): cc = Gcc() # raise an error when trying to get compiler version - with mock.patch.object(cc, 'run', side_effect=RuntimeError()): - with raises(RuntimeError) as err: + with mock.patch.object(cc, 'run', side_effect=FabToolError("hash", "")): + with raises(FabToolError) as err: cc.get_hash() - assert "Error asking for version of compiler" in str(err.value) + assert "unable to get compiler version" in str(err.value) def test_compiler_hash_invalid_version(): @@ -145,8 +147,8 @@ def test_compiler_hash_invalid_version(): with mock.patch.object(cc, "run", mock.Mock(return_value='foo v1')): with raises(RuntimeError) as err: cc.get_hash() - assert ("Unexpected version output format for compiler 'gcc'" - in str(err.value)) + assert isinstance(err.value, FabToolInvalidVersion) + assert "[gcc] invalid version" in str(err.value) def test_compiler_syntax_only(): @@ -294,12 +296,13 @@ def test_get_version_1_part_version(): GNU Fortran (gcc) 777 Copyright (C) 2022 Foo Software Foundation, Inc. """) - expected_error = "Unexpected version output format for compiler" + expected_error = "invalid version" c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: c.get_version() + assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -354,12 +357,13 @@ def test_get_version_non_int_version_format(version): GNU Fortran (gcc) {version} (Foo Hat 4.8.5) Copyright (C) 2022 Foo Software Foundation, Inc. """) - expected_error = "Unexpected version output format for compiler" + expected_error = "invalid version" c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: c.get_version() + assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -372,12 +376,13 @@ def test_get_version_unknown_version_format(): full_output = dedent(""" Foo Fortran version 175 """) - expected_error = "Unexpected version output format for compiler" + expected_error = "invalid version" c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: c.get_version() + assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -386,19 +391,21 @@ def test_get_version_command_failure(): c = Gfortran(exec_name="does_not_exist") with raises(RuntimeError) as err: c.get_version() - assert "Error asking for version of compiler" in str(err.value) + assert isinstance(err.value, FabToolError) + assert "unable to get compiler version" in str(err.value) def test_get_version_unknown_command_response(): '''If the full version output is in an unknown format, we must raise an error.''' full_output = 'GNU Fortran 1.2.3' - expected_error = "Unexpected version output format for compiler" + expected_error = "invalid version" c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: c.get_version() + assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -414,7 +421,7 @@ def test_get_version_good_result_is_cached(): # Now let the run method raise an exception, to make sure we get a cached # value back (and the run method isn't called again): - with mock.patch.object(c, 'run', side_effect=RuntimeError()): + with mock.patch.object(c, 'run', side_effect=FabToolError("cc", "")): assert c.get_version() == expected assert not c.run.called @@ -424,7 +431,7 @@ def test_get_version_bad_result_is_not_cached(): ''' # Set up the compiler to fail the first time c = Gfortran() - with mock.patch.object(c, 'run', side_effect=RuntimeError()): + with mock.patch.object(c, 'run', side_effect=RuntimeError("")): with raises(RuntimeError): c.get_version() @@ -469,7 +476,8 @@ def test_gcc_get_version_with_icc_string(): with mock.patch.object(gcc, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: gcc.get_version() - assert "Unexpected version output format for compiler" in str(err.value) + assert isinstance(err.value, FabToolInvalidVersion) + assert "invalid version" in str(err.value) # ============================================================================ @@ -575,7 +583,8 @@ def test_gfortran_get_version_with_ifort_string(): mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: gfortran.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -613,7 +622,8 @@ def test_icc_get_version_with_gcc_string(): with mock.patch.object(icc, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: icc.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -688,7 +698,8 @@ def test_ifort_get_version_with_icc_string(): with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: ifort.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -708,7 +719,8 @@ def test_ifort_get_version_invalid_version(version): with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: ifort.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -751,7 +763,8 @@ def test_icx_get_version_with_icc_string(): with mock.patch.object(icx, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: icx.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -790,7 +803,8 @@ def test_ifx_get_version_with_ifort_string(): with mock.patch.object(ifx, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: ifx.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -829,7 +843,8 @@ def test_nvc_get_version_with_icc_string(): with mock.patch.object(nvc, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: nvc.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -871,7 +886,8 @@ def test_nvfortran_get_version_with_ifort_string(): mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: nvfortran.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -937,7 +953,8 @@ def test_craycc_get_version_with_icc_string(): with mock.patch.object(craycc, "run", mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: craycc.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) @@ -987,5 +1004,6 @@ def test_crayftn_get_version_with_ifort_string(): mock.Mock(return_value=full_output)): with raises(RuntimeError) as err: crayftn.get_version() - assert ("Unexpected version output format for compiler" + assert isinstance(err.value, FabToolInvalidVersion) + assert ("invalid version" in str(err.value)) diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 0109d82ce..ff3eeef4e 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -19,6 +19,7 @@ from fab.tools.compiler_wrapper import (CompilerWrapper, CrayCcWrapper, CrayFtnWrapper, Mpicc, Mpif90) +from fab.errors import FabToolError def test_compiler_getter(stub_c_compiler: CCompiler) -> None: @@ -141,8 +142,8 @@ def test_syntax_only(stub_c_compiler: CCompiler) -> None: mpicc = Mpicc(stub_c_compiler) with raises(RuntimeError) as err: _ = mpicc.has_syntax_only - assert (str(err.value) == "Compiler 'some C compiler' has no " - "has_syntax_only.") + assert isinstance(err.value, FabToolError) + assert str(err.value) == "[some C compiler] no syntax-only feature" def test_module_output(stub_fortran_compiler: FortranCompiler, @@ -162,8 +163,8 @@ def test_module_output(stub_fortran_compiler: FortranCompiler, mpicc = Mpicc(stub_c_compiler) with raises(RuntimeError) as err: mpicc.set_module_output_path(Path("/tmp")) - assert str(err.value) == ("Compiler 'some C compiler' has " - "no 'set_module_output_path' function.") + assert isinstance(err.value, FabToolError) + assert str(err.value) == "[some C compiler] no module output path feature" def test_fortran_with_add_args(stub_fortran_compiler: FortranCompiler, @@ -233,8 +234,8 @@ def test_c_with_add_args(stub_c_compiler: CCompiler, mpicc.compile_file(Path("a.f90"), Path('a.o'), add_flags=["-O3"], syntax_only=True, config=stub_configuration) - assert (str(err.value) == "Syntax-only cannot be used with compiler " - "'mpicc-some C compiler'.") + assert isinstance(err.value, FabToolError) + assert (str(err.value) == "[some C compiler] syntax-only is Fortran-specific") # Check that providing the openmp flag in add_flag raises a warning: with warns(UserWarning, diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index c479f0e7c..aea8eea23 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -124,7 +124,7 @@ def test_get_lib_flags_unknown(stub_c_compiler: CCompiler) -> None: linker = Linker(compiler=stub_c_compiler) with raises(RuntimeError) as err: linker.get_lib_flags("unknown") - assert str(err.value) == "Unknown library name: 'unknown'" + assert str(err.value) == "unknown library unknown" def test_add_lib_flags(stub_c_compiler: CCompiler) -> None: @@ -263,7 +263,7 @@ def test_c_with_unknown_library(stub_c_compiler: CCompiler, # Try to use "customlib" when we haven't added it to the linker linker.link([Path("a.o")], Path("a.out"), libs=["customlib"], config=stub_configuration) - assert str(err.value) == "Unknown library name: 'customlib'" + assert str(err.value) == "unknown library customlib" def test_add_compiler_flag(stub_c_compiler: CCompiler, @@ -363,7 +363,7 @@ def test_linker_inheriting() -> None: with raises(RuntimeError) as err: wrapper_linker.get_lib_flags("does_not_exist") - assert str(err.value) == "Unknown library name: 'does_not_exist'" + assert str(err.value) == "unknown library does_not_exist" def test_linker_profile_flags_inheriting(stub_c_compiler): diff --git a/tests/unit_tests/tools/test_psyclone.py b/tests/unit_tests/tools/test_psyclone.py index da02dfa10..e29eb4fa4 100644 --- a/tests/unit_tests/tools/test_psyclone.py +++ b/tests/unit_tests/tools/test_psyclone.py @@ -19,6 +19,8 @@ import fab.tools.psyclone # Needed for mockery from fab.tools.psyclone import Psyclone +from fab.errors import FabToolPsycloneAPI, FabToolNotAvailable + from tests.conftest import call_list, not_found_callback @@ -108,7 +110,8 @@ def test_check_process_missing(fake_process: FakeProcess) -> None: with raises(RuntimeError) as err: psyclone.process(config, Path("x90file")) - assert str(err.value).startswith("PSyclone is not available") + assert isinstance(err.value, FabToolNotAvailable) + assert str(err.value).startswith("[psyclone] not available") def test_processing_errors_without_api(fake_process: FakeProcess) -> None: @@ -126,23 +129,23 @@ def test_processing_errors_without_api(fake_process: FakeProcess) -> None: Path('x90file'), api=None, psy_file=Path('psy_file')) - assert (str(err.value) == "PSyclone called without api, but psy_file " - "is specified.") + assert isinstance(err.value, FabToolPsycloneAPI) + assert (str(err.value) == "[psyclone] called without API and with psy_file") with raises(RuntimeError) as err: psyclone.process(config, Path('x90file'), api=None, alg_file=Path('alg_file')) - assert (str(err.value) == "PSyclone called without api, but alg_file is " - "specified.") + assert isinstance(err.value, FabToolPsycloneAPI) + assert (str(err.value) == "[psyclone] called without API and with alg_file") with raises(RuntimeError) as err: psyclone.process(config, Path('x90file'), api=None) - assert (str(err.value) == "PSyclone called without api, but " - "transformed_file is not specified.") + assert isinstance(err.value, FabToolPsycloneAPI) + assert (str(err.value) == "[psyclone] called without API but not with transformed_file") @mark.parametrize("api", ["dynamo0.3", "lfric"]) @@ -162,16 +165,18 @@ def test_processing_errors_with_api(api: str, Path("x90file"), api=api, psy_file=Path("psy_file")) + assert isinstance(err.value, FabToolPsycloneAPI) assert str(err.value).startswith( - f"PSyclone called with api '{api}', but no alg_file is specified" + f"[psyclone] called with {api} API but not with alg_file" ) with raises(RuntimeError) as err: psyclone.process(config, Path("x90file"), api=api, alg_file=Path("alg_file")) + assert isinstance(err.value, FabToolPsycloneAPI) assert str(err.value).startswith( - f"PSyclone called with api '{api}', but no psy_file is specified" + f"[psyclone] called with {api} API but not with psy_file" ) with raises(RuntimeError) as err: psyclone.process(config, @@ -180,8 +185,9 @@ def test_processing_errors_with_api(api: str, psy_file=Path("psy_file"), alg_file=Path("alg_file"), transformed_file=Path("transformed_file")) + assert isinstance(err.value, FabToolPsycloneAPI) assert str(err.value).startswith( - f"PSyclone called with api '{api}' and transformed_file" + f"[psyclone] called with {api} API and with transformed_file" ) diff --git a/tests/unit_tests/tools/test_tool.py b/tests/unit_tests/tools/test_tool.py index bbfa97380..f37e955b3 100644 --- a/tests/unit_tests/tools/test_tool.py +++ b/tests/unit_tests/tools/test_tool.py @@ -18,6 +18,8 @@ from fab.tools.flags import ProfileFlags from fab.tools.tool import CompilerSuiteTool, Tool +from fab.errors import FabCommandError, FabCommandNotFound + def test_constructor() -> None: """ @@ -89,8 +91,7 @@ def test_is_not_available(fake_process: FakeProcess) -> None: # an exception now: with raises(RuntimeError) as err: tool.run("--ops") - assert ("Tool 'gfortran' is not available to run '['gfortran', '--ops']" - in str(err.value)) + assert ("[gfortran] not available" in str(err.value)) def test_availability_argument(fake_process: FakeProcess) -> None: @@ -112,9 +113,8 @@ def test_run_missing(fake_process: FakeProcess) -> None: tool = Tool("some tool", "stool", Category.MISC) with raises(RuntimeError) as err: tool.run("--ops") - assert str(err.value).startswith( - "Unable to execute command: ['stool', '--ops']" - ) + assert isinstance(err.value, FabCommandNotFound) + assert str(err.value) == "unable to execute stool" # Check that stdout and stderr is returned fake_process.register(['stool', '--ops'], returncode=1, @@ -123,8 +123,9 @@ def test_run_missing(fake_process: FakeProcess) -> None: tool = Tool("some tool", "stool", Category.MISC) with raises(RuntimeError) as err: tool.run("--ops") - assert "this is stdout" in str(err.value) - assert "this is stderr" in str(err.value) + assert isinstance(err.value, FabCommandError) + assert "this is stdout" in str(err.value.output) + assert "this is stderr" in str(err.value.error) def test_tool_flags_no_profile() -> None: @@ -204,8 +205,12 @@ def test_error(self, fake_process: FakeProcess) -> None: tool = Tool("some tool", "tool", Category.MISC) with raises(RuntimeError) as err: tool.run() - assert str(err.value) == ("Command failed with return code 1:\n" - "['tool']\nBeef.") + assert isinstance(err.value, FabCommandError) + assert str(err.value) == "command 'tool' returned 1" + assert err.value.code == 1 + assert err.value.output == "Beef." + assert err.value.error == "" + assert call_list(fake_process) == [['tool']] def test_error_file_not_found(self, fake_process: FakeProcess) -> None: @@ -216,7 +221,8 @@ def test_error_file_not_found(self, fake_process: FakeProcess) -> None: tool = Tool('some tool', 'tool', Category.MISC) with raises(RuntimeError) as err: tool.run() - assert str(err.value) == "Unable to execute command: ['tool']" + assert isinstance(err.value, FabCommandNotFound) + assert str(err.value) == "unable to execute tool" assert call_list(fake_process) == [['tool']] diff --git a/tests/unit_tests/tools/test_tool_box.py b/tests/unit_tests/tools/test_tool_box.py index e777139ca..24e9738cb 100644 --- a/tests/unit_tests/tools/test_tool_box.py +++ b/tests/unit_tests/tools/test_tool_box.py @@ -17,6 +17,7 @@ from fab.tools.compiler import CCompiler, Gfortran from fab.tools.tool_box import ToolBox from fab.tools.tool_repository import ToolRepository +from fab.errors import FabToolNotAvailable def test_constructor() -> None: @@ -88,4 +89,5 @@ def test_add_unavailable_tool(fake_process: FakeProcess) -> None: gfortran = Gfortran() with raises(RuntimeError) as err: tb.add_tool(gfortran) - assert str(err.value).startswith(f"Tool '{gfortran}' is not available") + assert isinstance(err.value, FabToolNotAvailable) + assert str(err.value).startswith(f"[gfortran] not available") diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index af2ca8a26..e9d5f456a 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -16,6 +16,8 @@ from fab.tools.compiler_wrapper import Mpif90 from fab.tools.tool_repository import ToolRepository +from fab.errors import FabToolInvalidSetting, FabToolNotAvailable + def test_tool_repository_get_singleton_new(): '''Tests the singleton behaviour.''' @@ -128,7 +130,8 @@ def test_get_default_error_invalid_category() -> None: tr = ToolRepository() with raises(RuntimeError) as err: tr.get_default("unknown-category-type") # type: ignore[arg-type] - assert "Invalid category type 'str'." in str(err.value) + assert isinstance(err.value, FabToolInvalidSetting) + assert "[str] invalid category" in str(err.value) def test_get_default_error_missing_mpi() -> None: @@ -139,13 +142,13 @@ def test_get_default_error_missing_mpi() -> None: tr = ToolRepository() with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, openmp=True) - assert str(err.value) == ("Invalid or missing mpi specification " - "for 'FORTRAN_COMPILER'.") + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[FORTRAN_COMPILER] invalid MPI setting" with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=True) - assert str(err.value) == ("Invalid or missing openmp specification " - "for 'FORTRAN_COMPILER'.") + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" def test_get_default_error_missing_openmp() -> None: @@ -157,23 +160,24 @@ def test_get_default_error_missing_openmp() -> None: with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=True) - assert ("Invalid or missing openmp specification for 'FORTRAN_COMPILER'" - in str(err.value)) + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" + with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=True, openmp='123') # type: ignore[arg-type] - assert str(err.value) == ("Invalid or missing openmp specification " - "for 'FORTRAN_COMPILER'.") + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" @mark.parametrize("mpi, openmp, message", - [(False, False, "any 'FORTRAN_COMPILER'."), + [(False, False, "invalid category match"), (False, True, - "'FORTRAN_COMPILER' that supports OpenMP."), + "[FORTRAN_COMPILER] invalid OpenMP setting"), (True, False, - "'FORTRAN_COMPILER' that supports MPI."), - (True, True, "'FORTRAN_COMPILER' that supports MPI " - "and OpenMP.")]) + "[FORTRAN_COMPILER] invalid MPI setting"), + (True, True, "[FORTRAN_COMPILER] invalid MPI " + "and OpenMP setting")]) def test_get_default_error_missing_compiler(mpi, openmp, message, monkeypatch) -> None: """ @@ -185,7 +189,8 @@ def test_get_default_error_missing_compiler(mpi, openmp, message, with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=mpi, openmp=openmp) - assert str(err.value) == f"Could not find {message}" + assert isinstance(err.value, FabToolInvalidSetting) + assert message in str(err.value) def test_get_default_error_missing_openmp_compiler(monkeypatch) -> None: @@ -204,8 +209,8 @@ def test_get_default_error_missing_openmp_compiler(monkeypatch) -> None: with raises(RuntimeError) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=False, openmp=True) - assert (str(err.value) == "Could not find 'FORTRAN_COMPILER' that " - "supports OpenMP.") + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" @mark.parametrize('category', [Category.C_COMPILER, @@ -249,8 +254,8 @@ def test_default_suite_unknown() -> None: repo = ToolRepository() with raises(RuntimeError) as err: repo.set_default_compiler_suite("does-not-exist") - assert str(err.value) == ("Cannot find 'FORTRAN_COMPILER' in " - "the suite 'does-not-exist'.") + assert isinstance(err.value, FabToolNotAvailable) + assert str(err.value) == "[FORTRAN_COMPILER] not available in suite does-not-exist" def test_no_tool_available(fake_process: FakeProcess) -> None: @@ -266,8 +271,8 @@ def test_no_tool_available(fake_process: FakeProcess) -> None: with raises(RuntimeError) as err: tr.get_default(Category.SHELL) - assert (str(err.value) == "Can't find available 'SHELL' tool. Tools are " - "'sh'.") + assert isinstance(err.value, FabToolInvalidSetting) + assert str(err.value) == "[SHELL] invalid category where tool names are sh" def test_tool_repository_full_path(fake_process: FakeProcess) -> None: diff --git a/tests/unit_tests/tools/test_versioning.py b/tests/unit_tests/tools/test_versioning.py index bf9f41e27..e49ee4af6 100644 --- a/tests/unit_tests/tools/test_versioning.py +++ b/tests/unit_tests/tools/test_versioning.py @@ -22,6 +22,8 @@ from fab.tools.category import Category from fab.tools.versioning import Fcm, Git, Subversion +from fab.errors import FabCommandError + class TestGit: """ @@ -132,7 +134,8 @@ def test_git_fetch_error(self, fake_process: FakeProcess) -> None: git = Git() with raises(RuntimeError) as err: git.fetch("/src", "/dst", revision="revision") - assert str(err.value).startswith("Command failed with return code 1:") + assert isinstance(err.value, FabCommandError) + assert str(err.value) == "command 'git fetch /src revision' returned 1" assert call_list(fake_process) == [ ['git', 'fetch', "/src", "revision"] ] @@ -164,7 +167,8 @@ def test_git_checkout_error(self, fake_process: FakeProcess) -> None: git = Git() with raises(RuntimeError) as err: git.checkout("/src", "/dst", revision="revision") - assert str(err.value).startswith("Command failed with return code 1:") + assert isinstance(err.value, FabCommandError) + assert str(err.value) == "command 'git fetch /src revision' returned 1" assert call_list(fake_process) == [ ['git', 'fetch', "/src", "revision"] ] @@ -195,7 +199,7 @@ def test_git_merge_error(self, fake_process: FakeProcess) -> None: with raises(RuntimeError) as err: git.merge("/dst", revision="revision") assert str(err.value).startswith( - "Error merging revision. Merge aborted." + "[git] merge of revision failed:" ) assert call_list(fake_process) == [ ['git', 'merge', 'FETCH_HEAD'], @@ -216,7 +220,7 @@ def test_git_merge_collapse(self, fake_process: FakeProcess) -> None: git = Git() with raises(RuntimeError) as err: git.merge("/dst", revision="revision") - assert str(err.value).startswith("Command failed with return code 1:") + assert str(err.value).startswith("command 'git merge") assert call_list(fake_process) == [ ['git', 'merge', 'FETCH_HEAD'], ['git', 'merge', '--abort'] From 7e378ddac3f93cc3539f7249a2c13cc8535dd771 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Fri, 22 Aug 2025 16:25:41 +0100 Subject: [PATCH 2/7] Fix flake8 problems Some minor flake8 problems had slipped in. This fixes them. --- source/fab/steps/archive_objects.py | 2 +- tests/unit_tests/test_errors.py | 3 --- tests/unit_tests/tools/test_tool_box.py | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/source/fab/steps/archive_objects.py b/source/fab/steps/archive_objects.py index a24a2418d..32ac73bcc 100644 --- a/source/fab/steps/archive_objects.py +++ b/source/fab/steps/archive_objects.py @@ -19,7 +19,7 @@ from fab.tools import Ar, Category from fab.artefacts import ArtefactsGetter, CollectionGetter -from fab.errors import FabToolMismatch, FabCommandError +from fab.errors import FabToolMismatch logger = logging.getLogger(__name__) diff --git a/tests/unit_tests/test_errors.py b/tests/unit_tests/test_errors.py index b15aa9d6e..4d58e4dc3 100644 --- a/tests/unit_tests/test_errors.py +++ b/tests/unit_tests/test_errors.py @@ -27,9 +27,6 @@ FabUnknownLibraryError, ) -from fab.tools.category import Category -from fab.tools.tool import Tool - class TestErrors: """Basic tests for the FabError class hierarchy.""" diff --git a/tests/unit_tests/tools/test_tool_box.py b/tests/unit_tests/tools/test_tool_box.py index 24e9738cb..eee77cf73 100644 --- a/tests/unit_tests/tools/test_tool_box.py +++ b/tests/unit_tests/tools/test_tool_box.py @@ -90,4 +90,4 @@ def test_add_unavailable_tool(fake_process: FakeProcess) -> None: with raises(RuntimeError) as err: tb.add_tool(gfortran) assert isinstance(err.value, FabToolNotAvailable) - assert str(err.value).startswith(f"[gfortran] not available") + assert str(err.value).startswith("[gfortran] not available") From e420c7d2c12850db0bcb921fbc61a255c8b5ded4 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Tue, 21 Oct 2025 14:20:05 +0100 Subject: [PATCH 3/7] Remove old FabException references --- source/fab/__init__.py | 4 ---- source/fab/errors.py | 13 +++++++++++++ source/fab/parse/fortran_common.py | 5 ++--- source/fab/steps/analyse.py | 4 ++-- source/fab/steps/c_pragma_injector.py | 7 +++---- source/fab/steps/compile_c.py | 11 +++-------- 6 files changed, 23 insertions(+), 21 deletions(-) diff --git a/source/fab/__init__.py b/source/fab/__init__.py index 9bd8d2e80..bed1f3a68 100644 --- a/source/fab/__init__.py +++ b/source/fab/__init__.py @@ -15,7 +15,3 @@ logger = logging.getLogger(__name__) logger.addHandler(logging.StreamHandler(sys.stdout)) logger.setLevel(logging.INFO) - - -class FabException(Exception): - pass diff --git a/source/fab/errors.py b/source/fab/errors.py index 1dab3a48f..64d7d75a5 100644 --- a/source/fab/errors.py +++ b/source/fab/errors.py @@ -342,3 +342,16 @@ def __init__(self, source: str, reason: str) -> None: self.source = source self.reason = reason super().__init__(f"could not fetch {source}: {reason}") + + +class FabParseError(FabError): + """Error while parsing or analysing source code.""" + + def __init__(self, message, fpath=None, lineno=None): + if fpath is not None: + # Add the name of the source file and the line number + message = message.rstrip() + f" ({fpath.name}" + if lineno is not None: + message += f":{lineno}" + message += ")" + super().__init__(message) diff --git a/source/fab/parse/fortran_common.py b/source/fab/parse/fortran_common.py index b145aeb70..95e1922b6 100644 --- a/source/fab/parse/fortran_common.py +++ b/source/fab/parse/fortran_common.py @@ -16,7 +16,7 @@ from fparser.two.parser import ParserFactory # type: ignore from fparser.two.utils import FortranSyntaxError # type: ignore -from fab import FabException +from fab.errors import FabParseError from fab.build_config import BuildConfig from fab.dep_tree import AnalysedDependent from fab.parse import EmptySourceFile @@ -39,8 +39,7 @@ def _typed_child(parent, child_type: Type, must_exist=False): return children[0] if must_exist: - raise FabException(f'Could not find child of type {child_type} ' - f'in {parent}') + raise FabParseError(f'Child of type {child_type} is not in {parent}') return None diff --git a/source/fab/steps/analyse.py b/source/fab/steps/analyse.py index f56673c1f..1dc0358fe 100644 --- a/source/fab/steps/analyse.py +++ b/source/fab/steps/analyse.py @@ -40,7 +40,7 @@ from pathlib import Path from typing import Dict, Iterable, List, Optional, Set, Union -from fab import FabException +from fab.errors import FabError from fab.artefacts import ArtefactsGetter, ArtefactSet, CollectionConcat from fab.dep_tree import extract_sub_tree, validate_dependencies, AnalysedDependent from fab.mo import add_mo_commented_file_deps @@ -162,7 +162,7 @@ def analyse( if c_with_main: root_symbols.append('main') if len(c_with_main) > 1: - raise FabException("multiple c main() functions found") + raise FabError("multiple C main() functions found") logger.info(f'automatically found the following programs to build: {", ".join(root_symbols)}') diff --git a/source/fab/steps/c_pragma_injector.py b/source/fab/steps/c_pragma_injector.py index 196d08679..72355346f 100644 --- a/source/fab/steps/c_pragma_injector.py +++ b/source/fab/steps/c_pragma_injector.py @@ -11,7 +11,7 @@ from pathlib import Path from typing import Generator, Pattern, Optional, Match -from fab import FabException +from fab.errors import FabParseError from fab.artefacts import ArtefactSet, ArtefactsGetter, SuffixFilter from fab.steps import run_mp, step @@ -74,7 +74,7 @@ def inject_pragmas(fpath) -> Generator: _include_re: str = r'^\s*#include\s+(\S+)' _include_pattern: Pattern = re.compile(_include_re) - for line in open(fpath, 'rt', encoding='utf-8'): + for i, line in enumerate(open(fpath, 'rt', encoding='utf-8'), 1): include_match: Optional[Match] = _include_pattern.match(line) if include_match: # For valid C the first character of the matched @@ -90,7 +90,6 @@ def inject_pragmas(fpath) -> Generator: yield line yield '#pragma FAB UsrIncludeEnd\n' else: - msg = 'Found badly formatted #include' - raise FabException(msg) + raise FabParseError("Badly formatted #include", fpath, i) else: yield line diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 16c584cec..477bdc6f5 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -11,7 +11,6 @@ from dataclasses import dataclass from typing import cast, Dict, List, Optional, Tuple -from fab import FabException from fab.artefacts import (ArtefactsGetter, ArtefactSet, ArtefactStore, FilterBuildTrees) from fab.build_config import BuildConfig, FlagsConfig @@ -146,13 +145,9 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): else: obj_file_prebuild.parent.mkdir(parents=True, exist_ok=True) log_or_dot(logger, f'CompileC compiling {analysed_file.fpath}') - try: - compiler.compile_file(analysed_file.fpath, obj_file_prebuild, - config=config, - add_flags=flags) - except RuntimeError as err: - return FabException(f"error compiling " - f"{analysed_file.fpath}:\n{err}") + compiler.compile_file(analysed_file.fpath, obj_file_prebuild, + config=config, + add_flags=flags) send_metric( group="compile c", From fff620dee0c2fec4cb412b075a5b5e0ac3c3b584 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Wed, 22 Oct 2025 14:44:25 +0100 Subject: [PATCH 4/7] Remove most built-in python exceptions Replace the use of built-in python exceptions with Fab exception classes and AssertionErrors. --- source/fab/artefacts.py | 2 +- source/fab/errors.py | 48 ++++++++---- source/fab/fab_base/fab_base.py | 13 ++-- source/fab/metrics.py | 2 +- source/fab/parse/__init__.py | 10 +-- source/fab/parse/fortran_common.py | 6 +- source/fab/steps/analyse.py | 10 +-- source/fab/steps/archive_objects.py | 8 +- source/fab/steps/c_pragma_injector.py | 4 +- source/fab/steps/cleanup_prebuilds.py | 4 +- source/fab/steps/compile_c.py | 6 +- source/fab/steps/compile_fortran.py | 18 ++--- source/fab/steps/grab/svn.py | 4 +- source/fab/steps/link.py | 3 +- source/fab/steps/preprocess.py | 6 +- source/fab/tools/compiler.py | 6 +- source/fab/tools/compiler_wrapper.py | 10 +-- source/fab/tools/flags.py | 16 ++-- source/fab/tools/linker.py | 5 +- source/fab/tools/tool.py | 6 +- source/fab/tools/tool_box.py | 2 +- source/fab/tools/tool_repository.py | 22 +++--- tests/system_tests/git/test_git.py | 4 +- .../svn_fcm/test_svn_fcm_system_test.py | 8 +- tests/unit_tests/fab_base/test_fab_base.py | 32 ++++---- .../steps/grab/test_svn_fcm_unit_test.py | 3 +- tests/unit_tests/steps/test_analyse.py | 4 +- .../unit_tests/steps/test_archive_objects.py | 5 +- .../steps/test_cleanup_prebuilds.py | 2 +- tests/unit_tests/steps/test_compile_c.py | 9 +-- .../unit_tests/steps/test_compile_fortran.py | 17 ++-- tests/unit_tests/steps/test_link.py | 4 +- tests/unit_tests/steps/test_preprocess.py | 5 +- tests/unit_tests/steps/test_steps.py | 4 +- tests/unit_tests/test_artefacts.py | 2 +- tests/unit_tests/test_errors.py | 52 ++++++++++--- tests/unit_tests/tools/test_compiler.py | 78 +++++-------------- .../unit_tests/tools/test_compiler_wrapper.py | 12 +-- tests/unit_tests/tools/test_flags.py | 29 +++---- tests/unit_tests/tools/test_linker.py | 18 ++--- tests/unit_tests/tools/test_psyclone.py | 36 +++------ tests/unit_tests/tools/test_tool.py | 19 ++--- tests/unit_tests/tools/test_tool_box.py | 5 +- .../unit_tests/tools/test_tool_repository.py | 47 ++++------- tests/unit_tests/tools/test_versioning.py | 18 ++--- 45 files changed, 270 insertions(+), 354 deletions(-) diff --git a/source/fab/artefacts.py b/source/fab/artefacts.py index d60cf7ff1..134be848a 100644 --- a/source/fab/artefacts.py +++ b/source/fab/artefacts.py @@ -131,7 +131,7 @@ def replace(self, artefact: Union[str, ArtefactSet], art_set = self[artefact] if not isinstance(art_set, set): name = artefact if isinstance(artefact, str) else artefact.name - raise ValueError(f"{name} is not mutable") + raise AssertionError(f"{name} is not mutable") art_set.difference_update(set(remove_files)) art_set.update(add_files) diff --git a/source/fab/errors.py b/source/fab/errors.py index 64d7d75a5..e72da0559 100644 --- a/source/fab/errors.py +++ b/source/fab/errors.py @@ -39,13 +39,11 @@ class FabToolError(FabError): def __init__(self, tool: Union["Category", "Tool", str], message: str) -> None: self.tool = tool - # Check for name attributes rather than using isintance - # because Category and Tool ahve issues with circular + # Check for name attributes rather than using isinstance + # because Category and Tool have issues with circular # dependencies if hasattr(tool, "name"): self.name = tool.name - elif hasattr(tool, "_name"): - self.name = tool._name else: self.name = tool super().__init__(f"[{self.name}] {message}") @@ -145,11 +143,13 @@ class FabToolNotAvailable(FabToolError): """ def __init__( - self, tool: Union["Category", "Tool", str], suite: Optional[str] = None + self, + tool: Union["Category", "Tool", str], + suite: Optional[Union["Category", str]] = None, ) -> None: message = "not available" if suite: - message += f" in suite {suite}" + message += f" in {suite}" super().__init__(tool, message) @@ -164,8 +164,6 @@ class FabToolInvalidSetting(FabToolError): :param additional: optional additional information """ - # FIXME: improve these here and in tool_repository - def __init__( self, setting_type: str, @@ -192,7 +190,7 @@ class FabUnknownLibraryError(FabError): def __init__(self, library: str) -> None: self.library = library - super().__init__(f"unknown library {library}") + super().__init__(f"unknown library {repr(library)}") class FabCommandError(FabError): @@ -225,7 +223,7 @@ def __init__( self.output = self._decode(output) self.error = self._decode(error) self.cwd = cwd - super().__init__(f"command {repr(self.command)} returned {code}") + super().__init__(f"return code {code} from {repr(self.command)}") def _decode(self, value: Union[str, bytes, None]) -> str: """Convert from bytes to a string as necessary.""" @@ -256,7 +254,7 @@ def __init__(self, command: str) -> None: else: raise ValueError(f"invalid command: {command}") - super().__init__(f"unable to execute {self.target}") + super().__init__(f"unable to execute {repr(self.target)}") class FabMultiCommandError(FabError): @@ -322,7 +320,7 @@ def __init__(self, tool: str, reason: str, revision: Optional[str] = None) -> No message = f"[{tool}] merge " if revision: - message += f"of {revision} " + message += f"of {repr(revision)} " message += f"failed: {reason}" super().__init__(message) @@ -344,7 +342,7 @@ def __init__(self, source: str, reason: str) -> None: super().__init__(f"could not fetch {source}: {reason}") -class FabParseError(FabError): +class FabAnalysisError(FabError): """Error while parsing or analysing source code.""" def __init__(self, message, fpath=None, lineno=None): @@ -355,3 +353,27 @@ def __init__(self, message, fpath=None, lineno=None): message += f":{lineno}" message += ")" super().__init__(message) + + +class FabFileLoadError(FabError): + """Error loading a FabFile.""" + + def __init__(self, message, fpath): + super().__init__(message) + self.fpath = fpath + + +class FabProfileError(FabError): + """Error when an invalid profile is provided.""" + + def __init__(self, message, profile): + super().__init__(f"profile {repr(profile)} {message}") + self.profile = profile + + +class FabHashError(FabError): + """Error creating a file combination hash.""" + + def __init__(self, fpath): + super().__init__(f"failed to create hash for {fpath}") + self.fpath = fpath diff --git a/source/fab/fab_base/fab_base.py b/source/fab/fab_base/fab_base.py index d3ef10a1d..5635c58d7 100755 --- a/source/fab/fab_base/fab_base.py +++ b/source/fab/fab_base/fab_base.py @@ -124,8 +124,8 @@ def set_link_target(self, link_target: str) -> None: link_target = link_target.lower() valid_targets = ["executable", "static-library", "shared-library"] if link_target not in valid_targets: - raise ValueError(f"Invalid parameter '{link_target}', must be " - f"one of '{', '.join(valid_targets)}'.") + raise AssertionError(f"link target '{link_target}' not in " + f"{repr(valid_targets)}") self._link_target = link_target def define_project_name(self, name: str) -> str: @@ -416,7 +416,8 @@ class which can provide its own instance (to easily allow for a help="Enable OpenACC") parser.add_argument( '--host', '-host', default="cpu", type=str, - help="Determine the OpenACC or OpenMP: either 'cpu' or 'gpu'.") + choices=["cpu", "gpu"], + help="Determine the OpenACC or OpenMP.") parser.add_argument("--site", "-s", type=str, default="$SITE or 'default'", @@ -449,10 +450,6 @@ def handle_command_line_options(self, ''' # pylint: disable=too-many-branches self._args = parser.parse_args(sys.argv[1:]) - if self.args.host.lower() not in ["", "cpu", "gpu"]: - raise RuntimeError(f"Invalid host directive " - f"'{self.args.host}'. Must be " - f"'cpu' or 'gpu'.") tr = ToolRepository() if self.args.available_compilers: @@ -483,7 +480,7 @@ def handle_command_line_options(self, # profile in the site config file. if (self.args.profile and self.args.profile not in self._site_config.get_valid_profiles()): - raise RuntimeError(f"Invalid profile '{self.args.profile}") + parser.error(f"invalid profile '{self.args.profile}") if self.args.suite: tr.set_default_compiler_suite(self.args.suite) diff --git a/source/fab/metrics.py b/source/fab/metrics.py index aa7287506..3dd0ce418 100644 --- a/source/fab/metrics.py +++ b/source/fab/metrics.py @@ -62,7 +62,7 @@ def init_metrics(metrics_folder: Path): global _metric_recv_process if any([_metric_recv_conn, _metric_send_conn, _metric_recv_process]): - raise ConnectionError('Metrics already initialised. Only one concurrent user of init_metrics is expected.') + raise ConnectionError('metrics already initialised') # the pipe connections for individual metrics _metric_recv_conn, _metric_send_conn = Pipe(duplex=False) diff --git a/source/fab/parse/__init__.py b/source/fab/parse/__init__.py index dbc7a4735..f2f31c5f9 100644 --- a/source/fab/parse/__init__.py +++ b/source/fab/parse/__init__.py @@ -9,15 +9,12 @@ from pathlib import Path from typing import Any, Dict, Optional, Set, Union +from fab.errors import FabAnalysisError from fab.util import file_checksum logger = logging.getLogger(__name__) -class ParseException(Exception): - pass - - class AnalysedFile(ABC): """ Analysis results for a single file. Abstract base class. @@ -40,8 +37,6 @@ def __init__(self, fpath: Union[str, Path], file_hash: Optional[int] = None): @property def file_hash(self) -> int: if self._file_hash is None: - if not self.fpath.exists(): - raise ValueError(f"analysed file '{self.fpath}' does not exist") self._file_hash = file_checksum(self.fpath).file_hash return self._file_hash @@ -79,7 +74,8 @@ def load(cls, fpath: Union[str, Path]): d = json.load(open(fpath)) found_class = d["cls"] if found_class != cls.__name__: - raise ValueError(f"Expected class name '{cls.__name__}', found '{found_class}'") + raise FabAnalysisError(f"expected '{cls.__name__}' but found '{found_class}'", + fpath) return cls.from_dict(d) # human readability diff --git a/source/fab/parse/fortran_common.py b/source/fab/parse/fortran_common.py index 95e1922b6..a9c2d7cad 100644 --- a/source/fab/parse/fortran_common.py +++ b/source/fab/parse/fortran_common.py @@ -16,7 +16,7 @@ from fparser.two.parser import ParserFactory # type: ignore from fparser.two.utils import FortranSyntaxError # type: ignore -from fab.errors import FabParseError +from fab.errors import FabAnalysisError from fab.build_config import BuildConfig from fab.dep_tree import AnalysedDependent from fab.parse import EmptySourceFile @@ -33,13 +33,13 @@ def _typed_child(parent, child_type: Type, must_exist=False): children = list(filter(lambda child: isinstance(child, child_type), parent.children)) if len(children) > 1: - raise ValueError(f"too many children found of type {child_type}") + raise FabAnalysisError(f"too many children found of type {child_type}") if children: return children[0] if must_exist: - raise FabParseError(f'Child of type {child_type} is not in {parent}') + raise FabAnalysisError(f'child of type {child_type} is not in {parent}') return None diff --git a/source/fab/steps/analyse.py b/source/fab/steps/analyse.py index c07cf0988..50e9552e2 100644 --- a/source/fab/steps/analyse.py +++ b/source/fab/steps/analyse.py @@ -40,7 +40,7 @@ from pathlib import Path from typing import Dict, Iterable, List, Optional, Set, Union -from fab.errors import FabError +from fab.errors import FabAnalysisError from fab.artefacts import ArtefactsGetter, ArtefactSet, CollectionConcat from fab.dep_tree import extract_sub_tree, validate_dependencies, AnalysedDependent from fab.mo import add_mo_commented_file_deps @@ -121,7 +121,7 @@ def analyse( # because we're just creating steps at this point, so there's been no grab... if find_programs and root_symbol: - raise ValueError("find_programs and root_symbol can't be used together") + raise AssertionError("find_programs and root_symbol can't be used together") source_getter = source or DEFAULT_SOURCE_GETTER root_symbols: Optional[List[str]] = [root_symbol] if isinstance(root_symbol, str) else root_symbol @@ -161,7 +161,7 @@ def analyse( if c_with_main: root_symbols.append('main') if len(c_with_main) > 1: - raise FabError("multiple C main() functions found") + raise FabAnalysisError("multiple C main() functions found") logger.info(f'automatically found the following programs to build: {", ".join(root_symbols)}') @@ -285,7 +285,7 @@ def _add_manual_results(special_measure_analysis_results, analysed_files: Set[An # Note: This exception stops the user from being able to override results for files # which don't *crash* the parser. We don't have a use case to do this, but it's worth noting. # If we want to allow this we can raise a warning instead of an exception. - raise ValueError(f'Unnecessary ParserWorkaround for {r.fpath}') + raise FabAnalysisError(f'unnecessary ParserWorkaround for {r.fpath}') analysed_files.add(r.as_analysed_fortran()) logger.info(f'added {len(special_measure_analysis_results)} manual analysis results') @@ -314,7 +314,7 @@ def _gen_symbol_table(analysed_files: Iterable[AnalysedDependent]) -> Dict[str, # required to build the executable. # todo: put a big warning at the end of the build? logger.error("Error generating symbol table") - raise ValueError("Duplicate symbol definitions found") + raise FabAnalysisError("duplicate symbol definitions found") return symbols diff --git a/source/fab/steps/archive_objects.py b/source/fab/steps/archive_objects.py index c1969d7fe..5247c4287 100644 --- a/source/fab/steps/archive_objects.py +++ b/source/fab/steps/archive_objects.py @@ -109,15 +109,15 @@ def archive_objects(config: BuildConfig, ar = config.tool_box[Category.AR] if not isinstance(ar, Ar): raise FabToolMismatch(ar.name, type(ar), "Ar") - output_fpath = str(output_fpath) if output_fpath else None + # output_fpath = str(output_fpath) if output_fpath else None target_objects = source_getter(config.artefact_store) assert target_objects.keys() if output_fpath and list(target_objects.keys()) != [None]: - raise ValueError("You must not specify an output path (library) when " - "there are root symbols (executables)") + raise AssertionError("output path cannot be specified for an executable") + if not output_fpath and list(target_objects.keys()) == [None]: - raise ValueError("You must specify an output path when building a library.") + raise AssertionError("output path must be specified for a library") for root, objects in target_objects.items(): diff --git a/source/fab/steps/c_pragma_injector.py b/source/fab/steps/c_pragma_injector.py index 72355346f..1d8172062 100644 --- a/source/fab/steps/c_pragma_injector.py +++ b/source/fab/steps/c_pragma_injector.py @@ -11,7 +11,7 @@ from pathlib import Path from typing import Generator, Pattern, Optional, Match -from fab.errors import FabParseError +from fab.errors import FabAnalysisError from fab.artefacts import ArtefactSet, ArtefactsGetter, SuffixFilter from fab.steps import run_mp, step @@ -90,6 +90,6 @@ def inject_pragmas(fpath) -> Generator: yield line yield '#pragma FAB UsrIncludeEnd\n' else: - raise FabParseError("Badly formatted #include", fpath, i) + raise FabAnalysisError("Badly formatted #include", fpath, i) else: yield line diff --git a/source/fab/steps/cleanup_prebuilds.py b/source/fab/steps/cleanup_prebuilds.py index fcba7970a..eec801647 100644 --- a/source/fab/steps/cleanup_prebuilds.py +++ b/source/fab/steps/cleanup_prebuilds.py @@ -47,12 +47,12 @@ def cleanup_prebuilds( # If the user has not specified any cleanup parameters, we default to a hard cleanup. if not n_versions and not older_than: if all_unused not in [None, True]: - raise ValueError(f"unexpected value for all_unused: '{all_unused}'") + raise AssertionError(f"{repr(all_unused)} not in [None, True]") all_unused = True # if we're doing a hard cleanup, there's no point providing the softer options if all_unused and (n_versions or older_than): - raise ValueError("n_versions or older_than should not be specified with all_unused") + raise AssertionError("all_unused cannot be used with n_versions or older_than") num_removed = 0 diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 477bdc6f5..d6978e43c 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -20,7 +20,7 @@ from fab.tools import Category, Compiler, Flags from fab.util import CompiledFile, log_or_dot, Timer, by_type -from fab.errors import FabToolMismatch +from fab.errors import FabToolMismatch, FabHashError logger = logging.getLogger(__name__) @@ -167,6 +167,6 @@ def _get_obj_combo_hash(config: BuildConfig, compiler.get_hash(config.profile), ]) except TypeError as err: - raise ValueError("could not generate combo hash for " - "object file") from err + raise FabHashError(analysed_file.fpath) from err + return obj_combo_hash diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index 9c7e0e683..b4b279a5c 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -25,7 +25,7 @@ from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) -from fab.errors import FabToolMismatch +from fab.errors import FabToolMismatch, FabHashError, FabError logger = logging.getLogger(__name__) @@ -207,13 +207,7 @@ def get_compile_next(compiled: Dict[Path, CompiledFile], # unable to compile anything? if len(uncompiled) and not compile_next: - msg = 'Nothing more can be compiled due to unfulfilled dependencies:\n' - for f, unf in not_ready.items(): - msg += f'\n\n{f}' - for u in unf: - msg += f'\n {str(u)}' - - raise ValueError(msg) + raise FabError(f"remaining {len(not_ready)} items not ready for compilation") return compile_next @@ -357,8 +351,8 @@ def _get_obj_combo_hash(config: BuildConfig, compiler.get_hash(config.profile), ]) except TypeError as err: - raise ValueError("Could not generate combo hash " - "for object file") from err + raise FabHashError(analysed_file.fpath) from err + return obj_combo_hash @@ -370,8 +364,8 @@ def _get_mod_combo_hash(config, analysed_file, compiler: Compiler): compiler.get_hash(config.profile), ]) except TypeError as err: - raise ValueError("Could not generate combo " - "hash for mod files") from err + raise FabHashError(analysed_file.fpath) from err + return mod_combo_hash diff --git a/source/fab/steps/grab/svn.py b/source/fab/steps/grab/svn.py index 5c37e1660..126994649 100644 --- a/source/fab/steps/grab/svn.py +++ b/source/fab/steps/grab/svn.py @@ -37,8 +37,8 @@ def _get_revision(src, revision=None) -> Tuple[str, Union[str, None]]: if len(at_split) == 2: url_revision = at_split[1] if url_revision and revision and url_revision != revision: - raise ValueError('Conflicting revisions in url and argument. ' - 'Please provide as argument only.') + raise AssertionError('conflicting revisions in URL and argument') + src = at_split[0] else: assert len(at_split) == 1 diff --git a/source/fab/steps/link.py b/source/fab/steps/link.py index 4fef7f554..f814cd368 100644 --- a/source/fab/steps/link.py +++ b/source/fab/steps/link.py @@ -66,8 +66,7 @@ def link_exe(config, target_objects = source_getter(config.artefact_store) if len(target_objects) == 0: - raise ValueError("No target objects defined, linking aborted") - return + raise AssertionError("no target objects defined") if config.tool_box.has(Category.LINKER): linker = config.tool_box.get_tool(Category.LINKER, mpi=config.mpi, diff --git a/source/fab/steps/preprocess.py b/source/fab/steps/preprocess.py index c8122eb16..1e68a95af 100644 --- a/source/fab/steps/preprocess.py +++ b/source/fab/steps/preprocess.py @@ -118,11 +118,7 @@ def process_artefact(arg: Tuple[Path, MpCommonArgs]): log_or_dot(logger, f"PreProcessor running with parameters: " f"'{' '.join(params)}'.'") - try: - args.preprocessor.preprocess(input_fpath, output_fpath, params) - except Exception as err: - raise Exception(f"error preprocessing {input_fpath}:\n" - f"{err}") from err + args.preprocessor.preprocess(input_fpath, output_fpath, params) send_metric(args.name, str(input_fpath), {'time_taken': timer.taken, 'start': timer.start}) return output_fpath diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 806714709..a607c07fa 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -207,7 +207,7 @@ def get_version(self) -> Tuple[int, ...]: :returns: a tuple of at least 2 integers, representing the version e.g. (6, 10, 1) for version '6.10.1'. - :raises RuntimeError: if the compiler was not found, or if it returned + :raises FabToolInvalidVersion: if the compiler was not found, or if it returned an unrecognised output from the version command. """ if self._version is not None: @@ -252,7 +252,7 @@ def run_version_command( :returns: The output from the version command. - :raises RuntimeError: if the compiler was not found, or raised an + :raises FabToolInvalidVersion: if the compiler was not found, or raised an error. ''' try: @@ -267,7 +267,7 @@ def get_version_string(self) -> str: :returns: a string of at least 2 numeric version components, i.e. major.minor[.patch, ...] - :raises RuntimeError: if the compiler was not found, or if it returned + :raises FabToolInvalidVersion: if the compiler was not found, or if it returned an unrecognised output from the version command. """ version = self.get_version() diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 27e2d8972..4abd7110d 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -63,14 +63,14 @@ def openmp_flag(self) -> str: def has_syntax_only(self) -> bool: ''':returns: whether this compiler supports a syntax-only feature. - :raises RuntimeError: if this function is called for a non-Fortran + :raises FabToolError: if this function is called for a non-Fortran wrapped compiler. ''' if self._compiler.category == Category.FORTRAN_COMPILER: return cast(FortranCompiler, self._compiler).has_syntax_only - raise FabToolError(self._compiler.name, "no syntax-only feature") + raise FabToolError(self._compiler.name, "syntax-only is Fortran-specific") def get_flags(self, profile: Optional[str] = None) -> List[str]: ''':returns: the ProfileFlags for the given profile, combined @@ -86,12 +86,12 @@ def set_module_output_path(self, path: Path): :params path: the path to the output directory. - :raises RuntimeError: if this function is called for a non-Fortran + :raises FabToolError: if this function is called for a non-Fortran wrapped compiler. ''' if self._compiler.category != Category.FORTRAN_COMPILER: - raise FabToolError(self._compiler.name, "no module output path feature") + raise FabToolError(self._compiler.name, "not a vaild compiler") cast(FortranCompiler, self._compiler).set_module_output_path(path) def get_all_commandline_options( @@ -116,7 +116,7 @@ def get_all_commandline_options( :returns: command line flags for compiler wrapper. - :raises RuntimeError: if syntax_only is requested for a non-Fortran + :raises FabToolError: if syntax_only is requested for a non-Fortran compiler. ''' # We need to distinguish between Fortran and non-Fortran compiler, diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 23b04b3ca..6172159a9 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -13,6 +13,7 @@ from typing import Dict, List, Optional, Union import warnings +from fab.errors import FabProfileError from fab.util import string_checksum @@ -117,7 +118,7 @@ def __getitem__(self, profile: Optional[str] = None) -> List[str]: :param profile: the optional profile to use. - :raises KeyError: if a profile is specified it is not defined + :raises FabProfileError: if a profile is specified it is not defined ''' if profile is None: profile = "" @@ -138,7 +139,7 @@ def __getitem__(self, profile: Optional[str] = None) -> List[str]: try: flags.extend(self._profiles[profile]) except KeyError as err: - raise KeyError(f"Profile '{profile}' is not defined.") from err + raise FabProfileError("not defined", profile) from err return flags @@ -155,13 +156,12 @@ def define_profile(self, settings from. ''' if name in self._profiles: - raise KeyError(f"Profile '{name}' is already defined.") + raise FabProfileError("already defined", name) self._profiles[name.lower()] = Flags() if inherit_from is not None: if inherit_from not in self._profiles: - raise KeyError(f"Inherited profile '{inherit_from}' is " - f"not defined.") + raise FabProfileError("not defined for inheritance", inherit_from) self._inherit_from[name.lower()] = inherit_from.lower() def add_flags(self, @@ -178,7 +178,7 @@ def add_flags(self, profile = profile.lower() if profile not in self._profiles: - raise KeyError(f"add_flags: Profile '{profile}' is not defined.") + raise FabProfileError("not defined", profile) if isinstance(new_flags, str): new_flags = [new_flags] @@ -207,7 +207,7 @@ def remove_flag(self, profile = profile.lower() if profile not in self._profiles: - raise KeyError(f"remove_flag: Profile '{profile}' is not defined.") + raise FabProfileError("not defined", profile) self._profiles[profile].remove_flag(remove_flag, has_parameter) @@ -222,6 +222,6 @@ def checksum(self, profile: Optional[str] = None) -> str: profile = profile.lower() if profile not in self._profiles: - raise KeyError(f"checksum: Profile '{profile}' is not defined.") + raise FabProfileError("not defined", profile) return self._profiles[profile].checksum() diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 30e2a8433..c29f0bee8 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -34,9 +34,6 @@ class Linker(CompilerSuiteTool): :param compiler: a compiler instance :param linker: an optional linker instance :param name: name of the linker - - :raises RuntimeError: if both compiler and linker are specified. - :raises RuntimeError: if neither compiler nor linker is specified. ''' def __init__(self, compiler: Compiler, @@ -136,7 +133,7 @@ def get_lib_flags(self, lib: str) -> List[str]: :returns: a list of flags - :raises RuntimeError: if lib is not recognised + :raises FabUnknownLibraryError: if lib is not recognised ''' try: return self._lib_flags[lib] diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index 0ddfe3c29..e5023c003 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -53,7 +53,7 @@ def __init__(self, name: str, exec_name: Union[str, Path], # or not. It will be set to the output of `check_available` when # querying the `is_available` property. # If `_is_available` is False, any call to `run` will immediately - # raise a RuntimeError. As long as it is still set to None (or True), + # raise an exception. As long as it is still set to None (or True), # the `run` method will work, allowing the `check_available` method # to use `run` to determine if a tool is available or not. self._is_available: Optional[bool] = None @@ -178,8 +178,8 @@ def run(self, If True, capture and return stdout. If False, the command will print its output directly to the console. - :raises RuntimeError: if the code is not available. - :raises RuntimeError: if the return code of the executable is not 0. + :raises FabToolNotAvailable: if the tool is not available. + :raises FabCommandError: if the return code of the executable is not 0. """ command = [str(self.exec_path)] + self.get_flags(profile) if additional_parameters: diff --git a/source/fab/tools/tool_box.py b/source/fab/tools/tool_box.py index a820753d8..7b6c701fc 100644 --- a/source/fab/tools/tool_box.py +++ b/source/fab/tools/tool_box.py @@ -42,7 +42,7 @@ def add_tool(self, tool: Tool, :param silent_replace: if set, no warning will be printed if an existing tool is replaced. - :raises RuntimeError: if the tool to be added is not available. + :raises FabToolNotAvailable: if the tool to be added is not available. ''' if not tool.is_available: raise FabToolNotAvailable(tool) diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 40238577c..f1afb9708 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -26,7 +26,7 @@ Gcc, Gfortran, Icc, Icx, Ifort, Ifx, Nvc, Nvfortran, Psyclone, Rsync, Shell) -from fab.errors import FabToolInvalidSetting, FabToolNotAvailable +from fab.errors import FabToolInvalidSetting, FabToolNotAvailable, FabToolError class ToolRepository(dict): @@ -170,14 +170,12 @@ def get_tool(self, category: Category, name: str) -> Tool: in which case only the stem of the path is used, and the tool will be updated to use the absolute path specified. - :raises KeyError: if there is no tool in this category. - :raises KeyError: if no tool in the given category has the - requested name. + :raises FabToolNotAvailable: if there is no tool in this category or + if category does not have a tool of the requested name. ''' if category not in self: - raise KeyError(f"Unknown category '{category}' " - f"in ToolRepository.get_tool().") + raise FabToolNotAvailable(category) path_name = Path(name) all_tools = self[category] @@ -205,8 +203,7 @@ def get_tool(self, category: Category, name: str) -> Tool: if tool.is_available: return tool - raise KeyError(f"Unknown tool '{name}' in category '{category}' " - f"in ToolRepository.") + raise FabToolNotAvailable(name, category) def set_default_compiler_suite(self, suite: str): """ @@ -245,10 +242,10 @@ def get_default(self, category: Category, is used to specify if a Fortran-based linker is required. Otherwise, a C-based linker will be returned. - :raises KeyError: if the category does not exist. - :raises RuntimeError: if no tool in the requested category is + :raises FabToolNotAvailable: if the category does not exist. + :raises FabToolError: if no tool in the requested category is available on the system. - :raises RuntimeError: if no compiler/linker is found with the + :raises FabToolInvalidSetting: if no compiler/linker is found with the requested level of MPI support (yes or no). ''' @@ -273,8 +270,7 @@ def get_default(self, category: Category, if (category is Category.LINKER and not isinstance(enforce_fortran_linker, bool)): - raise RuntimeError(f"Invalid or missing enforce_fortran_linker " - f"specification for '{category}'.") + raise FabToolError(category, "invalid enforce_fortran_linker value") for tool in self[category]: tool = cast(Union[Compiler, Linker], tool) # make mypy happy diff --git a/tests/system_tests/git/test_git.py b/tests/system_tests/git/test_git.py index 3f5b837fe..2f45a72bf 100644 --- a/tests/system_tests/git/test_git.py +++ b/tests/system_tests/git/test_git.py @@ -25,6 +25,8 @@ from fab.steps.grab.git import git_checkout, git_merge from fab.tools import Git, ToolBox +from fab.errors import FabSourceMergeError + @pytest.fixture def config(tmp_path): @@ -89,7 +91,7 @@ def test_vanilla(self, repo_url, config): git_merge(config, src=repo_url, dst_label='tiny_fortran', revision='experiment_a') assert 'This is sentence one, with Experiment A modification.' in open(check_file).read() - with pytest.raises(RuntimeError): + with pytest.raises(FabSourceMergeError): git_merge(config, src=repo_url, dst_label='tiny_fortran', revision='experiment_b') # The conflicted merge must have been aborted, check that we can do another checkout of main diff --git a/tests/system_tests/svn_fcm/test_svn_fcm_system_test.py b/tests/system_tests/svn_fcm/test_svn_fcm_system_test.py index 3e52e7116..1f33a207e 100644 --- a/tests/system_tests/svn_fcm/test_svn_fcm_system_test.py +++ b/tests/system_tests/svn_fcm/test_svn_fcm_system_test.py @@ -21,6 +21,8 @@ from fab.steps.grab.fcm import fcm_checkout, fcm_export, fcm_merge from fab.steps.grab.svn import svn_checkout, svn_export, svn_merge +from fab.errors import FabCommandError, FabSourceMergeError + # Fcm isn't available in the github test images...unless we install it from github. # Which tools are available? @@ -199,7 +201,7 @@ def test_not_working_copy(self, trunk, config, export_func, checkout_func): export_func(config, src=trunk, dst_label='proj') # if we try to checkout into that folder, it should fail - with pytest.raises(RuntimeError): + with pytest.raises(FabCommandError): checkout_func(config, src=trunk, dst_label='proj') @@ -241,7 +243,7 @@ def test_not_working_copy(self, trunk, file2_experiment, config, export_func, me export_func(config, src=trunk, dst_label='proj') # try to merge into an export - with pytest.raises(RuntimeError): + with pytest.raises(FabCommandError): merge_func(config, src=file2_experiment, dst_label='proj', revision=7) @pytest.mark.parametrize('checkout_func,merge_func', zip(checkout_funcs, merge_funcs)) @@ -253,7 +255,7 @@ def test_conflict(self, file1_experiment_a, file1_experiment_b, config, confirm_file1_experiment_a(config) # this branch modifies the same line of text - with pytest.raises(RuntimeError): + with pytest.raises(FabSourceMergeError): merge_func(config, src=file1_experiment_b, dst_label='proj') @pytest.mark.parametrize('checkout_func,merge_func', diff --git a/tests/unit_tests/fab_base/test_fab_base.py b/tests/unit_tests/fab_base/test_fab_base.py index b85b133e8..e511361e2 100644 --- a/tests/unit_tests/fab_base/test_fab_base.py +++ b/tests/unit_tests/fab_base/test_fab_base.py @@ -63,10 +63,10 @@ def test_constructor(monkeypatch) -> None: ''' Tests constructor. ''' - with pytest.raises(ValueError) as err: + with pytest.raises(AssertionError) as err: _ = FabBase(name="test_name", link_target="wrong") - assert ("Invalid parameter 'wrong', must be one of 'executable, " - "static-library, shared-library'." in str(err.value)) + assert ("link target 'wrong' not in ['executable', 'static-library', 'shared-library']" + in str(err.value)) monkeypatch.setattr(sys, "argv", ["fab_base.py"]) fab_base = FabBase(name="test_name", link_target="executable") @@ -100,15 +100,17 @@ def test_site_platform(monkeypatch, arg) -> None: assert getattr(fab_base, attribute) == flag_list[1] -def test_arg_error(monkeypatch) -> None: +def test_arg_error(monkeypatch, capsys) -> None: ''' Tests handling of errors in the command line. ''' monkeypatch.setattr(sys, "argv", ["fab_base.py", "--host", "invalid"]) - with pytest.raises(RuntimeError) as err: + with pytest.raises(SystemExit): _ = FabBase(name="test-help") - assert ("Invalid host directive 'invalid'. Must be 'cpu' or 'gpu'." == - str(err.value)) + + captured = capsys.readouterr() + assert ("argument --host/-host: invalid choice: 'invalid' (choose from cpu, gpu)" + in captured.err) def test_available_compilers(monkeypatch, capsys) -> None: @@ -160,14 +162,16 @@ def test_profile_command_line(monkeypatch) -> None: assert fab_base.args.profile == "full-debug" -def test_profile_invalid(monkeypatch) -> None: +def test_profile_invalid(monkeypatch, capsys) -> None: ''' Tests trying to set an invalid profile: ''' monkeypatch.setattr(sys, "argv", ["fab_base.py", "--profile", "invalid"]) - with pytest.raises(RuntimeError) as err: + with pytest.raises(SystemExit): _ = FabBase(name="test-help") - assert "Invalid profile 'invalid" == str(err.value) + + captured = capsys.readouterr() + assert "invalid profile 'invalid" in captured.err def test_suite_no_compiler(monkeypatch) -> None: @@ -277,9 +281,9 @@ def test_workspace(monkeypatch, change_into_tmpdir) -> None: # is none, Fab will abort in the linking step (missing targets). # Note that the project directories are only created once # build is called. - with pytest.raises(ValueError) as err: + with pytest.raises(AssertionError) as err: fab_base.build() - assert "No target objects defined, linking aborted" in str(err.value) + assert "no target objects defined" in str(err.value) # Check that the project workspace is as expected: project_dir = fab_base.project_workspace @@ -359,9 +363,9 @@ def test_build_binary(monkeypatch) -> None: patcher = mock.patch(f"fab.fab_base.fab_base.{function_name}") mocks[function_name] = (patcher, patcher.start()) - with pytest.raises(ValueError) as err: + with pytest.raises(AssertionError) as err: fab_base.build() - assert "No target objects defined, linking aborted" in str(err.value) + assert "no target objects defined" in str(err.value) mocks["grab_folder"][0].stop() mocks["grab_folder"][1].assert_called_once_with( diff --git a/tests/unit_tests/steps/grab/test_svn_fcm_unit_test.py b/tests/unit_tests/steps/grab/test_svn_fcm_unit_test.py index 6d2115e4a..1c75cda17 100644 --- a/tests/unit_tests/steps/grab/test_svn_fcm_unit_test.py +++ b/tests/unit_tests/steps/grab/test_svn_fcm_unit_test.py @@ -53,5 +53,6 @@ def test_both_different(self): """ Tests mismatch between URL revision and argument. """ - with raises(ValueError): + with raises(AssertionError) as err: assert _get_revision(src='url@rev', revision='bez') + assert "conflicting revisions in URL and argument" in str(err.value) diff --git a/tests/unit_tests/steps/test_analyse.py b/tests/unit_tests/steps/test_analyse.py index 8558259ce..cc6446d40 100644 --- a/tests/unit_tests/steps/test_analyse.py +++ b/tests/unit_tests/steps/test_analyse.py @@ -13,6 +13,8 @@ from fab.tools.tool_repository import ToolRepository from fab.util import HashedFile +from fab.errors import FabAnalysisError + class Test_gen_symbol_table(object): """ @@ -45,7 +47,7 @@ def test_duplicate_symbol(self, """ analysed_files[1].symbol_defs.add('foo_1') - with raises(ValueError): + with raises(FabAnalysisError): result = _gen_symbol_table(analysed_files=analysed_files) assert result == { 'foo_1': Path('foo.c'), diff --git a/tests/unit_tests/steps/test_archive_objects.py b/tests/unit_tests/steps/test_archive_objects.py index 172d7193e..e2cbdf73c 100644 --- a/tests/unit_tests/steps/test_archive_objects.py +++ b/tests/unit_tests/steps/test_archive_objects.py @@ -114,9 +114,6 @@ def test_incorrect_tool(self, stub_tool_box, monkeypatch): # Now add this 'ar' tool to the tool box stub_tool_box.add_tool(cc) - with raises(RuntimeError) as err: + with raises(FabToolMismatch): archive_objects(config=config, output_fpath=config.build_output / 'mylib.a') - assert isinstance(err.value, FabToolMismatch) - assert str(err.value) == ("[some C compiler] got type " - " instead of Ar") diff --git a/tests/unit_tests/steps/test_cleanup_prebuilds.py b/tests/unit_tests/steps/test_cleanup_prebuilds.py index 476d47e00..b46882812 100644 --- a/tests/unit_tests/steps/test_cleanup_prebuilds.py +++ b/tests/unit_tests/steps/test_cleanup_prebuilds.py @@ -46,7 +46,7 @@ def test_init_bad_args(self): """ Tests clean-up with random bad arguments. """ - with raises(ValueError): + with raises(AssertionError): cleanup_prebuilds(config=None, all_unused=False) def test_by_age(self): diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py index dbe042f83..ac9a25831 100644 --- a/tests/unit_tests/steps/test_compile_c.py +++ b/tests/unit_tests/steps/test_compile_c.py @@ -20,7 +20,7 @@ from fab.tools.flags import Flags from fab.tools.tool_box import ToolBox -from fab.errors import FabToolMismatch +from fab.errors import FabCommandError, FabToolMismatch @fixture(scope='function') @@ -62,11 +62,8 @@ def test_compile_c_wrong_compiler(content, fake_process: FakeProcess) -> None: # Now check that _compile_file detects the incorrect class of the # C compiler mp_common_args = Mock(config=config) - with raises(RuntimeError) as err: + with raises(FabToolMismatch): _compile_file((Mock(), mp_common_args)) - assert isinstance(err.value, FabToolMismatch) - assert str(err.value) == ("[some C compiler] got type " - "FORTRAN_COMPILER instead of CCompiler") # This is more of an integration test than a unit test @@ -110,7 +107,7 @@ def test_exception_handling(self, content, 'scc', '-c', 'foo.c', '-o', str(config.build_output / '_prebuild/foo.101865856.o') ], returncode=1) - with raises(RuntimeError): + with raises(FabCommandError): compile_c(config=config) diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py index 2f55ce6d7..ae316b0bf 100644 --- a/tests/unit_tests/steps/test_compile_fortran.py +++ b/tests/unit_tests/steps/test_compile_fortran.py @@ -18,7 +18,7 @@ from fab.tools.tool_box import ToolBox from fab.util import CompiledFile -from fab.errors import FabToolMismatch +from fab.errors import FabToolMismatch, FabError @fixture(scope='function') @@ -60,19 +60,11 @@ def test_compile_cc_wrong_compiler(stub_tool_box, # Now check that _compile_file detects the incorrect category of the # Fortran compiler mp_common_args = Mock(config=config) - with raises(RuntimeError) as err: + with raises(FabToolMismatch): process_file((Mock(), mp_common_args)) - assert isinstance(err.value, FabToolMismatch) - assert str(err.value) \ - == "[some Fortran compiler] got type " \ - + "C_COMPILER instead of FortranCompiler" - with raises(RuntimeError) as err: + with raises(FabToolMismatch): handle_compiler_args(config) - assert isinstance(err.value, FabToolMismatch) - assert str(err.value) \ - == "[some Fortran compiler] got type " \ - + "C_COMPILER instead of FortranCompiler" class TestCompilePass: @@ -132,8 +124,9 @@ def test_unable_to_compile_anything(self, analysed_files): to_compile = {a, b} already_compiled_files = {} - with raises(ValueError): + with raises(FabError) as err: get_compile_next(already_compiled_files, to_compile) + assert "remaining 2 items not ready for compilation" in str(err.value) class TestStoreArtefacts: diff --git a/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index 77611d3f5..4179c22f6 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -179,8 +179,8 @@ def test_no_targets(fake_process: FakeProcess, config = BuildConfig('link_test', tool_box, fab_workspace=Path('/fab'), mpi=False, openmp=False, multiprocessing=False) - with raises(ValueError) as err: + with raises(AssertionError) as err: link_exe(config, libs=['mylib'], flags=['-fooflag', '-barflag']) - assert "No target objects defined, linking aborted" in str(err.value) + assert "no target objects defined" in str(err.value) assert call_list(fake_process) == [version_command] diff --git a/tests/unit_tests/steps/test_preprocess.py b/tests/unit_tests/steps/test_preprocess.py index 59e9dad2f..403c86882 100644 --- a/tests/unit_tests/steps/test_preprocess.py +++ b/tests/unit_tests/steps/test_preprocess.py @@ -76,8 +76,5 @@ def test_wrong_exe(self, stub_tool_repository: ToolRepository, tool_box.add_tool(cpp, silent_replace=True) config = BuildConfig('proj', tool_box, fab_workspace=tmp_path) - with raises(RuntimeError) as err: + with raises(FabToolMismatch): preprocess_fortran(config=config) - assert isinstance(err.value, FabToolMismatch) - assert str(err.value) == "[cpp] got type instead of CppFortran" diff --git a/tests/unit_tests/steps/test_steps.py b/tests/unit_tests/steps/test_steps.py index 191638b41..c39e6ba5e 100644 --- a/tests/unit_tests/steps/test_steps.py +++ b/tests/unit_tests/steps/test_steps.py @@ -10,6 +10,8 @@ from fab.steps import check_for_errors +from fab.errors import FabMultiCommandError + class Test_check_for_errors(object): """ @@ -25,5 +27,5 @@ def test_error(self): """ Tests the "error present" situation. """ - with raises(RuntimeError): + with raises(FabMultiCommandError): check_for_errors(['foo', MemoryError('bar')]) diff --git a/tests/unit_tests/test_artefacts.py b/tests/unit_tests/test_artefacts.py index 7590e5d42..329cfecbe 100644 --- a/tests/unit_tests/test_artefacts.py +++ b/tests/unit_tests/test_artefacts.py @@ -84,7 +84,7 @@ def test_artefact_store_replace() -> None: Path("c")]) # Test the behaviour for dictionaries - with pytest.raises(ValueError) as err: + with pytest.raises(AssertionError) as err: artefact_store.replace(ArtefactSet.OBJECT_FILES, remove_files=[Path("a")], add_files=["c"]) assert str(err.value) == "OBJECT_FILES is not mutable" diff --git a/tests/unit_tests/test_errors.py b/tests/unit_tests/test_errors.py index 4d58e4dc3..6ba816a34 100644 --- a/tests/unit_tests/test_errors.py +++ b/tests/unit_tests/test_errors.py @@ -8,6 +8,7 @@ """ import pytest +from pathlib import Path from unittest.mock import Mock, PropertyMock from fab.errors import ( @@ -18,8 +19,11 @@ FabToolPsycloneAPI, FabToolNotAvailable, FabToolInvalidSetting, + FabAnalysisError, FabCommandError, FabCommandNotFound, + FabFileLoadError, + FabHashError, FabMultiCommandError, FabSourceNoFilesError, FabSourceMergeError, @@ -41,7 +45,27 @@ def test_unknown_library(self): """Test unknown library errors.""" err = FabUnknownLibraryError("mylib") - assert str(err) == "unknown library mylib" + assert str(err) == "unknown library 'mylib'" + + @pytest.mark.parametrize( + "fpath,lineno,message", + [ + (None, None, "test message"), + (Path("/tmp/myfile.f90"), None, "test message (myfile.f90)"), + (Path("/tmp/myfile.f90"), 2, "test message (myfile.f90:2)"), + ], + ids=["message only", "fpath", "fpath+lineno"], + ) + def test_fab_analysis_error(self, fpath, lineno, message): + """Test analsys/parse errors.""" + + err = FabAnalysisError("test message", fpath, lineno) + assert str(err) == message + + def test_fab_file_load_error(self): + + err = FabFileLoadError("test message", Path("/tmp/FabFile")) + assert str(err) == "test message" class TestToolErrors: @@ -55,11 +79,6 @@ def test_tool_string(self): # Mock defines an internal name property, so special handling # is required to reset the value to support testing - tool = Mock(_name="tool cc") - del tool.name - err = FabToolError(tool, "compiler message") - assert str(err) == "[tool cc] compiler message" - category = Mock() del category._name type(category).name = PropertyMock(return_value="category cc") @@ -100,7 +119,7 @@ def test_not_available(self): assert str(err) == "[psyclone] not available" err = FabToolNotAvailable("gfortran", "GCC") - assert str(err) == "[gfortran] not available in suite GCC" + assert str(err) == "[gfortran] not available in GCC" def test_invalid_setting(self): """Test invalid setting class.""" @@ -119,19 +138,19 @@ def test_command(self): """Test FabCommandError in various configurations.""" err = FabCommandError(["ls", "-l", "/nosuch"], 1, b"", b"ls: cannot", "/") - assert str(err) == "command 'ls -l /nosuch' returned 1" + assert str(err) == "return code 1 from 'ls -l /nosuch'" err = FabCommandError("ls -l /nosuch", 1, None, "ls: cannot", "/") - assert str(err) == "command 'ls -l /nosuch' returned 1" + assert str(err) == "return code 1 from 'ls -l /nosuch'" def test_not_found(self): """Test command not found errors.""" err = FabCommandNotFound(["ls", "-l"]) - assert str(err) == "unable to execute ls" + assert str(err) == "unable to execute 'ls'" err = FabCommandNotFound("ls -l") - assert str(err) == "unable to execute ls" + assert str(err) == "unable to execute 'ls'" with pytest.raises(ValueError) as exc: FabCommandNotFound({"a": 1}) @@ -170,10 +189,19 @@ def test_merge(self): assert str(err) == "[git] merge failed: conflicting source files" err = FabSourceMergeError("git", "conflicting source files", "vn1.1") - assert str(err) == "[git] merge of vn1.1 failed: conflicting source files" + assert str(err) == "[git] merge of 'vn1.1' failed: conflicting source files" def test_fetch(self): """Test fetch errors.""" err = FabSourceFetchError("/my/dir1", "no such directory") assert str(err) == "could not fetch /my/dir1: no such directory" + + +class TestAnalyse: + """Test analyse and source processing exceptions.""" + + def test_hash(self): + + err = FabHashError(Path("/abc/test.f90")) + assert str(err) == "failed to create hash for /abc/test.f90" diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 33481fc14..81e839f35 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -145,10 +145,8 @@ def test_compiler_hash_invalid_version(): # returns an invalid compiler version string with mock.patch.object(cc, "run", mock.Mock(return_value='foo v1')): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): cc.get_hash() - assert isinstance(err.value, FabToolInvalidVersion) - assert "[gcc] invalid version" in str(err.value) def test_compiler_syntax_only(): @@ -300,9 +298,8 @@ def test_get_version_1_part_version(): c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion) as err: c.get_version() - assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -361,9 +358,8 @@ def test_get_version_non_int_version_format(version): c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion) as err: c.get_version() - assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) @@ -380,33 +376,27 @@ def test_get_version_unknown_version_format(): c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion) as err: c.get_version() - assert isinstance(err.value, FabToolInvalidVersion) assert expected_error in str(err.value) def test_get_version_command_failure(): '''If the version command fails, we must raise an error.''' c = Gfortran(exec_name="does_not_exist") - with raises(RuntimeError) as err: + with raises(FabToolError): c.get_version() - assert isinstance(err.value, FabToolError) - assert "unable to get compiler version" in str(err.value) def test_get_version_unknown_command_response(): '''If the full version output is in an unknown format, we must raise an error.''' full_output = 'GNU Fortran 1.2.3' - expected_error = "invalid version" c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): c.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert expected_error in str(err.value) def test_get_version_good_result_is_cached(): @@ -432,7 +422,7 @@ def test_get_version_bad_result_is_not_cached(): # Set up the compiler to fail the first time c = Gfortran() with mock.patch.object(c, 'run', side_effect=RuntimeError("")): - with raises(RuntimeError): + with raises(FabToolError): c.get_version() # Now let the run method run successfully and we should get the version. @@ -474,10 +464,8 @@ def test_gcc_get_version_with_icc_string(): """) with mock.patch.object(gcc, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): gcc.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert "invalid version" in str(err.value) # ============================================================================ @@ -581,11 +569,8 @@ def test_gfortran_get_version_with_ifort_string(): gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): gfortran.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -620,11 +605,8 @@ def test_icc_get_version_with_gcc_string(): """) icc = Icc() with mock.patch.object(icc, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): icc.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -696,11 +678,8 @@ def test_ifort_get_version_with_icc_string(): """) ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): ifort.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) @mark.parametrize("version", ["5.15f.2", @@ -717,11 +696,8 @@ def test_ifort_get_version_invalid_version(version): """) ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): ifort.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -761,11 +737,8 @@ def test_icx_get_version_with_icc_string(): """) icx = Icx() with mock.patch.object(icx, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): icx.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -801,11 +774,8 @@ def test_ifx_get_version_with_ifort_string(): """) ifx = Ifx() with mock.patch.object(ifx, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): ifx.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -841,11 +811,8 @@ def test_nvc_get_version_with_icc_string(): """) nvc = Nvc() with mock.patch.object(nvc, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): nvc.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -884,11 +851,8 @@ def test_nvfortran_get_version_with_ifort_string(): nvfortran = Nvfortran() with mock.patch.object(nvfortran, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): nvfortran.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -951,11 +915,8 @@ def test_craycc_get_version_with_icc_string(): """) craycc = Craycc() with mock.patch.object(craycc, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): craycc.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) # ============================================================================ @@ -1002,8 +963,5 @@ def test_crayftn_get_version_with_ifort_string(): crayftn = Crayftn() with mock.patch.object(crayftn, "run", mock.Mock(return_value=full_output)): - with raises(RuntimeError) as err: + with raises(FabToolInvalidVersion): crayftn.get_version() - assert isinstance(err.value, FabToolInvalidVersion) - assert ("invalid version" - in str(err.value)) diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index ff3eeef4e..b0addb5ea 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -140,10 +140,8 @@ def test_syntax_only(stub_c_compiler: CCompiler) -> None: assert mpif90.has_syntax_only mpicc = Mpicc(stub_c_compiler) - with raises(RuntimeError) as err: + with raises(FabToolError): _ = mpicc.has_syntax_only - assert isinstance(err.value, FabToolError) - assert str(err.value) == "[some C compiler] no syntax-only feature" def test_module_output(stub_fortran_compiler: FortranCompiler, @@ -161,10 +159,8 @@ def test_module_output(stub_fortran_compiler: FortranCompiler, assert stub_fortran_compiler._module_output_path == "/somewhere" mpicc = Mpicc(stub_c_compiler) - with raises(RuntimeError) as err: + with raises(FabToolError): mpicc.set_module_output_path(Path("/tmp")) - assert isinstance(err.value, FabToolError) - assert str(err.value) == "[some C compiler] no module output path feature" def test_fortran_with_add_args(stub_fortran_compiler: FortranCompiler, @@ -230,12 +226,10 @@ def test_c_with_add_args(stub_c_compiler: CCompiler, # Invoke C compiler with syntax-only flag (which is only supported # by Fortran compilers), which should raise an exception. - with raises(RuntimeError) as err: + with raises(FabToolError): mpicc.compile_file(Path("a.f90"), Path('a.o'), add_flags=["-O3"], syntax_only=True, config=stub_configuration) - assert isinstance(err.value, FabToolError) - assert (str(err.value) == "[some C compiler] syntax-only is Fortran-specific") # Check that providing the openmp flag in add_flag raises a warning: with warns(UserWarning, diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 008d262f9..970b686d7 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -9,6 +9,7 @@ import pytest +from fab.errors import FabProfileError from fab.tools import Flags, ProfileFlags from fab.util import string_checksum @@ -86,9 +87,9 @@ def test_profile_flags_with_profile(): # Check that we get an exception if we specify a profile # that does not exist - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: _ = pf["does_not_exist"] - assert "Profile 'does_not_exist' is not defined" in str(err.value) + assert "profile 'does_not_exist' not defined" in str(err.value) def test_profile_flags_without_profile(): @@ -101,15 +102,15 @@ def test_profile_flags_without_profile(): assert pf[""] == ["-base", "-base2", "-base3"] # Check that we get an exception if we specify a profile - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: _ = pf["does_not_exist"] - assert "Profile 'does_not_exist' is not defined" in str(err.value) + assert "profile 'does_not_exist' not defined" in str(err.value) # Check that we get an exception if we try to inherit from a profile # that does not exist - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: pf.define_profile("new_profile", "does_not_exist") - assert ("Inherited profile 'does_not_exist' is not defined." + assert ("profile 'does_not_exist' not defined for inheritance" in str(err.value)) # Test that inheriting from the default profile "" works @@ -182,21 +183,21 @@ def test_profile_flags_errors_invalid_profile_name(): ''' pf = ProfileFlags() pf.define_profile("base") - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: pf.define_profile("base") - assert "Profile 'base' is already defined." in str(err.value) + assert "profile 'base' already defined" in str(err.value) - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: pf.add_flags(["-some-flag"], "does not exist") - assert ("add_flags: Profile 'does not exist' is not defined." + assert ("profile 'does not exist' not defined" in str(err.value)) - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: pf.remove_flag("-some-flag", "does not exist") - assert ("remove_flag: Profile 'does not exist' is not defined." + assert ("profile 'does not exist' not defined" in str(err.value)) - with pytest.raises(KeyError) as err: + with pytest.raises(FabProfileError) as err: pf.checksum("does not exist") - assert ("checksum: Profile 'does not exist' is not defined." + assert ("profile 'does not exist' not defined" in str(err.value)) diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 39198ad14..6c89c4751 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -20,6 +20,8 @@ from fab.tools.compiler_wrapper import CompilerWrapper, Mpif90 from fab.tools.linker import Linker +from fab.errors import FabProfileError, FabUnknownLibraryError + def test_c_linker(stub_c_compiler: CCompiler) -> None: """ @@ -122,9 +124,8 @@ def test_get_lib_flags_unknown(stub_c_compiler: CCompiler) -> None: that is unknown. """ linker = Linker(compiler=stub_c_compiler) - with raises(RuntimeError) as err: + with raises(FabUnknownLibraryError): linker.get_lib_flags("unknown") - assert str(err.value) == "unknown library unknown" def test_add_lib_flags(stub_c_compiler: CCompiler) -> None: @@ -261,11 +262,10 @@ def test_c_with_unknown_library(stub_c_compiler: CCompiler, """ linker = Linker(compiler=stub_c_compiler) - with raises(RuntimeError) as err: + with raises(FabUnknownLibraryError): # Try to use "customlib" when we haven't added it to the linker linker.link([Path("a.o")], Path("a.out"), libs=["customlib"], config=stub_configuration) - assert str(err.value) == "unknown library customlib" def test_add_compiler_flag(stub_c_compiler: CCompiler, @@ -362,9 +362,8 @@ def test_linker_inheriting() -> None: compiler_linker.add_lib_flags("lib_a", ["a_from_1"]) assert compiler_linker.get_lib_flags("lib_a") == ["a_from_1"] - with raises(RuntimeError) as err: + with raises(FabUnknownLibraryError): wrapper_linker.get_lib_flags("does_not_exist") - assert str(err.value) == "unknown library does_not_exist" def test_linker_profile_flags_inheriting(stub_c_compiler): @@ -398,12 +397,11 @@ def test_linker_profile_modes(stub_linker): ''' # Make sure that we get the expected errors at the start: - with raises(KeyError) as err: + with raises(FabProfileError): stub_linker._pre_lib_flags["base"] - assert "Profile 'base' is not defined" in str(err.value) - with raises(KeyError) as err: + + with raises(FabProfileError): stub_linker._post_lib_flags["base"] - assert "Profile 'base' is not defined" in str(err.value) stub_linker.define_profile("base") assert stub_linker._pre_lib_flags["base"] == [] diff --git a/tests/unit_tests/tools/test_psyclone.py b/tests/unit_tests/tools/test_psyclone.py index e29eb4fa4..beed31611 100644 --- a/tests/unit_tests/tools/test_psyclone.py +++ b/tests/unit_tests/tools/test_psyclone.py @@ -107,11 +107,9 @@ def test_check_process_missing(fake_process: FakeProcess) -> None: psyclone = Psyclone() config = Mock() - with raises(RuntimeError) as err: + with raises(FabToolNotAvailable): psyclone.process(config, Path("x90file")) - assert isinstance(err.value, FabToolNotAvailable) - assert str(err.value).startswith("[psyclone] not available") def test_processing_errors_without_api(fake_process: FakeProcess) -> None: @@ -124,28 +122,22 @@ def test_processing_errors_without_api(fake_process: FakeProcess) -> None: psyclone = Psyclone() config = Mock() - with raises(RuntimeError) as err: + with raises(FabToolPsycloneAPI): psyclone.process(config, Path('x90file'), api=None, psy_file=Path('psy_file')) - assert isinstance(err.value, FabToolPsycloneAPI) - assert (str(err.value) == "[psyclone] called without API and with psy_file") - with raises(RuntimeError) as err: + with raises(FabToolPsycloneAPI): psyclone.process(config, Path('x90file'), api=None, alg_file=Path('alg_file')) - assert isinstance(err.value, FabToolPsycloneAPI) - assert (str(err.value) == "[psyclone] called without API and with alg_file") - with raises(RuntimeError) as err: + with raises(FabToolPsycloneAPI): psyclone.process(config, Path('x90file'), api=None) - assert isinstance(err.value, FabToolPsycloneAPI) - assert (str(err.value) == "[psyclone] called without API but not with transformed_file") @mark.parametrize("api", ["dynamo0.3", "lfric"]) @@ -160,35 +152,25 @@ def test_processing_errors_with_api(api: str, psyclone = Psyclone() config = Mock() - with raises(RuntimeError) as err: + with raises(FabToolPsycloneAPI): psyclone.process(config, Path("x90file"), api=api, psy_file=Path("psy_file")) - assert isinstance(err.value, FabToolPsycloneAPI) - assert str(err.value).startswith( - f"[psyclone] called with {api} API but not with alg_file" - ) - with raises(RuntimeError) as err: + + with raises(FabToolPsycloneAPI): psyclone.process(config, Path("x90file"), api=api, alg_file=Path("alg_file")) - assert isinstance(err.value, FabToolPsycloneAPI) - assert str(err.value).startswith( - f"[psyclone] called with {api} API but not with psy_file" - ) - with raises(RuntimeError) as err: + + with raises(FabToolPsycloneAPI): psyclone.process(config, Path("x90file"), api=api, psy_file=Path("psy_file"), alg_file=Path("alg_file"), transformed_file=Path("transformed_file")) - assert isinstance(err.value, FabToolPsycloneAPI) - assert str(err.value).startswith( - f"[psyclone] called with {api} API and with transformed_file" - ) @mark.parametrize("version", ["2.4.0", "2.5.0"]) diff --git a/tests/unit_tests/tools/test_tool.py b/tests/unit_tests/tools/test_tool.py index f37e955b3..8b1ecf1de 100644 --- a/tests/unit_tests/tools/test_tool.py +++ b/tests/unit_tests/tools/test_tool.py @@ -18,7 +18,7 @@ from fab.tools.flags import ProfileFlags from fab.tools.tool import CompilerSuiteTool, Tool -from fab.errors import FabCommandError, FabCommandNotFound +from fab.errors import FabCommandError, FabCommandNotFound, FabToolNotAvailable def test_constructor() -> None: @@ -89,9 +89,8 @@ def test_is_not_available(fake_process: FakeProcess) -> None: # When we try to run something with this tool, we should get # an exception now: - with raises(RuntimeError) as err: + with raises(FabToolNotAvailable): tool.run("--ops") - assert ("[gfortran] not available" in str(err.value)) def test_availability_argument(fake_process: FakeProcess) -> None: @@ -111,17 +110,15 @@ def test_run_missing(fake_process: FakeProcess) -> None: """ fake_process.register(['stool', '--ops'], callback=not_found_callback) tool = Tool("some tool", "stool", Category.MISC) - with raises(RuntimeError) as err: + with raises(FabCommandNotFound): tool.run("--ops") - assert isinstance(err.value, FabCommandNotFound) - assert str(err.value) == "unable to execute stool" # Check that stdout and stderr is returned fake_process.register(['stool', '--ops'], returncode=1, stdout="this is stdout", stderr="this is stderr") tool = Tool("some tool", "stool", Category.MISC) - with raises(RuntimeError) as err: + with raises(FabCommandError) as err: tool.run("--ops") assert isinstance(err.value, FabCommandError) assert "this is stdout" in str(err.value.output) @@ -203,10 +200,8 @@ def test_error(self, fake_process: FakeProcess) -> None: """ fake_process.register(['tool'], returncode=1, stdout="Beef.") tool = Tool("some tool", "tool", Category.MISC) - with raises(RuntimeError) as err: + with raises(FabCommandError) as err: tool.run() - assert isinstance(err.value, FabCommandError) - assert str(err.value) == "command 'tool' returned 1" assert err.value.code == 1 assert err.value.output == "Beef." assert err.value.error == "" @@ -219,10 +214,8 @@ def test_error_file_not_found(self, fake_process: FakeProcess) -> None: """ fake_process.register(['tool'], callback=not_found_callback) tool = Tool('some tool', 'tool', Category.MISC) - with raises(RuntimeError) as err: + with raises(FabCommandNotFound): tool.run() - assert isinstance(err.value, FabCommandNotFound) - assert str(err.value) == "unable to execute tool" assert call_list(fake_process) == [['tool']] diff --git a/tests/unit_tests/tools/test_tool_box.py b/tests/unit_tests/tools/test_tool_box.py index 94a3f0462..32cdfc9a4 100644 --- a/tests/unit_tests/tools/test_tool_box.py +++ b/tests/unit_tests/tools/test_tool_box.py @@ -16,7 +16,6 @@ from fab.tools.category import Category from fab.tools.compiler import CCompiler, FortranCompiler, Gfortran from fab.tools.tool_box import ToolBox -from fab.tools.tool_repository import ToolRepository from fab.errors import FabToolNotAvailable @@ -108,7 +107,5 @@ def test_add_unavailable_tool(fake_process: FakeProcess) -> None: tb = ToolBox() gfortran = Gfortran() - with raises(RuntimeError) as err: + with raises(FabToolNotAvailable): tb.add_tool(gfortran) - assert isinstance(err.value, FabToolNotAvailable) - assert str(err.value).startswith("[gfortran] not available") diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index fcae8f2d9..f7d9ac2fd 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -67,10 +67,9 @@ def test_tool_repository_get_tool_with_exec_name(stub_fortran_compiler): # If mpif90 is not available, an error is raised: mpif90._is_available = False - try: + + with raises(FabToolNotAvailable): tr.get_tool(Category.FORTRAN_COMPILER, "mpif90") - except KeyError as err: - assert "Unknown tool 'mpif90' in category" in str(err) # When using the exec name, the compiler must be available: mpif90._is_available = True @@ -100,14 +99,11 @@ def test_get_tool_error(): Tests error handling during tet_tool. """ tr = ToolRepository() - with raises(KeyError) as err: + with raises(FabToolNotAvailable): tr.get_tool("unknown-category", "something") - assert "Unknown category 'unknown-category'" in str(err.value) - with raises(KeyError) as err: + with raises(FabToolNotAvailable): tr.get_tool(Category.C_COMPILER, "something") - assert ("Unknown tool 'something' in category 'C_COMPILER'" - in str(err.value)) def test_get_default(stub_tool_repository, stub_fortran_compiler, @@ -132,10 +128,8 @@ def test_get_default_error_invalid_category() -> None: not e.g. a string. """ tr = ToolRepository() - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default("unknown-category-type") # type: ignore[arg-type] - assert isinstance(err.value, FabToolInvalidSetting) - assert "[str] invalid category" in str(err.value) def test_get_default_error_missing_mpi() -> None: @@ -144,15 +138,11 @@ def test_get_default_error_missing_mpi() -> None: parameter is missing (which is required for a compiler). """ tr = ToolRepository() - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.FORTRAN_COMPILER, openmp=True) - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[FORTRAN_COMPILER] invalid MPI setting" - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.FORTRAN_COMPILER, mpi=True) - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" def test_get_default_error_missing_openmp() -> None: @@ -162,16 +152,12 @@ def test_get_default_error_missing_openmp() -> None: """ tr = ToolRepository() - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.FORTRAN_COMPILER, mpi=True) - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.FORTRAN_COMPILER, mpi=True, openmp='123') # type: ignore[arg-type] - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" @mark.parametrize("mpi, openmp, message", @@ -191,9 +177,8 @@ def test_get_default_error_missing_compiler(mpi, openmp, message, tr = ToolRepository() monkeypatch.setitem(tr, Category.FORTRAN_COMPILER, []) - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting) as err: tr.get_default(Category.FORTRAN_COMPILER, mpi=mpi, openmp=openmp) - assert isinstance(err.value, FabToolInvalidSetting) assert message in str(err.value) @@ -211,10 +196,8 @@ def test_get_default_error_missing_openmp_compiler(monkeypatch) -> None: tr = ToolRepository() monkeypatch.setitem(tr, Category.FORTRAN_COMPILER, [fc]) - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.FORTRAN_COMPILER, mpi=False, openmp=True) - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[FORTRAN_COMPILER] invalid OpenMP setting" @mark.parametrize('category', [Category.C_COMPILER, @@ -260,10 +243,8 @@ def test_default_suite_unknown() -> None: Tests handling if a compiler suite is selected that does not exist. """ repo = ToolRepository() - with raises(RuntimeError) as err: + with raises(FabToolNotAvailable): repo.set_default_compiler_suite("does-not-exist") - assert isinstance(err.value, FabToolNotAvailable) - assert str(err.value) == "[FORTRAN_COMPILER] not available in suite does-not-exist" def test_no_tool_available(fake_process: FakeProcess) -> None: @@ -277,10 +258,8 @@ def test_no_tool_available(fake_process: FakeProcess) -> None: tr = ToolRepository() tr.set_default_compiler_suite("gnu") - with raises(RuntimeError) as err: + with raises(FabToolInvalidSetting): tr.get_default(Category.SHELL) - assert isinstance(err.value, FabToolInvalidSetting) - assert str(err.value) == "[SHELL] invalid category where tool names are sh" def test_tool_repository_full_path(fake_process: FakeProcess) -> None: diff --git a/tests/unit_tests/tools/test_versioning.py b/tests/unit_tests/tools/test_versioning.py index e49ee4af6..235f024ad 100644 --- a/tests/unit_tests/tools/test_versioning.py +++ b/tests/unit_tests/tools/test_versioning.py @@ -22,7 +22,7 @@ from fab.tools.category import Category from fab.tools.versioning import Fcm, Git, Subversion -from fab.errors import FabCommandError +from fab.errors import FabCommandError, FabSourceMergeError class TestGit: @@ -132,10 +132,8 @@ def test_git_fetch_error(self, fake_process: FakeProcess) -> None: ['git', 'fetch', '/src', 'revision'], returncode=1 ) git = Git() - with raises(RuntimeError) as err: + with raises(FabCommandError): git.fetch("/src", "/dst", revision="revision") - assert isinstance(err.value, FabCommandError) - assert str(err.value) == "command 'git fetch /src revision' returned 1" assert call_list(fake_process) == [ ['git', 'fetch', "/src", "revision"] ] @@ -165,10 +163,8 @@ def test_git_checkout_error(self, fake_process: FakeProcess) -> None: ) git = Git() - with raises(RuntimeError) as err: + with raises(FabCommandError): git.checkout("/src", "/dst", revision="revision") - assert isinstance(err.value, FabCommandError) - assert str(err.value) == "command 'git fetch /src revision' returned 1" assert call_list(fake_process) == [ ['git', 'fetch', "/src", "revision"] ] @@ -196,11 +192,8 @@ def test_git_merge_error(self, fake_process: FakeProcess) -> None: abort_record = fake_process.register(['git', 'merge', '--abort']) git = Git() - with raises(RuntimeError) as err: + with raises(FabSourceMergeError): git.merge("/dst", revision="revision") - assert str(err.value).startswith( - "[git] merge of revision failed:" - ) assert call_list(fake_process) == [ ['git', 'merge', 'FETCH_HEAD'], ['git', 'merge', '--abort'] @@ -218,9 +211,8 @@ def test_git_merge_collapse(self, fake_process: FakeProcess) -> None: returncode=1) git = Git() - with raises(RuntimeError) as err: + with raises(FabCommandError): git.merge("/dst", revision="revision") - assert str(err.value).startswith("command 'git merge") assert call_list(fake_process) == [ ['git', 'merge', 'FETCH_HEAD'], ['git', 'merge', '--abort'] From 15eb28b95aeacd4d35c14e528a40d7293bd55575 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Wed, 22 Oct 2025 15:20:12 +0100 Subject: [PATCH 5/7] Fix problems with CI testing There appear to be two problems with the github continuous integration tests. The format of the argparse help for choices is quoted in some versions of python, so omit the choice strings from the output check. The git system tests need details of the current user to run. Add a step which adds some temporary user details. --- .github/workflows/build.yml | 6 ++++++ tests/unit_tests/fab_base/test_fab_base.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 50524733a..ba553f9b5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -71,6 +71,12 @@ jobs: pip install .[dev] pip install .[docs] + # Some of the system tests require git to be configured + - name: Set up git configuration + run: | + git config --global user.email "fab" + git config --global user.name "Fab Testing" + - name: Type check with mypy run: python -m mypy source tests diff --git a/tests/unit_tests/fab_base/test_fab_base.py b/tests/unit_tests/fab_base/test_fab_base.py index e511361e2..3742f51e1 100644 --- a/tests/unit_tests/fab_base/test_fab_base.py +++ b/tests/unit_tests/fab_base/test_fab_base.py @@ -109,7 +109,7 @@ def test_arg_error(monkeypatch, capsys) -> None: _ = FabBase(name="test-help") captured = capsys.readouterr() - assert ("argument --host/-host: invalid choice: 'invalid' (choose from cpu, gpu)" + assert ("argument --host/-host: invalid choice: 'invalid' (choose from" in captured.err) From ebbb4bf903587b7bf2ce46ebac0496819a8bb0c9 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Thu, 23 Oct 2025 09:03:26 +0100 Subject: [PATCH 6/7] Add note about ExceptionGroup issue to docstring --- source/fab/errors.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/fab/errors.py b/source/fab/errors.py index e72da0559..a5b83eb95 100644 --- a/source/fab/errors.py +++ b/source/fab/errors.py @@ -264,8 +264,8 @@ class FabMultiCommandError(FabError): multiprocessing section into a single exception class for subsequent inspection. - This feature is is required because versions of python prior to - 3.11 do not support ExceptionGroups. + This feature is required because versions of python prior to + 3.11 do not support ExceptionGroups. See issue #513 on github. :param errors: a list ot exceptions :param label: an identifier for the multiprocessing section From 7b52c4736522dcd8dcf5107da89e55cf742e4464 Mon Sep 17 00:00:00 2001 From: Sam Clarke-Green Date: Thu, 23 Oct 2025 11:06:16 +0100 Subject: [PATCH 7/7] Apply remaining reviewer suggestions --- source/fab/errors.py | 35 +++++++++++++++------------------ source/fab/steps/grab/svn.py | 2 +- source/fab/tools/versioning.py | 2 +- tests/unit_tests/test_errors.py | 10 +++++----- 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/source/fab/errors.py b/source/fab/errors.py index a5b83eb95..8bdae6c54 100644 --- a/source/fab/errors.py +++ b/source/fab/errors.py @@ -61,14 +61,14 @@ class FabToolMismatch(FabToolError): def __init__( self, - tool: Union["Category", "Tool", str], - category: Union["Category", "Tool", type, str], + toolname: str, + category: Union["Category", "Tool", type], expected: str, ) -> None: self.category = category self.expected = expected - super().__init__(tool, f"got type {category} instead of {expected}") + super().__init__(toolname, f"got type {category} instead of {expected}") class FabToolInvalidVersion(FabToolError): @@ -85,7 +85,7 @@ class FabToolInvalidVersion(FabToolError): def __init__( self, - tool: Union["Category", "Tool", str], + toolname: str, value: str, expected: Optional[str] = None, ) -> None: @@ -96,7 +96,7 @@ def __init__( if expected is not None: message += f" should be {repr(expected)}" - super().__init__(tool, message) + super().__init__(toolname, message) class FabToolPsycloneAPI(FabToolError): @@ -167,7 +167,7 @@ class FabToolInvalidSetting(FabToolError): def __init__( self, setting_type: str, - tool: Union["Category", "Tool", str], + category: Union["Category", str], additional: Optional[str] = None, ) -> None: self.setting_type = setting_type @@ -176,7 +176,7 @@ def __init__( if additional: message += f" {additional}" - super().__init__(tool, message) + super().__init__(category, message) class FabUnknownLibraryError(FabError): @@ -209,16 +209,13 @@ class FabCommandError(FabError): def __init__( self, - command: str, + command: List[str], code: int, output: Union[str, bytes, None], error: Union[str, bytes, None], - cwd: Optional[Union[str, Path]] = None, + cwd: Optional[Union[Path, str]] = None, ) -> None: - if isinstance(command, list): - self.command: str = " ".join(command) - else: - self.command = str(command) + self.command: str = " ".join(command) self.code = int(code) self.output = self._decode(output) self.error = self._decode(error) @@ -245,7 +242,7 @@ class FabCommandNotFound(FabError): preserved for inspection by the caller. """ - def __init__(self, command: str) -> None: + def __init__(self, command: Union[List[str], str]) -> None: self.command = command if isinstance(command, list): self.target: str = command[0] @@ -271,9 +268,7 @@ class FabMultiCommandError(FabError): :param label: an identifier for the multiprocessing section """ - def __init__( - self, errors: List[Union[str, Exception]], label: Optional[str] = None - ) -> None: + def __init__(self, errors: List[Exception], label: Optional[str] = None) -> None: self.errors = errors self.label = label or "during multiprocessing" @@ -314,11 +309,13 @@ class FabSourceMergeError(FabSourceError): targeted """ - def __init__(self, tool: str, reason: str, revision: Optional[str] = None) -> None: + def __init__( + self, tool: "Tool", reason: str, revision: Optional[str] = None + ) -> None: self.tool = tool self.reason = reason - message = f"[{tool}] merge " + message = f"[{tool.name}] merge " if revision: message += f"of {repr(revision)} " message += f"failed: {reason}" diff --git a/source/fab/steps/grab/svn.py b/source/fab/steps/grab/svn.py index 126994649..0e4f503e8 100644 --- a/source/fab/steps/grab/svn.py +++ b/source/fab/steps/grab/svn.py @@ -125,6 +125,6 @@ def check_conflict(tool: Versioning, dst: Union[str, Path]): for element in entry: if (element.tag == 'wc-status' and element.attrib['item'] == 'conflicted'): - raise FabSourceMergeError("svn", xml_str) + raise FabSourceMergeError(tool, xml_str) return False diff --git a/source/fab/tools/versioning.py b/source/fab/tools/versioning.py index de12bc747..d72273da0 100644 --- a/source/fab/tools/versioning.py +++ b/source/fab/tools/versioning.py @@ -109,7 +109,7 @@ def merge(self, dst: Union[str, Path], self.run(['merge', 'FETCH_HEAD'], cwd=dst, capture_output=False) except RuntimeError as err: self.run(['merge', '--abort'], cwd=dst, capture_output=False) - raise FabSourceMergeError("git", str(err), revision) from err + raise FabSourceMergeError(self, str(err), revision) from err # ============================================================================= diff --git a/tests/unit_tests/test_errors.py b/tests/unit_tests/test_errors.py index 6ba816a34..f38978643 100644 --- a/tests/unit_tests/test_errors.py +++ b/tests/unit_tests/test_errors.py @@ -140,9 +140,6 @@ def test_command(self): err = FabCommandError(["ls", "-l", "/nosuch"], 1, b"", b"ls: cannot", "/") assert str(err) == "return code 1 from 'ls -l /nosuch'" - err = FabCommandError("ls -l /nosuch", 1, None, "ls: cannot", "/") - assert str(err) == "return code 1 from 'ls -l /nosuch'" - def test_not_found(self): """Test command not found errors.""" @@ -185,10 +182,13 @@ def test_no_files(self): def test_merge(self): """Test merge errors.""" - err = FabSourceMergeError("git", "conflicting source files") + tool = Mock() + type(tool).name = PropertyMock(return_value="git") + + err = FabSourceMergeError(tool, "conflicting source files") assert str(err) == "[git] merge failed: conflicting source files" - err = FabSourceMergeError("git", "conflicting source files", "vn1.1") + err = FabSourceMergeError(tool, "conflicting source files", "vn1.1") assert str(err) == "[git] merge of 'vn1.1' failed: conflicting source files" def test_fetch(self):