From ac63d9e846296db04c99cd62703a5a3d34b08708 Mon Sep 17 00:00:00 2001 From: graepaul_amdeng Date: Fri, 18 Sep 2026 10:06:48 -0700 Subject: [PATCH 1/5] Adding some tests --- nodescraper/cli/cli.py | 1 + nodescraper/cli/compare_runs.py | 2 +- nodescraper/cli/dynamicparserbuilder.py | 29 +- nodescraper/cli/helper.py | 23 +- nodescraper/cli/inputargtypes.py | 8 + nodescraper/configregistry.py | 23 +- .../connection/inband/inbandmanager.py | 4 + nodescraper/connection/inband/inbandremote.py | 11 +- .../connection/redfish/redfish_connection.py | 3 + .../connection/redfish/redfish_oem_diag.py | 21 +- nodescraper/interfaces/connectionmanager.py | 2 +- nodescraper/interfaces/dataanalyzertask.py | 6 +- nodescraper/models/collectorargs.py | 8 +- nodescraper/pluginregistry.py | 4 +- nodescraper/typeutils.py | 54 ++- .../connection/inband/test_shellcommand.py | 19 + .../redfish/test_redfish_oem_diag.py | 420 ++++++++++-------- test/unit/framework/test_cli_helper.py | 74 +++ test/unit/framework/test_compare_runs.py | 37 ++ test/unit/framework/test_config_registry.py | 13 + .../test_connection_manager_entrypoints.py | 18 + test/unit/framework/test_dataanalyzer.py | 28 ++ test/unit/framework/test_datacollector.py | 65 +++ test/unit/framework/test_dataplugin.py | 109 +++++ test/unit/framework/test_file_artifact.py | 12 + test/unit/framework/test_match_ignore.py | 6 + test/unit/framework/test_regexanalyzer.py | 18 + test/unit/framework/test_type_utils.py | 18 + 28 files changed, 788 insertions(+), 248 deletions(-) diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index a9671563..1e1b9783 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -649,6 +649,7 @@ def main( ) if parsed_args.skip_sudo: + # Add skip_sudo to the collection_args of the last plugin config instance plugin_config_inst_list[-1].global_args.setdefault("collection_args", {})[ "skip_sudo" ] = True diff --git a/nodescraper/cli/compare_runs.py b/nodescraper/cli/compare_runs.py index acbb92ca..f116e8fb 100644 --- a/nodescraper/cli/compare_runs.py +++ b/nodescraper/cli/compare_runs.py @@ -125,7 +125,7 @@ def _load_plugin_data_from_run( res_payload = json.loads(Path(res_path).read_text(encoding="utf-8")) task_res = TaskResult(**res_payload) plugin_name = task_res.parent - except (json.JSONDecodeError, TypeError, OSError) as e: + except (json.JSONDecodeError, TypeError, OSError, ValidationError) as e: logger.warning("Skipping %s: failed to load result: %s", res_path, e) continue diff --git a/nodescraper/cli/dynamicparserbuilder.py b/nodescraper/cli/dynamicparserbuilder.py index 8c0c9c68..7a608f8e 100644 --- a/nodescraper/cli/dynamicparserbuilder.py +++ b/nodescraper/cli/dynamicparserbuilder.py @@ -156,11 +156,14 @@ def get_literal_choices(cls, type_class_map: dict) -> Optional[list]: Returns: Optional[list]: list of valid choices for the Literal type, or None if not a Literal """ - # Check if Literal is in the type_class_map literal_type = type_class_map.get(Literal) - if literal_type and literal_type.inner_type is not None: + if literal_type is None or literal_type.inner_type is None: return None - return None + + values = literal_type.inner_type + if isinstance(values, (list, tuple)): + return list(values) + return [values] def add_argument( self, @@ -181,14 +184,16 @@ def add_argument( """ add_kw = {} if help_text is None else {"help": help_text} # Check for Literal types and extract choices - literal_choices = None - if Literal in type_class_map and annotation: - # Extract all arguments from the annotation - args = get_args(annotation) - for arg in args: - if get_origin(arg) is Literal: - literal_choices = list(get_args(arg)) - break + literal_choices = self.get_literal_choices(type_class_map) + if literal_choices is None and annotation is not None: + # fall back to pulling the choices out of the raw annotation + if get_origin(annotation) is Literal: + literal_choices = list(get_args(annotation)) + else: + for arg in get_args(annotation): + if get_origin(arg) is Literal: + literal_choices = list(get_args(arg)) + break if list in type_class_map: type_class = type_class_map[list] @@ -225,7 +230,7 @@ def add_argument( type=str, required=required, choices=literal_choices, - metavar=f"{{{','.join(literal_choices)}}}", + metavar=f"{{{','.join(str(choice) for choice in literal_choices)}}}", **add_kw, ) elif float in type_class_map: diff --git a/nodescraper/cli/helper.py b/nodescraper/cli/helper.py index 6eb0f1b6..e6ef2b7c 100644 --- a/nodescraper/cli/helper.py +++ b/nodescraper/cli/helper.py @@ -30,6 +30,7 @@ import logging import os import sys +from copy import deepcopy from pathlib import Path from typing import Optional, Sequence, Tuple @@ -115,14 +116,15 @@ def get_plugin_configs( base_config.global_args["system_interaction_level"] = system_interaction_level - plugin_configs = [base_config] + # Copy each until we are done + plugin_configs = [deepcopy(c) for c in [base_config]] if plugin_config_input: for config in plugin_config_input: if os.path.exists(config): plugin_configs.append(ModelArgHandler(PluginConfig).process_file_arg(config)) elif config in built_in_configs: - plugin_configs.append(built_in_configs[config]) + plugin_configs.append(deepcopy(built_in_configs[config])) else: raise argparse.ArgumentTypeError(f"No plugin config found for: {config}") @@ -269,7 +271,11 @@ def parse_gen_plugin_config( """ try: config = build_config( - config_reg, plugin_reg, logger, parsed_args.plugins, parsed_args.built_in_configs + config_reg, + plugin_reg, + logger, + parsed_args.plugins, + parsed_args.built_in_configs, ) config.name = parsed_args.config_name.split(".")[0] @@ -367,7 +373,7 @@ def generate_reference_config( data_model = obj.result_data.system_data if data_model is None: - logger.warning("Plugin: %s data model not found: %s, skipping", obj.source) + logger.warning("Plugin: %s data model not found: %s, skipping", obj.source, data_model) continue plugin = plugin_reg.plugins.get(obj.source) @@ -585,15 +591,20 @@ def dump_to_csv(all_rows: list, filename: str, fieldnames: list[str], logger: lo fieldnames (list[str]): header for csv file logger (logging.Logger): isntance of logger """ + DEFAULT_ERR_MSG_PREFIX = "Could not dump data to csv file" try: with open(filename, "w", newline="") as f: writer = csv.DictWriter(f, fieldnames=fieldnames) writer.writeheader() for row in all_rows: writer.writerow(row) + logger.info("Data written to csv file: %s", filename) + except FileNotFoundError as fnf_exp: + logger.error("%s, File not found: %s", DEFAULT_ERR_MSG_PREFIX, fnf_exp) + except ValueError as val_exp: + logger.error("%s, Value error: %s", DEFAULT_ERR_MSG_PREFIX, val_exp) except Exception as exp: - logger.error("Could not dump data to csv file: %s", exp) - logger.info("Data written to csv file: %s", filename) + logger.error("%s, Exception: %s", DEFAULT_ERR_MSG_PREFIX, exp) def generate_summary( diff --git a/nodescraper/cli/inputargtypes.py b/nodescraper/cli/inputargtypes.py index 7faa9c5c..18ead989 100644 --- a/nodescraper/cli/inputargtypes.py +++ b/nodescraper/cli/inputargtypes.py @@ -86,6 +86,13 @@ class ModelArgHandler(Generic[TModelType]): def __init__(self, model: Type[TModelType]) -> None: self.model = model + def arg_check(self, json_arg_input_data: dict): + """Check the validity of the JSON argument input data. + It must be a dict which can be ** into a pydantic model of the specified type. + """ + if not isinstance(json_arg_input_data, dict): + raise argparse.ArgumentTypeError("Input data must be a dictionary.") + def process_file_arg(self, file_path: str) -> TModelType: """load a json file into a pydantic model @@ -99,6 +106,7 @@ def process_file_arg(self, file_path: str) -> TModelType: TModelType: model instance """ data = json_arg(file_path) + self.arg_check(data) try: return self.model(**data) except ValidationError as e: diff --git a/nodescraper/configregistry.py b/nodescraper/configregistry.py index f6e9f37a..1d5cdcaf 100644 --- a/nodescraper/configregistry.py +++ b/nodescraper/configregistry.py @@ -79,16 +79,19 @@ def load_configs(self, config_path: Optional[str] = None): config_path = Path(config_path) for config_file in config_path.glob("*.json"): - with open(config_file, "r", encoding="utf-8") as in_file: - try: - file_data = json.load(in_file) - config_model = PluginConfig(**file_data) - if config_model.name: - self.configs[config_model.name] = config_model - else: - self.configs[config_file.name] = config_model - except (ValidationError, json.JSONDecodeError): - pass + try: + with open(config_file, "r", encoding="utf-8") as in_file: + try: + file_data = json.load(in_file) + config_model = PluginConfig(**file_data) + if config_model.name: + self.configs[config_model.name] = config_model + else: + self.configs[config_file.name] = config_model + except (ValidationError, json.JSONDecodeError, TypeError) as e: + raise RuntimeError(f"Failed to load config from {config_file}: {e}") + except (OSError, IOError, FileNotFoundError): + raise RuntimeError(f"Failed to open config file {config_file}") @staticmethod def _entry_points_for_group(group: str): diff --git a/nodescraper/connection/inband/inbandmanager.py b/nodescraper/connection/inband/inbandmanager.py index af369cdd..f0410e5e 100644 --- a/nodescraper/connection/inband/inbandmanager.py +++ b/nodescraper/connection/inband/inbandmanager.py @@ -119,6 +119,8 @@ def connect( priority=EventPriority.CRITICAL, console_log=True, ) + self.connection = None # Nullify the connection since its not usable + self.result.status = ExecutionStatus.EXECUTION_FAILURE except Exception as exception: self._log_event( category=EventCategory.SSH, @@ -127,6 +129,8 @@ def connect( priority=EventPriority.CRITICAL, console_log=True, ) + self.connection = None # Nullify the connection since its not usable + self.result.status = ExecutionStatus.EXECUTION_FAILURE return self.result def disconnect(self): diff --git a/nodescraper/connection/inband/inbandremote.py b/nodescraper/connection/inband/inbandremote.py index a528e513..282e3c47 100644 --- a/nodescraper/connection/inband/inbandremote.py +++ b/nodescraper/connection/inband/inbandremote.py @@ -154,16 +154,17 @@ def run_command( try: stdin, stdout, stderr = self.client.exec_command(cmd_str, timeout=timeout) - if write_password: stdin.write( - self.ssh_params.password.get_secret_value() - if self.ssh_params.password - else "" + "\n" + ( + self.ssh_params.password.get_secret_value() + if self.ssh_params.password + else "" + ) + + "\n" ) stdin.flush() stdin.channel.shutdown_write() - stdout_str = stdout.read().decode("utf-8", errors="replace") stderr_str = stderr.read().decode("utf-8", errors="replace") exit_code = stdout.channel.recv_exit_status() diff --git a/nodescraper/connection/redfish/redfish_connection.py b/nodescraper/connection/redfish/redfish_connection.py index d8cbcd2b..23a4ae86 100644 --- a/nodescraper/connection/redfish/redfish_connection.py +++ b/nodescraper/connection/redfish/redfish_connection.py @@ -327,6 +327,9 @@ def close(self) -> None: self._session.delete(self._session_uri, timeout=self.timeout) except Exception: pass + + if self._session: + self._session.close() self._session = None self._session_token = None self._session_uri = None diff --git a/nodescraper/connection/redfish/redfish_oem_diag.py b/nodescraper/connection/redfish/redfish_oem_diag.py index affabf6e..e6a5b55d 100644 --- a/nodescraper/connection/redfish/redfish_oem_diag.py +++ b/nodescraper/connection/redfish/redfish_oem_diag.py @@ -202,7 +202,9 @@ def _download_log_and_save( try: metadata_file.write_text(json.dumps(log_entry_json, indent=2), encoding="utf-8") log.info( - "Log metadata written to disk: %s -> %s", oem_diagnostic_type, metadata_file.name + "Log metadata written to disk: %s -> %s", + oem_diagnostic_type, + metadata_file.name, ) except Exception as e: log.exception("Failed to write log metadata to %s: %s", metadata_file, e) @@ -238,6 +240,7 @@ def collect_oem_diagnostic_data( (log_bytes, log_entry_metadata_dict, error_message). On success: (bytes, dict, None). On failure: (None, None, error_str). """ + SLEEP_S_DEFAULT = 1 log = logger if logger is not None else _module_logger if not oem_diagnostic_type or not oem_diagnostic_type.strip(): return None, None, "oem_diagnostic_type is required" @@ -249,7 +252,10 @@ def collect_oem_diagnostic_data( ) path_prefix = log_service_path.rstrip("/") action_path = f"{path_prefix}/Actions/LogService.CollectDiagnosticData" - payload = {"DiagnosticDataType": "OEM", "OEMDiagnosticDataType": oem_diagnostic_type} + payload = { + "DiagnosticDataType": "OEM", + "OEMDiagnosticDataType": oem_diagnostic_type, + } try: resp: Response = conn.post(action_path, json=payload) @@ -267,7 +273,10 @@ def collect_oem_diagnostic_data( location_header = resp.headers.get("Location") or resp.headers.get("Content-Location") if location_header and not location_header.startswith("http"): location_header = _resolve_path(conn, location_header) - sleep_s = int(resp.headers.get("Retry-After", 1) or 1) + try: + sleep_s = int(resp.headers.get("Retry-After", SLEEP_S_DEFAULT) or SLEEP_S_DEFAULT) + except ValueError: + sleep_s = SLEEP_S_DEFAULT try: oem_response = resp.json() except Exception: @@ -330,7 +339,11 @@ def collect_oem_diagnostic_data( return None, None, f"Task GET failed: {task_resp.status_code}" task_json = task_resp.json() if task_json.get("TaskState") != TaskState.completed.value: - return None, None, f"Task did not complete: TaskState={task_json.get('TaskState')}" + return ( + None, + None, + f"Task did not complete: TaskState={task_json.get('TaskState')}", + ) # LogEntry location from Payload.HttpHeaders headers_list = task_json.get("Payload", {}).get("HttpHeaders", []) or [] diff --git a/nodescraper/interfaces/connectionmanager.py b/nodescraper/interfaces/connectionmanager.py index 7c649021..60212130 100644 --- a/nodescraper/interfaces/connectionmanager.py +++ b/nodescraper/interfaces/connectionmanager.py @@ -95,7 +95,7 @@ def __init__( logger: Optional[logging.Logger] = None, max_event_priority_level: Union[EventPriority, str] = EventPriority.CRITICAL, parent: Optional[str] = None, - task_result_hooks: Optional[list[TaskResultHook], None] = None, + task_result_hooks: Optional[list[TaskResultHook]] = None, connection_args: Optional[Union[TConnectArg, dict[str, Any]]] = None, event_reporter: str = DEFAULT_EVENT_REPORTER, session_id: Optional[str] = None, diff --git a/nodescraper/interfaces/dataanalyzertask.py b/nodescraper/interfaces/dataanalyzertask.py index c91fb251..27936042 100644 --- a/nodescraper/interfaces/dataanalyzertask.py +++ b/nodescraper/interfaces/dataanalyzertask.py @@ -118,8 +118,12 @@ class DataAnalyzer(Task, abc.ABC, Generic[TDataModel, TAnalyzeArg]): def __init_subclass__(cls, **kwargs: dict[str, Any]) -> None: super().__init_subclass__(**kwargs) - if not inspect.isabstract(cls) and cls.DATA_MODEL is None: + if (not inspect.isabstract(cls) and not getattr(cls, "DATA_MODEL", None)) or ( + not inspect.isabstract(cls) and cls.DATA_MODEL is None + ): raise TypeError(f"No data model set for {cls.__name__}") + if not hasattr(cls, "analyze_data") or not callable(cls.analyze_data): + raise TypeError(f"No analyze_data method defined for {cls.__name__}") if "analyze_data" in vars(cls): setattr(cls, "analyze_data", analyze_decorator(cls.analyze_data)) # noqa diff --git a/nodescraper/models/collectorargs.py b/nodescraper/models/collectorargs.py index 02ba532f..56447c5d 100644 --- a/nodescraper/models/collectorargs.py +++ b/nodescraper/models/collectorargs.py @@ -23,10 +23,14 @@ # SOFTWARE. # ############################################################################### -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field class CollectorArgs(BaseModel): + """The base for all collector args in node-scraper. By default disallow extra args, they will raise Validation Error.""" + + model_config = ConfigDict(extra="forbid") + html_view: bool = Field( default=False, description=( @@ -34,5 +38,3 @@ class CollectorArgs(BaseModel): "using human-readable output." ), ) - - model_config = {"extra": "forbid", "exclude_none": True} diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index 537f470c..47c46979 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -221,11 +221,11 @@ def load_connection_managers_from_entry_points() -> dict[str, type]: PluginRegistry._use_cache and PluginRegistry._entry_point_connection_managers_cache is not None ): - return PluginRegistry._entry_point_connection_managers_cache + return PluginRegistry._entry_point_connection_managers_cache.copy() # If caching disabled, skip lock and always reload if not PluginRegistry._use_cache: - return PluginRegistry._load_connection_managers_uncached() + return PluginRegistry._load_connection_managers_uncached().copy() with PluginRegistry._cache_lock: # Check again inside the lock to prevent duplicate work diff --git a/nodescraper/typeutils.py b/nodescraper/typeutils.py index bc4ce244..65ccf605 100644 --- a/nodescraper/typeutils.py +++ b/nodescraper/typeutils.py @@ -25,7 +25,17 @@ ############################################################################### import inspect import types -from typing import Annotated, Any, Callable, Optional, Type, Union, get_args, get_origin +from typing import ( + Annotated, + Any, + Callable, + Literal, + Optional, + Type, + Union, + get_args, + get_origin, +) from pydantic import BaseModel, Field @@ -34,6 +44,7 @@ class TypeClass(BaseModel): """Class to hold type class information""" type_class: Any + # for a Literal type class this holds the full list of allowed values inner_type: Optional[Any] = None @@ -110,6 +121,26 @@ def get_func_arg_types( return type_map + @classmethod + def build_type_class(cls, input_type: Any, origin: Any) -> TypeClass: + """Build a TypeClass for a parameterized type + + Args: + input_type (Any): parameterized type, e.g. list[str] or Literal["a", "b"] + origin (Any): origin of the type + + Returns: + TypeClass: type class with inner type details + """ + if origin is Literal: + # args of a Literal are values rather than types, so keep all of them + return TypeClass(type_class=Literal, inner_type=list(get_args(input_type))) + + return TypeClass( + type_class=origin, + inner_type=next((arg for arg in get_args(input_type) if arg is not type(None)), None), + ) + @classmethod def process_type(cls, input_type: type[Any]) -> list[TypeClass]: """Process a type to extract its class and any inner types @@ -131,28 +162,17 @@ def process_type(cls, input_type: type[Any]) -> list[TypeClass]: input_types = [arg for arg in input_type.__args__ if arg is not type(None)] for type_item in input_types: origin = get_origin(type_item) + if origin is Annotated: + type_item = get_args(type_item)[0] + origin = get_origin(type_item) if origin is None: type_classes.append(TypeClass(type_class=type_item)) else: - type_classes.append( - TypeClass( - type_class=origin, - inner_type=next( - (arg for arg in get_args(type_item) if arg is not type(None)), None - ), - ) - ) + type_classes.append(cls.build_type_class(type_item, origin)) return type_classes else: - return [ - TypeClass( - type_class=origin, - inner_type=next( - (arg for arg in get_args(input_type) if arg is not type(None)), None - ), - ) - ] + return [cls.build_type_class(input_type, origin)] @classmethod def get_model_types(cls, model: type[BaseModel]) -> dict[str, TypeData]: diff --git a/test/unit/connection/inband/test_shellcommand.py b/test/unit/connection/inband/test_shellcommand.py index 0a1b3fc2..c336b589 100644 --- a/test/unit/connection/inband/test_shellcommand.py +++ b/test/unit/connection/inband/test_shellcommand.py @@ -26,6 +26,8 @@ from unittest.mock import MagicMock, patch from nodescraper.connection.inband.inbandlocal import LocalShell +from nodescraper.connection.inband.inbandremote import RemoteShell +from nodescraper.connection.inband.sshparams import SSHConnectionParams @patch("nodescraper.connection.inband.inbandlocal.subprocess.run") @@ -46,3 +48,20 @@ def test_localshell_string_with_sudo(mock_run): shell.run_command("cat /etc/shadow", sudo=True) assert mock_run.call_args.args[0] == "sudo cat /etc/shadow" + + +@patch("nodescraper.connection.inband.inbandremote.paramiko.SSHClient") +def test_remoteshell_sudo_password_is_newline_terminated(mock_client_cls): + """sudo -S reads a line, so the password written to stdin must end with a newline.""" + stdin, stdout, stderr = MagicMock(), MagicMock(), MagicMock() + stdout.read.return_value = b"" + stderr.read.return_value = b"" + stdout.channel.recv_exit_status.return_value = 0 + mock_client_cls.return_value.exec_command.return_value = (stdin, stdout, stderr) + + shell = RemoteShell( + SSHConnectionParams(hostname="127.0.0.1", username="user", password="hunter2") + ) + shell.run_command("cat /etc/shadow", sudo=True) + + stdin.write.assert_called_once_with("hunter2\n") diff --git a/test/unit/connection/redfish/test_redfish_oem_diag.py b/test/unit/connection/redfish/test_redfish_oem_diag.py index 727c1b69..e8a77710 100644 --- a/test/unit/connection/redfish/test_redfish_oem_diag.py +++ b/test/unit/connection/redfish/test_redfish_oem_diag.py @@ -1,186 +1,234 @@ -############################################################################### -# -# MIT License -# -# Copyright (c) 2026 Advanced Micro Devices, Inc. -# -# Permission is hereby granted, free of charge, to any person obtaining a copy -# of this software and associated documentation files (the "Software"), to deal -# in the Software without restriction, including without limitation the rights -# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -# copies of the Software, and to permit persons to whom the Software is -# furnished to do so, subject to the following conditions: -# -# The above copyright notice and this permission notice shall be included in all -# copies or substantial portions of the Software. -# -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -# SOFTWARE. -# -############################################################################### -import logging -from unittest.mock import MagicMock - -from requests.status_codes import codes - -from nodescraper.connection.redfish import RedfishConnectionError -from nodescraper.connection.redfish.redfish_oem_diag import ( - DEFAULT_TASK_TIMEOUT_S, - RF_ANNOTATION_ALLOWABLE, - _download_log_and_save, - _get_task_monitor_uri, - _strip_port_from_url, - get_oem_diagnostic_allowable_values, -) - - -def test_rf_annotation_allowable_constant(): - assert RF_ANNOTATION_ALLOWABLE == "OEMDiagnosticDataType@Redfish.AllowableValues" - - -def test_default_task_timeout_s(): - assert DEFAULT_TASK_TIMEOUT_S == 1800 - - -class TestGetOemDiagnosticAllowableValues: - def test_returns_list_from_collect_action(self): - conn = MagicMock() - conn.get.return_value = { - "Actions": { - "LogService.CollectDiagnosticData": { - "OEMDiagnosticDataType@Redfish.AllowableValues": ["Dmesg", "AllLogs"], - } - } - } - result = get_oem_diagnostic_allowable_values( - conn, "redfish/v1/Systems/1/LogServices/DiagLogs" - ) - assert result == ["Dmesg", "AllLogs"] - - def test_returns_list_from_octothorpe_action_key(self): - conn = MagicMock() - conn.get.return_value = { - "Actions": { - "#LogService.CollectDiagnosticData": { - "OEMDiagnosticDataType@Redfish.AllowableValues": ["JournalControl"], - } - } - } - result = get_oem_diagnostic_allowable_values( - conn, "redfish/v1/Systems/UBB/LogServices/DiagLogs" - ) - assert result == ["JournalControl"] - - def test_returns_none_on_connection_error(self): - conn = MagicMock() - conn.get.side_effect = RedfishConnectionError("fail") - result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") - assert result is None - - def test_returns_none_when_data_not_dict(self): - conn = MagicMock() - conn.get.return_value = [] - result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") - assert result is None - - def test_returns_none_when_no_actions(self): - conn = MagicMock() - conn.get.return_value = {} - result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") - assert result is None - - -class TestStripPortFromUrl: - def test_strips_port_443(self): - url = "https://host:443/redfish/v1/TaskService/Tasks/1" - assert _strip_port_from_url(url) == "https://host/redfish/v1/TaskService/Tasks/1" - - def test_strips_other_port(self): - url = "https://host:8443/redfish/v1" - assert _strip_port_from_url(url) == "https://host/redfish/v1" - - def test_returns_none_when_no_port(self): - url = "https://host/redfish/v1" - assert _strip_port_from_url(url) is None - - def test_returns_none_for_relative_path(self): - assert _strip_port_from_url("redfish/v1/Systems/1") is None - - -class TestGetTaskMonitorUri: - def test_returns_task_monitor_from_body(self): - conn = MagicMock() - conn.base_url = "https://host/redfish/v1" - body = {"TaskMonitor": "TaskService/Tasks/1/Monitor"} - result = _get_task_monitor_uri(body, conn) - assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" - - def test_returns_from_odata_id_plus_monitor(self): - conn = MagicMock() - conn.base_url = "https://host/redfish/v1" - body = {"@odata.id": "TaskService/Tasks/1"} - result = _get_task_monitor_uri(body, conn) - assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" - - def test_returns_none_for_empty_body(self): - conn = MagicMock() - assert _get_task_monitor_uri({}, conn) is None - - def test_prefers_task_monitor_over_odata_id(self): - conn = MagicMock() - conn.base_url = "https://host/redfish/v1" - body = { - "TaskMonitor": "TaskService/Tasks/1/Monitor", - "@odata.id": "TaskService/Tasks/2", - } - result = _get_task_monitor_uri(body, conn) - assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" - - -class TestDownloadLogAndSave: - def test_returns_none_when_no_additional_data_uri(self): - conn = MagicMock() - log_entry_json = {"Id": "1", "Name": "LogEntry"} - result = _download_log_and_save( - conn, log_entry_json, "Dmesg", None, logging.getLogger("test") - ) - assert result is None - conn.get_response.assert_not_called() - - def test_downloads_and_returns_bytes_when_additional_data_uri_present(self): - conn = MagicMock() - conn.base_url = "https://host/redfish/v1" - resp = MagicMock() - resp.status_code = codes.ok - resp.content = b"log bytes" - conn.get_response.return_value = resp - log_entry_json = {"AdditionalDataURI": "/redfish/v1/LogServices/1/Entries/1/Attachment"} - result = _download_log_and_save( - conn, log_entry_json, "Dmesg", None, logging.getLogger("test") - ) - assert result == b"log bytes" - conn.get_response.assert_called_once() - - def test_writes_archive_and_metadata_to_output_dir(self, tmp_path): - conn = MagicMock() - conn.base_url = "https://host/redfish/v1" - resp = MagicMock() - resp.status_code = codes.ok - resp.content = b"log bytes" - conn.get_response.return_value = resp - log_entry_json = { - "AdditionalDataURI": "/redfish/v1/LogServices/1/Entries/1/Attachment", - "Id": "1", - } - result = _download_log_and_save( - conn, log_entry_json, "AllLogs", tmp_path, logging.getLogger("test") - ) - assert result == b"log bytes" - assert (tmp_path / "AllLogs.tar.xz").read_bytes() == b"log bytes" - metadata = (tmp_path / "AllLogs_log_entry.json").read_text(encoding="utf-8") - assert "Id" in metadata and "1" in metadata +############################################################################### +# +# MIT License +# +# Copyright (c) 2026 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### +import logging +from unittest.mock import MagicMock, patch + +from requests.status_codes import codes + +from nodescraper.connection.redfish import RedfishConnectionError +from nodescraper.connection.redfish.redfish_oem_diag import ( + DEFAULT_TASK_TIMEOUT_S, + RF_ANNOTATION_ALLOWABLE, + _download_log_and_save, + _get_task_monitor_uri, + _strip_port_from_url, + collect_oem_diagnostic_data, + get_oem_diagnostic_allowable_values, +) + + +def test_rf_annotation_allowable_constant(): + assert RF_ANNOTATION_ALLOWABLE == "OEMDiagnosticDataType@Redfish.AllowableValues" + + +def test_default_task_timeout_s(): + assert DEFAULT_TASK_TIMEOUT_S == 1800 + + +class TestGetOemDiagnosticAllowableValues: + def test_returns_list_from_collect_action(self): + conn = MagicMock() + conn.get.return_value = { + "Actions": { + "LogService.CollectDiagnosticData": { + "OEMDiagnosticDataType@Redfish.AllowableValues": ["Dmesg", "AllLogs"], + } + } + } + result = get_oem_diagnostic_allowable_values( + conn, "redfish/v1/Systems/1/LogServices/DiagLogs" + ) + assert result == ["Dmesg", "AllLogs"] + + def test_returns_list_from_octothorpe_action_key(self): + conn = MagicMock() + conn.get.return_value = { + "Actions": { + "#LogService.CollectDiagnosticData": { + "OEMDiagnosticDataType@Redfish.AllowableValues": ["JournalControl"], + } + } + } + result = get_oem_diagnostic_allowable_values( + conn, "redfish/v1/Systems/UBB/LogServices/DiagLogs" + ) + assert result == ["JournalControl"] + + def test_returns_none_on_connection_error(self): + conn = MagicMock() + conn.get.side_effect = RedfishConnectionError("fail") + result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") + assert result is None + + def test_returns_none_when_data_not_dict(self): + conn = MagicMock() + conn.get.return_value = [] + result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") + assert result is None + + def test_returns_none_when_no_actions(self): + conn = MagicMock() + conn.get.return_value = {} + result = get_oem_diagnostic_allowable_values(conn, "redfish/v1/LogServices/DiagLogs") + assert result is None + + +class TestStripPortFromUrl: + def test_strips_port_443(self): + url = "https://host:443/redfish/v1/TaskService/Tasks/1" + assert _strip_port_from_url(url) == "https://host/redfish/v1/TaskService/Tasks/1" + + def test_strips_other_port(self): + url = "https://host:8443/redfish/v1" + assert _strip_port_from_url(url) == "https://host/redfish/v1" + + def test_returns_none_when_no_port(self): + url = "https://host/redfish/v1" + assert _strip_port_from_url(url) is None + + def test_returns_none_for_relative_path(self): + assert _strip_port_from_url("redfish/v1/Systems/1") is None + + +class TestGetTaskMonitorUri: + def test_returns_task_monitor_from_body(self): + conn = MagicMock() + conn.base_url = "https://host/redfish/v1" + body = {"TaskMonitor": "TaskService/Tasks/1/Monitor"} + result = _get_task_monitor_uri(body, conn) + assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" + + def test_returns_from_odata_id_plus_monitor(self): + conn = MagicMock() + conn.base_url = "https://host/redfish/v1" + body = {"@odata.id": "TaskService/Tasks/1"} + result = _get_task_monitor_uri(body, conn) + assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" + + def test_returns_none_for_empty_body(self): + conn = MagicMock() + assert _get_task_monitor_uri({}, conn) is None + + def test_prefers_task_monitor_over_odata_id(self): + conn = MagicMock() + conn.base_url = "https://host/redfish/v1" + body = { + "TaskMonitor": "TaskService/Tasks/1/Monitor", + "@odata.id": "TaskService/Tasks/2", + } + result = _get_task_monitor_uri(body, conn) + assert result == "https://host/redfish/v1/TaskService/Tasks/1/Monitor" + + +class TestDownloadLogAndSave: + def test_returns_none_when_no_additional_data_uri(self): + conn = MagicMock() + log_entry_json = {"Id": "1", "Name": "LogEntry"} + result = _download_log_and_save( + conn, log_entry_json, "Dmesg", None, logging.getLogger("test") + ) + assert result is None + conn.get_response.assert_not_called() + + def test_downloads_and_returns_bytes_when_additional_data_uri_present(self): + conn = MagicMock() + conn.base_url = "https://host/redfish/v1" + resp = MagicMock() + resp.status_code = codes.ok + resp.content = b"log bytes" + conn.get_response.return_value = resp + log_entry_json = {"AdditionalDataURI": "/redfish/v1/LogServices/1/Entries/1/Attachment"} + result = _download_log_and_save( + conn, log_entry_json, "Dmesg", None, logging.getLogger("test") + ) + assert result == b"log bytes" + conn.get_response.assert_called_once() + + def test_writes_archive_and_metadata_to_output_dir(self, tmp_path): + conn = MagicMock() + conn.base_url = "https://host/redfish/v1" + resp = MagicMock() + resp.status_code = codes.ok + resp.content = b"log bytes" + conn.get_response.return_value = resp + log_entry_json = { + "AdditionalDataURI": "/redfish/v1/LogServices/1/Entries/1/Attachment", + "Id": "1", + } + result = _download_log_and_save( + conn, log_entry_json, "AllLogs", tmp_path, logging.getLogger("test") + ) + assert result == b"log bytes" + assert (tmp_path / "AllLogs.tar.xz").read_bytes() == b"log bytes" + metadata = (tmp_path / "AllLogs_log_entry.json").read_text(encoding="utf-8") + assert "Id" in metadata and "1" in metadata + + +class TestCollectOemDiagnosticDataRetryAfter: + @staticmethod + def _conn_for_retry_after(retry_after: str) -> MagicMock: + conn = MagicMock() + conn.base_url = "https://host" + post_resp = MagicMock() + post_resp.status_code = codes.accepted + post_resp.headers = { + "Location": "/redfish/v1/TaskService/TaskMonitors/1", + "Retry-After": retry_after, + } + post_resp.json.return_value = {} + conn.post.return_value = post_resp + + monitor_resp = MagicMock() + monitor_resp.status_code = codes.ok + monitor_resp.json.return_value = {"@odata.id": "/redfish/v1/TaskService/Tasks/1"} + task_resp = MagicMock() + task_resp.status_code = codes.ok + task_resp.json.return_value = {"TaskState": "Completed", "Payload": {"HttpHeaders": []}} + conn.get_response.side_effect = [monitor_resp, task_resp] + return conn + + @patch("nodescraper.connection.redfish.redfish_oem_diag.time.sleep") + def test_numeric_retry_after_is_used_as_poll_interval(self, mock_sleep): + """Baseline: a Retry-After in seconds drives the poll interval.""" + conn = self._conn_for_retry_after("3") + + _, _, error = collect_oem_diagnostic_data( + conn, "redfish/v1/Systems/UBB/LogServices/DiagLogs", "AllLogs" + ) + + assert error == "Location header missing in task Payload.HttpHeaders" + mock_sleep.assert_called_with(3) + + @patch("nodescraper.connection.redfish.redfish_oem_diag.time.sleep") + def test_http_date_retry_after_does_not_raise(self, mock_sleep): + """An HTTP-date Retry-After must be handled, not crash the collection.""" + conn = self._conn_for_retry_after("Fri, 31 Dec 1999 23:59:59 GMT") + + _, _, error = collect_oem_diagnostic_data( + conn, "redfish/v1/Systems/UBB/LogServices/DiagLogs", "AllLogs" + ) + + assert error == "Location header missing in task Payload.HttpHeaders" diff --git a/test/unit/framework/test_cli_helper.py b/test/unit/framework/test_cli_helper.py index b0f666d0..efa6bfc5 100644 --- a/test/unit/framework/test_cli_helper.py +++ b/test/unit/framework/test_cli_helper.py @@ -80,6 +80,33 @@ def test_generate_reference_config(plugin_registry): assert dump["plugins"] == {"TestPluginA": {"analysis_args": {"model_attr": 17}}} +def test_generate_reference_config_missing_data_model_warning(plugin_registry, caplog): + """The 'data model not found' warning must be formattable.""" + caplog.set_level(logging.WARNING) + results = [ + PluginResult( + status=ExecutionStatus.OK, + source="TestPluginA", + message="Plugin tasks completed successfully", + result_data=DataPluginResult( + system_data=None, + collection_result=TaskResult( + status=ExecutionStatus.OK, + task="BiosCollector", + parent="TestPluginA", + artifacts=[], + ), + ), + ) + ] + + generate_reference_config(results, plugin_registry, logging.getLogger()) + + records = [r for r in caplog.records if "data model not found" in r.msg] + assert len(records) == 1 + assert "TestPluginA" in records[0].getMessage() + + def test_get_plugin_configs(): with pytest.raises(argparse.ArgumentTypeError): get_plugin_configs( @@ -299,3 +326,50 @@ def test_generate_summary(tmp_path): rows = list(csv.DictReader(f)) assert len(rows) == 1 assert rows[0]["plugin"] == "PluginA" + + +def test_get_plugin_configs_must_copy_to_build_its_configs_to_avoid_mutation(): + """cli --skip-sudo mutates the returned config in place; it must not corrupt the built-in.""" + built_in_configs = {"MyConfig": PluginConfig(name="MyConfig")} + + plugin_configs = get_plugin_configs( + system_interaction_level="INTERACTIVE", + plugin_config_input=["MyConfig"], + built_in_configs=built_in_configs, + parsed_plugin_args={}, + plugin_subparser_map={}, + ) + + # this is exactly what nodescraper/cli/cli.py does for --skip-sudo + plugin_configs[-1].global_args.setdefault("collection_args", {})["skip_sudo"] = True + # Ensure that skip_sudo is there + assert plugin_configs[-1].global_args["collection_args"]["skip_sudo"] is True + + assert built_in_configs["MyConfig"].global_args == {} + + +def test_dump_to_csv_logs_success_when_written(tmp_path: Path): + """Baseline: a successful write reports the output file.""" + from unittest.mock import MagicMock + + mock_logger = MagicMock() + out_file = str(tmp_path / "out.csv") + + dump_to_csv([{"a": "1"}], out_file, ["a"], mock_logger) + + mock_logger.error.assert_not_called() + mock_logger.info.assert_called_once() + + +def test_dump_to_csv_does_not_log_success_when_write_fails(tmp_path: Path): + """A failed write must not be reported as data written to the csv file.""" + from unittest.mock import MagicMock + + mock_logger = MagicMock() + # parent dir does not exist, so open() fails + out_file = str(tmp_path / "missing_dir" / "out.csv") + + dump_to_csv([{"a": "1"}], out_file, ["a"], mock_logger) + + mock_logger.error.assert_called_once() + mock_logger.info.assert_not_called() diff --git a/test/unit/framework/test_compare_runs.py b/test/unit/framework/test_compare_runs.py index 75c37cc6..b9fad638 100644 --- a/test/unit/framework/test_compare_runs.py +++ b/test/unit/framework/test_compare_runs.py @@ -23,11 +23,13 @@ # SOFTWARE. # ############################################################################### +import json import logging from nodescraper.cli.compare_runs import ( _diff_value, _format_value, + _load_plugin_data_from_run, run_compare_runs, ) from nodescraper.pluginregistry import PluginRegistry @@ -169,3 +171,38 @@ def test_run_compare_runs_one_run_missing_plugin(caplog, framework_fixtures_path assert "Loading run 1" in caplog.text assert "Loading run 2" in caplog.text assert "not found in run 2" in caplog.text or "NOT_RAN" in caplog.text + + +def test_load_plugin_data_skips_result_json_failing_model_validation( + caplog, framework_fixtures_path, tmp_path +): + """An unparsable result.json must be skipped, not abort the whole run load.""" + caplog.set_level(logging.WARNING) + logger = logging.getLogger() + + fixture_collector = framework_fixtures_path / "log_dir" / "collector" + run = tmp_path / "run" + + # A collector dir whose result.json fails TaskResult validation (bad status name) + bad_collector = run / "bad_collector" + bad_collector.mkdir(parents=True) + (bad_collector / "result.json").write_text( + json.dumps({"status": "BOGUS", "task": "BiosCollector", "parent": "BiosPlugin"}), + encoding="utf-8", + ) + (bad_collector / "biosdatamodel.json").write_text( + (fixture_collector / "biosdatamodel.json").read_text(encoding="utf-8"), + encoding="utf-8", + ) + + # A valid collector dir that should still be loaded + good_collector = run / "zz_collector" + good_collector.mkdir(parents=True) + for name in ("result.json", "biosdatamodel.json"): + (good_collector / name).write_text( + (fixture_collector / name).read_text(encoding="utf-8"), encoding="utf-8" + ) + + data = _load_plugin_data_from_run(str(run), PluginRegistry(), logger) + + assert "BiosPlugin" in data diff --git a/test/unit/framework/test_config_registry.py b/test/unit/framework/test_config_registry.py index 5ba78fd1..95353bbf 100644 --- a/test/unit/framework/test_config_registry.py +++ b/test/unit/framework/test_config_registry.py @@ -48,3 +48,16 @@ def test_config_registry(): global_args={}, plugins={}, result_collators={}, name=None, desc=None ), } + + +def test_config_registry_raises_on_invalid_json(tmp_path): + """A JSON file whose top level is not an object must be skipped, not crash the registry.""" + (tmp_path / "list.json").write_text("[1, 2, 3]", encoding="utf-8") + (tmp_path / "good.json").write_text('{"name": "GoodConfig", "plugins": {}}', encoding="utf-8") + import pytest + + with pytest.raises(RuntimeError, match="Failed to load config from"): + ConfigRegistry( + config_path=str(tmp_path), + load_entry_point_configs=False, + ) diff --git a/test/unit/framework/test_connection_manager_entrypoints.py b/test/unit/framework/test_connection_manager_entrypoints.py index a11d5b50..c5344c54 100644 --- a/test/unit/framework/test_connection_manager_entrypoints.py +++ b/test/unit/framework/test_connection_manager_entrypoints.py @@ -74,3 +74,21 @@ def test_plugin_registry_merges_entry_point_connection_managers(): def test_plugin_registry_can_disable_entry_point_connection_managers(): reg = PluginRegistry(load_entry_point_connection_managers=False) assert "InBandConnectionManager" in reg.connection_managers + + +def test_load_connection_managers_cache_is_not_mutable_by_callers(): + """The entry point cache must not be aliased out to callers (cf. load_plugins_from_entry_points).""" + mock_ep = MagicMock() + mock_ep.name = "AliasInBand" + mock_ep.load.return_value = InBandConnectionManager + PluginRegistry.clear_caches() + + with patch("nodescraper.pluginregistry.importlib.metadata.entry_points") as mock_eps: + mock_eps.side_effect = lambda *a, **k: _entry_points_side_effect_cm_only(mock_ep, *a, **k) + PluginRegistry.load_connection_managers_from_entry_points() + warm = PluginRegistry.load_connection_managers_from_entry_points() + warm["InjectedByCaller"] = InBandConnectionManager + + refetched = PluginRegistry.load_connection_managers_from_entry_points() + + assert "InjectedByCaller" not in refetched diff --git a/test/unit/framework/test_dataanalyzer.py b/test/unit/framework/test_dataanalyzer.py index ce13e3a8..78a1c4b0 100644 --- a/test/unit/framework/test_dataanalyzer.py +++ b/test/unit/framework/test_dataanalyzer.py @@ -89,3 +89,31 @@ def fake_err(self, data: dummy_data_model, args: dummy_arg): assert any( "Exception during data analysis: some_err" in event.description for event in result.events ) + + +def test_analyzer_subclass_with_none_data_model_raises_type_error(dummy_data_model): + """Baseline: DATA_MODEL explicitly set to None is rejected with a TypeError.""" + import pytest + + from nodescraper.interfaces.dataanalyzertask import DataAnalyzer + + with pytest.raises(TypeError): + + class NoneModelAnalyzer(DataAnalyzer): + DATA_MODEL = None + + def analyze_data(self, data, args=None): + return self.result + + +def test_analyzer_subclass_without_data_model_raises_type_error(): + """A concrete analyzer that never declares DATA_MODEL must raise TypeError, not AttributeError.""" + import pytest + + from nodescraper.interfaces.dataanalyzertask import DataAnalyzer + + with pytest.raises(TypeError): + + class MissingModelAnalyzer(DataAnalyzer): + def analyze_data(self, data, args=None): + return self.result diff --git a/test/unit/framework/test_datacollector.py b/test/unit/framework/test_datacollector.py index 410b4d85..14d5ac6a 100644 --- a/test/unit/framework/test_datacollector.py +++ b/test/unit/framework/test_datacollector.py @@ -194,3 +194,68 @@ def _init_result(self): def collect_data(self, args=None): return self.result, None + + +class LogPathCollector(DataCollector[None, DummyDataModel, None]): + DATA_MODEL = DummyDataModel + + def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: + return self.result, None + + +def test_data_collector_keeps_log_path(system_info, conn_mock): + """Baseline: the base collector stores the log path passed by DataPlugin.collect.""" + collector = LogPathCollector(system_info, conn_mock, log_path="/tmp/run/collector") + + assert collector.log_path == "/tmp/run/collector" + + +def test_inband_collector_keeps_log_path(system_info, conn_mock): + """InBandDataCollector must forward log_path to the base collector, not drop it.""" + from nodescraper.base.inbandcollectortask import InBandDataCollector + from nodescraper.enums import OSFamily + + class InBandLogPathCollector(InBandDataCollector[DummyDataModel, None]): + DATA_MODEL = DummyDataModel + + def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: + return self.result, None + + system_info.os_family = OSFamily.LINUX + collector = InBandLogPathCollector(system_info, conn_mock, log_path="/tmp/run/collector") + + assert collector.log_path == "/tmp/run/collector" + + +class UnsetStatusWithDataCollector(DataCollector[None, DummyDataModel, None]): + DATA_MODEL = DummyDataModel + + def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: + return self.result, DummyDataModel(foo=1) + + +class UnsetStatusNoDataCollector(DataCollector[None, DummyDataModel, None]): + DATA_MODEL = DummyDataModel + + def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: + return self.result, None + + +def test_collector_returning_data_with_unset_status_is_ok(system_info, conn_mock): + """Baseline: data collected with an unset status finalizes to OK.""" + collector = UnsetStatusWithDataCollector(system_info, conn_mock) + + result, data = collector.collect_data() + + assert data is not None + assert result.status == ExecutionStatus.OK + + +def test_collector_returning_no_data_with_unset_status_is_execution_failure(system_info, conn_mock): + """A collector that returns no data and never sets a status must not be reported as OK.""" + collector = UnsetStatusNoDataCollector(system_info, conn_mock) + + result, data = collector.collect_data() + + assert data is None + assert result.status == ExecutionStatus.EXECUTION_FAILURE diff --git a/test/unit/framework/test_dataplugin.py b/test/unit/framework/test_dataplugin.py index e0af541f..ac17b6ad 100644 --- a/test/unit/framework/test_dataplugin.py +++ b/test/unit/framework/test_dataplugin.py @@ -31,6 +31,7 @@ import pytest from framework.common.shared_utils import MockConnectionManager +from nodescraper import utils from nodescraper.enums import EventPriority, ExecutionStatus, SystemInteractionLevel from nodescraper.interfaces.dataanalyzertask import DataAnalyzer from nodescraper.interfaces.datacollectortask import DataCollector @@ -124,6 +125,13 @@ def test_data_property(self, plugin): assert isinstance(plugin.data, StandardDataModel) assert plugin.data.value == "dict_value" + def test_data_setter_error_names_expected_model(self, plugin): + """Invalid data should report the expected DATA_MODEL name, not its metaclass.""" + with pytest.raises(ValueError) as exc_info: + plugin.data = 12345 + + assert "StandardDataModel" in str(exc_info.value) + def test_collect_creates_connection_manager(self, plugin, conn_mock, system_info, logger): assert plugin.connection_manager is None @@ -507,6 +515,30 @@ def test_find_datamodel_path_success(self, tmp_path: Path) -> None: assert found is not None assert found.endswith("contentmodel.json") + def test_find_datamodel_path_prefers_datamodel_json_over_log( + self, tmp_path: Path, monkeypatch + ) -> None: + """A collector dir holding both files must yield the datamodel json, not the .log.""" + import os + + collector_dir = tmp_path / "extract_plugin" / "content_collector" + collector_dir.mkdir(parents=True) + (collector_dir / "result.json").write_text( + json.dumps({"parent": "ExtractPlugin"}), encoding="utf-8" + ) + (collector_dir / "contentmodel.json").write_text( + json.dumps({"value": "from_run"}), encoding="utf-8" + ) + (collector_dir / "capture.log").write_text("raw log", encoding="utf-8") + + # make directory listing order deterministic: "capture.log" sorts first + real_listdir = os.listdir + monkeypatch.setattr(os, "listdir", lambda path: sorted(real_listdir(path))) + + found = ExtractPlugin.find_datamodel_path_in_run(str(tmp_path)) + assert found is not None + assert found.endswith("contentmodel.json") + def test_find_datamodel_path_wrong_parent(self, tmp_path: Path) -> None: collector_dir = tmp_path / "extract_plugin" / "content_collector" collector_dir.mkdir(parents=True) @@ -764,3 +796,80 @@ def test_log_path_none_does_not_create_directories(self, plugin_with_conn): beta_call_kwargs = beta_init.call_args[1] assert beta_call_kwargs["log_path"] is None + + +class OverrideDataModel(DataModel): + value: str = "test" + + +class OverrideCollector(DataCollector): + DATA_MODEL = OverrideDataModel + + def collect_data(self, args=None): + self.result.status = ExecutionStatus.OK + return self.result, OverrideDataModel(value="collected") + + +class OverrideAnalyzer(DataAnalyzer): + DATA_MODEL = OverrideDataModel + + def analyze_data(self, data, args=None): + return TaskResult(status=ExecutionStatus.OK) + + +class OverrideLogDirPlugin(DataPlugin): + DATA_MODEL = OverrideDataModel + CONNECTION_TYPE = MockConnectionManager + COLLECTOR = OverrideCollector + ANALYZER = OverrideAnalyzer + + +class TestDataPluginLogDirNameOverride: + """find_datamodel_path_in_run must read from the dirs collect() actually writes to.""" + + @pytest.fixture(autouse=True) + def registered_overrides(self, monkeypatch): + monkeypatch.setitem( + utils._LOG_DIR_NAME_OVERRIDES, "OverrideLogDirPlugin", "override_LOGDIR_plugin" + ) + monkeypatch.setitem( + utils._LOG_DIR_NAME_OVERRIDES, "OverrideCollector", "override_LOGDIR_collector" + ) + + def test_find_datamodel_path_honors_registered_log_dir_name( + self, plugin_with_conn, tmp_path: Path + ) -> None: + run_path = tmp_path / "scraper_logs_run" + plugin = OverrideLogDirPlugin( + system_info=plugin_with_conn.system_info, + logger=plugin_with_conn.logger, + connection_manager=plugin_with_conn.connection_manager, + log_path=str(run_path), + ) + + assert plugin.collect(preserve_connection=True).status == ExecutionStatus.OK + + # collect() wrote the collector output under the registered override names + collector_dir = run_path / "override_LOGDIR_plugin" / "override_LOGDIR_collector" + assert (collector_dir / "result.json").is_file() + assert (collector_dir / "overridedatamodel.json").is_file() + + found = OverrideLogDirPlugin.find_datamodel_path_in_run(str(run_path)) + assert found is not None + assert Path(found).parent == collector_dir + + def test_load_run_data_honors_registered_log_dir_name( + self, plugin_with_conn, tmp_path: Path + ) -> None: + run_path = tmp_path / "scraper_logs_run" + plugin = OverrideLogDirPlugin( + system_info=plugin_with_conn.system_info, + logger=plugin_with_conn.logger, + connection_manager=plugin_with_conn.connection_manager, + log_path=str(run_path), + ) + plugin.collect(preserve_connection=True) + + loaded = OverrideLogDirPlugin.load_run_data(str(run_path)) + assert loaded is not None + assert loaded["value"] == "collected" diff --git a/test/unit/framework/test_file_artifact.py b/test/unit/framework/test_file_artifact.py index 991fbf48..87e048aa 100644 --- a/test/unit/framework/test_file_artifact.py +++ b/test/unit/framework/test_file_artifact.py @@ -72,3 +72,15 @@ def test_log_model_binary(tmp_path: Path): output_path = tmp_path / "binary.bin" assert output_path.exists() assert output_path.read_bytes() == binary_data + + +def test_log_model_absolute_filename_stays_in_log_path(tmp_path: Path): + """An artifact filename that is an absolute path must not write outside the log dir.""" + log_dir = tmp_path / "logs" + log_dir.mkdir() + outside = tmp_path / "outside.txt" + + artifact = TextFileArtifact(filename=str(outside), contents="leaked") + artifact.log_model(str(log_dir)) + + assert not outside.exists(), "log_model wrote outside of the provided log path" diff --git a/test/unit/framework/test_match_ignore.py b/test/unit/framework/test_match_ignore.py index bf29752e..c2e7ccaa 100644 --- a/test/unit/framework/test_match_ignore.py +++ b/test/unit/framework/test_match_ignore.py @@ -110,3 +110,9 @@ def test_should_ignore_match_mce_banks_only_when_all_banks_ignored(): def test_extract_mce_bank_from_line(): line = "[Hardware Error]: Machine Check: CPU0 MC21_STATUS[0xcafe|CE|Misc]: 0x0" assert extract_mce_bank_from_line(line) == 21 + + +def test_parse_mce_bank_spec_rejects_negative_bank_string(): + """A negative bank given as a string must be reported as an invalid bank number.""" + with pytest.raises(ValueError, match="Invalid MCE bank"): + parse_mce_bank_spec(["-5"]) diff --git a/test/unit/framework/test_regexanalyzer.py b/test/unit/framework/test_regexanalyzer.py index f07afb8c..38a3fb2c 100644 --- a/test/unit/framework/test_regexanalyzer.py +++ b/test/unit/framework/test_regexanalyzer.py @@ -24,6 +24,7 @@ # ############################################################################### import re +import signal from pydantic import BaseModel @@ -263,3 +264,20 @@ def test_check_all_regexes_skips_ignore_match_rules(system_info): assert len(events) == 1 assert "dummy error 3" in str(events[0].data["match_content"]) + + +def test_check_all_regexes_terminates_on_zero_width_match(system_info): + """A pattern able to match empty text must not spin forever in check_all_regexes.""" + analyzer = TestRegexAnalyzer(system_info=system_info) + zero_width = [ErrorRegex(regex=re.compile(r"x*"), message="Zero width")] + + def _on_timeout(signum, frame): + raise TimeoutError("check_all_regexes did not terminate") + + previous_handler = signal.signal(signal.SIGALRM, _on_timeout) + signal.setitimer(signal.ITIMER_REAL, 2.0) + try: + analyzer.check_all_regexes("abc", "src", zero_width) + finally: + signal.setitimer(signal.ITIMER_REAL, 0) + signal.signal(signal.SIGALRM, previous_handler) diff --git a/test/unit/framework/test_type_utils.py b/test/unit/framework/test_type_utils.py index a6bb1012..48843f56 100644 --- a/test/unit/framework/test_type_utils.py +++ b/test/unit/framework/test_type_utils.py @@ -105,3 +105,21 @@ def test_model_types(): assert res["optional_attr"] == TypeData( type_classes=[TypeClass(type_class=str, inner_type=None)], required=False ) + + +def test_process_type_strips_annotated(): + """Baseline: a top level Annotated type is unwrapped to its underlying type.""" + from typing import Annotated + + assert TypeUtils.process_type(Annotated[int, "meta"]) == [ + TypeClass(type_class=int, inner_type=None) + ] + + +def test_process_type_strips_annotated_inside_optional(): + """An Annotated type nested in a Union must still resolve to its underlying type.""" + from typing import Annotated + + assert TypeUtils.process_type(Optional[Annotated[int, "meta"]]) == [ + TypeClass(type_class=int, inner_type=None) + ] From 3a47799906856adfd3f13a7a7c2464aac10d52b4 Mon Sep 17 00:00:00 2001 From: graepaul_amdeng Date: Fri, 18 Sep 2026 10:20:44 -0700 Subject: [PATCH 2/5] Adding new tests --- test/unit/cli/test_dynamic_parser_builder.py | 47 ++++++++++++++++++++ test/unit/cli/test_input_arg_types.py | 46 +++++++++++++++++++ test/unit/framework/test_collectorargs.py | 30 +++++++++++++ test/unit/framework/test_datamodel.py | 38 ++++++++++++++++ test/unit/framework/test_plugin_interface.py | 38 ++++++++++++++++ test/unit/framework/test_task.py | 34 ++++++++++++++ test/unit/framework/test_utils.py | 41 +++++++++++++++++ 7 files changed, 274 insertions(+) create mode 100644 test/unit/cli/test_dynamic_parser_builder.py create mode 100644 test/unit/cli/test_input_arg_types.py create mode 100644 test/unit/framework/test_collectorargs.py create mode 100644 test/unit/framework/test_datamodel.py create mode 100644 test/unit/framework/test_plugin_interface.py create mode 100644 test/unit/framework/test_task.py create mode 100644 test/unit/framework/test_utils.py diff --git a/test/unit/cli/test_dynamic_parser_builder.py b/test/unit/cli/test_dynamic_parser_builder.py new file mode 100644 index 00000000..049e4eeb --- /dev/null +++ b/test/unit/cli/test_dynamic_parser_builder.py @@ -0,0 +1,47 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +import argparse +from typing import Literal, Optional + +from pydantic import BaseModel + +from nodescraper.cli.dynamicparserbuilder import DynamicParserBuilder +from nodescraper.typeutils import TypeUtils + + +class LiteralArgsModel(BaseModel): + mode: Literal["fast", "slow"] = "fast" + opt_mode: Optional[Literal["on", "off"]] = None + + +def _action_for(parser: argparse.ArgumentParser, option: str) -> argparse.Action: + for action in parser._actions: + if option in action.option_strings: + return action + raise AssertionError(f"no action for {option}") + + +def test_optional_literal_field_gets_choices() -> None: + """Baseline: Optional[Literal[...]] is rendered with argparse choices.""" + parser = argparse.ArgumentParser() + DynamicParserBuilder(parser, object).build_model_arg_parser(LiteralArgsModel, required=False) # type: ignore + + assert _action_for(parser, "--opt-mode").choices == ["on", "off"] + + +def test_plain_literal_field_gets_choices() -> None: + """A bare Literal[...] field must also be rendered with argparse choices.""" + parser = argparse.ArgumentParser() + DynamicParserBuilder(parser, object).build_model_arg_parser(LiteralArgsModel, required=False) # type: ignore + + assert _action_for(parser, "--mode").choices == ["fast", "slow"] + + +def test_get_literal_choices_returns_literal_values() -> None: + """get_literal_choices must return the Literal's allowed values.""" + type_classes = TypeUtils.process_type(Literal["fast", "slow"]) # type: ignore + type_class_map = {tc.type_class: tc for tc in type_classes} + + assert DynamicParserBuilder.get_literal_choices(type_class_map) == ["fast", "slow"] diff --git a/test/unit/cli/test_input_arg_types.py b/test/unit/cli/test_input_arg_types.py new file mode 100644 index 00000000..7046244a --- /dev/null +++ b/test/unit/cli/test_input_arg_types.py @@ -0,0 +1,46 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +import argparse +import json +from pathlib import Path + +import pytest +from pydantic import BaseModel + +from nodescraper.cli.inputargtypes import ModelArgHandler + + +class SampleArgs(BaseModel): + name: str = "" + count: int = 0 + + +def _write(tmp_path: Path, payload) -> str: + arg_file = tmp_path / "arg.json" + arg_file.write_text(json.dumps(payload), encoding="utf-8") + return str(arg_file) + + +def test_process_file_arg_builds_model(tmp_path: Path): + """Baseline: a valid json object is loaded into the model.""" + path = _write(tmp_path, {"name": "abc", "count": 2}) + + assert ModelArgHandler(SampleArgs).process_file_arg(path) == SampleArgs(name="abc", count=2) + + +def test_process_file_arg_invalid_value_raises_arg_type_error(tmp_path: Path): + """Baseline: a validation failure is surfaced as an argparse error.""" + path = _write(tmp_path, {"count": "not-an-int"}) + + with pytest.raises(argparse.ArgumentTypeError): + ModelArgHandler(SampleArgs).process_file_arg(path) + + +def test_process_file_arg_non_object_json_raises_arg_type_error(tmp_path: Path): + """A json file that is not an object must be reported as an argparse error.""" + path = _write(tmp_path, [{"name": "abc"}]) + + with pytest.raises(argparse.ArgumentTypeError): + ModelArgHandler(SampleArgs).process_file_arg(path) diff --git a/test/unit/framework/test_collectorargs.py b/test/unit/framework/test_collectorargs.py new file mode 100644 index 00000000..09eb7326 --- /dev/null +++ b/test/unit/framework/test_collectorargs.py @@ -0,0 +1,30 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +from typing import Optional + +import pytest + +from nodescraper.models.collectorargs import CollectorArgs + + +class MyCollectorArgs(CollectorArgs): + opt_arg: Optional[str] = None + set_arg: Optional[str] = None + + +def test_collector_args_forbids_extra(): + """Baseline: extra="forbid" from model_config is honored.""" + with pytest.raises(ValueError): + MyCollectorArgs(not_a_field=1) + + +def test_collector_args_dumps_none(): + """model_config declares should serialize all args.""" + args = MyCollectorArgs(set_arg="abc") + assert args.model_dump() == { + "html_view": False, + "set_arg": "abc", + "opt_arg": None, + } diff --git a/test/unit/framework/test_datamodel.py b/test/unit/framework/test_datamodel.py new file mode 100644 index 00000000..ed6c2567 --- /dev/null +++ b/test/unit/framework/test_datamodel.py @@ -0,0 +1,38 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +import json +import os +from pathlib import Path + +from nodescraper.models.datamodel import DataModel + + +class FolderDataModel(DataModel): + value: str = "" + + @classmethod + def build_from_folder(cls, folder_path: str) -> "FolderDataModel": + return cls(value=os.path.basename(folder_path)) + + +def test_import_model_from_dict(): + """Baseline: a dict is passed straight to the model constructor.""" + assert FolderDataModel.import_model({"value": "abc"}).value == "abc" + + +def test_import_model_from_json_file(tmp_path: Path): + """Baseline: a json file path is read and parsed.""" + model_file = tmp_path / "model.json" + model_file.write_text(json.dumps({"value": "from-file"}), encoding="utf-8") + + assert FolderDataModel.import_model(str(model_file)).value == "from-file" + + +def test_import_model_from_directory_uses_build_from_folder(tmp_path: Path): + """A directory path must be dispatched to build_from_folder.""" + folder = tmp_path / "collected" + folder.mkdir() + + assert FolderDataModel.import_model(str(folder)).value == "collected" diff --git a/test/unit/framework/test_plugin_interface.py b/test/unit/framework/test_plugin_interface.py new file mode 100644 index 00000000..6913d325 --- /dev/null +++ b/test/unit/framework/test_plugin_interface.py @@ -0,0 +1,38 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +from nodescraper.enums import ExecutionStatus +from nodescraper.interfaces.plugin import PluginInterface +from nodescraper.interfaces.taskresulthook import TaskResultHook +from nodescraper.models import PluginResult, TaskResult +from nodescraper.taskresulthooks.filesystemloghook import FileSystemLogHook + + +class DummyHook(TaskResultHook): + def process_result(self, task_result: TaskResult, **kwargs): + return None + + +class DummyPlugin(PluginInterface): + def run(self, **kwargs) -> PluginResult: + return PluginResult(source=self.__class__.__name__, status=ExecutionStatus.OK) + + +def test_log_path_adds_filesystem_hook(): + """Baseline: a plugin given a log path gets a filesystem hook for that path.""" + plugin = DummyPlugin(log_path="/tmp/run-a") + + hooks = [h for h in plugin.task_result_hooks if isinstance(h, FileSystemLogHook)] + assert [h.log_base_path for h in hooks] == ["/tmp/run-a"] + + +def test_shared_hook_list_does_not_leak_log_path_between_plugins(): + """A hook list shared by two plugins must not give the second plugin the first's log path.""" + shared_hooks: list = [DummyHook()] + + DummyPlugin(log_path="/tmp/run-a", task_result_hooks=shared_hooks) + plugin_b = DummyPlugin(log_path="/tmp/run-b", task_result_hooks=shared_hooks) + + hooks = [h for h in plugin_b.task_result_hooks if isinstance(h, FileSystemLogHook)] + assert "/tmp/run-b" in [h.log_base_path for h in hooks] diff --git a/test/unit/framework/test_task.py b/test/unit/framework/test_task.py new file mode 100644 index 00000000..a4af9699 --- /dev/null +++ b/test/unit/framework/test_task.py @@ -0,0 +1,34 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +import pytest + +from nodescraper.interfaces.task import Task + + +def test_task_subclass_with_task_type_is_allowed(): + """Baseline: a subclass that declares TASK_TYPE is created without error.""" + + class GoodTask(Task): + TASK_TYPE = "GOOD_TASK" + + assert GoodTask.TASK_TYPE == "GOOD_TASK" + + +def test_task_subclass_with_none_task_type_raises(): + """Baseline: TASK_TYPE explicitly set to None is rejected.""" + + with pytest.raises(TypeError): + + class NoneTask(Task): + TASK_TYPE = None + + +def test_task_subclass_without_task_type_raises_type_error(): + """A subclass that never declares TASK_TYPE must raise the documented TypeError.""" + + with pytest.raises(TypeError): + + class MissingTaskType(Task): + pass diff --git a/test/unit/framework/test_utils.py b/test/unit/framework/test_utils.py new file mode 100644 index 00000000..399ae438 --- /dev/null +++ b/test/unit/framework/test_utils.py @@ -0,0 +1,41 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +from typing import Literal, Optional, Union + +from nodescraper.utils import bytes_to_human_readable, find_annotation_in_container + + +class Target: + pass + + +def test_find_annotation_in_container_union(): + """Baseline: the target type is found inside a Union.""" + assert find_annotation_in_container(Union[int, str], int) == (int, [Union]) + + +def test_find_annotation_in_container_not_found(): + """Baseline: a missing target type returns None.""" + assert find_annotation_in_container(Union[int, str], Target) == (None, []) + + +def test_find_annotation_in_container_supports_literal(): + """Literal is documented as a supported container and must not raise.""" + assert find_annotation_in_container(Optional[Literal["a", "b"]], Target) == (None, []) + + +def test_find_annotation_in_container_literal_nested_in_generic(): + """A Literal nested inside another container must not raise.""" + assert find_annotation_in_container(dict[str, Literal["a", "b"]], Target) == (None, []) + + +def test_bytes_to_human_readable_terabytes(): + """Baseline: a terabyte scale value is rendered with the TB unit.""" + assert bytes_to_human_readable(2 * 10**12) == "2.0TB" + + +def test_bytes_to_human_readable_petabytes(): + """PB is a documented unit, so petabyte scale values must not be reported in TB.""" + assert bytes_to_human_readable(2 * 10**15) == "2.0PB" From ee90e6151eff18a20c0d6c723f176f2f280fda96 Mon Sep 17 00:00:00 2001 From: graepaul_amdeng Date: Fri, 18 Sep 2026 13:27:11 -0700 Subject: [PATCH 3/5] Adding new tests and fixes that those tests have found --- nodescraper/base/inbandcollectortask.py | 2 + nodescraper/base/match_ignore.py | 6 ++- nodescraper/base/regexanalyzer.py | 3 +- nodescraper/connection/inband/inband.py | 47 +++++++++++++++++-- nodescraper/interfaces/datacollectortask.py | 6 ++- nodescraper/interfaces/dataplugin.py | 16 ++++--- nodescraper/interfaces/plugin.py | 8 ++-- nodescraper/interfaces/task.py | 5 +- nodescraper/models/datamodel.py | 16 +++---- nodescraper/utils.py | 12 ++++- .../fixtures/{ => valid_configs}/example.json | 0 .../{ => valid_configs}/example_config.json | 0 test/unit/framework/test_cli.py | 24 ++++++++-- test/unit/framework/test_cli_helper.py | 2 +- test/unit/framework/test_config_registry.py | 2 +- test/unit/framework/test_datacollector.py | 26 +++++++++- 16 files changed, 139 insertions(+), 36 deletions(-) rename test/unit/framework/fixtures/{ => valid_configs}/example.json (100%) rename test/unit/framework/fixtures/{ => valid_configs}/example_config.json (100%) diff --git a/nodescraper/base/inbandcollectortask.py b/nodescraper/base/inbandcollectortask.py index fd079a27..877fd4f9 100644 --- a/nodescraper/base/inbandcollectortask.py +++ b/nodescraper/base/inbandcollectortask.py @@ -55,6 +55,7 @@ def __init__( task_result_hooks: Optional[list[TaskResultHook]] = None, event_reporter: str = DEFAULT_EVENT_REPORTER, session_id: Optional[str] = None, + log_path: Optional[str] = None, **kwargs, ): """Creates a InBandDataCollector Class @@ -83,6 +84,7 @@ def __init__( task_result_hooks=task_result_hooks, event_reporter=event_reporter, session_id=session_id, + log_path=log_path, ) if self.system_info.os_family not in self.SUPPORTED_OS_FAMILY: raise SystemCompatibilityError( diff --git a/nodescraper/base/match_ignore.py b/nodescraper/base/match_ignore.py index f1521a58..6e66b3c3 100644 --- a/nodescraper/base/match_ignore.py +++ b/nodescraper/base/match_ignore.py @@ -57,13 +57,15 @@ def parse_mce_bank_spec(spec: Sequence[MceBankSpec]) -> frozenset[int]: raise ValueError(f"Invalid MCE bank number: {entry}") banks.add(entry) continue - token = str(entry).strip() if not token: raise ValueError("Empty MCE bank entry") - if "-" in token: start_text, end_text = token.split("-", 1) + # Check if it looks like "-5" + if not start_text.strip() or not end_text.strip() or (int(end_text.strip()) < 0): + raise ValueError(f"Invalid MCE bank range: {entry}") + start = int(start_text.strip()) end = int(end_text.strip()) if start < 0 or end < 0 or start > end: diff --git a/nodescraper/base/regexanalyzer.py b/nodescraper/base/regexanalyzer.py index 1d7adf61..ca482c0e 100644 --- a/nodescraper/base/regexanalyzer.py +++ b/nodescraper/base/regexanalyzer.py @@ -402,8 +402,9 @@ def _is_within_interval(new_timestamp_str: str, existing_timestamps: list[str]) new_event.data["timestamp"] = timestamp regex_event_list.append(new_event) - search_from = match_obj.end() + if search_from == match_obj.start(): # Zero-width match + search_from += 1 # Force advancement all_events = list(regex_map.values()) if group else regex_event_list diff --git a/nodescraper/connection/inband/inband.py b/nodescraper/connection/inband/inband.py index 291491b4..0a38f2b8 100644 --- a/nodescraper/connection/inband/inband.py +++ b/nodescraper/connection/inband/inband.py @@ -25,6 +25,7 @@ ############################################################################### import abc import os +from pathlib import Path from typing import Optional from pydantic import BaseModel @@ -67,6 +68,41 @@ def log_model(self, log_path: str) -> None: def contents_str(self) -> str: pass + def resolve_write_path(self, log_path: str) -> Path: + """Resolve where this artifact should be written and create any parent dirs + + The filename is always treated as relative to log_path. Any anchor and + parent dir references are dropped so that a filename can never write + outside of log_path, while intentional sub directories are preserved. + + Args: + log_path (str): dir that the artifact should be written into + + Raises: + IOError: if the filename has no usable name or the dirs cannot be created + + Returns: + Path: path to write the artifact to + """ + filename_pathlib = Path(self.filename) + parts = [ + part + for part in filename_pathlib.parts + if part not in (filename_pathlib.anchor, os.pardir, os.curdir) + ] + + if not parts: + raise IOError(f"Artifact filename '{self.filename}' does not contain a file name") + + write_path = Path(log_path).joinpath(*parts) + + try: + write_path.parent.mkdir(parents=True, exist_ok=True) + except Exception as e: + raise IOError(f"Failed to create log directory {write_path.parent}: {e}") from e + + return write_path + @classmethod def from_bytes( cls, @@ -107,8 +143,10 @@ def log_model(self, log_path: str) -> None: Args: log_path (str): Path for file """ - path = os.path.join(log_path, self.filename) - with open(path, "w", encoding="utf-8") as f: + # the filename can look like a folder or a file, make sure it goes in log_path + write_path = self.resolve_write_path(log_path) + + with open(write_path, "w", encoding="utf-8") as f: f.write(self.contents) def contents_str(self) -> str: @@ -131,8 +169,9 @@ def log_model(self, log_path: str) -> None: Args: log_path (str): Path for file """ - log_name = os.path.join(log_path, self.filename) - with open(log_name, "wb") as f: + write_path = self.resolve_write_path(log_path) + + with open(write_path, "wb") as f: f.write(self.contents) def contents_str(self) -> str: diff --git a/nodescraper/interfaces/datacollectortask.py b/nodescraper/interfaces/datacollectortask.py index acd9c227..cc96e974 100644 --- a/nodescraper/interfaces/datacollectortask.py +++ b/nodescraper/interfaces/datacollectortask.py @@ -120,7 +120,11 @@ def wrapper( result = collector.result data = None - if data is None and not result.status: + if data is None and result.status in {ExecutionStatus.OK, ExecutionStatus.UNSET}: + # If the collector doesn't set the result status but no data was collected, mark it as a failure + # If a collector does not want to return data it should be under NOT_RAN, OK must return a data model. + if result.message == "": + result.message = "Data Model was not collected and collection failed without specific error message" result.status = ExecutionStatus.EXECUTION_FAILURE result.finalize(collector.logger) diff --git a/nodescraper/interfaces/dataplugin.py b/nodescraper/interfaces/dataplugin.py index 07ff437f..2669e4ca 100644 --- a/nodescraper/interfaces/dataplugin.py +++ b/nodescraper/interfaces/dataplugin.py @@ -46,7 +46,7 @@ SystemInfo, TaskResult, ) -from nodescraper.utils import pascal_to_snake, resolve_log_dir_name +from nodescraper.utils import resolve_log_dir_name from .connectionmanager import TConnectArg, TConnectionManager from .task import SystemCompatibilityError @@ -292,7 +292,7 @@ def data(self, data: Optional[Union[str, dict, TDataModel]]): if isinstance(data, (str, dict)): self._data = self.DATA_MODEL.import_model(data) elif not isinstance(data, self.DATA_MODEL): - raise ValueError(f"data is invalid type, expected {self.DATA_MODEL.__class__.__name__}") + raise ValueError(f"data is invalid type, expected {self.DATA_MODEL.__name__}") else: self._data = data @@ -435,7 +435,7 @@ def analyze( Args: max_event_priority_level (Union[EventPriority, str], optional): priority limit for events. Defaults to EventPriority.CRITICAL. - analysis_args (Optional[Union[TAnalyzeArg , dict]], optional): args for data analysis. Defaults to None. + analReaysis_args (Optional[Union[TAnalyzeArg , dict]], optional): args for data analysis. Defaults to None. data (Optional[Union[str, dict, TDataModel]], optional): data to analyze. Defaults to None. Returns: @@ -597,8 +597,8 @@ def find_datamodel_path_in_run(cls, run_path: str) -> Optional[str]: for collector_cls in cls.get_collector_classes(): collector_dir = os.path.join( run_path, - pascal_to_snake(cls.__name__), - pascal_to_snake(collector_cls.__name__), + resolve_log_dir_name(cls.__name__), + resolve_log_dir_name(collector_cls.__name__), ) if not os.path.isdir(collector_dir): continue @@ -612,10 +612,14 @@ def find_datamodel_path_in_run(cls, run_path: str) -> Optional[str]: except (json.JSONDecodeError, OSError): continue want_json = data_model_cls.__name__.lower() + ".json" + # First search all files for the json for fname in os.listdir(collector_dir): low = fname.lower() - if low.endswith("datamodel.json") or low == want_json: + if low.endswith(f"{data_model_cls.__name__.lower()}.json") or low == want_json: return os.path.join(collector_dir, fname) + # Then search for log since that is valid in some cases + for fname in os.listdir(collector_dir): + low = fname.lower() if low.endswith(".log"): return os.path.join(collector_dir, fname) return None diff --git a/nodescraper/interfaces/plugin.py b/nodescraper/interfaces/plugin.py index 5533ddc5..84ad5bf2 100644 --- a/nodescraper/interfaces/plugin.py +++ b/nodescraper/interfaces/plugin.py @@ -76,13 +76,13 @@ def __init__( system_info = SystemInfo() self.system_info: SystemInfo = system_info - if not task_result_hooks: - task_result_hooks = [] - self.task_result_hooks = task_result_hooks + # copy the hook list so that hooks added here are not leaked back to the caller + # If the copy is not performed then any modifications to the hook list here would affect the caller's list as well. + self.task_result_hooks = list(task_result_hooks) if task_result_hooks else [] if log_path: for hook in self.task_result_hooks: - if isinstance(hook, FileSystemLogHook): + if isinstance(hook, FileSystemLogHook) and hook.log_base_path == log_path: break else: self.task_result_hooks.append(FileSystemLogHook(log_base_path=log_path)) diff --git a/nodescraper/interfaces/task.py b/nodescraper/interfaces/task.py index 6503bde3..557c3034 100644 --- a/nodescraper/interfaces/task.py +++ b/nodescraper/interfaces/task.py @@ -26,6 +26,7 @@ import abc import copy import datetime +import inspect import logging import uuid from typing import Any, Optional, Union @@ -122,7 +123,9 @@ def max_event_priority_level(self, input_value: Union[str, EventPriority]): def __init_subclass__(cls, **kwargs) -> None: """Validates that the subclass contains a TASK_TYPE attribute which is not None.""" super().__init_subclass__(**kwargs) - if cls.TASK_TYPE is None: + if not inspect.isabstract(cls) and ( + (getattr(cls, "TASK_TYPE", None) is None) or (cls.TASK_TYPE is None) + ): raise TypeError(f"No value provided for TASK_TYPE in task class {cls.__name__}") def _build_event( diff --git a/nodescraper/models/datamodel.py b/nodescraper/models/datamodel.py index c310c810..09fc81f3 100644 --- a/nodescraper/models/datamodel.py +++ b/nodescraper/models/datamodel.py @@ -113,18 +113,18 @@ def import_model(cls: type[TDataModel], model_input: Union[dict, str]) -> TDataM return cls(**model_input) if isinstance(model_input, str): - # Build from tarfile if supported - if tarfile.is_tarfile(model_input): - return cls.build_from_tar(model_input) # Build from folder if supported if os.path.isdir(model_input): return cls.build_from_folder(model_input) - + # Build from tarfile if supported + elif tarfile.is_tarfile(model_input): + return cls.build_from_tar(model_input) # Build from json file - with open(model_input, "r", encoding="utf-8") as input_file: - data = json.load(input_file) - - return cls(**data) + else: + with open(model_input, "r", encoding="utf-8") as input_file: + data = json.load(input_file) + return cls(**data) + return cls() raise ValueError("Invalid input for model data") diff --git a/nodescraper/utils.py b/nodescraper/utils.py index 11c3ab57..7a0a60b7 100644 --- a/nodescraper/utils.py +++ b/nodescraper/utils.py @@ -233,7 +233,14 @@ def bytes_to_human_readable(input_bytes: int) -> str: return "0B" if input_bytes == 0: return "0B" - units = [(10**12, "TB"), (10**9, "GB"), (10**6, "MB"), (10**3, "KB"), (1, "B")] + units = [ + (10**15, "PB"), + (10**12, "TB"), + (10**9, "GB"), + (10**6, "MB"), + (10**3, "KB"), + (1, "B"), + ] for scale, label in units: if input_bytes >= scale: return f"{round(float(input_bytes) / scale, 2)}{label}" @@ -278,6 +285,9 @@ def find_annotation_in_container( if result: containers.append(origin) return result, containers + # Check if it is not a type cause if you put an origin in its anything + if not isinstance(item, type): + item = type(item) if len(get_args(item)) == 0 and issubclass(item, target_type): containers.append(origin) return item, containers diff --git a/test/unit/framework/fixtures/example.json b/test/unit/framework/fixtures/valid_configs/example.json similarity index 100% rename from test/unit/framework/fixtures/example.json rename to test/unit/framework/fixtures/valid_configs/example.json diff --git a/test/unit/framework/fixtures/example_config.json b/test/unit/framework/fixtures/valid_configs/example_config.json similarity index 100% rename from test/unit/framework/fixtures/example_config.json rename to test/unit/framework/fixtures/valid_configs/example_config.json diff --git a/test/unit/framework/test_cli.py b/test/unit/framework/test_cli.py index 8df56e95..8679b1da 100644 --- a/test/unit/framework/test_cli.py +++ b/test/unit/framework/test_cli.py @@ -68,7 +68,9 @@ def test_dict_arg(): def test_json_arg(framework_fixtures_path): - assert json_arg(os.path.join(framework_fixtures_path, "example.json")) == {"test": 123} + assert json_arg(os.path.join(framework_fixtures_path, "valid_configs", "example.json")) == { + "test": 123 + } with pytest.raises(argparse.ArgumentTypeError): json_arg(os.path.join(framework_fixtures_path, "invalid.json")) @@ -79,7 +81,7 @@ class TestArg(BaseModel): arg_handler = ModelArgHandler(TestArg) assert arg_handler.process_file_arg( - os.path.join(framework_fixtures_path, "example.json") + os.path.join(framework_fixtures_path, "valid_configs", "example.json") ) == TestArg(test=123) with pytest.raises(argparse.ArgumentTypeError): @@ -96,7 +98,10 @@ def test_system_info_builder(): system_config=None, ) ) == SystemInfo( - name="test_name", sku="test_sku", platform="test_plat", location=SystemLocation.LOCAL + name="test_name", + sku="test_sku", + platform="test_plat", + location=SystemLocation.LOCAL, ) with pytest.raises(argparse.ArgumentTypeError): @@ -122,7 +127,18 @@ def test_system_info_builder(): ( ["--sys-name", "test-sys", "--sys-sku", "test-sku", "run-plugins", "-h"], ["TestPlugin1", "TestPlugin2"], - (["--sys-name", "test-sys", "--sys-sku", "test-sku", "run-plugins", "-h"], {}, []), + ( + [ + "--sys-name", + "test-sys", + "--sys-sku", + "test-sku", + "run-plugins", + "-h", + ], + {}, + [], + ), ), ( [ diff --git a/test/unit/framework/test_cli_helper.py b/test/unit/framework/test_cli_helper.py index efa6bfc5..5139b601 100644 --- a/test/unit/framework/test_cli_helper.py +++ b/test/unit/framework/test_cli_helper.py @@ -163,7 +163,7 @@ def test_config_builder(plugin_registry): config = build_config( config_reg=ConfigRegistry( - config_path=os.path.join(os.path.dirname(__file__), "fixtures"), + config_path=os.path.join(os.path.dirname(__file__), "fixtures", "valid_configs"), load_entry_point_configs=False, ), plugin_reg=plugin_registry, diff --git a/test/unit/framework/test_config_registry.py b/test/unit/framework/test_config_registry.py index 95353bbf..0330196c 100644 --- a/test/unit/framework/test_config_registry.py +++ b/test/unit/framework/test_config_registry.py @@ -32,7 +32,7 @@ def test_config_registry(): config_registry = ConfigRegistry( - config_path=os.path.join(os.path.dirname(__file__), "fixtures"), + config_path=os.path.join(os.path.dirname(__file__), "fixtures", "valid_configs"), load_entry_point_configs=False, ) diff --git a/test/unit/framework/test_datacollector.py b/test/unit/framework/test_datacollector.py index 14d5ac6a..cc1798e6 100644 --- a/test/unit/framework/test_datacollector.py +++ b/test/unit/framework/test_datacollector.py @@ -49,6 +49,9 @@ def finalize(self, logger): pass +DUMMY_GIVES_THIS = DummyDataModel(foo=0xC0FFEE) + + class DummyCollector(DataCollector[None, DummyDataModel, None]): SUPPORTED_SKUS = {"GOOD"} SUPPORTED_PLATFORMS = {"X"} @@ -67,7 +70,7 @@ def _init_result(self): def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: self.result.status = ExecutionStatus.OK - return self.result, None + return self.result, DUMMY_GIVES_THIS def test_ok(system_info, conn_mock): @@ -82,7 +85,8 @@ def test_ok(system_info, conn_mock): result, data = dc.collect_data() assert result.status == ExecutionStatus.OK - assert ("hook", result, None) in calls + assert data == DUMMY_GIVES_THIS + assert ("hook", result, DUMMY_GIVES_THIS) in calls def test_exception(system_info, conn_mock): @@ -241,6 +245,14 @@ def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]] return self.result, None +class OkStatusNoDataCollector(DataCollector[None, DummyDataModel, None]): + DATA_MODEL = DummyDataModel + + def collect_data(self, args=None) -> Tuple[TaskResult, Optional[DummyDataModel]]: + self.result.status = ExecutionStatus.OK + return self.result, None + + def test_collector_returning_data_with_unset_status_is_ok(system_info, conn_mock): """Baseline: data collected with an unset status finalizes to OK.""" collector = UnsetStatusWithDataCollector(system_info, conn_mock) @@ -259,3 +271,13 @@ def test_collector_returning_no_data_with_unset_status_is_execution_failure(syst assert data is None assert result.status == ExecutionStatus.EXECUTION_FAILURE + + +def test_collector_returning_no_data_with_ok_status_is_execution_failure(system_info, conn_mock): + """A collector that returns no data and never sets a status must not be reported as OK.""" + collector = UnsetStatusNoDataCollector(system_info, conn_mock) + + result, data = collector.collect_data() + + assert data is None + assert result.status == ExecutionStatus.EXECUTION_FAILURE From e5848f80457b213830be7bfd73088efc97f890a8 Mon Sep 17 00:00:00 2001 From: graepaul_amdeng Date: Fri, 18 Sep 2026 13:35:38 -0700 Subject: [PATCH 4/5] Adding __test__ to make pytest to ignore the class --- nodescraper/models/datamodel.py | 2 +- test/unit/framework/test_cli_helper.py | 2 +- test/unit/framework/test_plugin_executor.py | 8 ++++++++ test/unit/framework/test_regexanalyzer.py | 2 ++ test/unit/framework/test_type_utils.py | 7 +++++++ 5 files changed, 19 insertions(+), 2 deletions(-) diff --git a/nodescraper/models/datamodel.py b/nodescraper/models/datamodel.py index 09fc81f3..c57ca93f 100644 --- a/nodescraper/models/datamodel.py +++ b/nodescraper/models/datamodel.py @@ -78,7 +78,7 @@ def log_model(self, log_path: str): ) exlude_fields = set() - for key in self.model_fields: + for key in self.__class__.model_fields: data = getattr(self, key) if isinstance(data, FileModel): data.log_model(log_path) diff --git a/test/unit/framework/test_cli_helper.py b/test/unit/framework/test_cli_helper.py index 5139b601..911588d3 100644 --- a/test/unit/framework/test_cli_helper.py +++ b/test/unit/framework/test_cli_helper.py @@ -76,7 +76,7 @@ def test_generate_reference_config(plugin_registry): ] ref_config = generate_reference_config(results, plugin_registry, logging.getLogger()) - dump = ref_config.dict() + dump = ref_config.model_dump() assert dump["plugins"] == {"TestPluginA": {"analysis_args": {"model_attr": 17}}} diff --git a/test/unit/framework/test_plugin_executor.py b/test/unit/framework/test_plugin_executor.py index 584c8ff6..f5d46ba1 100644 --- a/test/unit/framework/test_plugin_executor.py +++ b/test/unit/framework/test_plugin_executor.py @@ -47,6 +47,8 @@ class DummyArgs(BaseModel): class TestPluginA(PluginInterface[MockConnectionManager, None]): + __test__ = False # Tells pytest to ignore this class + CONNECTION_TYPE = MockConnectionManager COLLECTOR_ARGS = DummyArgs(foo="initial") ANALYZER_ARGS = DummyArgs(foo="initial") @@ -64,6 +66,8 @@ def run(self): class TestPluginB(PluginInterface[MockConnectionManager, None]): + __test__ = False # Tells pytest to ignore this class + CONNECTION_TYPE = MockConnectionManager def run(self, test_arg=None): @@ -75,6 +79,8 @@ def run(self, test_arg=None): class PostActionPlugin(PluginInterface[MockConnectionManager, None]): """Minimal plugin used as a post-action target in tests.""" + __test__ = False # Tells pytest to ignore this class + CONNECTION_TYPE = MockConnectionManager def run(self, **kwargs): @@ -84,6 +90,8 @@ def run(self, **kwargs): class TestPluginCapture(PluginInterface[MockConnectionManager, None]): """Records kwargs passed to run() for merge-order regression tests.""" + __test__ = False # Tells pytest to ignore this class + CONNECTION_TYPE = MockConnectionManager last_run_kwargs: Optional[dict] = None diff --git a/test/unit/framework/test_regexanalyzer.py b/test/unit/framework/test_regexanalyzer.py index 38a3fb2c..df4e8371 100644 --- a/test/unit/framework/test_regexanalyzer.py +++ b/test/unit/framework/test_regexanalyzer.py @@ -43,6 +43,8 @@ class DummyArgs(BaseModel): class TestRegexAnalyzer(RegexAnalyzer[DummyData, DummyArgs]): + __test__ = False # Tells pytest to ignore this class + DATA_MODEL = DummyData ERROR_REGEX = [ diff --git a/test/unit/framework/test_type_utils.py b/test/unit/framework/test_type_utils.py index 48843f56..ad613d83 100644 --- a/test/unit/framework/test_type_utils.py +++ b/test/unit/framework/test_type_utils.py @@ -33,6 +33,7 @@ class TestGenericBase(Generic[T]): + __test__ = False # Tells pytest to ignore this class def __init__(self, generic_type: T): self.generic_type = generic_type @@ -42,6 +43,8 @@ def test_func(self, arg: list[str], arg2: Union[bool, str], arg3: Optional[int] class TestGenericImpl(TestGenericBase[str]): + __test__ = False # Tells pytest to ignore this class + pass @@ -50,10 +53,14 @@ class WiringMixin: class TestMixinFirstImpl(WiringMixin, TestGenericBase[str]): + __test__ = False # Tells pytest to ignore this class + pass class TestModel(BaseModel): + __test__ = False # Tells pytest to ignore this class + str_attr: str int_attr: int list_attr: list[str] From ea45aa221c1a9454396b279093787b4905bd202b Mon Sep 17 00:00:00 2001 From: graepaul_amdeng Date: Mon, 21 Sep 2026 14:55:17 -0700 Subject: [PATCH 5/5] A little cleanup after performing a self-review --- nodescraper/cli/helper.py | 2 +- nodescraper/configregistry.py | 10 ++++++---- nodescraper/interfaces/dataanalyzertask.py | 4 +--- nodescraper/interfaces/dataplugin.py | 2 +- nodescraper/models/datamodel.py | 7 +++---- nodescraper/pluginregistry.py | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/nodescraper/cli/helper.py b/nodescraper/cli/helper.py index e6ef2b7c..5385de76 100644 --- a/nodescraper/cli/helper.py +++ b/nodescraper/cli/helper.py @@ -117,7 +117,7 @@ def get_plugin_configs( base_config.global_args["system_interaction_level"] = system_interaction_level # Copy each until we are done - plugin_configs = [deepcopy(c) for c in [base_config]] + plugin_configs = [deepcopy(base_config)] if plugin_config_input: for config in plugin_config_input: diff --git a/nodescraper/configregistry.py b/nodescraper/configregistry.py index 1d5cdcaf..c56cf922 100644 --- a/nodescraper/configregistry.py +++ b/nodescraper/configregistry.py @@ -89,9 +89,9 @@ def load_configs(self, config_path: Optional[str] = None): else: self.configs[config_file.name] = config_model except (ValidationError, json.JSONDecodeError, TypeError) as e: - raise RuntimeError(f"Failed to load config from {config_file}: {e}") - except (OSError, IOError, FileNotFoundError): - raise RuntimeError(f"Failed to open config file {config_file}") + raise RuntimeError(f"Failed to load config from {config_file}: {e}") from e + except (OSError, IOError, FileNotFoundError) as e: + raise RuntimeError(f"Failed to open config file {config_file}") from e @staticmethod def _entry_points_for_group(group: str): @@ -110,7 +110,9 @@ def _entry_points_for_group(group: str): return all_eps.get(group, []) # type: ignore[assignment, attr-defined, arg-type] @staticmethod - def _resolve_entry_point_config(loaded: Any) -> PluginConfig | dict[str, Any] | None: + def _resolve_entry_point_config( + loaded: Any, + ) -> PluginConfig | dict[str, Any] | None: """Resolve a loaded entry point object into a plugin config. Args: diff --git a/nodescraper/interfaces/dataanalyzertask.py b/nodescraper/interfaces/dataanalyzertask.py index 27936042..36a46e55 100644 --- a/nodescraper/interfaces/dataanalyzertask.py +++ b/nodescraper/interfaces/dataanalyzertask.py @@ -118,9 +118,7 @@ class DataAnalyzer(Task, abc.ABC, Generic[TDataModel, TAnalyzeArg]): def __init_subclass__(cls, **kwargs: dict[str, Any]) -> None: super().__init_subclass__(**kwargs) - if (not inspect.isabstract(cls) and not getattr(cls, "DATA_MODEL", None)) or ( - not inspect.isabstract(cls) and cls.DATA_MODEL is None - ): + if not inspect.isabstract(cls) and not getattr(cls, "DATA_MODEL", None): raise TypeError(f"No data model set for {cls.__name__}") if not hasattr(cls, "analyze_data") or not callable(cls.analyze_data): raise TypeError(f"No analyze_data method defined for {cls.__name__}") diff --git a/nodescraper/interfaces/dataplugin.py b/nodescraper/interfaces/dataplugin.py index 2669e4ca..da46e47b 100644 --- a/nodescraper/interfaces/dataplugin.py +++ b/nodescraper/interfaces/dataplugin.py @@ -435,7 +435,7 @@ def analyze( Args: max_event_priority_level (Union[EventPriority, str], optional): priority limit for events. Defaults to EventPriority.CRITICAL. - analReaysis_args (Optional[Union[TAnalyzeArg , dict]], optional): args for data analysis. Defaults to None. + analysis_args (Optional[Union[TAnalyzeArg , dict]], optional): args for data analysis. Defaults to None. data (Optional[Union[str, dict, TDataModel]], optional): data to analyze. Defaults to None. Returns: diff --git a/nodescraper/models/datamodel.py b/nodescraper/models/datamodel.py index c57ca93f..45c21049 100644 --- a/nodescraper/models/datamodel.py +++ b/nodescraper/models/datamodel.py @@ -27,7 +27,7 @@ import json import os import tarfile -from typing import TypeVar, Union +from typing import Any, TypeVar, Union from pydantic import BaseModel, field_validator @@ -92,7 +92,7 @@ def merge_data(self, input_data: "DataModel") -> None: pass @classmethod - def import_model(cls: type[TDataModel], model_input: Union[dict, str]) -> TDataModel: + def import_model(cls: type[TDataModel], model_input: Union[dict[str, Any], str]) -> TDataModel: """import a data model if the input is a string attempt to read data from file using the string as a file name if input is a dict, pass key value pairs directly to init function @@ -100,7 +100,7 @@ def import_model(cls: type[TDataModel], model_input: Union[dict, str]) -> TDataM Args: cls (type[DataModel]): Data model class - model_input (Union[dict, str]): model data input + model_input (Union[dict[str, Any], str]): model data input Raises: ValueError: if model_input has an invalid type @@ -124,7 +124,6 @@ def import_model(cls: type[TDataModel], model_input: Union[dict, str]) -> TDataM with open(model_input, "r", encoding="utf-8") as input_file: data = json.load(input_file) return cls(**data) - return cls() raise ValueError("Invalid input for model data") diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index 47c46979..ff6a7478 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -230,7 +230,7 @@ def load_connection_managers_from_entry_points() -> dict[str, type]: with PluginRegistry._cache_lock: # Check again inside the lock to prevent duplicate work if PluginRegistry._entry_point_connection_managers_cache is not None: - return PluginRegistry._entry_point_connection_managers_cache + return PluginRegistry._entry_point_connection_managers_cache.copy() managers = PluginRegistry._load_connection_managers_uncached()