test(param editor table): add more BDD tests

amilcarlucas · amilcarlucas · commit d09ceca5e912 · 2025-11-05T21:31:21.000+01:00
diff --git a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py
@@ -327,6 +327,58 @@ def _create_value_different_label(self, param: ArduPilotParameter) -> ttk.Label:
         """Create a label indicating if the new value is different from current FC value."""
         return ttk.Label(self.view_port, text=NEW_VALUE_DIFFERENT_STR if param.is_different_from_fc else " ")
 
+    def _handle_parameter_value_update(  # pylint: disable=too-many-return-statements # noqa: PLR0911
+        self, param: ArduPilotParameter, new_value: str, include_range_check: bool = True
+    ) -> bool:
+        """
+        Handle parameter value updates with consistent error handling.
+
+        This method centralizes the error handling logic for parameter value updates,
+        including user confirmation for out-of-range values.
+
+        Args:
+            param: The ArduPilotParameter to update
+            new_value: The new value to set (as string)
+            include_range_check: Whether to perform range checking and ask user for confirmation
+
+        Returns:
+            True if the value was successfully updated, False otherwise
+
+        """
+        try:
+            # Attempt to set the new value; the model will validate it
+            param.set_new_value(new_value)
+            return True
+        except ParameterUnchangedError:
+            # Valid but no change — just refresh the UI and do not mark edited
+            return False
+        except ParameterOutOfRangeError as oor:
+            # User-visible warning from model about out-of-range value
+            if not include_range_check:
+                # Caller doesn't want range checking, treat as error
+                logging_exception(_("Parameter %s out of range: %s"), param.name, oor)
+                messagebox.showerror(_("Out-of-range value"), str(oor))
+                return False
+
+            # Ask the user if they want to accept the out-of-range value
+            msg = str(oor) + _(" Use out-of-range value?")
+            if messagebox.askyesno(_("Out-of-range value"), msg, icon="warning"):
+                # Retry accepting the value while telling the model to ignore range checks
+                try:
+                    param.set_new_value(new_value, ignore_out_of_range=True)
+                    return True
+                except (ValueError, TypeError) as exc:
+                    # Even with ignore_out_of_range, the value might be invalid
+                    logging_exception(_("Could not set parameter %s to %s: %s"), param.name, new_value, exc)
+                    messagebox.showerror(_("Invalid value"), str(exc))
+                    return False
+            return False
+        except (ValueError, TypeError) as exc:
+            # Invalid input according to model
+            logging_exception(_("Invalid value for %s: %s"), param.name, exc)
+            messagebox.showerror(_("Invalid value"), str(exc))
+            return False
+
     def _update_combobox_style_on_selection(  # pylint: disable=too-many-arguments, too-many-positional-arguments
         self,
         combobox_widget: PairTupleCombobox,
@@ -337,17 +389,9 @@ def _update_combobox_style_on_selection(  # pylint: disable=too-many-arguments,
     ) -> None:
         """Update the combobox style based on selection."""
         new_value_str = combobox_widget.get_selected_key() or ""
-        try:
-            # Pass the string to the domain model; it will validate and raise on error
-            param.set_new_value(new_value_str)
-        except ParameterUnchangedError:
-            # valid but no change; just refresh style
-            pass
-        except (ValueError, TypeError) as exc:  # user provided invalid input
-            msg = _("Could not apply the selected value: {new_value_str}").format(new_value_str=new_value_str)
-            logging_exception(msg, exc)
-            messagebox.showerror(_("Error"), str(exc))
-        else:
+
+        # Use centralized error handling for parameter value updates
+        if self._handle_parameter_value_update(param, new_value_str, include_range_check=False):
             # Success: mark edited and sync the ArduPilotParameter back to filesystem
             show_tooltip(change_reason_widget, param.tooltip_change_reason)
             value_is_different.config(text=NEW_VALUE_DIFFERENT_STR if param.is_different_from_fc else " ")
@@ -465,26 +509,8 @@ def _on_parameter_value_change(event: tk.Event) -> None:
             if hasattr(event, "type") and event.type == tk.EventType.KeyPress:  # KeyPress event (Return/Enter)
                 self._last_return_values[event.widget] = new_value
 
-            valid = True
-
-            try:
-                # first attempt: let the model validate the provided string
-                # (it will convert/validate as required)
-                param.set_new_value(new_value)
-            except ParameterOutOfRangeError as oor:  # user-visible warning from model
-                # Ask the user if they want to accept the out-of-range value
-                if messagebox.askyesno(_("Out-of-range value"), str(oor) + _(" Use out-of-range value?"), icon="warning"):
-                    # Retry accepting the value while telling the model to ignore range checks
-                    param.set_new_value(new_value, ignore_out_of_range=True)
-                else:
-                    valid = False
-            except ParameterUnchangedError:
-                # Valid but no change — just refresh the UI and do not mark edited
-                valid = False
-            except (ValueError, TypeError) as exc:  # invalid input according to model
-                logging_exception(_("Invalid value for %s: %s"), param.name, exc)
-                messagebox.showerror(_("Invalid value"), str(exc))
-                valid = False
+            # Use centralized error handling for parameter value updates
+            valid = self._handle_parameter_value_update(param, new_value, include_range_check=True)
 
             if valid:
                 logging_debug(_("Parameter %s changed, will later ask if change(s) should be saved to file."), param.name)
@@ -538,24 +564,9 @@ def on_close() -> None:
                 )
                 return
 
