-
Notifications
You must be signed in to change notification settings - Fork 50
Fix test regressions #1206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix test regressions #1206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses test regressions by removing outdated tests, configuring warning filters, consolidating type checker configuration, and adjusting Python version support for CI testing.
Changes:
- Removed obsolete
TestParameterAdditionWorkflowstest class that was testing an old implementation of parameter addition workflows - Added matplotlib deprecation warning filter to pytest.ini to suppress noisy warnings during test runs
- Removed redundant
pyrightconfig.jsonfile since pyright configuration already exists inpyproject.toml - Added extensive file exclusion lists to ruff and pyright configurations in
pyproject.toml - Changed CI testing from Python 3.9/3.14/3.14t to Python 3.9/3.13
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_frontend_tkinter_parameter_editor_table.py | Removes outdated test class that was testing an old implementation of parameter addition dialog |
| pytest.ini | Adds deprecation warning filter for matplotlib to reduce test output noise |
| pyrightconfig.json | Completely removes file as configuration is consolidated in pyproject.toml |
| pyproject.toml | Adds extensive exclusion lists for both ruff linting and pyright type checking |
| .github/workflows/pytest.yml | Changes Python versions tested in CI from 3.9/3.14/3.14t to 3.9/3.13 |
|
|
||
|
|
||
| class TestParameterAdditionWorkflows: | ||
| """Cover add-parameter dialog flows and error handling.""" | ||
|
|
||
| def test_on_parameter_add_invokes_confirmation_handler(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.get_possible_add_param_names.return_value = ["NEW"] | ||
| parameter_editor_table._confirm_parameter_addition = MagicMock(return_value=True) | ||
| mock_window = MagicMock() | ||
| mock_window.root = MagicMock() | ||
| mock_window.main_frame = MagicMock() | ||
| entry_widget = MagicMock() | ||
| entry_widget.get.return_value = "NEW" | ||
|
|
||
| with ( | ||
| patch( | ||
| "ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.BaseWindow", return_value=mock_window | ||
| ), | ||
| patch( | ||
| "ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.EntryWithDynamicalyFilteredListbox", | ||
| return_value=entry_widget, | ||
| ), | ||
| ): | ||
| parameter_editor_table._on_parameter_add() | ||
|
|
||
| handler = entry_widget.bind.call_args_list[0][0][1] | ||
| handler(SimpleNamespace(widget=entry_widget)) | ||
| parameter_editor_table._confirm_parameter_addition.assert_called_with("NEW") | ||
|
|
||
| def test_on_parameter_add_handles_operation_not_possible(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.get_possible_add_param_names.side_effect = OperationNotPossibleError("nope") | ||
| parameter_editor_table._on_parameter_add() | ||
| parameter_editor_table._dialogs.show_error.assert_called_once() | ||
|
|
||
| def test_confirm_parameter_addition_handles_errors(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.add_parameter_to_current_file.side_effect = InvalidParameterNameError("bad") | ||
| assert parameter_editor_table._confirm_parameter_addition("bad") is False | ||
| parameter_editor_table._dialogs.show_error.assert_called() | ||
|
|
||
| parameter_editor_table._dialogs.show_error.reset_mock() | ||
| parameter_editor_table.parameter_editor.add_parameter_to_current_file.side_effect = OperationNotPossibleError("ops") | ||
| assert parameter_editor_table._confirm_parameter_addition("bad") is False | ||
| parameter_editor_table._dialogs.show_error.assert_called() | ||
|
|
||
|
|
||
| class TestUploadSelectionBehavior: |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed test test_confirm_parameter_addition_handles_errors was covering error handling for OperationNotPossibleError in the _confirm_parameter_addition() method. While other tests cover InvalidParameterNameError, there is no remaining test coverage for the OperationNotPossibleError exception handling at lines 982-984 of ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py.
This exception can be raised in certain scenarios (e.g., when operations are not possible due to system state), and the error handling should be tested to ensure users see appropriate error messages.
.github/workflows/pytest.yml
Outdated
| os: [ubuntu-latest] | ||
| python-version: ["3.9", "3.14", "3.14t"] | ||
| #python-version: ["3.9", "3.14", "3.14t"] | ||
| python-version: ["3.9", "3.13"] |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR removes testing for Python 3.14 and 3.14t, but the pyproject.toml file still declares support for Python 3.14 in the classifiers (line 39) and includes version-specific dependencies for Python 3.14 (lines 51-52). This creates an inconsistency where the project claims to support Python 3.14 but doesn't test it in CI.
Consider either:
- Removing Python 3.14 from the classifiers and version-specific dependencies in pyproject.toml if it's not officially supported yet
- Re-enabling Python 3.14 testing once the version is stable and available in GitHub Actions
| python-version: ["3.9", "3.13"] | |
| python-version: ["3.9", "3.13", "3.14", "3.14t"] |
| "scripts/test_gui_environment.py", | ||
| "svg_comparison_demo.py", | ||
| "scripts/show_svg_tk_variants.py", | ||
| "scripts/pycairo_render.py", | ||
| "scripts/cairosvg_test.py", | ||
| "scripts/cairo_svg_tkinter_demo.py", | ||
| "scripts/cairo_svg_renderer.py", | ||
| "post_install.py", | ||
| "param_zip.py", | ||
| "param_filter.py", | ||
| "mavproxy_param.py", | ||
| "mavproxy_ftp.py", | ||
| "mavftp.py", | ||
| "fix_with_statements.py", | ||
| "find_exclusive_parameter_names.py", |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These excluded files do not exist in the repository. Adding non-existent files to the exclusion list serves no purpose and can cause confusion. Consider removing these entries from the exclusion list:
- scripts/test_gui_environment.py
- svg_comparison_demo.py
- scripts/show_svg_tk_variants.py
- scripts/pycairo_render.py
- scripts/cairosvg_test.py
- scripts/cairo_svg_tkinter_demo.py
- scripts/cairo_svg_renderer.py
- post_install.py
- param_zip.py
- param_filter.py
- mavproxy_param.py
- mavproxy_ftp.py
- mavftp.py
- fix_with_statements.py
- find_exclusive_parameter_names.py
Only exclude files that actually exist and need to be excluded from linting.
| "scripts/test_gui_environment.py", | |
| "svg_comparison_demo.py", | |
| "scripts/show_svg_tk_variants.py", | |
| "scripts/pycairo_render.py", | |
| "scripts/cairosvg_test.py", | |
| "scripts/cairo_svg_tkinter_demo.py", | |
| "scripts/cairo_svg_renderer.py", | |
| "post_install.py", | |
| "param_zip.py", | |
| "param_filter.py", | |
| "mavproxy_param.py", | |
| "mavproxy_ftp.py", | |
| "mavftp.py", | |
| "fix_with_statements.py", | |
| "find_exclusive_parameter_names.py", |
| "scripts/test_gui_environment.py", | ||
| "svg_comparison_demo.py", | ||
| "scripts/show_svg_tk_variants.py", | ||
| "scripts/pycairo_render.py", | ||
| "scripts/cairosvg_test.py", | ||
| "scripts/cairo_svg_tkinter_demo.py", | ||
| "scripts/cairo_svg_renderer.py", | ||
| "post_install.py", | ||
| "param_zip.py", | ||
| "param_filter.py", | ||
| "mavproxy_param.py", | ||
| "mavproxy_ftp.py", | ||
| "mavftp.py", | ||
| "fix_with_statements.py", | ||
| "find_exclusive_parameter_names.py", |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These excluded files do not exist in the repository. Adding non-existent files to the exclusion list serves no purpose and can cause confusion. Consider removing these entries from the exclusion list:
- scripts/test_gui_environment.py
- svg_comparison_demo.py
- scripts/show_svg_tk_variants.py
- scripts/pycairo_render.py
- scripts/cairosvg_test.py
- scripts/cairo_svg_tkinter_demo.py
- scripts/cairo_svg_renderer.py
- post_install.py
- param_zip.py
- param_filter.py
- mavproxy_param.py
- mavproxy_ftp.py
- mavftp.py
- fix_with_statements.py
- find_exclusive_parameter_names.py
Only exclude files that actually exist and need to be excluded from type checking.
| "scripts/test_gui_environment.py", | |
| "svg_comparison_demo.py", | |
| "scripts/show_svg_tk_variants.py", | |
| "scripts/pycairo_render.py", | |
| "scripts/cairosvg_test.py", | |
| "scripts/cairo_svg_tkinter_demo.py", | |
| "scripts/cairo_svg_renderer.py", | |
| "post_install.py", | |
| "param_zip.py", | |
| "param_filter.py", | |
| "mavproxy_param.py", | |
| "mavproxy_ftp.py", | |
| "mavftp.py", | |
| "fix_with_statements.py", | |
| "find_exclusive_parameter_names.py", |
|
|
||
|
|
||
| class TestParameterAdditionWorkflows: | ||
| """Cover add-parameter dialog flows and error handling.""" | ||
|
|
||
| def test_on_parameter_add_invokes_confirmation_handler(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.get_possible_add_param_names.return_value = ["NEW"] | ||
| parameter_editor_table._confirm_parameter_addition = MagicMock(return_value=True) | ||
| mock_window = MagicMock() | ||
| mock_window.root = MagicMock() | ||
| mock_window.main_frame = MagicMock() | ||
| entry_widget = MagicMock() | ||
| entry_widget.get.return_value = "NEW" | ||
|
|
||
| with ( | ||
| patch( | ||
| "ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.BaseWindow", return_value=mock_window | ||
| ), | ||
| patch( | ||
| "ardupilot_methodic_configurator.frontend_tkinter_parameter_editor_table.EntryWithDynamicalyFilteredListbox", | ||
| return_value=entry_widget, | ||
| ), | ||
| ): | ||
| parameter_editor_table._on_parameter_add() | ||
|
|
||
| handler = entry_widget.bind.call_args_list[0][0][1] | ||
| handler(SimpleNamespace(widget=entry_widget)) | ||
| parameter_editor_table._confirm_parameter_addition.assert_called_with("NEW") | ||
|
|
||
| def test_on_parameter_add_handles_operation_not_possible(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.get_possible_add_param_names.side_effect = OperationNotPossibleError("nope") | ||
| parameter_editor_table._on_parameter_add() | ||
| parameter_editor_table._dialogs.show_error.assert_called_once() | ||
|
|
||
| def test_confirm_parameter_addition_handles_errors(self, parameter_editor_table: ParameterEditorTable) -> None: | ||
| parameter_editor_table.parameter_editor.add_parameter_to_current_file.side_effect = InvalidParameterNameError("bad") | ||
| assert parameter_editor_table._confirm_parameter_addition("bad") is False | ||
| parameter_editor_table._dialogs.show_error.assert_called() | ||
|
|
||
| parameter_editor_table._dialogs.show_error.reset_mock() | ||
| parameter_editor_table.parameter_editor.add_parameter_to_current_file.side_effect = OperationNotPossibleError("ops") | ||
| assert parameter_editor_table._confirm_parameter_addition("bad") is False | ||
| parameter_editor_table._dialogs.show_error.assert_called() | ||
|
|
||
|
|
||
| class TestUploadSelectionBehavior: |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed test test_on_parameter_add_handles_operation_not_possible was covering error handling for when get_possible_add_param_names() raises OperationNotPossibleError. However, the current implementation of _on_parameter_add() at line 903 of ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py does not have a try-catch block to handle this exception. This creates a test coverage gap for an important error scenario that can occur when there's no apm.pdef.xml file and no FC connected.
Consider either:
- Adding error handling to
_on_parameter_add()to catchOperationNotPossibleErrorand show an appropriate error dialog - Adding a test to verify that the application handles this error gracefully
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Test Results 3 files 3 suites 34m 4s ⏱️ Results for commit 3e19890. ♻️ This comment has been updated with latest results. |
12a9eb7 to
5ad6ec9
Compare
…g in the repository - unify pyright configuration - ignore some matplotlib deprecation warnings
This should be reverted and improved upon at some point, but for now just fix it
5ad6ec9 to
33a5d07
Compare
for more information, see https://pre-commit.ci
No description provided.