test: fix MockSensor do_command state tracking and add missing coverage#1188
test: fix MockSensor do_command state tracking and add missing coverage#1188Shailesh katara (luckys00) wants to merge 5 commits into
Conversation
Shailesh katara (luckys00)
requested review from
Naveed Jooma (njooma) and
Ethan (stuqdog)
|
👋 Thanks for requesting a review from the team! We aim to review PRs within one business day. If this is urgent |
|
|
Naveed Jooma (njooma)
left a comment
Naveed Jooma (njooma)
left a comment
There was a problem hiding this comment.
Hi! Thanks for contributing! We are actually in the process of updating all our tests. If you want to take a look at how the test_camera.py tests are now implemented, we are moving towards that for all our resources.
If you like, you can update the sensor tests to use that framework!
|
Thanks for the review and the heads-up, Naveed Jooma (@njooma) ! It's great to hear about the new testing framework. I'll take a look at how test_camera.py is implemented and update the do_command tests for the Sensor components to match that new structure. Will push the changes shortly! |
|
hey Naveed Jooma (@njooma) is this ok. |
Naveed Jooma (njooma)
left a comment
Naveed Jooma (njooma)
left a comment
There was a problem hiding this comment.
This is closer! Some notes that follow the new direction we want to go with testing
| @@ -28,39 +28,16 @@ | |||
| def sensor() -> MockSensor: | |||
There was a problem hiding this comment.
We should no longer be using components in the mocks file. Instead, we should make a magic mock (see here)
| assert geometries == GEOMETRIES | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") |
There was a problem hiding this comment.
Remove the manager fixture
| async with ChannelFor([service]) as channel: | ||
| client = SensorServiceStub(channel) | ||
| command = {"command": "args"} | ||
|
|
There was a problem hiding this comment.
Set the return type of the mock's method to return an expected value (see here)
|
|
||
| response: DoCommandResponse = await client.DoCommand(request, timeout=2.34) | ||
| result = struct_to_dict(response.result) | ||
|
|
There was a problem hiding this comment.
Assert the returned response contains the expected value set above (see here)
|
|
||
| assert result == {"command": command} | ||
|
|
||
| assert sensor.timeout ==expected_grpc_timeout(2.34) |
There was a problem hiding this comment.
Instead of checking fields on the mock, assert that the mocked's method is called with the expected inputs (see here)
4 participants
What this PR does
Fixes an inconsistency in the
MockSensorclass wheredo_commandwas dropping state parameters (timeoutandextra), and adds the missing test coverage fordo_commandacross the Sensor, Service, and Client test suites.Why is it needed?
While
get_readingsandget_geometriesproperly saveself.extraandself.timeoutto the mock's state, thedo_commandimplementation was bypassing this and simply returning the command. This makes it impossible to assert if custom commands with timeouts were actually processed correctly by the mock.Additionally, this PR adds proper test coverage for
do_commandintest_sensor.pyto ensure the timeout parameter successfully travels through the gRPC channel.Changes Made:
do_commandintests/mocks/components.pyto properly trackself.timeoutandself.extra.do_commandtests inTestSensor,TestService, andTestClient(asserting timeout state logic while acknowledging gRPC dropsextrafor this specific request).