test: add unit branch coverage for FC commands/params and split safe_write test helpers

- Add unit branch tests for FlightControllerCommands failure/missing-connection paths
- Add unit tests for parameter download edge cases (MAVLink/MAVFTP timeout, exception, progress callback)
- Add shared fixture for connected mock master in conftest
- Move make_capture_safe_write test helper out of production code into tests
- Add bdd safe_write behavior tests and helper-focused unit tests
- Update vehicle components tests to import helper from tests module
- Ignore tuning_report.csv in gitignore

Agent-Logs-Url: https://github.com/ArduPilot/MethodicConfigurator/sessions/20139120-bd7b-4efe-8256-126ef58e373b

Co-authored-by: amilcarlucas <24453563+amilcarlucas@users.noreply.github.com>

Author: Copilot

.gitignore

@@ -23,3 +23,5 @@ test.xml
 sitl/arducopter
 sitl/firmware-version.txt
 sitl/git-version.txt
+
+tuning_report.csv

ardupilot_methodic_configurator/backend_safe_file_io.py

@@ -9,12 +9,10 @@
 SPDX-License-Identifier: GPL-3.0-or-later
 """
 
-import io
-import json
 import os
 import tempfile
 from contextlib import suppress
-from typing import IO, Any, Callable
+from typing import IO, Callable
 
 
 def safe_write(filepath: str, write_func: Callable[[IO[str]], object]) -> None:
@@ -75,38 +73,3 @@ def safe_write(filepath: str, write_func: Callable[[IO[str]], object]) -> None:
         if not replaced:
             with suppress(OSError):
                 os.unlink(tmp_path)
-
-
-# ==================== SAFE-WRITE TEST HELPERS ====================
-
-
-def make_capture_safe_write() -> tuple[  # pragma: no cover
-    dict[str, Any], list[bool], Callable[[str, Callable[[IO[str]], object]], None]
-]:
-    """
-    Create a ``safe_write`` side-effect that captures written JSON data.
-
-    Returns a (captured_data, called, side_effect) triple:
-    - ``captured_data``: dict updated with the JSON that the write_func produces.
-    - ``called``: single-element list (``[False]``) flipped to ``True`` on invocation.
-    - ``side_effect``: function to assign to ``mock_safe_write.side_effect``.
-
-    Usage::
-
-        captured_data, called, side_effect = make_capture_safe_write()
-        mock_safe_write.side_effect = side_effect
-        ...
-        assert called[0]
-        assert captured_data == expected
-
-    """
-    captured_data: dict[str, Any] = {}
-    called: list[bool] = [False]
-
-    def _capture(_filepath: str, write_func: Callable[[IO[str]], object]) -> None:
-        fake_file = io.StringIO()
-        write_func(fake_file)
-        captured_data.update(json.loads(fake_file.getvalue()))
-        called[0] = True
-
-    return captured_data, called, _capture

tests/bdd_backend_safe_file_io.py

@@ -0,0 +1,227 @@
+#!/usr/bin/env python3
+
+"""
+BDD-style 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 <amilcar.lucas@iav.de>
+
+SPDX-License-Identifier: GPL-3.0-or-later
+"""
+
+import io
+import json
+import os
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+
+from ardupilot_methodic_configurator.backend_safe_file_io import 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_user_can_write_unicode_content(self, tmp_file: Path) -> None:
+        """
+        User can write Unicode content to a file using safe_write.
+
+        GIVEN: Text content containing non-ASCII Unicode characters
+        WHEN: User writes the content via safe_write
+        THEN: File should be readable with the correct Unicode content preserved
+        """
+        unicode_content = "Héllo Wörld — 日本語テスト\n"
+
+        safe_write(str(tmp_file), lambda f: f.write(unicode_content))
+
+        assert tmp_file.read_text(encoding="utf-8") == unicode_content
+
+    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 not tmp_files, f"Found leftover temp files: {tmp_files}"
+
+
+class TestSafeWriteFilePermissions:
+    """Tests for safe_write file permission handling."""
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX permission bits are not meaningful on Windows")
+    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)
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX permission bits are not meaningful on Windows")
+    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 not 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
+        """
+        with patch(
+            "ardupilot_methodic_configurator.backend_safe_file_io.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"

tests/conftest.py

@@ -23,7 +23,7 @@
 from collections.abc import Callable, Generator
 from pathlib import Path
 from typing import Any, NamedTuple, Optional
-from unittest.mock import patch
+from unittest.mock import MagicMock, Mock, patch
 
 import pyautogui
 import pytest
@@ -36,6 +36,26 @@
 from ardupilot_methodic_configurator.frontend_tkinter_base_window import BaseWindow
 from ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table import NEW_VALUE_DIFFERENT_STR
 
+# ==================== SHARED FLIGHT CONTROLLER FIXTURES ====================
+
+
+@pytest.fixture
+def mock_connected_master() -> tuple[MagicMock, Mock]:
+    """
+    Provide a connected mock master and connection manager for flight controller tests.
+
+    Returns (mock_master, mock_conn_mgr) where mock_master has target_system=1 and
+    target_component=1 pre-configured. Tests that require different system/component IDs
+    should override those attributes directly after unpacking.
+    """
+    mock_master = MagicMock()
+    mock_master.target_system = 1
+    mock_master.target_component = 1
+    mock_conn_mgr = Mock()
+    mock_conn_mgr.master = mock_master
+    return mock_master, mock_conn_mgr
+
+
 # ==================== SHARED TKINTER TESTING CONFIGURATION ====================
 
 

tests/test_backend_filesystem_vehicle_components.py

@@ -15,9 +15,9 @@
 from unittest.mock import mock_open, patch
 
 import pytest
+from test_backend_safe_file_io_write_capture import make_capture_safe_write
 
 from ardupilot_methodic_configurator.backend_filesystem_vehicle_components import VehicleComponents
-from ardupilot_methodic_configurator.backend_safe_file_io import make_capture_safe_write
 from ardupilot_methodic_configurator.data_model_template_overview import TemplateOverview
 
 # pylint: disable=too-many-lines,too-many-public-methods,attribute-defined-outside-init