Fix for camera image captures

[shred86]

This fixes the capture method to work with both Abode Iota and the stand alone streaming cameras (#60). There's two things that still need to be done:

1. Update tests to support this change.
2. Remove CAMS_ID_CAPTURE_URL from constants.py once the tests are updated since it will no longer be used.

I'm not very good at writing tests so I'll likely need help on this. At a first glance, I think we need to create a camera.py file in tests/mock/devices that returns mock JSON data for an Abode camera. From there, I think the test_camera.py will need to be updated, specifically line 105 to make it match the changes in this pull request.

[shred86]

Pushed some changes that updates test_camera.py and removes the unused CAMS_ID_CAPTURE_URL variable in constants.py. @MisterWil, will the update to test_camera.py suffice? It appears we can just ir_camera.py since the JSON structure is nearly identical to a normal camera.

Edit: The difference here is the ir_camera.py JSON data doesn't have a control_url_snapshot key which is really what we're using.

[MisterWil]

You should probably modify and rename ir_camera.py so that it can create mock JSON for all the different types of cameras - or at least so that you can test the two known different capture URL's. Then you can modify the tests to verify that it uses the capture URL in the json to send the request.

[shred86]

After looking into this, I've identified some changes I made to camera.py. We have to check what type of camera it is to determine whether to use control_url (motion sensor with cameras) or control_url_snapshot (streaming cameras).

That being said, I'm still stuck on updating the tests. I know what I want to do, just can't seem to figure out how to get it to work. So far I've created a new ipcam.py file in test/devices. This is to return the JSON data for an IP camera device. I thought about the idea you proposed of renaming ir_camera.py and having it be able to return different JSON data for different cameras, but I'm not sure how to do that while working with tests. Now I'm not quite sure how I would test the JSON data in ipcam.py without creating a new version of test_camera.py. I'm assuming I'd want to run all the same test methods against the ipcam.py JSON data just to make sure the code works with different JSON data.

Anyways, I'll try to look at this some more later. I'm sure this is something super simple but my I'm a coding newbie (still). :)

Edit: Committed my changes to camera.py and reverted the changes I made to test_camera.py and constants.py.

[coveralls]

Coverage increased (+0.05%) to 85.554% when pulling f9b0ddcf9dd548089cac53c1087d4c5d60d780a4 on shred86:camera_fix into 81d8238256e2a8947494d50153beb37b1c0251fc on MisterWil:master.

[shred86]

@MisterWil, I’m a bit confused as to how requests_mock is being used in some of these tests. For example, in test_camera.py, the test_camera_properties test method, you have:

m.post(CONST.LOGIN_URL, text=LOGIN.post_response_ok())

If I’m understanding the purpose of requests_mock, the line above is basically saying if you were to subsequently use the post method with CONST.LOGIN_URL, it would return LOGIN.post_response_ok(). However, I don’t see the post method with CONST.LOGIN_URL ever being used in the test_camera_properties test method, so I’m confused what’s the point of having m.post(CONST.LOGIN_URL, text=LOGIN.post_response_ok())? I don’t see any of the post or get mocked methods being used in test_camera_properties, so I’m thinking I may be completely missing something.

Edit: Actually, I think they are being used when methods like logout or get_device is called in Abodepy, so that would make sense.

[MisterWil]

When this line is called in the test it executes a login flow since I explicitly called logout previously. The code will go through and actually issue requests, however since I've used requests_mock to say "if this URL is called, just return this mock response" it won't actually ever reach out to Abode.

So, requests_mock will catch the given request URL's from the real code and return exactly what you tell it to return. This allows you to test your code against multiple possible return values and error states.

It is also possible that some of the tests I have written include request_mock URL's that never get called for the tests intended purpose. This is simply because I got lazy and just copy/pasted blocks of the mock setup and modified what I needed to test the intended purpose.

So when you write a test, start out by thinking "What am I trying to test?". For example: OK how do I test that the correct URL is called if its Camera A? Okay now how do I test if its Camera B? What if I Abode adds a new camera I don't recognize, can I test that? What if the capture URL returns a 404? What if the capture URL doesn't exist in my camera JSON? All of those should be separate test methods - you write mock JSON that simulates those different states and the tests verify that the code does what you want it to do. If the test fails you modify the code to work with your test, and then you re-run ALL tests to verify that you fixed your broken test but also all your previous tests still pass. You're literally just thinking of any possible case that might occur and writing tests to simulate it to verify the code is robust enough to handle it.

Eventually you might start thinking about code coverage. When you run the AbodePy tests it will output what files have lines that are missing coverage. Coveralls commented above that Coverage decreased (-0.06%) to 85.442%. This means there are methods or branch statements in the code that aren't "covered" by a test. For example, you might have an if statement in the code that says if the response I got has a status of 200 then do this, but none of your tests ever return a status that ISN'T 200 so you've missed a possible branch. Or you might have the inverse with code that says if the response I got has a status of 400 then do this but no tests return 400 that's considered a missing line of coverage.

The place you're "intended" to aim for is 100% coverage, but that is usually difficult and becomes even more difficult as the code base increases in size. Most of the time high-80's or 90's is good enough, and if real-world testing or use results in a new bug you can just write a test to recreate it so that the test fails, fix it in your code, re-run all the tests to see that you fixed the bug without breaking another test, and call it a day.

[shred86]

Thanks for the info. Just committed my first hack at writing tests for specifically calling the capture method in camera.py. I realize this probably isn't the most elegant or complete way. What I did:

1. Created a new ipcam.py file in tests/mock/devices that contains just the device method to return the JSON data for Abode IP cameras.
2. Added tests_streaming_camera_capture method to test_camera.py which tests the capture method for the IP cameras.
3. Deleted the unused constant CAMS_ID_CAPTURE_URL in constants.py. For tests, I'm simply referencing the dummy device constants.

A few thoughts on this implementation:

• I considered just adding the JSON data for the IP cam to the existing ir_camera.py and renaming it. However, I personally think it's cleaner to keep these as separate modules/files since they are a different class of devices (motion camera vs. streaming camera).
• I considered creating a new class in test_camera.py to rerun all of the same tests on the new IP cam JSON data. I think this is probably the most thorough way to do it since it is testing all of the IP cam JSON data against camera.py. While this would create a lot of repeated code, I suppose the ideal solution would be to modify the TestCamera class to be "object oriented" in the sense that I could simply pass a new camera device/JSON data and retest it within that same class. I'm not quite sure how to do this with unittest though.

[shred86]

I'm debating if this is better or not, but I currently modified the capture method to be:

    def capture(self):
        """Request a new camera image."""
        # Abode IP cameras use a different URL for image captures.
        if 'control_url_snapshot' in self._json_state:
            url = CONST.BASE_URL + self._json_state['control_url_snapshot']

        elif 'control_url' in self._json_state:
            url = CONST.BASE_URL + self._json_state['control_url']

        else:
            raise AbodeException((ERROR.MISSING_CONTROL_URL))

        try:
            response = self._abode.send_request("put", url)

            _LOGGER.debug("Capture image response: %s", response.text)

            return True

        except AbodeException as exc:
            _LOGGER.warning("Failed to capture image: %s", exc)

        return False

Added to errors.py:

MISSING_CONTROL_URL = (
    30, "Control URL does not exist in device JSON.")

My thought here is being more explicit is probably better in the event the Abode ever changes the control_url or control_url_snapshot keys in the device JSON. Probably unlikely, but would make troubleshooting a little easier. I've also added the following to test_camera_capture in test_camera.py to test this situation:

        # Remove 'control_url' from JSON
        del device._json_state['control_url']

        # Test that AbodeException is raised
        with self.assertRaises(AbodeException) as exc:
            device.capture()
            self.assertEqual(str(exc.exception), ERROR.MISSING_CONTROL_URL)

Any thoughts/inputs? I haven't committed these changes yet.

Edit: I'm also working on rewriting test_cameras_capture to test both camera JSON within that test method so I can get rid of test_streaming_camera_capture.

[shred86]

Just committed some of the changes mentioned above along with a lot of changes to test_camera.py. It's now testing both camera's JSON data against all these methods. Some things I'd still like to do if I can figure it out:

• Refactor the repeated code. A lot of the setup URLs are repeated in each test method. I'm trying to figure out if it's possible to have them be setup in the setUp method but I'm not sure it's possible to use requests_mock in the setUp method.
• The timeline event JSON data for ipcam.py is copied from ir_camera.py. I need to get the actual timeline event for an IP cam which I'm assuming will be nearly identical.
• Pylint is complaining about my use of the CAM_TYPE variable as it doesn't conform to snake_case style. I can easily rename it to cam_type but I thought it's easier to read since it's referring to the IPCAM and IRCAMERA imports.

[MisterWil]

This is looking good. When you feel like you're done let me know and I'll merge it and push out a new version. :-)

[shred86]

I think I'm done for now. I've hit a dead end on trying to figure out how to use requests_mock in the setUp class (I don't think it's possible). There's probably another way to do it but I'm not too concerned. I'm just happy I understand unit tests a little better now. :)

[MisterWil]

Just for your learning, I fixed a couple of linting issues with this commit that popped up when I ran tox -e lint. Just small code quality changes.

[shred86]

Ah, sorry, not sure why my linter didn't catch those in Visual Studio Code. I'll make sure to run tox -e lint next time! Thanks for merging this.