refactor(main): make the code more testable and easier to read#1347
refactor(main): make the code more testable and easier to read#1347amilcarlucas merged 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the main entrypoint to improve readability and testability by extracting connection and vehicle-directory resolution logic into dedicated functions.
Changes:
- Split FC connection + vehicle type resolution into
connect_to_fc()andresolve_vehicle_type(). - Extracted vehicle directory selection/writability logic into
get_preferred_vehicle_dir()andresolve_writable_vehicle_dir_for_initial_download(). - Added tests covering the new behavior and updated existing connection tests to patch the new progress context manager.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test__main__.py | Adds/updates unit tests for new connection progress patching and vehicle directory resolution helpers. |
| ardupilot_methodic_configurator/main.py | Refactors FC connection/vehicle type logic and extracts vehicle directory resolution + writability checks into helpers. |
| path.mkdir(parents=True, exist_ok=True) | ||
| test_file = path / ".write_test.tmp" | ||
| with open(test_file, "w", encoding="utf-8") as tmp: | ||
| tmp.write("test") | ||
| test_file.unlink(missing_ok=True) | ||
| return True |
There was a problem hiding this comment.
Using a fixed filename (.write_test.tmp) can cause collisions if multiple instances/threads/processes run this check concurrently in the same directory (one call can delete/overwrite another’s file and produce flaky results). Consider generating a unique temp filename (e.g., via tempfile with the target directory) and ensure cleanup happens in a finally block so the temp file is removed even if the write fails mid-way.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| result = get_preferred_vehicle_dir(mock_args) | ||
|
|
||
| assert result == Path("C:/cwd") | ||
|
|
There was a problem hiding this comment.
This test case models an args object without a vehicle_dir attribute, but in real usage argparse.Namespace typically includes the attribute (often set to None). Add a test where args.vehicle_dir exists but is None (and/or empty) to cover the realistic fallback behavior and prevent regressions like Path(None) raising.
| def test_falls_back_to_cwd_when_vehicle_dir_is_none(self) -> None: | |
| """Given args.vehicle_dir is None, preferred directory should be current working directory.""" | |
| mock_args = MagicMock() | |
| mock_args.vehicle_dir = None | |
| with patch("ardupilot_methodic_configurator.__main__.Path.cwd", return_value=Path("C:/cwd")): | |
| result = get_preferred_vehicle_dir(mock_args) | |
| assert result == Path("C:/cwd") |
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, #1349, to work on those changes. Once the pull request is ready, I'll request review from you. |
…leanup in _is_directory_writable Co-authored-by: amilcarlucas <24453563+amilcarlucas@users.noreply.github.com>
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Test Results 2 files 2 suites 28m 30s ⏱️ Results for commit d6fedc7. |
No description provided.