Raise the abstraction and coverage level #1033
Conversation
Split the test into three files, one for each API served Use a new naming convention for unit tests.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the test suite by splitting a large 2845-line test file into three focused files based on the APIs they serve:
unit_configuration_manager_param_editor.py- Tests for parameter editor API (1275 lines)unit_configuration_manager_param_editor_table.py- Tests for parameter editor table API (505 lines)unit_configuration_manager_doc_frame.py- Tests for documentation frame API (798 lines)
Key changes:
- Introduces
unit_prefix naming convention for unit tests - Moves shared fixtures and utilities to
tests/conftest.py - Updates
pytest.inito recognize the new naming pattern - Deletes the original
test_configuration_manager.pyfile
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit_configuration_manager_param_editor_table.py |
New file containing parameter editor table API tests (filtering, export, copy, navigation, upload workflows) |
tests/unit_configuration_manager_param_editor.py |
New file containing parameter editor API tests (integration, upload, download, file operations, IMU calibration, properties) |
tests/unit_configuration_manager_doc_frame.py |
New file containing documentation frame API tests (configuration validation, file downloads, summary generation, documentation methods) |
tests/test_configuration_manager.py |
Deleted - split into three focused test files |
tests/conftest.py |
Added shared fixtures (mock_flight_controller, mock_local_filesystem, configuration_manager) and utilities moved from deleted file |
pytest.ini |
Added unit_*.py pattern to recognize new naming convention |
| def gui_test_environment() -> None: | ||
| """Set up GUI test environment with screen validation.""" | ||
| # Only import pyautogui within the fixture to avoid slowing down non-GUI tests | ||
| import pyautogui # pylint: disable=import-outside-toplevel # noqa: PLC0415 |
There was a problem hiding this comment.
The import of pyautogui has moved inside the gui_test_environment fixture, but the global import at line 20 was removed. This could break other tests or fixtures that depend on pyautogui being imported globally. Verify that no other code in the test suite requires the global pyautogui import.
| # Assert: No export occurred | ||
| configuration_manager._local_filesystem.export_to_param.assert_not_called() | ||
| mock_log.assert_called_once() | ||
| log_call_args = mock_log.call_args[0] |
There was a problem hiding this comment.
The assertion extracts log_call_args[0] without verifying the structure of call_args first. If the logging call format changes, this could raise an IndexError. Consider adding a length check or using a safer access pattern like log_call_args[0] if log_call_args else None.
| log_call_args = mock_log.call_args[0] | |
| log_call_args = mock_log.call_args[0] | |
| assert log_call_args, "Expected logging call to have at least one positional argument" |
|
|
||
| # Assert: Upload completed with 1 changed parameter | ||
| configuration_manager._upload_parameters_to_fc.assert_called_once_with(selected_params, show_error) | ||
| # Note: _at_least_one_changed assertion removed due to test fixture isolation |
There was a problem hiding this comment.
[nitpick] This comment references a removed assertion but doesn't explain why it was removed or what 'test fixture isolation' means in this context. Consider either removing this comment or expanding it to explain the reason more clearly for future maintainers.
| # Note: _at_least_one_changed assertion removed due to test fixture isolation | |
| # The _at_least_one_changed assertion was removed because the test fixture does not guarantee | |
| # isolation of parameter state between tests. This means that the parameters may not always | |
| # reflect changes as expected, leading to unreliable test results. To avoid false positives/negatives, | |
| # we do not assert that at least one parameter was changed in this test. |
Split the test into three files, one for each API served Use a new naming convention for unit tests.