refactor(parameter editor): do not unnecessarily reset the FC#1348
refactor(parameter editor): do not unnecessarily reset the FC#1348amilcarlucas merged 3 commits intomasterfrom
Conversation
No need to reset the FC if SID_AXIS changes, only when it changes from 0 to something else
There was a problem hiding this comment.
Pull request overview
Refactors the parameter reset workflow to avoid unnecessarily prompting for/triggering a flight controller reset when SID_AXIS changes between non-zero values, and adds tests covering the intended SID_AXIS edge cases.
Changes:
- Updates
upload_parameters_that_require_reset_workflowsoSID_AXISonly triggers the “possible reset” path when moving from0(or missing) to a non-zero value. - Adds test cases for
SID_AXIStransitions:0→nonzero,nonzero→nonzero,nonzero→0, and missing→nonzero.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_data_model_parameter_editor.py | Adds unit tests validating the new SID_AXIS reset behavior. |
| ardupilot_methodic_configurator/data_model_parameter_editor.py | Refines the reset-trigger condition for SID_AXIS to only apply on 0→non-zero transitions. |
| mock_ask_confirmation = MagicMock(return_value=True) | ||
| mock_show_error = MagicMock() |
There was a problem hiding this comment.
These new tests set up mock_ask_confirmation, but the assertions don’t verify whether the confirmation callback was invoked (or not) for each scenario. Adding explicit assertions like mock_ask_confirmation.assert_called_once() for the reset-triggering cases, and mock_ask_confirmation.assert_not_called() for the non-reset cases would make the tests more precise and prevent regressions where the UI prompt behavior changes unintentionally.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Amilcar Lucas <amilcar.lucas@iav.de>
|
@amilcarlucas I've opened a new pull request, #1350, to work on those changes. Once the pull request is ready, I'll request review from you. |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Test Results 2 files 2 suites 28m 16s ⏱️ Results for commit 8e8d0ca. |
Co-authored-by: amilcarlucas <24453563+amilcarlucas@users.noreply.github.com>
No need to reset the FC if SID_AXIS changes, only when it changes from 0 to something else