diff --git a/.gitignore b/.gitignore index 870feacd3..b3f8784f8 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,5 @@ test.xml sitl/arducopter sitl/firmware-version.txt sitl/git-version.txt + +tuning_report.csv diff --git a/tests/test_backend_flightcontroller_commands.py b/tests/test_backend_flightcontroller_commands.py index 55547a90c..e6ede0fae 100755 --- a/tests/test_backend_flightcontroller_commands.py +++ b/tests/test_backend_flightcontroller_commands.py @@ -954,3 +954,221 @@ def test_send_command_exception_in_send(self) -> None: # Then assert success is False assert "failed to send command" in error.lower() + + +class TestFlightControllerCommandsMissingConnectionBranches: + """Tests for commands that need a connection but master is None.""" + + def test_send_command_and_wait_ack_fails_without_connection(self) -> None: + """ + send_command_and_wait_ack fails gracefully when master is None. + + GIVEN: No flight controller connection (master is None) + WHEN: send_command_and_wait_ack is called + THEN: False should be returned with appropriate error message + AND: No exceptions should be raised + """ + mock_conn_mgr = Mock() + mock_conn_mgr.master = None + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.send_command_and_wait_ack(command=999, timeout=0.5) + + assert success is False + assert "connection" in error.lower() + + def test_stop_all_motors_fails_without_connection(self) -> None: + """ + stop_all_motors fails gracefully when master is None. + + GIVEN: No flight controller connection + WHEN: stop_all_motors is called + THEN: False should be returned with error message + """ + mock_conn_mgr = Mock() + mock_conn_mgr.master = None + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.stop_all_motors() + + assert success is False + assert "connection" in error.lower() + + def test_request_periodic_battery_status_fails_without_connection(self) -> None: + """ + request_periodic_battery_status fails gracefully when master is None. + + GIVEN: No flight controller connection + WHEN: request_periodic_battery_status is called + THEN: False should be returned with error message + """ + mock_conn_mgr = Mock() + mock_conn_mgr.master = None + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.request_periodic_battery_status() + + assert success is False + assert "connection" in error.lower() + + +class TestFlightControllerCommandsFailureBranches: + """Tests for failure branches in command methods.""" + + def _make_commands_mgr_with_ack(self, result_code: int) -> "FlightControllerCommands": + """Create a commands manager that returns a given ACK result code.""" + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + mock_ack = MagicMock() + mock_ack.command = mavutil.mavlink.MAV_CMD_DO_MOTOR_TEST + mock_ack.result = result_code + + mock_master.recv_match.return_value = mock_ack + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_params_mgr = Mock() + mock_params_mgr.fc_parameters = {} + + return FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + def test_reset_all_parameters_handles_command_failure(self) -> None: + """ + reset_all_parameters_to_default handles command failure correctly. + + GIVEN: Connected flight controller that rejects the parameter reset command + WHEN: User calls reset_all_parameters_to_default + THEN: False should be returned with error description + AND: fc_parameters should NOT be cleared on failure + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + mock_ack = MagicMock() + mock_ack.command = mavutil.mavlink.MAV_CMD_PREFLIGHT_STORAGE + mock_ack.result = mavutil.mavlink.MAV_RESULT_DENIED + + mock_master.recv_match.return_value = mock_ack + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_params_mgr = Mock() + mock_params_mgr.fc_parameters = {"PARAM1": 1.0} + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.reset_all_parameters_to_default() + + assert success is False + assert "failed" in error.lower() or "denied" in error.lower() + # fc_parameters should NOT be cleared on failure + assert len(mock_params_mgr.fc_parameters) > 0 + + def test_test_motors_in_sequence_handles_command_failure(self) -> None: + """ + test_motors_in_sequence handles command failure correctly. + + GIVEN: Connected flight controller that rejects the sequential motor test command + WHEN: User calls test_motors_in_sequence + THEN: False should be returned with error description + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + mock_ack = MagicMock() + mock_ack.command = mavutil.mavlink.MAV_CMD_DO_MOTOR_TEST + mock_ack.result = mavutil.mavlink.MAV_RESULT_FAILED + + mock_master.recv_match.return_value = mock_ack + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.test_motors_in_sequence( + start_motor=1, motor_count=4, throttle_percent=10, timeout_seconds=2 + ) + + assert success is False + assert "failed" in error.lower() + + def test_stop_all_motors_handles_command_failure(self) -> None: + """ + stop_all_motors handles command failure correctly. + + GIVEN: Connected flight controller that rejects the stop command + WHEN: User calls stop_all_motors + THEN: False should be returned with error description + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + mock_ack = MagicMock() + mock_ack.command = mavutil.mavlink.MAV_CMD_DO_MOTOR_TEST + mock_ack.result = mavutil.mavlink.MAV_RESULT_UNSUPPORTED + + mock_master.recv_match.return_value = mock_ack + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + success, error = commands_mgr.stop_all_motors() + + assert success is False + assert error # Non-empty error message + + def test_send_command_handles_in_progress_with_zero_progress(self) -> None: + """ + send_command_and_wait_ack handles MAV_RESULT_IN_PROGRESS with zero progress. + + GIVEN: Flight controller sends IN_PROGRESS with progress=0 + WHEN: send_command_and_wait_ack receives an IN_PROGRESS ACK + THEN: Processing continues waiting without logging (progress <= 0) + AND: Eventually times out and returns False + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + # Return IN_PROGRESS ACK with progress=0 (should NOT log) then timeout + mock_ack = MagicMock() + mock_ack.command = 999 + mock_ack.result = mavutil.mavlink.MAV_RESULT_IN_PROGRESS + mock_ack.progress = 0 # <= 0, so debug logging is skipped + + call_count = [0] + + def side_effect_recv_match(*_args, **_kwargs) -> object: + call_count[0] += 1 + if call_count[0] <= 2: + return mock_ack + return None # Stop returning ACK after a couple of calls + + mock_master.recv_match.side_effect = side_effect_recv_match + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_params_mgr = Mock() + + commands_mgr = FlightControllerCommands(params_manager=mock_params_mgr, connection_manager=mock_conn_mgr) + + # Short timeout so test doesn't hang + success, _error = commands_mgr.send_command_and_wait_ack(command=999, timeout=0.3) + + # Should timeout after IN_PROGRESS messages + assert success is False diff --git a/tests/test_backend_flightcontroller_params.py b/tests/test_backend_flightcontroller_params.py index a9e17c698..5fa20e1c4 100755 --- a/tests/test_backend_flightcontroller_params.py +++ b/tests/test_backend_flightcontroller_params.py @@ -1058,5 +1058,188 @@ def test_multiple_set_param_operations_sequence(self) -> None: assert isinstance(result3[0], bool) +class TestDownloadParamsViaMavlink: + """Tests for the _download_params_via_mavlink method.""" + + def test_download_params_returns_empty_when_master_is_none(self) -> None: + """ + _download_params_via_mavlink returns empty dict when master is None. + + GIVEN: No flight controller connection (master is None) + WHEN: _download_params_via_mavlink is called + THEN: An empty dictionary should be returned + AND: No exceptions should be raised + """ + mock_conn_mgr = Mock() + mock_conn_mgr.master = None + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + result = params_mgr._download_params_via_mavlink() + + assert result == {} + + def test_download_params_handles_none_message(self) -> None: + """ + _download_params_via_mavlink handles None message (timeout) gracefully. + + GIVEN: Connected FC but recv_match returns None (timeout) + WHEN: _download_params_via_mavlink is called + THEN: An empty dictionary should be returned + AND: No exceptions should be raised + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + mock_master.recv_match.return_value = None # Simulate timeout + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "COM1" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + result = params_mgr._download_params_via_mavlink() + + assert result == {} + + def test_download_params_receives_parameters_with_progress_callback(self) -> None: + """ + _download_params_via_mavlink calls progress callback while receiving parameters. + + GIVEN: Connected FC that returns parameter messages + WHEN: _download_params_via_mavlink is called with a progress callback + THEN: The callback should be called with current count and total + AND: The parameters should be returned in the result dict + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + + # Create two parameter messages + param1_msg = MagicMock() + param1_msg.to_dict.return_value = {"param_id": "PARAM1", "param_value": 1.0} + param1_msg.param_count = 2 + + param2_msg = MagicMock() + param2_msg.to_dict.return_value = {"param_id": "PARAM2", "param_value": 2.0} + param2_msg.param_count = 2 + + mock_master.recv_match.side_effect = [param1_msg, param2_msg, None] + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "COM1" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + progress_calls = [] + + def progress_callback(current: int, total: int) -> None: + progress_calls.append((current, total)) + + result = params_mgr._download_params_via_mavlink(progress_callback=progress_callback) + + assert "PARAM1" in result + assert "PARAM2" in result + assert result["PARAM1"] == 1.0 + assert result["PARAM2"] == 2.0 + assert len(progress_calls) >= 1 + + def test_download_params_handles_exception_from_recv_match(self) -> None: + """ + _download_params_via_mavlink handles exceptions from recv_match gracefully. + + GIVEN: Connected FC where recv_match raises an exception + WHEN: _download_params_via_mavlink is called + THEN: The exception should be caught + AND: Any parameters received before the error should be returned + """ + mock_master = MagicMock() + mock_master.target_system = 1 + mock_master.target_component = 1 + mock_master.recv_match.side_effect = Exception("Serial port disconnected") + + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "COM1" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + # Should not raise an exception + result = params_mgr._download_params_via_mavlink() + + assert isinstance(result, dict) + + +class TestDownloadParamsViaMavftp: + """Tests for the _download_params_via_mavftp method.""" + + def test_download_via_mavftp_returns_empty_when_master_is_none(self) -> None: + """ + _download_params_via_mavftp returns empty dicts when master is None. + + GIVEN: No flight controller connection (master is None) + WHEN: _download_params_via_mavftp is called + THEN: An empty dict and empty ParDict should be returned + """ + mock_conn_mgr = Mock() + mock_conn_mgr.master = None + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + result_params, result_defaults = params_mgr._download_params_via_mavftp() + + assert result_params == {} + assert len(result_defaults) == 0 + + def test_download_via_mavftp_calls_progress_callback(self) -> None: + """ + _download_params_via_mavftp calls progress callback with correct arguments. + + GIVEN: Connected FC with MAVFTP support + WHEN: _download_params_via_mavftp is called with a progress callback + THEN: The callback should be called with percentage values (0-100) + """ + mock_master = MagicMock() + mock_conn_mgr = Mock() + mock_conn_mgr.master = mock_master + mock_conn_mgr.info = FlightControllerInfo() + mock_conn_mgr.comport_device = "COM1" + + params_mgr = FlightControllerParams(connection_manager=mock_conn_mgr) + + progress_calls = [] + + def progress_callback(current: int, total: int) -> None: + progress_calls.append((current, total)) + + with patch("ardupilot_methodic_configurator.backend_flightcontroller_params.create_mavftp") as mock_mavftp_factory: + mock_mavftp = MagicMock() + # process_ftp_reply returns an object with error_code attribute + mock_ftp_reply = MagicMock() + mock_ftp_reply.error_code = 1 # Simulate failure so we don't need real files + mock_mavftp.process_ftp_reply.return_value = mock_ftp_reply + mock_mavftp_factory.return_value = mock_mavftp + + _result_params, _result_defaults = params_mgr._download_params_via_mavftp(progress_callback=progress_callback) + + # The inner callback should be set up to call the outer progress_callback + # We verify by calling cmd_getparams with the inner callback + call_args = mock_mavftp.cmd_getparams.call_args + inner_callback = call_args[1].get("progress_callback") or call_args[0][-1] + if callable(inner_callback): + inner_callback(0.5) # 50% completion + assert len(progress_calls) >= 1 + assert (50, 100) in progress_calls + + if __name__ == "__main__": pytest.main([__file__, "-v"]) diff --git a/tests/test_backend_safe_file_io.py b/tests/test_backend_safe_file_io.py new file mode 100755 index 000000000..8695e8b9c --- /dev/null +++ b/tests/test_backend_safe_file_io.py @@ -0,0 +1,287 @@ +#!/usr/bin/env python3 + +""" +Behavior-driven tests for the backend_safe_file_io.py file. + +Tests for crash-safe atomic file writing utilities. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import io +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +import ardupilot_methodic_configurator.backend_safe_file_io as sio +from ardupilot_methodic_configurator.backend_safe_file_io import make_capture_safe_write, safe_write + +# pylint: disable=redefined-outer-name + + +@pytest.fixture +def tmp_file(tmp_path: Path) -> Path: + """Fixture providing a temporary file path.""" + return tmp_path / "test_output.json" + + +class TestSafeWriteBasicBehavior: + """Tests for basic safe_write behavior.""" + + def test_user_can_write_content_to_new_file(self, tmp_file: Path) -> None: + """ + User can write content to a new file atomically. + + GIVEN: A target file path that does not yet exist + WHEN: User calls safe_write with a write function + THEN: File should be created with the correct content + AND: No temporary files should remain + """ + content = "Hello, World!\n" + + safe_write(str(tmp_file), lambda f: f.write(content)) + + assert tmp_file.exists() + assert tmp_file.read_text(encoding="utf-8") == content + + def test_user_can_overwrite_existing_file(self, tmp_file: Path) -> None: + """ + User can atomically overwrite an existing file. + + GIVEN: A target file already containing some content + WHEN: User calls safe_write with new content + THEN: File should contain only the new content + AND: Original content should be replaced + """ + tmp_file.write_text("old content\n", encoding="utf-8") + + safe_write(str(tmp_file), lambda f: f.write("new content\n")) + + assert tmp_file.read_text(encoding="utf-8") == "new content\n" + + def test_file_is_fully_written_before_target_is_replaced(self, tmp_file: Path) -> None: + """ + File is fully written to a temp location before replacing the target. + + GIVEN: A target file with existing content + WHEN: safe_write is called with large content + THEN: Target should contain the complete new content + AND: File should be readable immediately after write + """ + large_content = "x" * 100000 + "\n" + + safe_write(str(tmp_file), lambda f: f.write(large_content)) + + result = tmp_file.read_text(encoding="utf-8") + assert result == large_content + + def test_user_can_write_json_content(self, tmp_file: Path) -> None: + """ + User can write JSON content to a file using safe_write. + + GIVEN: A dictionary of data to persist + WHEN: User writes JSON data via safe_write + THEN: File should contain valid JSON with the expected data + """ + data = {"key": "value", "number": 42, "nested": {"list": [1, 2, 3]}} + + safe_write(str(tmp_file), lambda f: json.dump(data, f, indent=2)) + + written = json.loads(tmp_file.read_text(encoding="utf-8")) + assert written == data + + def test_no_temporary_files_remain_after_successful_write(self, tmp_path: Path) -> None: + """ + No temporary files remain after a successful write operation. + + GIVEN: A target file path + WHEN: safe_write completes successfully + THEN: No .tmp files should remain in the directory + """ + tmp_file = tmp_path / "output.json" + + safe_write(str(tmp_file), lambda f: f.write("content")) + + tmp_files = list(tmp_path.glob("*.tmp")) + assert tmp_files == [], f"Found leftover temp files: {tmp_files}" + + +class TestSafeWriteFilePermissions: + """Tests for safe_write file permission handling.""" + + def test_new_file_gets_default_permissions(self, tmp_file: Path) -> None: + """ + New file created by safe_write gets reasonable default permissions. + + GIVEN: A target file path that does not exist + WHEN: safe_write creates the file + THEN: File should have readable permissions + """ + safe_write(str(tmp_file), lambda f: f.write("content")) + + assert os.access(str(tmp_file), os.R_OK) + assert os.access(str(tmp_file), os.W_OK) + + def test_file_permissions_are_preserved_on_overwrite(self, tmp_file: Path) -> None: + """ + Existing file permissions are preserved when safe_write overwrites the file. + + GIVEN: An existing file with specific permissions + WHEN: safe_write overwrites it + THEN: The file permissions should be preserved (or close to original) + """ + tmp_file.write_text("original content", encoding="utf-8") + original_mode = os.stat(str(tmp_file)).st_mode + + safe_write(str(tmp_file), lambda f: f.write("new content")) + + new_mode = os.stat(str(tmp_file)).st_mode + # Permissions should be preserved (same mode bits for owner/group/other read+write) + assert (new_mode & 0o666) == (original_mode & 0o666) + + def test_safe_write_handles_file_not_found_for_stat(self, tmp_path: Path) -> None: + """ + safe_write handles the case where the target doesn't exist (for stat). + + GIVEN: A target file path that does not exist + WHEN: safe_write tries to stat the target to preserve permissions + THEN: FileNotFoundError should be handled gracefully + AND: The write should still succeed with default permissions + """ + new_file = tmp_path / "nonexistent_parent" / "output.txt" + new_file.parent.mkdir(parents=True) + + safe_write(str(new_file), lambda f: f.write("content")) + + assert new_file.exists() + assert new_file.read_text(encoding="utf-8") == "content" + + +class TestSafeWriteErrorHandling: + """Tests for safe_write error handling behavior.""" + + def test_cleanup_happens_when_write_fails(self, tmp_path: Path) -> None: + """ + Temporary file is cleaned up when write function raises an exception. + + GIVEN: A write function that raises an exception + WHEN: safe_write is called and the write fails + THEN: No temporary files should remain in the directory + AND: An exception should be raised to the caller + """ + + def failing_write(_f: io.TextIOWrapper) -> None: + msg = "Simulated disk full" + raise OSError(msg) + + tmp_file = tmp_path / "output.txt" + + with pytest.raises(OSError, match="Simulated disk full"): + safe_write(str(tmp_file), failing_write) + + tmp_files = list(tmp_path.glob("*.tmp")) + assert tmp_files == [], f"Temp files not cleaned up: {tmp_files}" + + # Target should not have been created + assert not tmp_file.exists() + + def test_directory_fsync_failure_does_not_break_write(self, tmp_file: Path) -> None: + """ + Directory fsync failure does not break the write operation. + + GIVEN: A system where directory fsync fails + WHEN: safe_write is called + THEN: The file write should still succeed + AND: The error from fsync should be silently ignored + """ + original_open = os.open + + def mock_os_open(path: str, flags: int, *_args, **_kwargs) -> int: + return original_open(path, flags, *_args, **_kwargs) + + with patch("os.fsync", side_effect=[None, OSError("fsync failed")]): + safe_write(str(tmp_file), lambda f: f.write("test content")) + + assert tmp_file.exists() + assert tmp_file.read_text(encoding="utf-8") == "test content" + + +class TestMakeCapturesSafeWrite: + """Tests for the make_capture_safe_write helper function.""" + + def test_capture_helper_records_written_json_data(self) -> None: + """ + make_capture_safe_write helper records JSON data written via safe_write. + + GIVEN: A mock that uses make_capture_safe_write + WHEN: A function writes JSON data through the mock + THEN: The captured_data should contain the written JSON + AND: called[0] should be True + """ + captured_data, called, side_effect = make_capture_safe_write() + + assert called[0] is False + assert captured_data == {} + + # Simulate using the side_effect + mock_file = io.StringIO() + test_data = {"key": "value", "number": 42} + mock_file.write(json.dumps(test_data)) + + # Call the side_effect directly + def write_json(f: io.TextIOWrapper) -> None: + json.dump(test_data, f) + + side_effect("/fake/path", write_json) + + assert called[0] is True + assert captured_data == test_data + + def test_capture_helper_can_be_used_as_mock_side_effect(self) -> None: + """ + make_capture_safe_write helper integrates correctly as a mock side_effect. + + GIVEN: A function that calls safe_write internally + WHEN: safe_write is mocked with the capture side_effect + THEN: The written data should be captured for verification + """ + captured_data, called, side_effect = make_capture_safe_write() + + test_data = {"component": "Flight Controller", "version": "4.5.x"} + + with patch("ardupilot_methodic_configurator.backend_safe_file_io.safe_write") as mock_safe_write: + mock_safe_write.side_effect = side_effect + + # Call something that uses safe_write internally + sio.safe_write("/fake/path", lambda f: json.dump(test_data, f)) + + assert called[0] is True + assert captured_data == test_data + + def test_capture_helper_returns_three_elements(self) -> None: + """ + make_capture_safe_write returns exactly three elements. + + GIVEN: The make_capture_safe_write function + WHEN: Called with no arguments + THEN: Should return (captured_data, called, side_effect) tuple + AND: captured_data should be empty dict initially + AND: called should be [False] initially + AND: side_effect should be callable + """ + result = make_capture_safe_write() + + assert len(result) == 3 + captured_data, called, side_effect = result + assert isinstance(captured_data, dict) + assert len(captured_data) == 0 + assert isinstance(called, list) + assert called == [False] + assert callable(side_effect) diff --git a/tests/test_data_model_configuration_step.py b/tests/test_data_model_configuration_step.py index e0e339e80..92922bbf0 100755 --- a/tests/test_data_model_configuration_step.py +++ b/tests/test_data_model_configuration_step.py @@ -1006,3 +1006,152 @@ def test_user_receives_expresslrs_warning_with_both_bits_set_and_fltmode_ch_5(se assert len(ui_infos) == 1 assert ui_infos[0][0] == "ExpressLRS Configuration Warning" assert ui_errors == [] + + +class TestDerivedParametersFiltering: + """Tests for derived parameters filtering logic.""" + + def test_derived_parameters_filtered_by_fc_keys_when_fc_provided(self, processor, mock_local_filesystem) -> None: + """ + Derived parameters are filtered by FC keys when fc_parameters is provided. + + GIVEN: A configuration step with derived parameters and FC parameters + WHEN: Processing produces derived parameters, some existing in FC and some not + THEN: Only derived parameters that exist in FC should be collected for application + AND: Parameters not in FC keys should be excluded from derived_params_to_apply + """ + selected_file = "test_file.param" + # Set up derived parameters that will be returned + derived_params = { + "SERIAL1_PROTOCOL": Par(value=5.0, comment="derived"), # in file AND in FC + "CAN_P1_DRIVER": Par(value=2.0, comment="derived"), # in file, NOT in FC + } + mock_local_filesystem.configuration_steps = {selected_file: {"derived": {}}} + mock_local_filesystem.compute_parameters.return_value = None + mock_local_filesystem.derived_parameters = {selected_file: derived_params} + mock_local_filesystem.file_parameters = { + selected_file: { + "SERIAL1_PROTOCOL": Par(value=4.0, comment="original"), + "CAN_P1_DRIVER": Par(value=1.0, comment="original"), + } + } + + # FC only has SERIAL1_PROTOCOL (not CAN_P1_DRIVER) + limited_fc_params = {"SERIAL1_PROTOCOL": 4.0} + + _, ui_errors, _, _, _, derived_to_apply = processor.process_configuration_step(selected_file, limited_fc_params) + + assert ui_errors == [] + # SERIAL1_PROTOCOL is in both file and FC, so it should be in derived_to_apply + assert "SERIAL1_PROTOCOL" in derived_to_apply + # CAN_P1_DRIVER is in file but NOT in FC, so it should be excluded + assert "CAN_P1_DRIVER" not in derived_to_apply + + def test_derived_parameters_not_in_file_are_excluded(self, processor, mock_local_filesystem) -> None: + """ + Derived parameters not currently in the file are excluded from derived_params_to_apply. + + GIVEN: A configuration step with derived parameters where some are add-from-FC shorthands + WHEN: Processing produces derived parameters + THEN: Parameters that don't yet exist in the file should be excluded + AND: Only parameters already in the file should be in derived_params_to_apply + """ + selected_file = "test_file.param" + derived_params = { + "SERIAL1_PROTOCOL": Par(value=5.0, comment="derived"), # in file + "NEW_PARAM": Par(value=99.0, comment="derived"), # NOT in file (add-from-FC shorthand) + } + mock_local_filesystem.configuration_steps = {selected_file: {"derived": {}}} + mock_local_filesystem.compute_parameters.return_value = None + mock_local_filesystem.derived_parameters = {selected_file: derived_params} + mock_local_filesystem.file_parameters = { + selected_file: { + "SERIAL1_PROTOCOL": Par(value=4.0, comment="original"), + # NEW_PARAM not in file + } + } + + fc_params = {"SERIAL1_PROTOCOL": 4.0, "NEW_PARAM": 99.0} + + _, ui_errors, _, _, _, derived_to_apply = processor.process_configuration_step(selected_file, fc_params) + + assert ui_errors == [] + assert "SERIAL1_PROTOCOL" in derived_to_apply + assert "NEW_PARAM" not in derived_to_apply + + def test_derived_parameters_all_included_when_no_fc_parameters(self, processor, mock_local_filesystem) -> None: + """ + All file-matching derived parameters are included when no FC parameters provided. + + GIVEN: A configuration step with derived parameters but no FC connection + WHEN: Processing produces derived parameters with empty fc_parameters + THEN: All derived parameters that exist in the file should be in derived_params_to_apply + """ + selected_file = "test_file.param" + derived_params = { + "SERIAL1_PROTOCOL": Par(value=5.0, comment="derived"), + "CAN_P1_DRIVER": Par(value=2.0, comment="derived"), + } + mock_local_filesystem.configuration_steps = {selected_file: {"derived": {}}} + mock_local_filesystem.compute_parameters.return_value = None + mock_local_filesystem.derived_parameters = {selected_file: derived_params} + mock_local_filesystem.file_parameters = { + selected_file: { + "SERIAL1_PROTOCOL": Par(value=4.0, comment="original"), + "CAN_P1_DRIVER": Par(value=1.0, comment="original"), + } + } + + # No FC parameters (empty dict simulates offline mode) + _, ui_errors, _, _, _, derived_to_apply = processor.process_configuration_step(selected_file, {}) + + assert ui_errors == [] + # Both should be included since fc_param_keys is empty (no FC filter) + assert "SERIAL1_PROTOCOL" in derived_to_apply + assert "CAN_P1_DRIVER" in derived_to_apply + + +class TestConnectionRenamingWithSameNameSkip: + """Tests for connection renaming edge case where rename results in same name.""" + + def test_rename_operation_skips_when_new_name_equals_old_name(self, processor) -> None: + """ + Rename operations are skipped when the new name equals the old name. + + GIVEN: A parameter that would be renamed to itself (no-op rename) + WHEN: The rename operation is calculated + THEN: No rename should be in the result list for that parameter + AND: No duplicates should be tracked for it + """ + # Use a parameter set where CAN1 → CAN1 (same) would be a no-op + # by using CAN1 as the new_connection_prefix + parameters = { + "CAN_P1_DRIVER": Par(value=1.0), + } + + # Rename to CAN1 - same connection, which means CAN_P1 → CAN_P1 (no-op) + _duplicates, renamed_pairs = processor._calculate_connection_rename_operations(parameters, "CAN1", None) + + # CAN_P1_DRIVER → CAN_P1_DRIVER (no change), should not be in renamed_pairs + assert all(old != new or old != "CAN_P1_DRIVER" for old, new in renamed_pairs) + + def test_rename_operation_detects_conflict_with_existing_parameter(self, processor) -> None: + """ + Rename operation handles conflict when target name already exists. + + GIVEN: Parameters where a rename would create a conflict with an existing parameter + WHEN: The rename operation is calculated + THEN: The conflicting rename should not be added to renamed_pairs + AND: No exception should be raised + """ + parameters = { + "CAN_P1_DRIVER": Par(value=1.0), + "CAN_P2_DRIVER": Par(value=2.0), # Would be the target of CAN_P1 rename to CAN2 + } + + # Renaming to CAN2 would conflict with existing CAN_P2_DRIVER + _duplicates, renamed_pairs = processor._calculate_connection_rename_operations(parameters, "CAN2", None) + + # CAN_P1_DRIVER → CAN_P2_DRIVER but CAN_P2_DRIVER already exists + # The conflicting rename should be silently skipped + assert ("CAN_P1_DRIVER", "CAN_P2_DRIVER") not in renamed_pairs diff --git a/tests/test_frontend_tkinter_about_popup_window.py b/tests/test_frontend_tkinter_about_popup_window.py new file mode 100755 index 000000000..121954c42 --- /dev/null +++ b/tests/test_frontend_tkinter_about_popup_window.py @@ -0,0 +1,315 @@ +#!/usr/bin/env python3 + +""" +Behavior-driven tests for the frontend_tkinter_about_popup_window.py file. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import contextlib +import tkinter as tk +from unittest.mock import patch + +import pytest + +from ardupilot_methodic_configurator.frontend_tkinter_about_popup_window import AboutWindow + +# pylint: disable=redefined-outer-name + + +@pytest.fixture +def root() -> tk.Tk: + """Provide a hidden Tk root window for tests.""" + root = tk.Tk() + root.withdraw() + yield root + root.update_idletasks() + root.destroy() + + +def _create_about_window(root: tk.Tk, version: str = "1.2.3") -> AboutWindow: + """Helper to create an AboutWindow with all heavy operations mocked.""" + with ( + patch("ardupilot_methodic_configurator.frontend_tkinter_base_window.BaseWindow._setup_application_icon"), + patch("ardupilot_methodic_configurator.frontend_tkinter_base_window.BaseWindow._setup_theme_and_styling"), + patch( + "ardupilot_methodic_configurator.frontend_tkinter_base_window.BaseWindow._get_dpi_scaling_factor", + return_value=1.0, + ), + patch("ardupilot_methodic_configurator.frontend_tkinter_base_window.BaseWindow.center_window"), + ): + return AboutWindow(root, version) + + +class TestAboutWindowCreation: + """Tests for About window creation and initialization.""" + + def test_user_can_open_about_window(self, root: tk.Tk) -> None: + """ + User can open the About popup window. + + GIVEN: The application is running + WHEN: User opens the About window + THEN: A window with title "About" should appear + AND: Version information should be displayed + """ + window = _create_about_window(root, version="3.0.5") + + assert window is not None + assert window.root is not None + assert "About" in window.root.title() + + def test_user_sees_version_information_in_about_window(self, root: tk.Tk) -> None: + """ + User sees correct version information in the About window. + + GIVEN: The application has a specific version + WHEN: User opens the About window + THEN: The version number should be displayed in the about message + """ + version = "3.0.5" + window = _create_about_window(root, version=version) + + # Find the about label and check it contains the version + found_version = False + for widget in window.main_frame.winfo_children(): + if hasattr(widget, "cget"): + try: + text = widget.cget("text") + if version in str(text): + found_version = True + break + except tk.TclError: + pass + assert found_version, f"Version '{version}' not found in any widget text" + + def test_user_sees_copyright_notice_in_about_window(self, root: tk.Tk) -> None: + """ + User sees copyright notice in the About window. + + GIVEN: The application is open + WHEN: User views the About window + THEN: Copyright information should be visible + """ + window = _create_about_window(root) + + found_copyright = False + for widget in window.main_frame.winfo_children(): + if hasattr(widget, "cget"): + try: + text = str(widget.cget("text")) + if "Copyright" in text or "copyright" in text.lower(): + found_copyright = True + break + except tk.TclError: + pass + assert found_copyright, "Copyright notice not found in About window" + + def test_about_window_has_action_buttons(self, root: tk.Tk) -> None: + """ + About window has action buttons for external resources. + + GIVEN: User opens the About window + WHEN: They look at the window + THEN: Buttons for User Manual, Support Forum, Report a Bug, Licenses, and Source Code should be present + """ + window = _create_about_window(root) + + button_texts = [] + for widget in window.main_frame.winfo_children(): + widget_class = widget.winfo_class() + if widget_class in ("TButton", "Button"): + with contextlib.suppress(tk.TclError): + button_texts.append(widget.cget("text")) + + # Should have 5 action buttons + assert len(button_texts) >= 5 + + def test_about_window_has_usage_popup_checkboxes(self, root: tk.Tk) -> None: + """ + About window contains usage popup preference checkboxes. + + GIVEN: User opens the About window + WHEN: They look at the window + THEN: Checkboxes for controlling usage popups should be present + """ + window = _create_about_window(root) + + found_frame = False + for widget in window.main_frame.winfo_children(): + widget_class = widget.winfo_class() + if widget_class in ("TFrame", "Frame"): + for child in widget.winfo_children(): + child_class = child.winfo_class() + if child_class in ("TCheckbutton", "Checkbutton"): + found_frame = True + break + + assert found_frame, "Usage popup checkboxes not found in About window" + + +class TestAboutWindowButtonInteractions: + """Tests for button interactions in the About window.""" + + def _find_button_by_text(self, window: AboutWindow, text_fragment: str) -> None: + """Find a button in the window and invoke it.""" + for widget in window.main_frame.winfo_children(): + widget_class = widget.winfo_class() + if widget_class == "TButton": + try: + btn_text = widget.cget("text") + if text_fragment.lower() in str(btn_text).lower(): + widget.invoke() + return + except tk.TclError: + pass + + def test_user_can_open_user_manual_via_about_window(self, root: tk.Tk) -> None: + """ + User can open the User Manual from the About window. + + GIVEN: User has the About window open + WHEN: User clicks the User Manual button + THEN: The User Manual URL should be opened in the default web browser + """ + window = _create_about_window(root) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_about_popup_window.webbrowser_open_url") as mock_open: + self._find_button_by_text(window, "Manual") + mock_open.assert_called_once() + call_args = mock_open.call_args[0][0] + assert "USERMANUAL" in call_args or "usermanual" in call_args.lower() + + def test_user_can_open_support_forum_via_about_window(self, root: tk.Tk) -> None: + """ + User can access the support forum from the About window. + + GIVEN: User has the About window open + WHEN: User clicks the Support Forum button + THEN: The support forum URL should be opened in the default web browser + """ + window = _create_about_window(root) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_about_popup_window.webbrowser_open_url") as mock_open: + self._find_button_by_text(window, "Support") + mock_open.assert_called_once() + call_args = mock_open.call_args[0][0] + assert "ardupilot" in call_args.lower() or "discuss" in call_args.lower() + + def test_user_can_report_a_bug_via_about_window(self, root: tk.Tk) -> None: + """ + User can report a bug via the About window. + + GIVEN: User has the About window open + WHEN: User clicks the Report a Bug button + THEN: The GitHub issues URL should be opened in the default web browser + """ + window = _create_about_window(root) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_about_popup_window.webbrowser_open_url") as mock_open: + self._find_button_by_text(window, "Bug") + mock_open.assert_called_once() + call_args = mock_open.call_args[0][0] + assert "issues" in call_args.lower() or "github" in call_args.lower() + + def test_user_can_view_licenses_via_about_window(self, root: tk.Tk) -> None: + """ + User can view licenses from the About window. + + GIVEN: User has the About window open + WHEN: User clicks the Licenses button + THEN: The CREDITS.md URL should be opened in the default web browser + """ + window = _create_about_window(root) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_about_popup_window.webbrowser_open_url") as mock_open: + self._find_button_by_text(window, "License") + mock_open.assert_called_once() + call_args = mock_open.call_args[0][0] + assert "CREDITS" in call_args or "credits" in call_args.lower() + + def test_user_can_view_source_code_via_about_window(self, root: tk.Tk) -> None: + """ + User can view source code from the About window. + + GIVEN: User has the About window open + WHEN: User clicks the Source Code button + THEN: The GitHub repository URL should be opened in the default web browser + """ + window = _create_about_window(root) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_about_popup_window.webbrowser_open_url") as mock_open: + self._find_button_by_text(window, "Source") + mock_open.assert_called_once() + call_args = mock_open.call_args[0][0] + assert "github.com/ArduPilot/MethodicConfigurator" in call_args + + +class TestAboutWindowUsagePopupPreferences: + """Tests for usage popup preference checkboxes in the About window.""" + + def test_user_can_toggle_usage_popup_display_preference(self, root: tk.Tk) -> None: + """ + User can toggle usage popup display preferences from the About window. + + GIVEN: User has the About window open + WHEN: User toggles a usage popup checkbox + THEN: The preference should be updated via ProgramSettings + """ + window = _create_about_window(root) + + with patch( + "ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.set_display_usage_popup" + ): + # Find and click a checkbox in the usage popup frame + for widget in window.main_frame.winfo_children(): + if widget.winfo_class() in ("TFrame", "Frame"): + for child in widget.winfo_children(): + if child.winfo_class() in ("TCheckbutton", "Checkbutton"): + child.invoke() + break + + def test_about_window_reads_current_usage_popup_preferences(self, root: tk.Tk) -> None: + """ + About window displays current usage popup preferences correctly. + + GIVEN: The user has previously set usage popup preferences + WHEN: User opens the About window + THEN: The checkboxes should reflect the current preferences + """ + with patch( + "ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.display_usage_popup", + return_value=True, + ): + window = _create_about_window(root) + + assert window is not None + + def test_about_window_initializes_with_different_version_strings(self, root: tk.Tk) -> None: + """ + About window initializes correctly with different version strings. + + GIVEN: Various valid version strings + WHEN: About window is created with each version + THEN: Window should display correctly for each version + """ + for version in ["1.0.0", "2.5.3", "10.0.0", "0.1.0-beta"]: + window = _create_about_window(root, version=version) + assert window is not None + + found_version = False + for widget in window.main_frame.winfo_children(): + if hasattr(widget, "cget"): + try: + text = str(widget.cget("text")) + if version in text: + found_version = True + break + except tk.TclError: + pass + assert found_version, f"Version '{version}' not found in About window" + window.root.destroy() diff --git a/tests/test_frontend_tkinter_directory_selection.py b/tests/test_frontend_tkinter_directory_selection.py index 67761c7ba..489a620e4 100755 --- a/tests/test_frontend_tkinter_directory_selection.py +++ b/tests/test_frontend_tkinter_directory_selection.py @@ -16,7 +16,10 @@ import pytest +from ardupilot_methodic_configurator.data_model_vehicle_project_creator import VehicleProjectCreationError +from ardupilot_methodic_configurator.data_model_vehicle_project_opener import VehicleProjectOpenError from ardupilot_methodic_configurator.frontend_tkinter_directory_selection import ( + BinLogSelectionWidgets, DirectorySelectionWidgets, PathEntryWidget, VehicleDirectorySelectionWidgets, @@ -757,3 +760,262 @@ def test_directory_name_widget_get_selected_directory_returns_name(self, root) - # Assert: Returns the initial name assert isinstance(result, str) assert result == "TestVehicle" + + +# ==================== BIN LOG SELECTION WIDGET TESTS ==================== + + +class TestBinLogSelectionWidgets: + """Tests for BinLogSelectionWidgets component behavior.""" + + @pytest.fixture + def bin_log_widget_setup(self, root) -> BinLogSelectionWidgets: + """Provide a BinLogSelectionWidgets instance for testing.""" + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + callback = MagicMock() + return BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=callback) + + def test_user_can_select_bin_log_file_successfully(self, root) -> None: + """ + User can select a .bin log file successfully. + + GIVEN: A BinLogSelectionWidgets instance with a callback + WHEN: User clicks the select file button and picks a file + THEN: The callback should be called with the selected file path + AND: True should be returned + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + mock_callback = MagicMock() + widget = BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=mock_callback) + + selected_file = "/path/to/flight.bin" + with patch("tkinter.filedialog.askopenfilename", return_value=selected_file): + result = widget.on_select_file() + + assert result is True + mock_callback.assert_called_once_with(selected_file) + + def test_user_can_cancel_bin_log_file_selection(self, root) -> None: + """ + User can cancel .bin log file selection. + + GIVEN: A BinLogSelectionWidgets instance + WHEN: User opens the file dialog but cancels + THEN: False should be returned + AND: The callback should not be called + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + mock_callback = MagicMock() + widget = BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=mock_callback) + + with patch("tkinter.filedialog.askopenfilename", return_value=""): + result = widget.on_select_file() + + assert result is False + mock_callback.assert_not_called() + + def test_bin_log_selection_handles_vehicle_creation_error(self, root) -> None: + """ + BinLog selection handles VehicleProjectCreationError gracefully. + + GIVEN: A BinLogSelectionWidgets instance with a callback that raises VehicleProjectCreationError + WHEN: User selects a file and the callback fails with VehicleProjectCreationError + THEN: An error dialog should be shown + AND: False should be returned + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + error = VehicleProjectCreationError(title="Creation Failed", message="Cannot create project") + mock_callback = MagicMock(side_effect=error) + widget = BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=mock_callback) + + with ( + patch("tkinter.filedialog.askopenfilename", return_value="/path/to/flight.bin"), + patch("tkinter.messagebox.showerror") as mock_error, + ): + result = widget.on_select_file() + + assert result is False + mock_error.assert_called_once() + + def test_bin_log_selection_handles_vehicle_open_error(self, root) -> None: + """ + BinLog selection handles VehicleProjectOpenError gracefully. + + GIVEN: A BinLogSelectionWidgets instance with a callback that raises VehicleProjectOpenError + WHEN: User selects a file and the callback fails with VehicleProjectOpenError + THEN: An error dialog should be shown + AND: False should be returned + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + error = VehicleProjectOpenError(title="Open Failed", message="Cannot open project") + mock_callback = MagicMock(side_effect=error) + widget = BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=mock_callback) + + with ( + patch("tkinter.filedialog.askopenfilename", return_value="/path/to/flight.bin"), + patch("tkinter.messagebox.showerror") as mock_error, + ): + result = widget.on_select_file() + + assert result is False + mock_error.assert_called_once() + + def test_bin_log_selection_handles_os_error(self, root) -> None: + """ + BinLog selection handles OSError gracefully. + + GIVEN: A BinLogSelectionWidgets instance with a callback that raises OSError + WHEN: User selects a file and an OS error occurs + THEN: An error dialog should be shown + AND: False should be returned + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent = MagicMock() + parent.root = root + parent_frame = ttk.Frame(root) + mock_callback = MagicMock(side_effect=OSError("Permission denied")) + widget = BinLogSelectionWidgets(parent=parent, parent_frame=parent_frame, on_select_file_callback=mock_callback) + + with ( + patch("tkinter.filedialog.askopenfilename", return_value="/path/to/flight.bin"), + patch("tkinter.messagebox.showerror") as mock_error, + ): + result = widget.on_select_file() + + assert result is False + mock_error.assert_called_once() + + +# ==================== TARGETED COVERAGE TESTS ==================== + + +class TestDirectorySelectionEdgeCases: + """Tests targeting specific uncovered edge cases in directory selection.""" + + def test_update_directory_display_with_parent_without_root(self, root) -> None: + """ + update_directory_display works when parent lacks a root attribute. + + GIVEN: A DirectorySelectionWidgets with a parent that has no 'root' attribute + WHEN: update_directory_display is called + THEN: Directory should still be updated without calling update_idletasks + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + parent_no_root = MagicMock(spec=[]) # No 'root' attribute + parent_frame = ttk.Frame(root) + + widget = DirectorySelectionWidgets( + parent=parent_no_root, + parent_frame=parent_frame, + initialdir="/initial/dir", + label_text="Test:", + autoresize_width=False, + dir_tooltip="tooltip", + button_tooltip="Browse", + ) + + # Directly call update_directory_display + widget.update_directory_display("/new/directory") + + assert widget.directory == "/new/directory" + + def test_vehicle_directory_widget_destroys_parent_on_open(self, root) -> None: + """ + VehicleDirectorySelectionWidgets destroys parent when destroy_parent_on_open is True. + + GIVEN: A VehicleDirectorySelectionWidgets with destroy_parent_on_open=True + WHEN: User successfully selects a directory + THEN: The parent's root.destroy() should be called + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + mock_parent = MagicMock() + mock_parent_root = MagicMock() + mock_parent.root = mock_parent_root + parent_frame = ttk.Frame(root) + + widget = VehicleDirectorySelectionWidgets( + parent=mock_parent, + parent_frame=parent_frame, + initial_dir="/initial/dir", + destroy_parent_on_open=True, + on_select_directory_callback=None, + ) + + with patch("tkinter.filedialog.askdirectory", return_value="/new/vehicle/dir"): + result = widget.on_select_directory() + + assert result is True + mock_parent_root.destroy.assert_called_once() + + def test_vehicle_directory_widget_no_callback_with_destroy(self, root) -> None: + """ + VehicleDirectorySelectionWidgets works without callback but with destroy. + + GIVEN: A VehicleDirectorySelectionWidgets with destroy_parent_on_open=True but no callback + WHEN: User selects a directory + THEN: Parent should be destroyed and True should be returned + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + mock_parent = MagicMock() + parent_frame = ttk.Frame(root) + + widget = VehicleDirectorySelectionWidgets( + parent=mock_parent, + parent_frame=parent_frame, + initial_dir="/initial", + destroy_parent_on_open=True, + on_select_directory_callback=None, + ) + + with patch("tkinter.filedialog.askdirectory", return_value="/selected/dir"): + result = widget.on_select_directory() + + assert result is True + mock_parent.root.destroy.assert_called_once() + + def test_vehicle_directory_widget_handles_open_error_in_callback(self, root) -> None: + """ + VehicleDirectorySelectionWidgets handles VehicleProjectOpenError in callback. + + GIVEN: A VehicleDirectorySelectionWidgets with a callback that raises VehicleProjectOpenError + WHEN: User selects a directory and the callback fails + THEN: Error dialog should be shown and False should be returned + """ + error = VehicleProjectOpenError(title="Error", message="Cannot open vehicle directory") + mock_callback = MagicMock(side_effect=error) + + with patch("ardupilot_methodic_configurator.frontend_tkinter_directory_selection.show_tooltip"): + mock_parent = MagicMock() + parent_frame = ttk.Frame(root) + + widget = VehicleDirectorySelectionWidgets( + parent=mock_parent, + parent_frame=parent_frame, + initial_dir="/initial", + destroy_parent_on_open=False, + on_select_directory_callback=mock_callback, + ) + + with ( + patch("tkinter.filedialog.askdirectory", return_value="/selected/dir"), + patch("tkinter.messagebox.showerror") as mock_error_dialog, + ): + result = widget.on_select_directory() + + assert result is False + mock_error_dialog.assert_called_once_with("Error", "Cannot open vehicle directory") diff --git a/tests/test_frontend_tkinter_rich_text.py b/tests/test_frontend_tkinter_rich_text.py index 6e336b0a7..fd32a23f3 100755 --- a/tests/test_frontend_tkinter_rich_text.py +++ b/tests/test_frontend_tkinter_rich_text.py @@ -16,7 +16,11 @@ from tkinter import ttk from unittest.mock import MagicMock, patch -from ardupilot_methodic_configurator.frontend_tkinter_rich_text import RichText, get_widget_font_family_and_size +from ardupilot_methodic_configurator.frontend_tkinter_rich_text import ( + RichText, + _get_ttk_label_color, + get_widget_font_family_and_size, +) class TestRichText(unittest.TestCase): @@ -200,3 +204,102 @@ def test_get_widget_font_family_and_size(self) -> None: if __name__ == "__main__": unittest.main() + + +class TestRichTextColorAndFontFallbacks(unittest.TestCase): + """Tests for color and font fallback behavior in RichText.""" + + def setUp(self) -> None: + self.root = tk.Tk() + self.root.withdraw() + + def tearDown(self) -> None: + self.root.update_idletasks() + self.root.destroy() + + def test_user_can_create_rich_text_with_explicit_background_color(self) -> None: + """ + User can create RichText with an explicit background color. + + GIVEN: User wants a custom background color for the text widget + WHEN: RichText is created with a background kwarg + THEN: The background kwarg should skip the auto-detection + AND: Widget should use the provided background color + """ + rich_text = RichText(self.root, background="red") + assert rich_text.cget("background") == "red" + + def test_user_can_create_rich_text_with_bg_shorthand(self) -> None: + """ + User can create RichText with bg shorthand. + + GIVEN: User wants a custom background color using 'bg' alias + WHEN: RichText is created with bg kwarg + THEN: The bg kwarg should skip the auto-detection + AND: Widget should use the provided background color + """ + rich_text = RichText(self.root, bg="blue") + # Color may be returned as RGB by Tk on some platforms + assert rich_text.cget("background") + + def test_user_can_create_rich_text_with_explicit_foreground_color(self) -> None: + """ + User can create RichText with an explicit foreground color. + + GIVEN: User wants a custom foreground color + WHEN: RichText is created with foreground kwarg + THEN: The foreground kwarg should skip auto-detection + AND: Widget should use provided foreground color + """ + rich_text = RichText(self.root, foreground="green") + assert rich_text.cget("foreground") + + def test_user_can_create_rich_text_with_fg_shorthand(self) -> None: + """ + User can create RichText with fg shorthand. + + GIVEN: User wants a custom foreground color using 'fg' alias + WHEN: RichText is created with fg kwarg + THEN: Widget should be created successfully + """ + rich_text = RichText(self.root, fg="purple") + assert rich_text.cget("foreground") + + def test_user_can_create_rich_text_when_safe_font_nametofont_returns_none(self) -> None: + """ + User can create RichText even when safe_font_nametofont returns None. + + GIVEN: The font lookup fails (returns None) + WHEN: RichText is created without an explicit font + THEN: Should fall back to get_safe_font_config for font configuration + AND: Widget should be created successfully + """ + with patch("ardupilot_methodic_configurator.frontend_tkinter_rich_text.safe_font_nametofont", return_value=None): + rich_text = RichText(self.root) + + assert rich_text is not None + assert "bold" in rich_text.tag_names() + assert "italic" in rich_text.tag_names() + assert "h1" in rich_text.tag_names() + + def test_get_ttk_label_color_handles_cget_tclerror(self) -> None: + """ + _get_ttk_label_color handles TclError from widget.cget gracefully. + + GIVEN: A widget where cget raises TclError for the requested option + WHEN: _get_ttk_label_color is called + THEN: TclError should be caught and fallback color should be used + """ + mock_widget = MagicMock() + mock_widget.cget.side_effect = tk.TclError("Unknown option") + + # Mock style lookup to return empty so we enter the fallback branch + with patch("ardupilot_methodic_configurator.frontend_tkinter_rich_text.ttk.Style") as mock_style_class: + mock_style = MagicMock() + mock_style.lookup.return_value = "" # Empty = no style found + mock_style_class.return_value = mock_style + + result = _get_ttk_label_color(mock_widget, "background", "white") + + # Should use fallback since cget raised TclError + assert result == "white" diff --git a/tests/unit_backend_filesystem_vehicle_components.py b/tests/unit_backend_filesystem_vehicle_components.py index f1696e812..f8ff03dc4 100755 --- a/tests/unit_backend_filesystem_vehicle_components.py +++ b/tests/unit_backend_filesystem_vehicle_components.py @@ -335,3 +335,271 @@ def test_recursively_clear_dict_non_dict_input(self) -> None: none_input = None self.vehicle_components._recursively_clear_dict(none_input) assert none_input is None + + +class TestVehicleComponentsTemplateLoadingErrors: + """Unit tests for VehicleComponents template loading error handling.""" + + @pytest.fixture(autouse=True) + def setup(self) -> None: + """Set up test fixtures.""" + self.vehicle_components = VehicleComponents() + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_system_templates_file_not_found(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_system_templates handles FileNotFoundError gracefully. + + GIVEN: System templates file doesn't exist + WHEN: Loading system templates + THEN: Should return empty dict without crashing + """ + mock_get_base_dir.return_value = "/app/path" + mock_file.side_effect = FileNotFoundError("File not found") + + result = self.vehicle_components._load_system_templates() + + assert result == {} + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_system_templates_os_error(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_system_templates handles OSError gracefully. + + GIVEN: OS error occurs while reading system templates file + WHEN: Loading system templates + THEN: Should return empty dict without crashing + """ + mock_get_base_dir.return_value = "/app/path" + mock_file.side_effect = OSError("Permission denied") + + result = self.vehicle_components._load_system_templates() + + assert result == {} + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_system_templates_unexpected_exception(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_system_templates handles unexpected exceptions gracefully. + + GIVEN: An unexpected exception occurs while reading system templates + WHEN: Loading system templates + THEN: Should return empty dict without crashing + """ + mock_get_base_dir.return_value = "/app/path" + mock_file.side_effect = RuntimeError("Unexpected error") + + result = self.vehicle_components._load_system_templates() + + assert result == {} + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_user_templates_file_not_found(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_user_templates handles FileNotFoundError gracefully. + + GIVEN: User templates file doesn't exist + WHEN: Loading user templates + THEN: Should return empty dict without crashing (debug message only) + """ + mock_get_base_dir.return_value = "/user/templates" + mock_file.side_effect = FileNotFoundError("File not found") + + result = self.vehicle_components._load_user_templates() + + assert result == {} + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_user_templates_os_error(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_user_templates handles OSError gracefully. + + GIVEN: OS error occurs while reading user templates file + WHEN: Loading user templates + THEN: Should return empty dict without crashing + """ + mock_get_base_dir.return_value = "/user/templates" + mock_file.side_effect = OSError("Disk I/O error") + + result = self.vehicle_components._load_user_templates() + + assert result == {} + + @patch("ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir") + @patch("builtins.open", new_callable=mock_open) + @patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.json_load") + def test_load_user_templates_unexpected_exception(self, mock_json_load, mock_file, mock_get_base_dir) -> None: + """ + _load_user_templates handles unexpected exceptions gracefully. + + GIVEN: An unexpected exception occurs while reading user templates + WHEN: Loading user templates + THEN: Should return empty dict without crashing + """ + mock_get_base_dir.return_value = "/user/templates" + mock_file.side_effect = RuntimeError("Unexpected error") + + result = self.vehicle_components._load_user_templates() + + assert result == {} + + +class TestVehicleComponentsTemplateMerging: + """Tests for template merging behavior.""" + + @pytest.fixture(autouse=True) + def setup(self) -> None: + """Set up test fixtures.""" + self.vehicle_components = VehicleComponents() + + def test_load_component_templates_handles_user_template_without_name(self) -> None: + """ + load_component_templates skips user templates without a name. + + GIVEN: User templates containing entries without a 'name' field + WHEN: Loading component templates + THEN: Templates without names should be skipped silently + AND: Named templates should still be loaded correctly + """ + system_templates = {"Motor": [{"name": "Standard 2205", "data": {"kv": 2300}}]} + user_templates = { + "Motor": [ + {"data": {"kv": 1500}}, # No 'name' field - should be skipped + {"name": "", "data": {"kv": 1800}}, # Empty name - should be skipped + {"name": "Custom Motor", "data": {"kv": 2000}}, # Valid - should be added + ] + } + + with ( + patch.object(self.vehicle_components, "_load_system_templates", return_value=system_templates), + patch.object(self.vehicle_components, "_load_user_templates", return_value=user_templates), + ): + result = self.vehicle_components.load_component_templates() + + # Valid user template should be present + assert "Motor" in result + motor_names = [t.get("name") for t in result["Motor"]] + assert "Custom Motor" in motor_names + # Unnamed templates should NOT be present + assert None not in motor_names + assert "" not in motor_names + + def test_save_component_templates_to_file_fallback_when_system_file_not_found(self) -> None: + """ + save_component_templates_to_file uses package path when system path doesn't exist. + + GIVEN: Saving to system templates and system path doesn't exist on filesystem + WHEN: save_component_templates_to_file is called + THEN: Should fall back to package path + AND: Should not raise an exception + """ + vehicle_components_system = VehicleComponents(save_component_to_system_templates=True) + templates_to_save = {"Component1": [{"name": "Test", "data": {}}]} + + with ( + patch( + "ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir" + ) as mock_get_base_dir, + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.os_path.exists") as mock_exists, + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.os_makedirs"), + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.safe_write"), + ): + mock_get_base_dir.return_value = "/nonexistent/path" + mock_exists.return_value = False # System file does NOT exist at primary location + + result = vehicle_components_system.save_component_templates_to_file(templates_to_save) + assert isinstance(result, tuple) + assert len(result) == 2 + + def test_save_component_templates_to_file_handles_oserror_in_makedirs_for_system_path(self) -> None: + """ + save_component_templates_to_file handles OSError when checking system path. + + GIVEN: An OSError occurs when checking for the system template path + WHEN: save_component_templates_to_file is called for system templates + THEN: Should handle the error gracefully and continue + AND: Should fall back to default templates_dir + """ + vehicle_components_system = VehicleComponents(save_component_to_system_templates=True) + templates_to_save = {"Component1": [{"name": "Test", "data": {}}]} + + with ( + patch( + "ardupilot_methodic_configurator.backend_filesystem_program_settings.ProgramSettings.get_templates_base_dir" + ) as mock_get_base_dir, + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.os_path.exists") as mock_exists, + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.os_makedirs") as mock_makedirs, + patch("ardupilot_methodic_configurator.backend_filesystem_vehicle_components.safe_write"), + ): + mock_get_base_dir.return_value = "/test/templates" + mock_exists.side_effect = OSError("Permission denied") + mock_makedirs.return_value = None + + # Should not raise, should return a result + result = vehicle_components_system.save_component_templates_to_file(templates_to_save) + assert isinstance(result, tuple) + + +class TestVehicleComponentsWipeInfo: + """Tests for wipe_component_info behavior.""" + + def test_wipe_component_info_handles_none_data(self) -> None: + """ + wipe_component_info does nothing when vehicle_components_fs.data is None. + + GIVEN: vehicle_components_fs has no data (data is None) + WHEN: wipe_component_info is called + THEN: No exception should be raised + AND: Nothing should be modified + """ + vehicle_components = VehicleComponents() + vehicle_components.vehicle_components_fs.data = None + + # Should not raise any exception + vehicle_components.wipe_component_info() + + assert vehicle_components.vehicle_components_fs.data is None + + def test_merge_defaults_applies_default_when_key_missing(self) -> None: + """ + merge_defaults sets default when key is not in target. + + GIVEN: A target dict missing a key that has a default value + WHEN: wipe_component_info is called to reset to defaults + THEN: The missing key should be set to its default value + """ + vehicle_components = VehicleComponents() + # Provide minimal data structure that wipe_component_info can work with + data = { + "Components": { + "RC Receiver": {}, # Missing FC Connection + "Battery": { + "Specifications": {} # Missing Chemistry and other defaults + }, + "Motors": { + "Specifications": {} # Missing Poles + }, + "GNSS Receiver": {}, + "Telemetry": {}, + "Battery Monitor": {}, + "ESC": {}, + } + } + vehicle_components.vehicle_components_fs.data = data + + vehicle_components.wipe_component_info() + + # After wipe, defaults should have been applied + components = vehicle_components.vehicle_components_fs.data["Components"] + assert "FC Connection" in components["RC Receiver"] + assert "FC Connection" in components["GNSS Receiver"]