Give sensible image when webcam responds with error#1462
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1462 +/- ##
=======================================
Coverage 98.73% 98.73%
=======================================
Files 237 237
Lines 8683 8684 +1
=======================================
+ Hits 8573 8574 +1
Misses 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks! I really like the use of in-built library functions for this. A comment in code, additionally:
Must: I think this might be always creating a placeholder image. If I run the following:
async def test_webcam_system_test(RE):
async with init_devices(mock=True):
webcam = Webcam(
url=URL("http://i03-webcam1/axis-cgi/jpg/image.cgi"),
)
import pathlib
this_folder = pathlib.Path(__file__).parent.resolve()
await webcam.filename.set("test")
await webcam.directory.set(str(this_folder))
await webcam.trigger()on main I get a webcam image, if I run on this branch I get a placeholder. I think this might be because Image.open expects a file path and we have data but I'm not sure. Can you add that test into the repository but skipped with a comment similar to test_grid_overlay. Once you have the test it should hopefully be obvious from the exception on verify why it is failing.
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks! Some minor nitpicks which I have quickly addressed myself
tests/devices/test_webcam.py
Outdated
|
|
||
| @pytest.mark.skip(reason="System test that hits a real webcam, not suitable for CI") | ||
| async def test_webcam_system_test(RE): | ||
| async with init_devices(mock=True): |
There was a problem hiding this comment.
Nit: It's not really mock device if it's being set up against a real stream
| mock_file.write.assert_called_once_with(test_placeholder_data) | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="System test that hits a real webcam, not suitable for CI") |
There was a problem hiding this comment.
Nit: I think we can use @pytest.mark.system_test here so it will get run by our internal system tests but not in GH CI
There was a problem hiding this comment.
No we can't, we're in dodal not mx-bluesky
tests/devices/test_webcam.py
Outdated
| url=URL("http://i03-webcam1/axis-cgi/jpg/image.cgi"), | ||
| ) | ||
|
|
||
| import pathlib |
There was a problem hiding this comment.
Nit: By convention imports are all at the top of the file unless there is a specific reason for them not to be
tests/devices/test_webcam.py
Outdated
| @patch("dodal.devices.webcam.create_placeholder_image", autospec=True) | ||
| @patch("dodal.devices.webcam.ClientSession.get", autospec=True) | ||
| @patch("dodal.devices.webcam.Image.open") | ||
| async def test_given_response_read_passes_but_image_is_invalid( |
There was a problem hiding this comment.
Nit This is a bit of a confusing name
DominicOram
changed the title
2 participants
Fixes DiamondLightSource/mx-bluesky#1002
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}