Add tests for EigerHandler and EigerConfigHandler #53
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 72.20% 81.69% +9.49%
==========================================
Files 4 4
Lines 295 295
==========================================
+ Hits 213 241 +28
+ Misses 82 54 -28 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Just a few comments.
I think it might be a good idea to intentionally strip down the mock connection here to what we actually use. Or maybe just a MagicMock with specific responses in each test. We do have the system tests for running the introspection against the tickit sim to test the full api.
| assert top_controller.stale_parameters.get() | ||
|
|
||
| # need to update again to make detector controller update its | ||
| # stale parameter attribute |
There was a problem hiding this comment.
This is actually a problem that we will need to figure out.
tests/test_controller.py
Outdated
| # eiger API does not return a list of updated parameters when we set status keys | ||
| # so _parameter_updates set to default case where we only update the key we changed | ||
| assert subsystem_controller._parameter_updates == {"humidity"} | ||
| # humidity is really read-only but given here for demonstration |
There was a problem hiding this comment.
Could we use a writable parameter for this just to avoid any confusion?
There was a problem hiding this comment.
Need to check if any of the parameters handled by EigerHandler actually are rw, but will do
There was a problem hiding this comment.
All parameters with the mode status are read only (see tests/system/parameters.json) - so maybe we should move EigerHandler.update to EigerConfigHandler and make attributes with EigerHandlers read only
There was a problem hiding this comment.
Do you mean move put to EigerConfigHandler? I guess that would make sense, but I don't think this is a real issue - attr.sender should fail because humidity should be an AttrR. However, in the test for some reason (in the debugger) humidity is an AttrRW. In fact all of the controllers attributes are AttrRW. Is there something broken in the mock connection causing that?
There was a problem hiding this comment.
Yes, every parameter (besides <subsystem>/api/1.8.0/<mode>/keys) just gets a dummy response at the moment
else:
# dummy response
return {"access_mode": "rw", "value": 0.0, "value_type": "float"}
There was a problem hiding this comment.
Could you try out removing the mock_connection in one of the tests and instead just pass a MagicMock as the controller connection with return_value set for the things that will be called? It doesn't have to have the full api, just pick a few parameters from the API to demonstrate the possible code branches.
remove redundant EigerHandler put test, fix test names
Closes (#45)