-            valid = True
-            # Update the parameter value and entry text
-            try:
-                param.set_new_value(BitmaskHelper.get_value_from_keys(checked_keys))
-            except ParameterOutOfRangeError as oor:  # user-visible warning from model
-                # Ask the user if they want to accept the out-of-range value
-                if messagebox.askyesno(_("Unknown bit set"), str(oor) + _(" Use out-of-range value?"), icon="warning"):
-                    # Retry accepting the value while telling the model to ignore range checks
-                    param.set_new_value(BitmaskHelper.get_value_from_keys(checked_keys), ignore_out_of_range=True)
-                else:
-                    valid = False
-            except ParameterUnchangedError:
-                # Valid but no change — just refresh the UI and do not mark edited
-                valid = False
-            except (ValueError, TypeError) as exc:  # invalid input according to model
-                logging_exception(_("Could not set bitmask value for %s: %s"), param.name, exc)
-                messagebox.showerror(_("Error"), str(exc))
-                valid = False
+            # Use centralized error handling for parameter value updates
+            bitmask_value = BitmaskHelper.get_value_from_keys(checked_keys)
+            valid = self._handle_parameter_value_update(param, str(bitmask_value), include_range_check=True)
 
             if valid:
                 show_tooltip(change_reason_widget, param.tooltip_change_reason)
diff --git a/tests/gui_frontend_tkinter_parameter_editor_table.py b/tests/gui_frontend_tkinter_parameter_editor_table.py
@@ -23,13 +23,52 @@
 from conftest import PARAMETER_EDITOR_TABLE_HEADERS_ADVANCED, PARAMETER_EDITOR_TABLE_HEADERS_SIMPLE
 
 from ardupilot_methodic_configurator.configuration_manager import ConfigurationManager
-from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter
+from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter, Par
+from ardupilot_methodic_configurator.data_model_par_dict import ParDict
 from ardupilot_methodic_configurator.frontend_tkinter_pair_tuple_combobox import PairTupleCombobox
 from ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table import ParameterEditorTable
 
 # pylint: disable=protected-access
 
 
+def create_mock_data_model_ardupilot_parameter(  # pylint: disable=too-many-arguments,too-many-positional-arguments  # noqa: PLR0913
+    name: str = "TEST_PARAM",
+    value: float = 1.0,
+    default_value: float | None = None,
+    comment: str = "test comment",
+    metadata: dict | None = None,
+    fc_value: float | None = None,
+    is_forced: bool = False,
+    is_calibration: bool = False,
+    is_readonly: bool = False,
+    min_value: float | None = None,
+    max_value: float | None = None,
+) -> ArduPilotParameter:
+    """Create a mock ArduPilotParameter for testing in GUI workflows."""
+    metadata = metadata or {}
+
+    if is_calibration:
+        metadata["Calibration"] = True
+    if is_readonly:
+        metadata["ReadOnly"] = True
+    if min_value is not None:
+        metadata["min"] = min_value
+    if max_value is not None:
+        metadata["max"] = max_value
+
+    metadata.setdefault("unit", "")
+    metadata.setdefault("doc_tooltip", "Test tooltip")
+    metadata.setdefault("unit_tooltip", "Unit tooltip")
+
+    par_obj = Par(value, comment)
+    default_par = Par(default_value if default_value is not None else 0.0, "default")
+    forced_par = Par(value, "forced comment") if is_forced else None
+
+    return ArduPilotParameter(
+        name=name, par_obj=par_obj, metadata=metadata, default_par=default_par, fc_value=fc_value, forced_par=forced_par
+    )
+
+
 class TestParameterEditorTableUserWorkflows:
     """Test user workflows and behaviors for ParameterEditorTable GUI components."""
 
