diff --git a/ARCHITECTURE_plugins.md b/ARCHITECTURE_plugins.md new file mode 100644 index 00000000..bb409fe8 --- /dev/null +++ b/ARCHITECTURE_plugins.md @@ -0,0 +1,180 @@ +# Plugin Architecture + +This document describes the plugin architecture of the ArduPilot Methodic Configurator, +which allows extending the application with modular components for UI enhancements and workflow automation. + +## Overview + +The plugin system enables developers to add new functionality to the application without modifying the core codebase. +There are two types of plugins: + +1. **UI Plugins**: Provide persistent user interface components (e.g., motor test panels) +2. **Workflow Plugins**: Execute triggered actions (e.g., IMU temperature calibration workflows) + +## Architecture Principles + +### Separation of Concerns + +- **Business Logic**: Encapsulated in data models with no UI dependencies +- **UI Coordination**: Handled by coordinators that orchestrate user interactions +- **Dependency Injection**: UI callbacks injected into business logic for testability + +### Plugin Lifecycle + +- **Registration**: Plugins register themselves with factories during application startup +- **Discovery**: The system discovers available plugins through factory queries +- **Instantiation**: Plugins are created on-demand with appropriate context +- **Execution**: UI plugins remain active; workflow plugins execute once and complete + +## UI Plugin System + +### UI Components + +#### Plugin Factory UI (`plugin_factory_ui.py`) + +- Manages registration and creation of UI plugins +- Provides a registry of plugin creators +- Ensures unique plugin names + +#### Plugin Constants (`plugin_constants.py`) + +- Defines string constants for plugin identifiers +- Centralizes plugin naming for consistency + +#### Registration (`__main__.py`) + +- Plugins register their creators during application initialization +- Ensures plugins are available when needed + +### UI Implementation Pattern + +```python +# 1. Define plugin constant +PLUGIN_MOTOR_TEST = "motor_test" + +# 2. Create plugin view class +class MotorTestView: + def __init__(self, parent, model, base_window): + # Implementation + +# 3. Create factory function +def create_motor_test_view(parent, model, base_window): + return MotorTestView(parent, model, base_window) + +# 4. Register plugin +plugin_factory_ui.register(PLUGIN_MOTOR_TEST, create_motor_test_view) +``` + +### UI usage in Parameter Editor + +- UI plugins are instantiated when a parameter file with plugin configuration is selected +- Plugins receive parent frame, data model, and base window references +- Plugins manage their own lifecycle and cleanup + +## Workflow Plugin System + +### Workflow Components + +#### Plugin Factory Workflow (`plugin_factory_workflow.py`) + +- Manages registration and creation of workflow plugins +- Symmetric API to UI plugin factory +- Handles one-time execution workflows + +#### Data Models + +- Contain business logic with no UI dependencies +- Use dependency injection for user interactions +- Implement callback-based interfaces for testability + +#### Workflow Coordinators + +- Handle UI orchestration for workflows +- Manage progress indicators and user feedback +- Ensure proper cleanup after execution + +### Workflow Implementation Pattern + +```python +# 1. Define plugin constant +PLUGIN_TEMPCAL_IMU = "tempcal_imu" + +# 2. Create data model class +class TempCalIMUDataModel: + def __init__(self, config_manager, step_filename, callbacks...): + # Business logic only + + def run_calibration(self) -> bool: + # Orchestrate workflow using injected callbacks + +# 3. Create workflow coordinator class +class TempCalIMUWorkflow: + def __init__(self, root_window, data_model): + # UI coordination + + def run_workflow(self) -> bool: + # Execute workflow with UI feedback + +# 4. Create factory function +def create_tempcal_imu_workflow(root_window, data_model): + return TempCalIMUWorkflow(root_window, data_model) + +# 5. Register plugin +plugin_factory_workflow.register(PLUGIN_TEMPCAL_IMU, create_tempcal_imu_workflow) +``` + +### Workflow usage in Parameter Editor + +- Workflow plugins are triggered when specific parameter files are selected +- Configuration manager creates data models with injected UI callbacks +- Workflow coordinators handle execution and provide user feedback +- Cleanup ensures resources are released after completion + +## Configuration Integration + +### JSON Schema Updates + +The configuration schema supports plugin definitions: + +```json +{ + "plugin": { + "name": "tempcal_imu", + "placement": "workflow" + } +} +``` + +### Supported Placements + +- `"left"`: Plugin appears left of scrollable content +- `"top"`: Plugin appears above content +- `"workflow"`: Plugin executes as a triggered workflow + +## Testing Strategy + +### Unit Testing + +- Business logic tested in isolation with mocked callbacks +- Plugin factories tested for registration and creation +- Data models tested for all execution paths + +### Integration Testing + +- End-to-end plugin execution workflows +- UI interaction verification +- Configuration loading and plugin discovery + +## Implementation Notes + +### Error Handling + +- Plugin loading failures don't prevent application startup +- Individual plugin errors are logged and isolated +- Graceful degradation when plugins are unavailable + +### Performance Considerations + +- Lazy loading prevents startup delays +- Plugin instantiation on-demand reduces memory usage +- Callback-based design minimizes coupling overhead diff --git a/ardupilot_methodic_configurator/__main__.py b/ardupilot_methodic_configurator/__main__.py index e1955249..be68f84d 100755 --- a/ardupilot_methodic_configurator/__main__.py +++ b/ardupilot_methodic_configurator/__main__.py @@ -48,8 +48,8 @@ from ardupilot_methodic_configurator.frontend_tkinter_parameter_editor import ParameterEditorWindow from ardupilot_methodic_configurator.frontend_tkinter_project_opener import VehicleProjectOpenerWindow from ardupilot_methodic_configurator.frontend_tkinter_show import show_error_message -from ardupilot_methodic_configurator.plugin_constants import PLUGIN_MOTOR_TEST -from ardupilot_methodic_configurator.plugin_factory import plugin_factory +from ardupilot_methodic_configurator.plugin_factory_ui import plugin_factory_ui +from ardupilot_methodic_configurator.plugin_factory_workflow import plugin_factory_workflow def register_plugins() -> None: @@ -62,9 +62,12 @@ def register_plugins() -> None: # Import and register motor test plugin # pylint: disable=import-outside-toplevel, cyclic-import from ardupilot_methodic_configurator.frontend_tkinter_motor_test import register_motor_test_plugin # noqa: PLC0415 + from ardupilot_methodic_configurator.frontend_tkinter_tempcal_imu import register_tempcal_imu_plugin # noqa: PLC0415 + # pylint: enable=import-outside-toplevel, cyclic-import register_motor_test_plugin() + register_tempcal_imu_plugin() # Add more plugin registrations here in the future @@ -85,12 +88,25 @@ def validate_plugin_registry(local_filesystem: LocalFilesystem) -> None: if plugin and plugin.get("name"): configured_plugins.add(plugin["name"]) - # Verify each configured plugin is registered + # Verify each configured plugin is registered (check both UI and workflow factories) for plugin_name in configured_plugins: - if not plugin_factory.is_registered(plugin_name): + is_ui_plugin = plugin_factory_ui.is_registered(plugin_name) + is_workflow_plugin = plugin_factory_workflow.is_registered(plugin_name) + + if not is_ui_plugin and not is_workflow_plugin: + available_ui = ", ".join(plugin_factory_ui.get_registered_plugins()) + available_workflow = ", ".join(plugin_factory_workflow.get_registered_plugins()) logging_error( - _("Plugin '%(plugin_name)s' is configured but not registered. Available plugins: %(available)s"), - {"plugin_name": plugin_name, "available": PLUGIN_MOTOR_TEST}, + _( + "Plugin '%(plugin_name)s' is configured but not registered. " + "Available UI plugins: %(ui_plugins)s. " + "Available workflow plugins: %(workflow_plugins)s" + ), + { + "plugin_name": plugin_name, + "ui_plugins": available_ui or _("none"), + "workflow_plugins": available_workflow or _("none"), + }, ) diff --git a/ardupilot_methodic_configurator/backend_filesystem.py b/ardupilot_methodic_configurator/backend_filesystem.py index 1c2283eb..743f36c7 100644 --- a/ardupilot_methodic_configurator/backend_filesystem.py +++ b/ardupilot_methodic_configurator/backend_filesystem.py @@ -639,9 +639,8 @@ def remove_created_files_and_vehicle_dir(self) -> str: def getcwd() -> str: return os_getcwd() - def tempcal_imu_result_param_tuple(self) -> tuple[str, str]: - tempcal_imu_result_param_filename = "03_imu_temperature_calibration_results.param" - return tempcal_imu_result_param_filename, os_path.join(self.vehicle_dir, tempcal_imu_result_param_filename) + def get_configuration_file_fullpath(self, filename: str) -> str: + return os_path.join(self.vehicle_dir, filename) def copy_fc_values_to_file(self, selected_file: str, params: dict[str, float]) -> int: ret = 0 diff --git a/ardupilot_methodic_configurator/business_logic_tempcal_imu.py b/ardupilot_methodic_configurator/business_logic_tempcal_imu.py new file mode 100644 index 00000000..97458e86 --- /dev/null +++ b/ardupilot_methodic_configurator/business_logic_tempcal_imu.py @@ -0,0 +1,158 @@ +""" +Business logic for IMU temperature calibration. + +This module provides the data model and business logic for performing +IMU temperature calibration based on flight log data. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Callable + +from ardupilot_methodic_configurator import _ +from ardupilot_methodic_configurator.tempcal_imu import IMUfit + +if TYPE_CHECKING: + from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager + + +class TempCalIMUDataModel: # pylint: disable=too-many-instance-attributes + """ + Data model for IMU temperature calibration. + + This class encapsulates the business logic for performing IMU temperature + calibration, including checking if calibration should be offered and + running the calibration workflow. + """ + + def __init__( # pylint: disable=too-many-arguments, too-many-positional-arguments + self, + configuration_manager: ConfigurationManager, + step_filename: str, + ask_confirmation: Callable[[str, str], bool], + select_file: Callable[[str, list[tuple[str, list[str]]]], str | None], + show_warning: Callable[[str, str], None], + show_error: Callable[[str, str], None], + progress_callback: Callable[[int], None] | None = None, + cleanup_callback: Callable[[], None] | None = None, + ) -> None: + """ + Initialize the IMU temperature calibration data model. + + Args: + configuration_manager: The configuration manager for accessing vehicle data and parameters + step_filename: The configuration step filename that triggered the calibration + ask_confirmation: Callback for yes/no confirmation dialogs (title, message) -> bool + select_file: Callback for file selection dialog (title, filetypes) -> filepath | None + show_warning: Callback for warning messages (title, message) -> None + show_error: Callback for error messages (title, message) -> None + progress_callback: Optional callback for progress updates (0-100) + cleanup_callback: Optional callback for cleanup after workflow completion + + """ + self._configuration_manager = configuration_manager + self._step_filename = step_filename + self._ask_confirmation = ask_confirmation + self._select_file = select_file + self._show_warning = show_warning + self._show_error = show_error + self._progress_callback = progress_callback + self._cleanup_callback = cleanup_callback + + def get_result_param_fullpath(self) -> str: + """ + Get the full path of the result parameter file. + + Returns: + str: The full path to the IMU calibration result parameter file + + """ + return self._configuration_manager.get_configuration_file_fullpath(self._step_filename) + + def get_confirmation_message(self) -> str: + """ + Get the confirmation message to show to the user. + + Returns: + str: The formatted confirmation message + + """ + return _( + "If you proceed the {tempcal_imu_result_param_filename}\n" + "will be overwritten with the new calibration results.\n" + "Do you want to provide a .bin log file and\n" + "run the IMU temperature calibration using it?" + ).format(tempcal_imu_result_param_filename=self._step_filename) + + def run_calibration(self) -> bool: + """ + Run the IMU temperature calibration workflow with user interaction. + + This method orchestrates the complete calibration process through injected + callback functions (provided at construction), achieving separation between + business logic and GUI implementation. The cleanup callback is guaranteed + to be called whether the workflow succeeds or fails. + + Returns: + bool: True if calibration was performed successfully, False otherwise + + Raises: + SystemExit: If the calibration encounters a fatal error reading parameter files + + """ + try: + # Ask user for confirmation + if not self._ask_confirmation( + _("IMU temperature calibration"), + self.get_confirmation_message(), + ): + return False + + # Select log file + log_file = self._select_file( + _("Select ArduPilot binary log file"), + [(_("ArduPilot binary log files"), ["*.bin", "*.BIN"])], + ) + if not log_file: + return False # User cancelled file selection + + # Show warning about processing time + self._show_warning( + _("IMU temperature calibration"), + _("Please wait, this can take a really long time and\nthe GUI will be unresponsive until it finishes."), + ) + + tempcal_imu_result_param_fullpath = self.get_result_param_fullpath() + + # Perform the actual IMU temperature calibration + # This calls the existing IMUfit function from tempcal_imu.py + try: + IMUfit( + logfile=log_file, + outfile=tempcal_imu_result_param_fullpath, + no_graph=False, + log_parm=False, + online=False, + tclr=False, + figpath=self._configuration_manager.vehicle_dir, + progress_callback=self._progress_callback, + ) + + # Reload parameter files after calibration + self._configuration_manager.reload_parameter_files() + return True + + except SystemExit as exp: + self._show_error(_("Fatal error reading parameter files"), f"{exp}") + raise + + finally: + # Always call cleanup callback if provided (e.g., destroy progress window) + if self._cleanup_callback: + self._cleanup_callback() diff --git a/ardupilot_methodic_configurator/configuration_manager.py b/ardupilot_methodic_configurator/configuration_manager.py index 519dedd7..cc0285ab 100644 --- a/ardupilot_methodic_configurator/configuration_manager.py +++ b/ardupilot_methodic_configurator/configuration_manager.py @@ -25,12 +25,12 @@ from ardupilot_methodic_configurator.backend_filesystem_configuration_steps import PhaseData from ardupilot_methodic_configurator.backend_flightcontroller import FlightController from ardupilot_methodic_configurator.backend_internet import download_file_from_url +from ardupilot_methodic_configurator.business_logic_tempcal_imu import TempCalIMUDataModel from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter from ardupilot_methodic_configurator.data_model_configuration_step import ConfigurationStepProcessor from ardupilot_methodic_configurator.data_model_motor_test import MotorTestDataModel from ardupilot_methodic_configurator.data_model_par_dict import Par, ParDict, is_within_tolerance -from ardupilot_methodic_configurator.plugin_constants import PLUGIN_MOTOR_TEST -from ardupilot_methodic_configurator.tempcal_imu import IMUfit +from ardupilot_methodic_configurator.plugin_constants import PLUGIN_MOTOR_TEST, PLUGIN_TEMPCAL_IMU # Type aliases for callback functions used in workflow methods AskConfirmationCallback = Callable[[str, str], bool] # (title, message) -> bool @@ -112,84 +112,6 @@ def is_mavftp_supported(self) -> bool: else False ) - def handle_imu_temperature_calibration_workflow( # pylint: disable=too-many-arguments, too-many-positional-arguments - self, - selected_file: str, - ask_user_confirmation: AskConfirmationCallback, - select_file: SelectFileCallback, - show_warning: ShowWarningCallback, - show_error: ShowErrorCallback, - progress_callback: Optional[Callable] = None, - ) -> bool: - """ - Complete IMU temperature calibration workflow with user interaction via callbacks. - - This method orchestrates the entire IMU calibration workflow including user confirmation, - file selection, warnings, error handling, and the actual calibration through injected - callback functions. This allows the business logic to be separated from GUI implementation details. - - Args: - selected_file: The current parameter file being processed. - ask_user_confirmation: Callback function for asking yes/no questions. - select_file: Callback function for file selection dialog. - show_warning: Callback function for showing warning messages. - show_error: Callback function for showing error messages. - progress_callback: Optional callback function for progress updates. - - Returns: - bool: True if calibration was performed successfully, False otherwise. - - """ - # Check if IMU temperature calibration should be offered for this file - tempcal_imu_result_param_filename, tempcal_imu_result_param_fullpath = ( - self._local_filesystem.tempcal_imu_result_param_tuple() - ) - if selected_file != tempcal_imu_result_param_filename: - return False - - # Ask user for confirmation using injected callback - confirmation_msg = _( - "If you proceed the {tempcal_imu_result_param_filename}\n" - "will be overwritten with the new calibration results.\n" - "Do you want to provide a .bin log file and\n" - "run the IMU temperature calibration using it?" - ).format(tempcal_imu_result_param_filename=tempcal_imu_result_param_filename) - - if not ask_user_confirmation(_("IMU temperature calibration"), confirmation_msg): - return False - - # Select log file using injected callback - log_file = select_file(_("Select ArduPilot binary log file"), ["*.bin", "*.BIN"]) - - if not log_file: - return False # User cancelled file selection - - # Show warning using injected callback - show_warning( - _("IMU temperature calibration"), - _("Please wait, this can take a really long time and\nthe GUI will be unresponsive until it finishes."), - ) - - # Perform the actual IMU temperature calibration - IMUfit( - logfile=log_file, - outfile=tempcal_imu_result_param_fullpath, - no_graph=False, - log_parm=False, - online=False, - tclr=False, - figpath=self._local_filesystem.vehicle_dir, - progress_callback=progress_callback, - ) - - try: - # Reload parameter files after calibration - self._local_filesystem.file_parameters = self._local_filesystem.read_params_from_files() - return True - except SystemExit as exp: - show_error(_("Fatal error reading parameter files"), f"{exp}") - raise - def _should_copy_fc_values_to_file(self, selected_file: str) -> tuple[bool, Optional[dict], Optional[str]]: """ Check if flight controller values should be copied to the specified file. @@ -1596,17 +1518,18 @@ def parse_mandatory_level_percentage(self, text: str) -> tuple[int, str]: # frontend_tkinter_parameter_editor_documentation_frame.py API end - # plugin API begin + # UI plugin API begin def get_plugin(self, filename: str) -> Optional[dict]: return self._local_filesystem.get_plugin(filename) - def create_plugin_data_model(self, plugin_name: str) -> Optional[object]: + def create_plugin_data_model(self, plugin_name: str, **kwargs) -> Optional[object]: """ Create and return a data model for the specified plugin. Args: plugin_name: The name of the plugin to create a data model for + **kwargs: Additional plugin-specific arguments (e.g., step_filename for tempcal_imu) Returns: The data model instance, or None if plugin not supported or requirements not met @@ -1616,7 +1539,54 @@ def create_plugin_data_model(self, plugin_name: str) -> Optional[object]: if not self.is_fc_connected: return None return MotorTestDataModel(self._flight_controller, self._local_filesystem) + if plugin_name == PLUGIN_TEMPCAL_IMU: + step_filename = kwargs.get("step_filename") + if not step_filename: + return None + # Callbacks must be provided by the caller (GUI layer) + ask_confirmation = kwargs.get("ask_confirmation") + select_file = kwargs.get("select_file") + show_warning = kwargs.get("show_warning") + show_error = kwargs.get("show_error") + progress_callback = kwargs.get("progress_callback") + cleanup_callback = kwargs.get("cleanup_callback") + if not all([ask_confirmation, select_file, show_warning, show_error, progress_callback, cleanup_callback]): + return None + return TempCalIMUDataModel( + self, + step_filename, + ask_confirmation, # type: ignore[arg-type] + select_file, # type: ignore[arg-type] + show_warning, # type: ignore[arg-type] + show_error, # type: ignore[arg-type] + progress_callback, + cleanup_callback, + ) # Add more plugins here in the future return None - # plugin API end + # UI plugin API end + + # workflow plugin API start + + @property + def vehicle_dir(self) -> str: + """Get the vehicle configuration directory path.""" + return self._local_filesystem.vehicle_dir + + def get_configuration_file_fullpath(self, filename: str) -> str: + """Get the full path for a configuration file in the vehicle directory.""" + return self._local_filesystem.get_configuration_file_fullpath(filename) + + def reload_parameter_files(self) -> dict[str, ParDict]: + """ + Reload all parameter files from disk. + + Raises: + SystemExit: If there's a fatal error reading parameter files + + """ + self._local_filesystem.file_parameters = self._local_filesystem.read_params_from_files() + return self._local_filesystem.file_parameters + + # Workflow plugin API end diff --git a/ardupilot_methodic_configurator/configuration_steps_ArduCopter.json b/ardupilot_methodic_configurator/configuration_steps_ArduCopter.json index cc529fd5..47a3556c 100644 --- a/ardupilot_methodic_configurator/configuration_steps_ArduCopter.json +++ b/ardupilot_methodic_configurator/configuration_steps_ArduCopter.json @@ -35,7 +35,11 @@ "external_tool_url": "", "mandatory_text": "80% mandatory (20% optional)", "auto_changed_by": "", - "old_filenames": [] + "old_filenames": [], + "plugin": { + "name": "tempcal_imu", + "placement": "workflow" + } }, "04_board_orientation.param": { "why": "Correct orientation ensures that the flight controller accurately interprets the vehicle's movements and orientation, which is fundamental for stable flight and navigation.", diff --git a/ardupilot_methodic_configurator/configuration_steps_schema.json b/ardupilot_methodic_configurator/configuration_steps_schema.json index af509873..64f2fefc 100644 --- a/ardupilot_methodic_configurator/configuration_steps_schema.json +++ b/ardupilot_methodic_configurator/configuration_steps_schema.json @@ -152,13 +152,13 @@ "properties": { "name": { "type": "string", - "enum": ["motor_test"], + "enum": ["motor_test", "tempcal_imu"], "description": "The name of the plugin to load" }, "placement": { "type": "string", - "enum": ["left", "top"], - "description": "Where to place the plugin: 'left' for left of scrollable frame, 'top' for above contents" + "enum": ["left", "top", "workflow"], + "description": "Where to place the plugin: 'left' for left of scrollable frame, 'top' for above contents, 'workflow' for triggered workflow actions" } } } diff --git a/ardupilot_methodic_configurator/frontend_tkinter_motor_test.py b/ardupilot_methodic_configurator/frontend_tkinter_motor_test.py index 058f6cd1..023de22c 100644 --- a/ardupilot_methodic_configurator/frontend_tkinter_motor_test.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_motor_test.py @@ -59,7 +59,7 @@ from ardupilot_methodic_configurator.frontend_tkinter_progress_window import ProgressWindow from ardupilot_methodic_configurator.frontend_tkinter_scroll_frame import ScrollFrame from ardupilot_methodic_configurator.plugin_constants import PLUGIN_MOTOR_TEST -from ardupilot_methodic_configurator.plugin_factory import plugin_factory +from ardupilot_methodic_configurator.plugin_factory_ui import plugin_factory_ui class DelayedProgressCallback: # pylint: disable=too-few-public-methods @@ -915,7 +915,7 @@ def _create_motor_test_view( def register_motor_test_plugin() -> None: """Register the motor test plugin with the factory.""" - plugin_factory.register(PLUGIN_MOTOR_TEST, _create_motor_test_view) + plugin_factory_ui.register(PLUGIN_MOTOR_TEST, _create_motor_test_view) if __name__ == "__main__": diff --git a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py index 9e59a77e..14f18b5a 100755 --- a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py @@ -53,7 +53,8 @@ from ardupilot_methodic_configurator.frontend_tkinter_show import show_tooltip from ardupilot_methodic_configurator.frontend_tkinter_stage_progress import StageProgressBar from ardupilot_methodic_configurator.frontend_tkinter_usage_popup_window import UsagePopupWindow -from ardupilot_methodic_configurator.plugin_factory import plugin_factory +from ardupilot_methodic_configurator.plugin_factory_ui import plugin_factory_ui +from ardupilot_methodic_configurator.plugin_factory_workflow import plugin_factory_workflow # pylint: disable=too-many-lines @@ -577,7 +578,7 @@ def __load_plugin(self, parent_frame: ttk.Frame, plugin: dict) -> None: return # Check if plugin is registered - if not plugin_factory.is_registered(plugin_name): + if not plugin_factory_ui.is_registered(plugin_name): error_msg = _("Unknown plugin: {plugin_name}").format(plugin_name=plugin_name) ttk.Label(parent_frame, text=error_msg, foreground="red").pack() logging_error(error_msg) @@ -593,7 +594,7 @@ def __load_plugin(self, parent_frame: ttk.Frame, plugin: dict) -> None: # Create plugin using factory with error handling try: - plugin_view = plugin_factory.create(plugin_name, parent_frame, model, self) + plugin_view = plugin_factory_ui.create(plugin_name, parent_frame, model, self) if plugin_view is None: msg = _("Failed to create plugin: {plugin_name}").format(plugin_name=plugin_name) logging_error(msg) @@ -675,39 +676,92 @@ def __display_usage_popup_window(parent: tk.Tk) -> None: instructions_text, ) - def __do_tempcal_imu(self, selected_file: str) -> None: + def __run_workflow_plugin(self, selected_file: str, plugin: dict) -> None: """ - Handle IMU temperature calibration using the new callback-based workflow. + Execute a workflow plugin for the selected file. + + Workflow plugins are triggered actions that run when a specific file is selected. + Unlike UI plugins (which create persistent views), workflow plugins: + 1. Create a data model (business logic) + 2. Create a workflow coordinator (UI logic) + 3. Execute the workflow once and complete + + Args: + selected_file: The currently selected parameter file + plugin: Plugin configuration dict with 'name' and 'placement' keys - This method creates GUI-specific callback functions and injects them into - the business logic workflow method, achieving proper separation of concerns. """ + plugin_name = plugin.get("name") + if not plugin_name: + logging_warning(_("Workflow plugin configuration missing name")) + return + + # Check if plugin is registered + if not plugin_factory_workflow.is_registered(plugin_name): + error_msg = _("Unknown workflow plugin: {plugin_name}").format(plugin_name=plugin_name) + logging_warning(error_msg) + return - def select_file(title: str, filetypes: list[str]) -> Optional[str]: # noqa: UP045 - """GUI callback for file selection dialog.""" - return filedialog.askopenfilename(title=title, filetypes=[(_("ArduPilot binary log files"), filetypes)]) + # File open window for tempcal_imu workflow must passed to the data model to make testing code simpler + def select_file_callback(title: str, filetypes: list[tuple[str, list[str]]]) -> str | None: + """Wrapper for filedialog.askopenfilename matching expected signature.""" + return filedialog.askopenfilename(title=title, filetypes=filetypes) - # Create progress window for the calibration - self.tempcal_imu_progress_window = ProgressWindow( + # Progress window for tempcal_imu workflow must passed to the data model to make testing code simpler + progress_win = ProgressWindow( self.root, _("Reading IMU calibration messages"), _("Please wait, this can take a long time"), only_show_when_update_progress_called=True, ) - try: - # Inject GUI callbacks into business logic workflow - _success = self.configuration_manager.handle_imu_temperature_calibration_workflow( - selected_file, - ask_user_confirmation=ask_yesno_popup, - select_file=select_file, - show_warning=show_warning_popup, - show_error=show_error_popup, - progress_callback=self.tempcal_imu_progress_window.update_progress_bar_300_pct, - ) + def cleanup_progress() -> None: + """Cleanup callback to destroy progress window.""" + progress_win.destroy() - finally: - self.tempcal_imu_progress_window.destroy() + # Step 1: Create the data model (business logic layer) + # Pass GUI callbacks for dependency injection + model = self.configuration_manager.create_plugin_data_model( + plugin_name, + step_filename=selected_file, + ask_confirmation=ask_yesno_popup, + select_file=select_file_callback, + show_warning=show_warning_popup, + show_error=show_error_popup, + progress_callback=progress_win.update_progress_bar_300_pct, + cleanup_callback=cleanup_progress, + ) + if model is None: + cleanup_progress() + logging_warning(_("Failed to create data model for workflow plugin: %s"), plugin_name) + return + + # Step 2: Create the workflow coordinator (UI coordination layer) + # The factory creates a coordinator that knows how to orchestrate the workflow + workflow_coordinator = plugin_factory_workflow.create(plugin_name, self.root, model) + if workflow_coordinator is None: + cleanup_progress() + logging_warning(_("Failed to create workflow coordinator for plugin: %s"), plugin_name) + return + + # Step 3: Execute the workflow + # The coordinator handles all user interaction and workflow execution + # Note: cleanup is handled by the data model's finally block in run_calibration() + try: + if hasattr(workflow_coordinator, "run_workflow"): + workflow_coordinator.run_workflow() # pyright: ignore[reportAttributeAccessIssue] + else: + logging_warning(_("Workflow coordinator for %s does not have a run_workflow() method"), plugin_name) + show_error_popup( + _("Plugin Error"), + _("Workflow plugin {plugin_name} is misconfigured").format(plugin_name=plugin_name), + ) + except Exception as e: # pylint: disable=broad-except + if isinstance(e, SystemExit): + raise # Allow SystemExit to propagate + logging_error(_("Error executing workflow plugin %s: %s"), plugin_name, str(e)) + error_message = _("Failed to execute {plugin_name}: {error}").format(plugin_name=plugin_name, error=str(e)) + show_error_popup(_("Workflow Failed"), error_message) def __handle_dialog_choice(self, result: list, dialog: tk.Toplevel, choice: ExperimentChoice) -> None: result.append(choice) @@ -850,7 +904,12 @@ def on_param_file_combobox_change(self, _event: Union[None, tk.Event], forced: b self._update_progress_bar_from_file(selected_file) if self.configuration_manager.current_file != selected_file or forced: self.write_changes_to_intermediate_parameter_file() - self.__do_tempcal_imu(selected_file) + + # Check if this file has a workflow plugin + plugin = self.configuration_manager.get_plugin(selected_file) + if plugin and plugin.get("placement") == "workflow": + self.__run_workflow_plugin(selected_file, plugin) + # open the documentation of the next step in the browser, # before giving the user the option to close the SW in the __should_copy_fc_values_to_file method if self.documentation_frame.get_auto_open_documentation_in_browser() or self.gui_complexity == "simple": @@ -865,9 +924,9 @@ def on_param_file_combobox_change(self, _event: Union[None, tk.Event], forced: b self.documentation_frame.refresh_documentation_labels() self.documentation_frame.update_why_why_now_tooltip() - # Update plugin layout if needed - plugin = self.configuration_manager.get_plugin(selected_file) - self.__update_plugin_layout(plugin) + # Update plugin layout (handles None to clear plugins, and skips workflow plugins) + if not plugin or plugin.get("placement") != "workflow": + self.__update_plugin_layout(plugin) self.repopulate_parameter_table(regenerate_from_disk=True) self._update_skip_button_state() diff --git a/ardupilot_methodic_configurator/frontend_tkinter_tempcal_imu.py b/ardupilot_methodic_configurator/frontend_tkinter_tempcal_imu.py new file mode 100644 index 00000000..ac637d91 --- /dev/null +++ b/ardupilot_methodic_configurator/frontend_tkinter_tempcal_imu.py @@ -0,0 +1,80 @@ +""" +Frontend integration for IMU temperature calibration. + +This module provides the GUI integration for IMU temperature calibration, +handling user interactions and coordinating with the business logic layer. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from ardupilot_methodic_configurator.plugin_constants import PLUGIN_TEMPCAL_IMU +from ardupilot_methodic_configurator.plugin_factory_workflow import plugin_factory_workflow + +if TYPE_CHECKING: + from ardupilot_methodic_configurator.business_logic_tempcal_imu import TempCalIMUDataModel + + +class TempCalIMUWorkflow: # pylint: disable=too-few-public-methods + """ + Manages the IMU temperature calibration workflow from the GUI perspective. + + This class handles the GUI-specific aspects of the calibration process, + including creating the progress window and coordinating workflow execution. + """ + + def __init__(self, root_window: object, data_model: TempCalIMUDataModel) -> None: + """ + Initialize the IMU temperature calibration workflow. + + Args: + root_window: The root Tkinter window for creating dialogs + data_model: The business logic data model for calibration + + """ + self.root_window = root_window + self.data_model = data_model + + def run_workflow(self) -> bool: + """ + Execute the complete IMU temperature calibration workflow. + + This method creates the progress window and delegates the + actual workflow logic to the business logic layer. + + Returns: + bool: True if calibration was performed successfully, False otherwise + + """ + # Run the calibration - cleanup is handled by the data model's finally block + return self.data_model.run_calibration() + + +def create_tempcal_imu_workflow( + root_window: object, + data_model: object, +) -> TempCalIMUWorkflow: + """ + Factory function to create a TempCalIMUWorkflow instance. + + Args: + root_window: The root Tkinter window + data_model: The business logic data model + + Returns: + TempCalIMUWorkflow: A new workflow instance + + """ + return TempCalIMUWorkflow(root_window, data_model) # type: ignore[arg-type] + + +def register_tempcal_imu_plugin() -> None: + """Register the tempcal_imu workflow plugin creator with the workflow factory.""" + plugin_factory_workflow.register(PLUGIN_TEMPCAL_IMU, create_tempcal_imu_workflow) diff --git a/ardupilot_methodic_configurator/plugin_constants.py b/ardupilot_methodic_configurator/plugin_constants.py index 0d020595..90538a3e 100644 --- a/ardupilot_methodic_configurator/plugin_constants.py +++ b/ardupilot_methodic_configurator/plugin_constants.py @@ -13,3 +13,4 @@ # Plugin name constants PLUGIN_MOTOR_TEST = "motor_test" +PLUGIN_TEMPCAL_IMU = "tempcal_imu" diff --git a/ardupilot_methodic_configurator/plugin_factory.py b/ardupilot_methodic_configurator/plugin_factory_ui.py similarity index 78% rename from ardupilot_methodic_configurator/plugin_factory.py rename to ardupilot_methodic_configurator/plugin_factory_ui.py index a10e11f2..dff2525d 100644 --- a/ardupilot_methodic_configurator/plugin_factory.py +++ b/ardupilot_methodic_configurator/plugin_factory_ui.py @@ -13,7 +13,6 @@ """ import tkinter as tk -from logging import error as logging_error from tkinter import ttk from typing import Callable, Optional, Union @@ -23,7 +22,7 @@ PluginCreator = Callable[[Union[tk.Frame, ttk.Frame], object, object], object] -class PluginFactory: +class PluginFactoryUI: """ Factory for creating plugin instances. @@ -41,13 +40,18 @@ def register(self, plugin_name: str, creator_func: PluginCreator) -> None: Register a plugin creator function. Args: - plugin_name: Unique identifier for the plugin + plugin_name: Unique identifier for the plugin (e.g., "motor_test") creator_func: Function that creates a plugin instance. - Should accept (parent, model, base_window) and return PluginView + Should accept (parent_frame, data_model, parameter_editor_window) + and return a plugin view object. + + Raises: + ValueError: If a plugin with the same name is already registered """ if plugin_name in self._creators: - logging_error("Plugin '%s' is already registered, overwriting", plugin_name) + msg = f"Plugin '{plugin_name}' is already registered" + raise ValueError(msg) self._creators[plugin_name] = creator_func def create( @@ -88,6 +92,16 @@ def is_registered(self, plugin_name: str) -> bool: """ return plugin_name in self._creators + def get_registered_plugins(self) -> list[str]: + """ + Get list of all registered plugin names. + + Returns: + list[str]: List of registered plugin names + + """ + return list(self._creators.keys()) + # Global factory instance -plugin_factory = PluginFactory() +plugin_factory_ui = PluginFactoryUI() diff --git a/ardupilot_methodic_configurator/plugin_factory_workflow.py b/ardupilot_methodic_configurator/plugin_factory_workflow.py new file mode 100755 index 00000000..833f3534 --- /dev/null +++ b/ardupilot_methodic_configurator/plugin_factory_workflow.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 + +""" +Workflow plugin factory for managing workflow-based plugins. + +Workflow plugins are triggered actions that execute when specific parameter files +are selected, rather than persistent UI components. They handle one-time operations +like calibration workflows, file uploads, or batch operations. + +This factory implements a pattern symmetric to the UI plugin factory, properly +separating business logic (data models) from UI logic (workflow coordinators). + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from __future__ import annotations + +from typing import Callable + +# Type alias for workflow creator functions +# Workflow creators accept (root_window, data_model) and return a workflow coordinator object +WorkflowCreator = Callable[[object, object], object] + + +class PluginFactoryWorkflow: + """ + Factory for creating workflow plugin coordinators. + + Workflow plugins differ from UI plugins in their lifecycle: + - UI plugins: Create persistent views that remain visible + - Workflow plugins: Create one-time coordinators that execute and complete + + However, both follow the same architectural pattern: + 1. Business logic is in a data model (e.g., TempCalIMUDataModel) + 2. UI coordination is in a coordinator/view (e.g., TempCalIMUWorkflow) + 3. Creation uses dependency injection via factory registration + + The factory maps plugin names to creator functions that instantiate + workflow coordinators when triggered by parameter file selection. + """ + + def __init__(self) -> None: + """Initialize the workflow plugin factory with an empty registry.""" + self._creators: dict[str, WorkflowCreator] = {} + + def register(self, plugin_name: str, creator_func: WorkflowCreator) -> None: + """ + Register a workflow plugin creator function. + + Args: + plugin_name: Unique identifier for the plugin (e.g., "tempcal_imu") + creator_func: Function that creates a workflow coordinator instance. + Should accept (root_window, data_model) and return a workflow + coordinator object with a run_workflow() method. + + Raises: + ValueError: If a plugin with the same name is already registered + + """ + if plugin_name in self._creators: + msg = f"Workflow plugin '{plugin_name}' is already registered" + raise ValueError(msg) + self._creators[plugin_name] = creator_func + + def create( + self, + plugin_name: str, + root_window: object, + data_model: object, + ) -> object | None: + """ + Create a workflow coordinator instance. + + Args: + plugin_name: The name of the workflow plugin to create + root_window: The root window for creating dialogs + data_model: The business logic data model for the workflow + + Returns: + The created workflow coordinator instance, or None if plugin not found + + """ + creator = self._creators.get(plugin_name) + if creator: + return creator(root_window, data_model) + return None + + def is_registered(self, plugin_name: str) -> bool: + """ + Check if a workflow plugin is registered. + + Args: + plugin_name: The plugin name to check + + Returns: + bool: True if the plugin is registered, False otherwise + + """ + return plugin_name in self._creators + + def get_registered_plugins(self) -> list[str]: + """ + Get list of all registered workflow plugin names. + + Returns: + list[str]: List of registered plugin names + + """ + return list(self._creators.keys()) + + +# Global workflow plugin factory instance +plugin_factory_workflow = PluginFactoryWorkflow() diff --git a/tests/integration_plugin_system.py b/tests/integration_plugin_system.py new file mode 100755 index 00000000..e29a7c77 --- /dev/null +++ b/tests/integration_plugin_system.py @@ -0,0 +1,373 @@ +#!/usr/bin/env python3 + +""" +Integration tests for plugin system end-to-end workflows. + +This file tests the complete plugin execution flow from registration +through configuration loading to actual plugin execution. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from collections.abc import Generator +from unittest.mock import MagicMock, patch + +import pytest + +from ardupilot_methodic_configurator import __main__ +from ardupilot_methodic_configurator.business_logic_tempcal_imu import TempCalIMUDataModel +from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager +from ardupilot_methodic_configurator.frontend_tkinter_tempcal_imu import TempCalIMUWorkflow +from ardupilot_methodic_configurator.plugin_constants import PLUGIN_MOTOR_TEST, PLUGIN_TEMPCAL_IMU +from ardupilot_methodic_configurator.plugin_factory_ui import plugin_factory_ui +from ardupilot_methodic_configurator.plugin_factory_workflow import plugin_factory_workflow + + +class TestPluginSystemIntegration: + """Integration tests for the complete plugin system.""" + + @pytest.fixture(autouse=True) + def cleanup_plugin_registries(self) -> Generator[None, None, None]: + """ + Clean up plugin registries before and after each test. + + Ensures test isolation by clearing any registered plugins. + """ + # Store original registrations + original_ui_plugins = dict(plugin_factory_ui._creators) # pylint: disable=protected-access + original_workflow_plugins = dict(plugin_factory_workflow._creators) # pylint: disable=protected-access + + # Clear registries + plugin_factory_ui._creators.clear() # pylint: disable=protected-access + plugin_factory_workflow._creators.clear() # pylint: disable=protected-access + + yield + + # Restore original registrations + plugin_factory_ui._creators.update(original_ui_plugins) # pylint: disable=protected-access + plugin_factory_workflow._creators.update(original_workflow_plugins) # pylint: disable=protected-access + + def test_plugin_registration_during_app_startup_registers_all_plugins(self) -> None: + """ + Test that register_plugins() registers all expected plugins. + + GIVEN: Clean plugin registries + WHEN: Calling register_plugins() during app startup + THEN: All expected plugins should be registered in their respective factories + """ + __main__.register_plugins() + + # Check UI plugins + assert plugin_factory_ui.is_registered(PLUGIN_MOTOR_TEST) + + # Check workflow plugins + assert plugin_factory_workflow.is_registered(PLUGIN_TEMPCAL_IMU) + + def test_plugin_registration_populates_factory_with_correct_creators(self) -> None: + """ + Test that registered plugins can be created successfully. + + GIVEN: Plugins are registered during startup + WHEN: Attempting to create plugin instances + THEN: Factories should return valid plugin instances + """ + __main__.register_plugins() + + # Test UI plugin creation - skip because it requires complex widget mocking + # The factory registration itself is tested in other tests + assert plugin_factory_ui.is_registered(PLUGIN_MOTOR_TEST) + + # Test workflow plugin creation + mock_root = MagicMock() + mock_data_model = MagicMock() + + workflow_plugin = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, mock_root, mock_data_model) + assert isinstance(workflow_plugin, TempCalIMUWorkflow) + + @patch("ardupilot_methodic_configurator.frontend_tkinter_tempcal_imu.TempCalIMUWorkflow.run_workflow") + def test_workflow_plugin_execution_flow_from_factory_to_run_workflow(self, mock_run_workflow) -> None: + """ + Test complete workflow plugin execution from factory creation to workflow run. + + GIVEN: A registered workflow plugin + WHEN: Creating and executing the plugin through the factory + THEN: The workflow should execute successfully + """ + __main__.register_plugins() + + mock_root = MagicMock() + mock_data_model = MagicMock() + mock_run_workflow.return_value = True + + # Create plugin through factory + workflow = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, mock_root, mock_data_model) + assert isinstance(workflow, TempCalIMUWorkflow) + + # Execute workflow + result = workflow.run_workflow() + + assert result is True + mock_run_workflow.assert_called_once() + + def test_configuration_manager_can_create_tempcal_imu_data_model(self) -> None: + """ + Test ConfigurationManager can create TempCalIMUDataModel for workflow plugins. + + GIVEN: A configuration manager instance + WHEN: Requesting data model creation for tempcal_imu plugin + THEN: Should return a valid TempCalIMUDataModel instance + """ + # Create minimal configuration manager (mock dependencies) + mock_fs = MagicMock() + mock_fc = MagicMock() + mock_current_file = "test.param" + + config_manager = ConfigurationManager(mock_current_file, mock_fc, mock_fs) + + # Test data model creation + data_model = config_manager.create_plugin_data_model( + PLUGIN_TEMPCAL_IMU, + step_filename="03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + progress_callback=MagicMock(), + cleanup_callback=MagicMock(), + ) + + assert isinstance(data_model, TempCalIMUDataModel) + + def test_configuration_manager_returns_none_for_invalid_plugin_data_model_request(self) -> None: + """ + Test ConfigurationManager returns None for invalid plugin requests. + + GIVEN: A configuration manager instance + WHEN: Requesting data model for unknown plugin or with missing parameters + THEN: Should return None gracefully + """ + mock_fs = MagicMock() + mock_fc = MagicMock() + mock_current_file = "test.param" + + config_manager = ConfigurationManager(mock_current_file, mock_fc, mock_fs) + + # Test unknown plugin + result = config_manager.create_plugin_data_model("unknown_plugin") + assert result is None + + # Test tempcal_imu without required parameters + result = config_manager.create_plugin_data_model(PLUGIN_TEMPCAL_IMU) + assert result is None + + @patch("ardupilot_methodic_configurator.configuration_manager.LocalFilesystem") + @patch("ardupilot_methodic_configurator.configuration_manager.FlightController") + def test_end_to_end_plugin_workflow_execution_via_configuration_manager(self, mock_fc, mock_fs) -> None: + """ + Test end-to-end workflow execution through ConfigurationManager interface. + + GIVEN: A complete plugin setup with registered plugins + WHEN: Executing workflow through configuration manager + THEN: The workflow should complete successfully + """ + # Setup mocks + mock_fs_instance = MagicMock() + mock_fs_instance.vehicle_dir = "/test/vehicle" + mock_fs_instance.get_configuration_file_fullpath.return_value = "/test/vehicle/03_imu_temp_cal.param" + mock_fs.return_value = mock_fs_instance + + mock_fc_instance = MagicMock() + mock_fc.return_value = mock_fc_instance + + # Create configuration manager + config_manager = ConfigurationManager("test.param", mock_fc_instance, mock_fs_instance) + + # Register plugins + __main__.register_plugins() + + # Create data model + data_model = config_manager.create_plugin_data_model( + PLUGIN_TEMPCAL_IMU, + step_filename="03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + progress_callback=MagicMock(), + cleanup_callback=MagicMock(), + ) + + # Create workflow coordinator + workflow = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, MagicMock(), data_model) + + # Execute workflow + with patch.object(data_model, "run_calibration", return_value=True) as mock_calibration: + result = workflow.run_workflow() + + assert result is True + mock_calibration.assert_called_once() + + def test_plugin_system_handles_missing_registrations_gracefully(self) -> None: + """ + Test plugin system handles missing plugin registrations gracefully. + + GIVEN: A request for an unregistered plugin + WHEN: Attempting to create the plugin + THEN: Should return None without raising exceptions + """ + # Ensure plugin is not registered + assert not plugin_factory_ui.is_registered("nonexistent_ui_plugin") + assert not plugin_factory_workflow.is_registered("nonexistent_workflow_plugin") + + # Test UI plugin + result = plugin_factory_ui.create("nonexistent_ui_plugin", MagicMock(), MagicMock(), MagicMock()) + assert result is None + + # Test workflow plugin + result = plugin_factory_workflow.create("nonexistent_workflow_plugin", MagicMock(), MagicMock()) + assert result is None + + def test_plugin_factories_maintain_independence_between_ui_and_workflow_plugins(self) -> None: + """ + Test UI and workflow plugin factories maintain separate registries. + + GIVEN: Plugins registered in both factories + WHEN: Querying each factory + THEN: Each factory should only know about its own plugins + """ + # Register a UI plugin + plugin_factory_ui.register("test_ui_plugin", MagicMock()) + + # Register a workflow plugin + plugin_factory_workflow.register("test_workflow_plugin", MagicMock()) + + # Verify separation + assert plugin_factory_ui.is_registered("test_ui_plugin") + assert not plugin_factory_ui.is_registered("test_workflow_plugin") + + assert plugin_factory_workflow.is_registered("test_workflow_plugin") + assert not plugin_factory_workflow.is_registered("test_ui_plugin") + + # Verify plugin lists + ui_plugins = plugin_factory_ui.get_registered_plugins() + workflow_plugins = plugin_factory_workflow.get_registered_plugins() + + assert "test_ui_plugin" in ui_plugins + assert "test_workflow_plugin" not in ui_plugins + + assert "test_workflow_plugin" in workflow_plugins + assert "test_ui_plugin" not in workflow_plugins + + def test_configuration_manager_returns_none_when_partial_callbacks_provided(self) -> None: + """ + Test ConfigurationManager handles missing callbacks gracefully. + + GIVEN: A configuration manager instance + WHEN: Requesting data model with only some callbacks provided (missing some) + THEN: Should return None due to incomplete callback set + """ + mock_fs = MagicMock() + mock_fc = MagicMock() + mock_current_file = "test.param" + + config_manager = ConfigurationManager(mock_current_file, mock_fc, mock_fs) + + # Test with missing cleanup_callback + result = config_manager.create_plugin_data_model( + PLUGIN_TEMPCAL_IMU, + step_filename="03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + progress_callback=MagicMock(), + # cleanup_callback missing + ) + assert result is None + + # Test with missing progress_callback + result = config_manager.create_plugin_data_model( + PLUGIN_TEMPCAL_IMU, + step_filename="03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + cleanup_callback=MagicMock(), + # progress_callback missing + ) + assert result is None + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_workflow_execution_handles_exceptions_gracefully(self, mock_imufit) -> None: + """ + Test workflow execution handles exceptions from business logic. + + GIVEN: A registered workflow plugin with data model + WHEN: The business logic raises an unexpected exception + THEN: Workflow should handle it gracefully and cleanup should still occur + """ + __main__.register_plugins() + + mock_root = MagicMock() + cleanup_callback = MagicMock() + mock_configuration_manager = MagicMock() + mock_configuration_manager.vehicle_dir = "/test/vehicle" + mock_configuration_manager.get_configuration_file_fullpath.return_value = "/test/vehicle/03_imu_temp_cal.param" + + # Create data model that will fail during calibration + mock_imufit.side_effect = RuntimeError("Unexpected calibration error") + + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + progress_callback=MagicMock(), + cleanup_callback=cleanup_callback, + ) + + # Create workflow coordinator + workflow = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, mock_root, data_model) + assert workflow is not None + + # Execute workflow - should raise the RuntimeError but cleanup should be called + with pytest.raises(RuntimeError, match="Unexpected calibration error"): + workflow.run_workflow() # pyright: ignore[reportAttributeAccessIssue] + + # Verify cleanup was called even though calibration failed + cleanup_callback.assert_called_once() + + def test_workflow_plugin_with_none_progress_callback(self) -> None: + """ + Test workflow plugin works with None progress_callback. + + GIVEN: A workflow plugin data model + WHEN: progress_callback is None (optional parameter) + THEN: Data model should be created successfully and work without progress updates + """ + mock_fs = MagicMock() + mock_fc = MagicMock() + mock_current_file = "test.param" + + config_manager = ConfigurationManager(mock_current_file, mock_fc, mock_fs) + + # Test with None progress_callback + result = config_manager.create_plugin_data_model( + PLUGIN_TEMPCAL_IMU, + step_filename="03_imu_temp_cal.param", + ask_confirmation=MagicMock(return_value=True), + select_file=MagicMock(return_value="/test/log.bin"), + show_warning=MagicMock(), + show_error=MagicMock(), + progress_callback=None, + cleanup_callback=MagicMock(), + ) + # Should return None because progress_callback is None + assert result is None diff --git a/tests/test_backend_filesystem.py b/tests/test_backend_filesystem.py index 6fcd2d31..59f44042 100755 --- a/tests/test_backend_filesystem.py +++ b/tests/test_backend_filesystem.py @@ -405,19 +405,6 @@ def test_vehicle_image_filepath(self) -> None: assert result == "vehicle_dir/vehicle.jpg" mock_join.assert_called_once_with("vehicle_dir", "vehicle.jpg") - def test_tempcal_imu_result_param_tuple(self) -> None: - lfs = LocalFilesystem( - "vehicle_dir", "vehicle_type", None, allow_editing_template_files=False, save_component_to_system_templates=False - ) - with patch("os.path.join") as mock_join: - mock_join.return_value = "vehicle_dir/03_imu_temperature_calibration_results.param" - result = lfs.tempcal_imu_result_param_tuple() - assert result == ( - "03_imu_temperature_calibration_results.param", - "vehicle_dir/03_imu_temperature_calibration_results.param", - ) - mock_join.assert_called_once_with("vehicle_dir", "03_imu_temperature_calibration_results.param") - def test_zip_file_path(self) -> None: lfs = LocalFilesystem( "vehicle_dir", "vehicle_type", None, allow_editing_template_files=False, save_component_to_system_templates=False diff --git a/tests/test_business_logic_tempcal_imu.py b/tests/test_business_logic_tempcal_imu.py new file mode 100755 index 00000000..c149e751 --- /dev/null +++ b/tests/test_business_logic_tempcal_imu.py @@ -0,0 +1,318 @@ +#!/usr/bin/env python3 + +""" +Tests for IMU temperature calibration business logic. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from ardupilot_methodic_configurator.business_logic_tempcal_imu import TempCalIMUDataModel + + +class TestTempCalIMUDataModel: + """Test suite for IMU temperature calibration business logic.""" + + @pytest.fixture + def mock_configuration_manager(self) -> MagicMock: + """Create a mock configuration manager.""" + config_mgr = MagicMock() + config_mgr.vehicle_dir = "/test/vehicle/dir" + config_mgr.get_configuration_file_fullpath.return_value = "/test/vehicle/dir/03_imu_temp_cal.param" + return config_mgr + + @pytest.fixture + def mock_callbacks(self) -> dict: + """Create mock callbacks for user interaction.""" + return { + "ask_confirmation": MagicMock(return_value=True), + "select_file": MagicMock(return_value="/test/log.bin"), + "show_warning": MagicMock(), + "show_error": MagicMock(), + } + + @pytest.fixture + def data_model(self, mock_configuration_manager, mock_callbacks) -> TempCalIMUDataModel: + """Create a TempCalIMUDataModel instance with mocked dependencies.""" + return TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + ) + + def test_calibration_result_file_path_is_in_vehicle_directory(self, data_model, mock_configuration_manager) -> None: + """ + Calibration results are saved to the vehicle configuration directory. + + GIVEN: A calibration workflow for a specific configuration step + WHEN: Determining where to save calibration results + THEN: Results should be saved in the vehicle's configuration directory + """ + result = data_model.get_result_param_fullpath() + + assert result == "/test/vehicle/dir/03_imu_temp_cal.param" + mock_configuration_manager.get_configuration_file_fullpath.assert_called_once_with("03_imu_temp_cal.param") + + def test_get_confirmation_message(self, data_model) -> None: + """ + User receives clear warning about calibration file being overwritten. + + GIVEN: A calibration workflow is about to start + WHEN: Requesting user confirmation + THEN: Message should warn that existing calibration file will be overwritten + """ + message = data_model.get_confirmation_message() + + assert "03_imu_temp_cal.param" in message + assert "overwritten" in message + assert ".bin" in message + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_user_successfully_completes_calibration_workflow( + self, mock_imufit, mock_configuration_manager, mock_callbacks + ) -> None: + """ + User successfully completes IMU temperature calibration. + + GIVEN: User has a valid flight log file + WHEN: User confirms all dialogs and calibration completes successfully + THEN: Calibration should run, parameters should reload, and workflow returns success + """ + progress_callback = MagicMock() + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + progress_callback=progress_callback, + ) + + result = data_model.run_calibration() + + assert result is True + mock_callbacks["ask_confirmation"].assert_called_once() + mock_callbacks["select_file"].assert_called_once() + mock_callbacks["show_warning"].assert_called_once() + mock_imufit.assert_called_once_with( + logfile="/test/log.bin", + outfile="/test/vehicle/dir/03_imu_temp_cal.param", + no_graph=False, + log_parm=False, + online=False, + tclr=False, + figpath="/test/vehicle/dir", + progress_callback=progress_callback, + ) + mock_configuration_manager.reload_parameter_files.assert_called_once() + + def test_run_calibration_user_declines_confirmation(self, data_model, mock_callbacks) -> None: + """ + User can cancel calibration at confirmation step. + + GIVEN: User is presented with calibration confirmation dialog + WHEN: User clicks "No" or cancels the confirmation + THEN: Workflow should exit gracefully without starting calibration + """ + mock_callbacks["ask_confirmation"].return_value = False + + result = data_model.run_calibration() + + assert result is False + mock_callbacks["ask_confirmation"].assert_called_once() + mock_callbacks["select_file"].assert_not_called() + mock_callbacks["show_warning"].assert_not_called() + + def test_run_calibration_user_cancels_file_selection(self, data_model, mock_callbacks) -> None: + """ + User can cancel calibration during file selection. + + GIVEN: User confirmed calibration but is selecting a log file + WHEN: User cancels the file selection dialog + THEN: Workflow should exit gracefully without running calibration + """ + mock_callbacks["select_file"].return_value = None + + result = data_model.run_calibration() + + assert result is False + mock_callbacks["ask_confirmation"].assert_called_once() + mock_callbacks["select_file"].assert_called_once() + mock_callbacks["show_warning"].assert_not_called() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_run_calibration_handles_system_exit(self, mock_imufit, data_model, mock_callbacks) -> None: + """ + User is informed when calibration encounters fatal errors. + + GIVEN: User starts calibration workflow + WHEN: Calibration encounters a fatal error that causes SystemExit + THEN: User should see error message and application should exit gracefully + """ + mock_imufit.side_effect = SystemExit("Fatal calibration error") + + with pytest.raises(SystemExit): + data_model.run_calibration() + + mock_callbacks["show_error"].assert_called_once() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_calibration_works_without_visual_progress_updates( + self, mock_imufit, mock_configuration_manager, mock_callbacks + ) -> None: + """ + Calibration completes successfully even without progress bar. + + GIVEN: User runs calibration + WHEN: No progress callback is provided (headless mode or simple UI) + THEN: Calibration should complete successfully without visual feedback + """ + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + progress_callback=None, + ) + + result = data_model.run_calibration() + + assert result is True + mock_imufit.assert_called_once() + assert mock_imufit.call_args[1]["progress_callback"] is None + mock_configuration_manager.reload_parameter_files.assert_called_once() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_cleanup_callback_is_called_after_successful_calibration( + self, + mock_imufit, # pylint: disable=unused-argument + mock_configuration_manager, + mock_callbacks, + ) -> None: + """ + Cleanup actions execute after successful calibration. + + GIVEN: User runs calibration with cleanup callback (e.g., close progress window) + WHEN: Calibration completes successfully + THEN: Cleanup callback should be invoked to release resources + """ + cleanup_callback = MagicMock() + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + cleanup_callback=cleanup_callback, + ) + + data_model.run_calibration() + + cleanup_callback.assert_called_once() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_cleanup_callback_is_called_even_when_calibration_fails( + self, mock_imufit, mock_configuration_manager, mock_callbacks + ) -> None: + """ + Cleanup actions execute even when calibration fails. + + GIVEN: User runs calibration with cleanup callback + WHEN: Calibration encounters an error + THEN: Cleanup callback should still be invoked (finally block behavior) + """ + cleanup_callback = MagicMock() + mock_imufit.side_effect = SystemExit("Calibration failed") + + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + cleanup_callback=cleanup_callback, + ) + + with pytest.raises(SystemExit): + data_model.run_calibration() + + cleanup_callback.assert_called_once() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_cleanup_callback_is_called_when_user_cancels_at_confirmation( + self, + mock_imufit, # pylint: disable=unused-argument + mock_configuration_manager, + mock_callbacks, + ) -> None: + """ + Cleanup callback is called even when user cancels at confirmation step. + + GIVEN: User runs calibration with cleanup callback + WHEN: User cancels at the confirmation dialog + THEN: Cleanup callback should still be invoked + """ + cleanup_callback = MagicMock() + mock_callbacks["ask_confirmation"].return_value = False + + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + cleanup_callback=cleanup_callback, + ) + + result = data_model.run_calibration() + + assert result is False + cleanup_callback.assert_called_once() + + @patch("ardupilot_methodic_configurator.business_logic_tempcal_imu.IMUfit") + def test_cleanup_callback_is_called_when_user_cancels_file_selection( + self, + mock_imufit, # pylint: disable=unused-argument + mock_configuration_manager, + mock_callbacks, + ) -> None: + """ + Cleanup callback is called when user cancels file selection. + + GIVEN: User runs calibration with cleanup callback + WHEN: User cancels the file selection dialog + THEN: Cleanup callback should still be invoked + """ + cleanup_callback = MagicMock() + mock_callbacks["select_file"].return_value = None + + data_model = TempCalIMUDataModel( + mock_configuration_manager, + "03_imu_temp_cal.param", + mock_callbacks["ask_confirmation"], + mock_callbacks["select_file"], + mock_callbacks["show_warning"], + mock_callbacks["show_error"], + cleanup_callback=cleanup_callback, + ) + + result = data_model.run_calibration() + + assert result is False + cleanup_callback.assert_called_once() diff --git a/tests/test_configuration_manager.py b/tests/test_configuration_manager.py index 97398d8e..e746a047 100755 --- a/tests/test_configuration_manager.py +++ b/tests/test_configuration_manager.py @@ -1527,210 +1527,6 @@ def test_user_handles_new_parameter_upload(self, configuration_manager) -> None: # Tests for newly refactored business logic methods -class TestIMUTemperatureCalibrationMethods: - """Test suite for IMU temperature calibration business logic methods.""" - - @patch("ardupilot_methodic_configurator.configuration_manager.IMUfit") - def test_handle_imu_temperature_calibration_workflow_success(self, mock_imufit, configuration_manager) -> None: - """ - User successfully completes IMU calibration workflow with callbacks. - - GIVEN: A valid configuration step that requires IMU calibration - WHEN: User completes the workflow with all confirmations - THEN: Calibration should be performed successfully - """ - # Arrange: Set up filesystem mocks - configuration_manager._local_filesystem.tempcal_imu_result_param_tuple.return_value = ( - "25_imu_temperature_calibration.param", - "/path/to/25_imu_temperature_calibration.param", - ) - configuration_manager._local_filesystem.vehicle_dir = "/vehicle/dir" - configuration_manager._local_filesystem.read_params_from_files.return_value = {"new": "params"} - - # Set up mock callbacks - ask_confirmation_mock = MagicMock(return_value=True) - select_file_mock = MagicMock(return_value="/path/to/logfile.bin") - show_warning_mock = MagicMock() - show_error_mock = MagicMock() - progress_callback_mock = MagicMock() - - # Act: Run the workflow - result = configuration_manager.handle_imu_temperature_calibration_workflow( - "25_imu_temperature_calibration.param", - ask_user_confirmation=ask_confirmation_mock, - select_file=select_file_mock, - show_warning=show_warning_mock, - show_error=show_error_mock, - progress_callback=progress_callback_mock, - ) - - # Assert: Workflow should succeed - assert result is True - ask_confirmation_mock.assert_called_once() - select_file_mock.assert_called_once_with("Select ArduPilot binary log file", ["*.bin", "*.BIN"]) - show_warning_mock.assert_called_once() - show_error_mock.assert_not_called() - mock_imufit.assert_called_once_with( - logfile="/path/to/logfile.bin", - outfile="/path/to/25_imu_temperature_calibration.param", - no_graph=False, - log_parm=False, - online=False, - tclr=False, - figpath="/vehicle/dir", - progress_callback=progress_callback_mock, - ) - configuration_manager._local_filesystem.read_params_from_files.assert_called_once() - - def test_handle_imu_temperature_calibration_workflow_user_declines_confirmation(self, configuration_manager) -> None: - """ - User declines IMU calibration confirmation. - - GIVEN: A valid configuration step that requires IMU calibration - WHEN: User declines the confirmation dialog - THEN: Workflow should exit early without performing calibration - """ - # Arrange: Set up filesystem mock - configuration_manager._local_filesystem.tempcal_imu_result_param_tuple.return_value = ( - "25_imu_temperature_calibration.param", - "/path/to/25_imu_temperature_calibration.param", - ) - - # Set up mock callbacks with user declining - ask_confirmation_mock = MagicMock(return_value=False) - select_file_mock = MagicMock() - show_warning_mock = MagicMock() - show_error_mock = MagicMock() - - # Act: Run the workflow - result = configuration_manager.handle_imu_temperature_calibration_workflow( - "25_imu_temperature_calibration.param", - ask_user_confirmation=ask_confirmation_mock, - select_file=select_file_mock, - show_warning=show_warning_mock, - show_error=show_error_mock, - ) - - # Assert: Workflow should exit early - assert result is False - ask_confirmation_mock.assert_called_once() - select_file_mock.assert_not_called() - show_warning_mock.assert_not_called() - show_error_mock.assert_not_called() - - def test_handle_imu_temperature_calibration_workflow_user_cancels_file_selection(self, configuration_manager) -> None: - """ - User cancels file selection dialog. - - GIVEN: A valid configuration step that requires IMU calibration - WHEN: User confirms but cancels file selection - THEN: Workflow should exit without performing calibration - """ - # Arrange: Set up filesystem mock - configuration_manager._local_filesystem.tempcal_imu_result_param_tuple.return_value = ( - "25_imu_temperature_calibration.param", - "/path/to/25_imu_temperature_calibration.param", - ) - - # Set up mock callbacks with file selection cancelled - ask_confirmation_mock = MagicMock(return_value=True) - select_file_mock = MagicMock(return_value=None) # User cancelled - show_warning_mock = MagicMock() - show_error_mock = MagicMock() - - # Act: Run the workflow - result = configuration_manager.handle_imu_temperature_calibration_workflow( - "25_imu_temperature_calibration.param", - ask_user_confirmation=ask_confirmation_mock, - select_file=select_file_mock, - show_warning=show_warning_mock, - show_error=show_error_mock, - ) - - # Assert: Workflow should exit without calibration - assert result is False - ask_confirmation_mock.assert_called_once() - select_file_mock.assert_called_once() - show_warning_mock.assert_not_called() - show_error_mock.assert_not_called() - - @patch("ardupilot_methodic_configurator.configuration_manager.IMUfit") - def test_handle_imu_temperature_calibration_workflow_calibration_fails(self, mock_imufit, configuration_manager) -> None: - """ - User completes workflow but calibration fails. - - GIVEN: A valid configuration step that requires IMU calibration - WHEN: User completes workflow but calibration fails - THEN: Error callback should be called and workflow should return False - """ - # Arrange: Set up filesystem mocks for failure scenario - configuration_manager._local_filesystem.tempcal_imu_result_param_tuple.return_value = ( - "25_imu_temperature_calibration.param", - "/path/to/25_imu_temperature_calibration.param", - ) - configuration_manager._local_filesystem.vehicle_dir = "/vehicle/dir" - mock_imufit.return_value = None # Simulate calibration failure (no result) - - # Set up mock callbacks - ask_confirmation_mock = MagicMock(return_value=True) - select_file_mock = MagicMock(return_value="/path/to/logfile.bin") - show_warning_mock = MagicMock() - show_error_mock = MagicMock() - - # Act: Run the workflow - result = configuration_manager.handle_imu_temperature_calibration_workflow( - "25_imu_temperature_calibration.param", - ask_user_confirmation=ask_confirmation_mock, - select_file=select_file_mock, - show_warning=show_warning_mock, - show_error=show_error_mock, - ) - - # NOTE: The code does not handle IMUfit returning None as a failure, so result will be True. - assert result is True - ask_confirmation_mock.assert_called_once() - select_file_mock.assert_called_once() - # show_error_mock.assert_called_once() # Not called in current code - show_warning_mock.assert_called_once() - mock_imufit.assert_called_once() - - def test_handle_imu_temperature_calibration_workflow_with_non_matching_file(self, configuration_manager) -> None: - """ - User attempts workflow with non-matching file. - - GIVEN: A configuration step that doesn't require IMU calibration - WHEN: User attempts IMU calibration workflow - THEN: Workflow should exit early without any user interaction - """ - # Arrange: Set up filesystem mock for non-matching file - configuration_manager._local_filesystem.tempcal_imu_result_param_tuple.return_value = ( - "25_imu_temperature_calibration.param", - "/path/to/25_imu_temperature_calibration.param", - ) - - # Set up mock callbacks (should not be called) - ask_confirmation_mock = MagicMock() - select_file_mock = MagicMock() - show_warning_mock = MagicMock() - show_error_mock = MagicMock() - - # Act: Run the workflow with non-matching file - result = configuration_manager.handle_imu_temperature_calibration_workflow( - "other_file.param", - ask_user_confirmation=ask_confirmation_mock, - select_file=select_file_mock, - show_warning=show_warning_mock, - show_error=show_error_mock, - ) - - # Assert: Workflow should exit early without user interaction - assert result is False - ask_confirmation_mock.assert_not_called() - select_file_mock.assert_not_called() - show_warning_mock.assert_not_called() - show_error_mock.assert_not_called() - - class TestParameterSummaryMethods: """Test suite for parameter summary generation business logic methods.""" diff --git a/tests/test_frontend_tkinter_parameter_editor.py b/tests/test_frontend_tkinter_parameter_editor.py index d70d8528..4772e996 100755 --- a/tests/test_frontend_tkinter_parameter_editor.py +++ b/tests/test_frontend_tkinter_parameter_editor.py @@ -249,7 +249,7 @@ def fake_wait_window(*args: Any, **kwargs: Any) -> None: # noqa: ANN401 # pylin frame_mock = mock_frame.return_value frame_mock.pack.assert_called_once_with(pady=10) - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") def test_load_plugin_creates_motor_test_view_when_model_available(self, mock_factory, parameter_editor) -> None: """ Test that __load_plugin creates plugin view using factory when data model is available. @@ -279,7 +279,7 @@ def test_load_plugin_creates_motor_test_view_when_model_available(self, mock_fac mock_factory.create.assert_called_once_with("motor_test", mock_parent_frame, mock_model, parameter_editor) mock_plugin_view.pack.assert_called_once() - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") def test_load_plugin_shows_error_message_when_model_creation_fails(self, mock_factory, parameter_editor) -> None: """ Test that __load_plugin shows error message when data model creation fails. @@ -307,7 +307,7 @@ def test_load_plugin_shows_error_message_when_model_creation_fails(self, mock_fa mock_parent_frame.children = [] # Simulate ttk.Frame behavior # The actual label creation happens inside the method - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.ttk.Label") def test_load_plugin_shows_error_when_model_creation_returns_none( self, mock_label, mock_factory, parameter_editor @@ -335,7 +335,7 @@ def test_load_plugin_shows_error_when_model_creation_returns_none( mock_label.assert_called_with(mock_parent_frame, text="Plugin requires flight controller connection") mock_label.return_value.pack.assert_called_once() - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") def test_load_plugin_shows_error_for_unknown_plugin(self, mock_factory, parameter_editor) -> None: """ Test that __load_plugin shows error message for unknown plugin names. @@ -398,7 +398,7 @@ def test_update_plugin_layout_switches_from_plugin_to_no_plugin(self, parameter_ # Verify current_plugin was updated to None assert parameter_editor.current_plugin is None - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") def test_load_plugin_handles_factory_exception(self, mock_factory, parameter_editor) -> None: """ Test that __load_plugin handles exceptions from the plugin factory gracefully. @@ -428,7 +428,7 @@ def test_load_plugin_handles_factory_exception(self, mock_factory, parameter_edi assert "foreground" in call_args[1] assert call_args[1]["foreground"] == "red" - @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory") + @patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.plugin_factory_ui") def test_switching_between_two_plugin_files_calls_lifecycle_hooks(self, mock_factory, parameter_editor) -> None: """ Test that switching between two different plugin files calls lifecycle hooks properly. diff --git a/tests/test_frontend_tkinter_tempcal_imu.py b/tests/test_frontend_tkinter_tempcal_imu.py new file mode 100755 index 00000000..f1dcdd5d --- /dev/null +++ b/tests/test_frontend_tkinter_tempcal_imu.py @@ -0,0 +1,217 @@ +#!/usr/bin/env python3 + +""" +Tests for IMU temperature calibration frontend workflow. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from collections.abc import Generator +from unittest.mock import MagicMock + +import pytest + +from ardupilot_methodic_configurator.frontend_tkinter_tempcal_imu import ( + TempCalIMUWorkflow, + create_tempcal_imu_workflow, + register_tempcal_imu_plugin, +) +from ardupilot_methodic_configurator.plugin_constants import PLUGIN_TEMPCAL_IMU +from ardupilot_methodic_configurator.plugin_factory_workflow import plugin_factory_workflow + + +class TestTempCalIMUWorkflow: + """Test suite for IMU temperature calibration frontend workflow.""" + + @pytest.fixture + def mock_root_window(self) -> MagicMock: + """Create a mock root window for testing.""" + return MagicMock() + + @pytest.fixture + def mock_data_model(self) -> MagicMock: + """Create a mock data model for testing.""" + data_model = MagicMock() + data_model.run_calibration.return_value = True + return data_model + + @pytest.fixture + def workflow(self, mock_root_window, mock_data_model) -> TempCalIMUWorkflow: + """Create a workflow instance for testing.""" + return TempCalIMUWorkflow(mock_root_window, mock_data_model) + + def test_workflow_initialization_stores_dependencies(self, mock_root_window, mock_data_model) -> None: + """ + Test workflow initialization stores provided dependencies. + + GIVEN: A root window and data model + WHEN: Creating a TempCalIMUWorkflow instance + THEN: Both dependencies should be stored for later use + """ + workflow = TempCalIMUWorkflow(mock_root_window, mock_data_model) + + assert workflow.root_window is mock_root_window + assert workflow.data_model is mock_data_model + + def test_user_successfully_runs_calibration_workflow(self, workflow, mock_data_model) -> None: + """ + Test user can successfully run the calibration workflow. + + GIVEN: A user has initiated IMU temperature calibration + WHEN: The workflow is executed + THEN: The business logic should be invoked and return success status + """ + result = workflow.run_workflow() + + assert result is True + mock_data_model.run_calibration.assert_called_once() + + def test_workflow_returns_failure_when_calibration_fails(self, workflow, mock_data_model) -> None: + """ + Test workflow returns failure status when calibration fails. + + GIVEN: A calibration workflow is prepared + WHEN: The underlying calibration process fails + THEN: The workflow should return False to indicate failure + """ + mock_data_model.run_calibration.return_value = False + + result = workflow.run_workflow() + + assert result is False + mock_data_model.run_calibration.assert_called_once() + + def test_workflow_delegates_all_logic_to_data_model(self, workflow, mock_data_model) -> None: + """ + Test workflow properly delegates to business logic layer. + + GIVEN: A frontend workflow instance + WHEN: User runs the calibration + THEN: All business logic should be handled by the data model, not the frontend + """ + workflow.run_workflow() + + # Verify delegation - the data model's run_calibration should be called + mock_data_model.run_calibration.assert_called_once_with() + + def test_workflow_propagates_exceptions_from_data_model(self, workflow, mock_data_model) -> None: + """ + Test workflow allows exceptions from data model to propagate. + + GIVEN: A workflow with a data model that raises an exception + WHEN: Running the calibration workflow + THEN: The exception should propagate to allow proper error handling upstream + """ + mock_data_model.run_calibration.side_effect = RuntimeError("Calibration hardware error") + + with pytest.raises(RuntimeError, match="Calibration hardware error"): + workflow.run_workflow() + + +class TestTempCalIMUWorkflowFactory: # pylint: disable=too-few-public-methods + """Test suite for workflow factory function.""" + + def test_factory_creates_workflow_with_correct_dependencies(self) -> None: + """ + Test factory function creates workflow with provided dependencies. + + GIVEN: A root window and data model + WHEN: Using the factory function to create a workflow + THEN: A properly configured TempCalIMUWorkflow should be returned + """ + root_window = MagicMock() + data_model = MagicMock() + + workflow = create_tempcal_imu_workflow(root_window, data_model) + + assert isinstance(workflow, TempCalIMUWorkflow) + assert workflow.root_window is root_window + assert workflow.data_model is data_model + + +class TestTempCalIMUPluginRegistration: + """Test suite for plugin registration.""" + + @pytest.fixture(autouse=True) + def cleanup_plugin_registry(self) -> Generator[None, None, None]: + """ + Clean up plugin registry before and after each test. + + This ensures test isolation by removing any registered plugins. + """ + # Remove plugin if it exists before test + if plugin_factory_workflow.is_registered(PLUGIN_TEMPCAL_IMU): + # Access private attribute for cleanup (acceptable in tests) + plugin_factory_workflow._creators.pop(PLUGIN_TEMPCAL_IMU, None) # pylint: disable=protected-access + + yield + + # Clean up after test + if plugin_factory_workflow.is_registered(PLUGIN_TEMPCAL_IMU): + plugin_factory_workflow._creators.pop(PLUGIN_TEMPCAL_IMU, None) # pylint: disable=protected-access + + def test_plugin_registration_adds_to_factory(self) -> None: + """ + Test plugin registration adds tempcal_imu to the workflow factory. + + GIVEN: A clean plugin factory + WHEN: Registering the tempcal_imu plugin + THEN: The plugin should be available in the factory registry + """ + register_tempcal_imu_plugin() + + assert plugin_factory_workflow.is_registered(PLUGIN_TEMPCAL_IMU) + + def test_plugin_can_be_created_after_registration(self) -> None: + """ + Test registered plugin can be created through the factory. + + GIVEN: The tempcal_imu plugin is registered + WHEN: Creating a plugin instance via the factory + THEN: A valid TempCalIMUWorkflow instance should be returned + """ + register_tempcal_imu_plugin() + root_window = MagicMock() + data_model = MagicMock() + + workflow = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, root_window, data_model) + + assert isinstance(workflow, TempCalIMUWorkflow) + assert workflow.root_window is root_window + assert workflow.data_model is data_model + + def test_unregistered_plugin_cannot_be_created(self) -> None: + """ + Test unregistered plugin returns None when creation is attempted. + + GIVEN: The tempcal_imu plugin is NOT registered + WHEN: Attempting to create the plugin via the factory + THEN: None should be returned indicating plugin is unavailable + """ + # Ensure plugin is not registered + assert not plugin_factory_workflow.is_registered(PLUGIN_TEMPCAL_IMU) + + result = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, MagicMock(), MagicMock()) + + assert result is None + + def test_factory_creates_new_instances_each_time(self) -> None: + """ + Test factory creates fresh workflow instances for each request. + + GIVEN: A registered tempcal_imu plugin + WHEN: Creating multiple workflow instances + THEN: Each instance should be independent (not singleton pattern) + """ + register_tempcal_imu_plugin() + + workflow1 = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, MagicMock(), MagicMock()) + workflow2 = plugin_factory_workflow.create(PLUGIN_TEMPCAL_IMU, MagicMock(), MagicMock()) + + assert workflow1 is not workflow2 + assert isinstance(workflow1, TempCalIMUWorkflow) + assert isinstance(workflow2, TempCalIMUWorkflow) diff --git a/tests/test_plugin_factory_ui.py b/tests/test_plugin_factory_ui.py new file mode 100755 index 00000000..79fc6bc8 --- /dev/null +++ b/tests/test_plugin_factory_ui.py @@ -0,0 +1,172 @@ +#!/usr/bin/env python3 + +""" +Tests for UI plugin factory. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from unittest.mock import MagicMock + +import pytest + +from ardupilot_methodic_configurator.plugin_factory_ui import PluginFactoryUI + + +class TestPluginFactoryUI: + """Test suite for UI plugin factory.""" + + @pytest.fixture + def factory(self) -> PluginFactoryUI: + """Create a fresh factory instance for each test.""" + return PluginFactoryUI() + + @pytest.fixture + def mock_creator(self) -> MagicMock: + """ + Create a mock creator function. + + Returns a mock plugin view instance when called. + """ + mock_plugin_view = MagicMock() + mock_plugin_view.show.return_value = None + return MagicMock(return_value=mock_plugin_view) + + def test_register_plugin(self, factory, mock_creator) -> None: + """ + Test registering a plugin. + + GIVEN: A factory and a creator function + WHEN: Registering a plugin + THEN: Plugin should be registered successfully + """ + factory.register("test_plugin", mock_creator) + + assert factory.is_registered("test_plugin") + assert "test_plugin" in factory.get_registered_plugins() + + def test_register_duplicate_plugin_raises_error(self, factory, mock_creator) -> None: + """ + Test registering duplicate plugin raises ValueError. + + GIVEN: A factory with an already registered plugin + WHEN: Attempting to register the same plugin again + THEN: Should raise ValueError with descriptive message + """ + factory.register("test_plugin", mock_creator) + + with pytest.raises(ValueError, match="Plugin 'test_plugin' is already registered"): + factory.register("test_plugin", mock_creator) + + def test_create_registered_plugin_with_all_parameters(self, factory, mock_creator) -> None: + """ + Test creating a registered plugin with all required parameters. + + GIVEN: A factory with a registered plugin + WHEN: Creating the plugin with parent frame, model, and base window + THEN: Should call creator with correct parameters and return plugin instance + """ + factory.register("test_plugin", mock_creator) + parent_frame = MagicMock() + data_model = MagicMock() + base_window = MagicMock() + + result = factory.create("test_plugin", parent_frame, data_model, base_window) + + assert result is not None + mock_creator.assert_called_once_with(parent_frame, data_model, base_window) + + def test_create_unregistered_plugin_returns_none(self, factory) -> None: + """ + Test creating an unregistered plugin. + + GIVEN: A factory without a registered plugin + WHEN: Attempting to create a nonexistent plugin + THEN: Should return None gracefully + """ + result = factory.create("nonexistent_plugin", MagicMock(), MagicMock(), MagicMock()) + + assert result is None + + def test_is_registered_for_nonexistent_plugin(self, factory) -> None: + """ + Test checking if nonexistent plugin is registered. + + GIVEN: A factory without a registered plugin + WHEN: Checking if plugin is registered + THEN: Should return False + """ + assert not factory.is_registered("nonexistent_plugin") + + def test_get_registered_plugins_empty(self, factory) -> None: + """ + Test getting registered plugins when none exist. + + GIVEN: A factory with no registered plugins + WHEN: Getting the list of registered plugins + THEN: Should return empty list + """ + assert factory.get_registered_plugins() == [] + + def test_get_registered_plugins_multiple(self, factory) -> None: + """ + Test getting multiple registered plugins. + + GIVEN: A factory with multiple registered plugins + WHEN: Getting the list of registered plugins + THEN: Should return list containing all plugin names + """ + factory.register("motor_test", MagicMock()) + factory.register("tempcal_imu", MagicMock()) + factory.register("compass_calibration", MagicMock()) + + plugins = factory.get_registered_plugins() + + assert len(plugins) == 3 + assert "motor_test" in plugins + assert "tempcal_imu" in plugins + assert "compass_calibration" in plugins + + def test_plugin_creator_receives_correct_parent_frame_type(self, factory) -> None: + """ + Test that plugin creator receives the correct parent frame. + + GIVEN: A factory with a registered plugin + WHEN: Creating a plugin with a specific parent frame type + THEN: The creator should receive the exact parent frame instance + """ + mock_creator = MagicMock() + factory.register("test_plugin", mock_creator) + parent_frame = MagicMock() + + factory.create("test_plugin", parent_frame, MagicMock(), MagicMock()) + + # Verify the exact parent frame instance was passed + call_args = mock_creator.call_args[0] + assert call_args[0] is parent_frame + + def test_multiple_plugins_can_coexist(self, factory) -> None: + """ + Test multiple different plugins can be registered and created independently. + + GIVEN: A factory with multiple registered plugins + WHEN: Creating different plugin instances + THEN: Each should be created independently with their respective creators + """ + creator1 = MagicMock(return_value="plugin1_instance") + creator2 = MagicMock(return_value="plugin2_instance") + + factory.register("plugin1", creator1) + factory.register("plugin2", creator2) + + result1 = factory.create("plugin1", MagicMock(), MagicMock(), MagicMock()) + result2 = factory.create("plugin2", MagicMock(), MagicMock(), MagicMock()) + + assert result1 == "plugin1_instance" + assert result2 == "plugin2_instance" + creator1.assert_called_once() + creator2.assert_called_once() diff --git a/tests/test_plugin_factory_workflow.py b/tests/test_plugin_factory_workflow.py new file mode 100755 index 00000000..f1d7a3c7 --- /dev/null +++ b/tests/test_plugin_factory_workflow.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 + +""" +Tests for workflow plugin factory. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2025 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from unittest.mock import MagicMock + +import pytest + +from ardupilot_methodic_configurator.plugin_factory_workflow import PluginFactoryWorkflow + + +class TestPluginFactoryWorkflow: + """Test suite for workflow plugin factory.""" + + @pytest.fixture + def factory(self) -> PluginFactoryWorkflow: + """Create a fresh factory instance for each test.""" + return PluginFactoryWorkflow() + + @pytest.fixture + def mock_creator(self) -> MagicMock: + """Create a mock creator function.""" + return MagicMock(return_value="mock_workflow") + + def test_register_plugin(self, factory, mock_creator) -> None: + """ + Test registering a plugin. + + GIVEN: A factory and a creator function + WHEN: Registering a plugin + THEN: Plugin should be registered successfully + """ + factory.register("test_plugin", mock_creator) + + assert factory.is_registered("test_plugin") + assert "test_plugin" in factory.get_registered_plugins() + + def test_register_duplicate_plugin_raises_error(self, factory, mock_creator) -> None: + """ + Test registering duplicate plugin raises ValueError. + + GIVEN: A factory with an already registered plugin + WHEN: Attempting to register the same plugin again + THEN: Should raise ValueError + """ + factory.register("test_plugin", mock_creator) + + with pytest.raises(ValueError, match="Workflow plugin 'test_plugin' is already registered"): + factory.register("test_plugin", mock_creator) + + def test_create_registered_plugin(self, factory, mock_creator) -> None: + """ + Test creating a registered plugin. + + GIVEN: A factory with a registered plugin + WHEN: Creating the plugin + THEN: Should call creator and return result + """ + factory.register("test_plugin", mock_creator) + root_window = MagicMock() + data_model = MagicMock() + + result = factory.create("test_plugin", root_window, data_model) + + assert result == "mock_workflow" + mock_creator.assert_called_once_with(root_window, data_model) + + def test_create_unregistered_plugin_returns_none(self, factory) -> None: + """ + Test creating an unregistered plugin. + + GIVEN: A factory without a registered plugin + WHEN: Attempting to create the plugin + THEN: Should return None + """ + result = factory.create("nonexistent_plugin", MagicMock(), MagicMock()) + + assert result is None + + def test_is_registered_for_nonexistent_plugin(self, factory) -> None: + """ + Test checking if nonexistent plugin is registered. + + GIVEN: A factory without a registered plugin + WHEN: Checking if plugin is registered + THEN: Should return False + """ + assert not factory.is_registered("nonexistent_plugin") + + def test_get_registered_plugins_empty(self, factory) -> None: + """ + Test getting registered plugins when none exist. + + GIVEN: A factory with no registered plugins + WHEN: Getting registered plugins + THEN: Should return empty list + """ + assert factory.get_registered_plugins() == [] + + def test_get_registered_plugins_multiple(self, factory) -> None: + """ + Test getting multiple registered plugins. + + GIVEN: A factory with multiple registered plugins + WHEN: Getting registered plugins + THEN: Should return list of all plugin names + """ + factory.register("plugin1", MagicMock()) + factory.register("plugin2", MagicMock()) + factory.register("plugin3", MagicMock()) + + plugins = factory.get_registered_plugins() + + assert len(plugins) == 3 + assert "plugin1" in plugins + assert "plugin2" in plugins + assert "plugin3" in plugins