Conversation
ThirteenFish
left a comment
ThirteenFish
left a comment
There was a problem hiding this comment.
Don't worry about the CI at the moment, I'll fix that up after we get this merged (or possibly before? might be easier), along with a few improvements to the overall python project files. Speaking of those, remember to add any new dependencies (specifically pytest) to pyproject.toml and requirements.txt (the latter will be removed in the aforementioned changes, but for now it's still here).
On the test themselves I like the thoroughness and added coverage tooling. That said, it's important to test as much though only the public interface to the thing under test as is feasible so as not to tie the tests to a specific implementation. Python doesn't have private as a language concept but by convention members and methods that start with an underscore should be treated as private, that is as implementation details. Additionally for StarTrackerService the methods that start with on_ could be considered protected, as part of the parent Service class internal implementation and not part of the public interface either.
The public interface to StarTrackerService are the public parts of Service (start(), stop(), cancel(), status()) and the Object Dictionary parts of the Node class that gets passed to start(), Node gets a little complicated because of the SDO callbacks. So for example testing _filter() would involve setting up the filter OD parameters and then setting the state to CAPTURE_ONLY though an SDO and observing the result. Likewise checking _state should be done through an SDO that calls on_read_status().
Not everything is going to be immediately easy to test this way but it's OK to change the original code to make it easier to test. In this way the tests can improve the original code. I, a user, probably want to know if a capture I requested got caught by the filter but the only feedback is a debug log message, something I'm never going to see. Getting feedback back out to the public interface would both make testing easier and give useful information to the user.
Another example is the BOOT -> STANDBY transition waiting for 70 seconds as measured by monotonic(). This isn't great for automated tests because that's a lot of unnecessary waiting but it also wouldn't be great for testing by hand. Having that 70 second timeout be configurable, down to at least 0 when we're running a test, would be helpful for everyone.
Finally, this will be more relevant when CI is fixed and the linter and formatter is able to run again but remember to trim trailing whitespace.
| class TestLost(unittest.TestCase): | ||
| """Test the LOST star tracker solving algo""" | ||
| """ | ||
| Test suite for the star attitude solving process provided by the 'lost' module. | ||
|
|
||
| This suite verifies that the data flow and function outputs (using mock data | ||
| from lost.py) are correct and as expected by the system. | ||
| """ | ||
| def setUp(self): |
There was a problem hiding this comment.
Test classes can be converted to pytest just by removing the superclass, here class TestLost: would be sufficient. The method setUp should then be converted to a fixture.
| expected_ra_int = 77 | ||
| actual_ra_int = int(self.lost_data.get("attitude_ra", 0)) | ||
| self.assertEqual(actual_ra_int, expected_ra_int, "RA integer value mismatch") |
There was a problem hiding this comment.
pytest can make use of just regular assert instead of self.assertEqual but here we should be using pytest.approx for floating point comparisons. That way we don't have to do integer truncation.
| print("\n--- Starting mock camera setup ---") | ||
|
|
||
| # 3. Yield the mock instance to the test function. | ||
| yield mock_cam_instance | ||
|
|
||
| # 4. Teardown code. This runs automatically after the test finishes. | ||
| # The mocker automatically stops the patch, so this is usually just for cleanup. | ||
| print("--- Running mock camera teardown ---") |
There was a problem hiding this comment.
print() shouldn't be used in test code. Additionally if there's no teardown to be done this can be return instead of yield.
| def test_on_stop(star_tracker_service, mock_canopen_node): | ||
| """ | ||
| Test on_stop method: verifies the state changes to OFF and critical data | ||
| (like the last captured image and OD variables) are cleared/reset. | ||
| """ | ||
| # Simulate an active state before stopping | ||
| star_tracker_service._last_capture = np.array([1]) | ||
| star_tracker_service._state = State.STAR_TRACK | ||
|
|
||
| # Act: Stop the service | ||
| star_tracker_service.on_stop() | ||
|
|
||
| # Assert | ||
| assert star_tracker_service._state == State.OFF | ||
| assert star_tracker_service._last_capture is None | ||
| # Check if OD variables were reset to 8 (spot check) | ||
| mock_canopen_node.od["orientation"]["right_ascension"].value == 0 | ||
| mock_canopen_node.od["capture"]["last_capture_time"].value == 0 |
There was a problem hiding this comment.
This function got indented on accident and now it's part of the preceding function as only a definition. It won't get run.
| # The mocker automatically stops the patch, so this is usually just for cleanup. | ||
| print("--- Running mock camera teardown ---") | ||
|
|
||
| # Define a fixture for the mock CANopen node |
There was a problem hiding this comment.
I'm picking on this comment in particular but try to keep this in mind across all comments: they should try where possible to describe why, not what. The code already tells us what. @pytest.fixture literally means define a fixture, and def mock_canopen_node tells me that this fixture is for the mock CANopen node. What would be more helpful is to have some explanation of why the node is being mocked. Was creating one too costly or complicated? Was it not necessary? Or if it's obvious why this is here is this comment even needed?
2 participants
Summary
This branch transitions the testing framework from unittest to PyTest and introduces a comprehensive suite of unit tests for the core StarTrackerService logic.
Key Changes
Testing Framework Migration: Switched testing harness from unittest (still keeping old harness to be removed later) to the more robust and flexible PyTest framework.
Service Test Expansion (New File): Added tests/test_star_tracker_service.py with 25+ new test cases (including parameterized tests) covering critical service functionality, including:
State transitions (BOOT to STANDBY, STAR_TRACK to STANDBY/ERROR).
CANopen OD initialization and SDO callbacks (read/write status).
Image processing (_encode_compress_tiff) and caching (_save_to_cache).
Image filtering logic (_filter) under various conditions.
Error handling for camera and general exceptions.
LOST Test Update: Modified tests/test_lost.py to be compatible with the PyTest framework.
Documentation Update: Updated README.md to reflect the new command for running unit tests using pytest with code coverage (--cov) reporting.
Testing Instructions
To run the full suite of tests and check coverage, please use the command documented in the updated README.md:
Bash
$ PYTHONPATH=. python3 -m pytest --cov=oresat_star_tracker --cov-report=term-missing
This improves test coverage and stability of the star_tracker_service.