@@ -354,3 +393,104 @@ def test_user_can_work_with_fully_populated_parameter_table(self, parameter_tabl
         verify the building blocks work correctly.
         """
         pytest.skip("Full table population requires complex parameter data setup - focus on component testing instead")
+
+    def test_user_can_edit_multiple_parameters_in_complete_workflow(self, parameter_table: ParameterEditorTable) -> None:
+        """
+        User can manage multiple parameters with visual indicators throughout workflow.
+
+        GIVEN: A user has multiple parameters with different states
+        WHEN: Parameters have different values (default vs changed)
+        THEN: Visual indicators show parameter states correctly
+        AND: Each parameter maintains independent state
+        AND: The system handles multiple parameter contexts simultaneously
+        """
+        # Arrange: Create parameters with different value states
+        param_default = create_mock_data_model_ardupilot_parameter(
+            name="PARAM_DEFAULT",
+            value=10.0,
+            default_value=10.0,  # Same as default
+        )
+        param_changed = create_mock_data_model_ardupilot_parameter(
+            name="PARAM_CHANGED",
+            value=20.0,
+            default_value=15.0,  # Different from default
+        )
+
+        # Verify: Parameters have correct comparison states
+        assert param_default.new_value_equals_default_value is True  # Default
+        assert param_changed.new_value_equals_default_value is False  # Changed
+
+        # Verify: Configuration manager can handle multiple parameters
+        assert parameter_table.configuration_manager.current_file == "04_board_orientation.param"
+        assert parameter_table.configuration_manager.is_fc_connected is False
+
+    def test_user_can_switch_between_gui_complexity_modes_seamlessly(self, parameter_table: ParameterEditorTable) -> None:
+        """
+        User can work with different GUI complexity modes.
+
+        GIVEN: A user switches between GUI complexity modes
+        WHEN: The table needs to adapt to show/hide upload column
+        THEN: Upload column visibility changes based on complexity level
+        AND: Simple mode hides advanced features
+        AND: Advanced/Expert modes show full functionality
+        """
+        # Verify: Simple mode hides upload column
+        assert parameter_table._should_show_upload_column("simple") is False
+
+        # Verify: Advanced mode shows upload column
+        assert parameter_table._should_show_upload_column("advanced") is True
+
+        # Verify: Expert mode shows upload column
+        assert parameter_table._should_show_upload_column("expert") is True
+
+        # Verify: Change reason column index adapts to upload column visibility
+        change_reason_idx_simple = parameter_table._get_change_reason_column_index(show_upload_column=False)
+        change_reason_idx_advanced = parameter_table._get_change_reason_column_index(show_upload_column=True)
+
+        # Verify: Column index is one less without upload column
+        assert change_reason_idx_advanced == change_reason_idx_simple + 1
+
+    def test_user_recovers_gracefully_from_validation_errors(self, parameter_table: ParameterEditorTable) -> None:
+        """
+        User receives clear feedback for validation errors and can recover.
+
+        GIVEN: A user enters parameter values
+        WHEN: Values are outside allowed ranges
+        THEN: System provides clear error handling
+        AND: Valid values are accepted
+        AND: Invalid values trigger appropriate error responses
+        """
+        # Arrange: Create parameter with validation constraints
+        param_constrained = create_mock_data_model_ardupilot_parameter(
+            name="CONSTRAINED_PARAM", value=50.0, default_value=50.0, min_value=0.0, max_value=100.0
+        )
+
+        # Configure test file parameters
+        parameter_table.configuration_manager._local_filesystem.file_parameters = ParDict(
+            {"04_board_orientation.param": ParDict({"CONSTRAINED_PARAM": Par(50.0, "constrained")})}
+        )
+
+        # Act & Verify: Attempt out-of-range value with rejection
+        with patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.messagebox") as mock_msgbox:
+            mock_msgbox.askyesno.return_value = False  # User rejects invalid value
+
+            result_invalid = parameter_table._handle_parameter_value_update(
+                param_constrained,
+                "150.0",  # Out of range
+                include_range_check=True,
+            )
+
+            # Verify: Invalid value rejected
+            assert result_invalid is False
+            mock_msgbox.askyesno.assert_called_once()
+
+        # Act & Verify: Valid value accepted
+        with patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.show_tooltip"):
+            result_valid = parameter_table._handle_parameter_value_update(
+                param_constrained,
+                "75.0",  # Valid value within range
+                include_range_check=True,
+            )
+
+            # Verify: Valid value accepted
+            assert result_valid is True
diff --git a/tests/test_frontend_tkinter_parameter_editor_table.py b/tests/test_frontend_tkinter_parameter_editor_table.py
