Unit tests for all methods in ControlInterface

[Anders-Holst]

And if you are interested, there are now unit tests for all the methods of ControlInterface (except firmware update. And except the realitime stuff already tested separately of course). I managed to convert my "manual tests" into these.
Again successfully tested, flaked and blacked (although, really, I am not convinced that black always makes the code more readable...)

I actually suggest removing the existing test file tests/test_control.py including its cassettes, since they are now covered by this new test file. And maybe rename the new file to test_control.py instead. I can add such a commit if you like.

[Anders-Holst]

Hi @scrool
I have now, in line with our discussion in the gitter chat, tried to enhance the unit tests so they are easier to run on other devices then the one I used to record them. What I did:

• The previously fixed hostadress, and some device characteristics that are needed for the tests, are now moved into the "setUp" function, so if you want to test another device it suffices to change the adress, number of leds, and led profile bytes in this single place. (Hypothetically you might like it to ask the device about these characteristics rather than writing them in yourself, but note that it would generate http traffic, and since it is done in setUp it would not be recorded nor later replayed, so it would not work in the testing phase.)
• I have moved the tests around a little, so that as many as possible of them should be device-independent (or almost so). For most of them it should now work to run with another device and the tests should still pass. For some tests I use the trick you mentioned to set it first and get it after, so we know what it should be when we get it, such as device_name and mqtt_config. (In both these cases I take care to reset the original values after the test, so no factory-reset should be needed.) For others I relax the comparison to only selected attributes, ignoring those that are different in every call (such as the internal time in get_timer, and some unique ids and movie handles). However, some tests are not possible to deal with like this, such as get_device_info, get_firmware_version, etc whose very point it is to return device specific information, and network_scan and network_mode_ap/station, which returns values from the specific network surroundings. There are some borderlines too, like get_effects which is slightly different between firmware versions, but most modern ones should be the same.
• Also in "setUp" there is a flag "isrecording" which can be set to True. (I searched the documentation of unittest, but failed to find any existing flag to check whether it is currently recording or testing, so lacking that you have to set it yourself.) You have to manually remove or rename the existing cassettes too. Then when you run it with a new device, the current code will not compare the output to any expected output, but instead print it out, so you can inspect it to see that it does the right thing, and paste the output back into the testing code so that it compares with the appropriate thing for this device in the future. This will do the job in those cases mentioned above when the specific output depends on the device.

It might still not be the complete vision of being able to use the same file for both unit testing that the code has not changed, and for testing that the same code works for new devices. But I actually got it further in this direction than I had expected. Hopefully it is sufficient. In either way, it is a significantly increased coverage compared to the previous tests of control.py (which only tested set/get_mode).
If you are satisfied with it, I can throw in a last commit: deleting the old test_control.py and its corresponding three cassettes, which are now completely obsolete, and then rename test_control_low.py to test_control.py. Ok?

[scrool]

@Anders-Holst I've tried to generate my own test data by removing all tests, setting isrecording=True and replacing IP address of the device, number of leds and led bytes with mine.

Immediate diference seems to be that yours yaml files are formatted differently. Have you reformatted them since vcr recorded them?

I'm running this on:

$ python3 --version
Python 3.8.12
$ pip freeze | grep vcr
vcrpy==4.1.1
vcrpy-unittest==0.1.7

---

Secondly, test_movie_newif fails for me:

requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: http://(MYIP)/xled/v1/movies/full

I guess you haven't been working with factory-reset device but you have uploaded some movie(s) before this?

---

Similarly:

requests.exceptions.HTTPError: 416 Client Error: Requested Range Not Satisfiable for url: http://(MYIP)/xled/v1/playlist/current

---

Finally, I have skipped tests that are switching my network to AP: test_network_scan, test_network_mode_ap, test_network_mode_station for now as those would require me to change network which I wasn't able to do.

[scrool]

One more note - with saved recordings I'm able to reproduce tests. 👍

I'll still need to review the code itself though.

[Anders-Holst]

@scrool wrote:

Immediate diference seems to be that yours yaml files are formatted differently. Have you reformatted them since vcr recorded them?

No, I have not edited them after recording.
My versions look like:

aho@lysithea:~/src/xled/xled$ python3 --version
Python 3.6.9
aho@lysithea:~/src/xled/xled$ pip freeze | grep vcr
vcrpy==4.1.1

Appears I have no vcrpy-unittest, which might explain the difference. I suppose we can use your recorded cassettes instead of mine then. Both formats appear to work though.

Secondly, test_movie_newif fails for me:

Hmm, odd. It is right that it is not factory reset, but the test does a delete-movies right before, so the movie list should be "as if" reset. I can not immediately see that this would make a difference.
My guess is that something goes wrong already in the movies/new call. Do you still have the printouts it did while recording, the "delete_movies:", "get_movies:", "set_movies_new 1:" etc? They might contain a clue what is going wrong.
And just to make sure, which fw version did you use? In case the protocol is changing - it is a moving target...

Similarly:

The test_playlist error is most likely a consequential error: If there are no movies uploaded there is no playlist, and no playlist entry with the requested number.

Finally, I have skipped tests that are switching my network to AP: test_network_scan, test_network_mode_ap, test_network_mode_station for now as those would require me to change network which I wasn't able to do.

Yes, I know. In my original "manual tests" I had omitted them too, but then I though "Damn it Anders, you need to be thorough, even if it takes more effort", so I added them. Once recorded though, they are unproblematic to run, so the "unit test" aspect of these is still valuable. We can use my cassettes for those cases. (Do you think I should manually edit the cassettes to remove details from my network environment? Are these things sensitive?) Or we can leave them out entirely.
Note however that "test_network_scan" can be run from station mode too, and does not change the mode, so this should be harmless to run and straightforward to keep.

[scrool]

re vcrpy-unittest - that requirement for tests is listed in setup.py. It is somehow magically processed by tox when it creates a test environment. You could configure yur test environment "manually" with pip install -e ".[tests]".

I'd prefer to commit cassettes that are closer to what others would be able to generate - especially to be able to compare them in the future. If you would like to look into that, feel free to commit those if you manage to get some different yamls. E.g. on my end it looks like this:

 interactions:
 - request:
-    body: '{"challenge": "xnXlgGxr1EYbUVLgTJRODc9dLX8FtwgoLeZYWypdNz4="}'
-    headers:
-      Accept: ['*/*']
-      Accept-Encoding: ['gzip, deflate']
-      Connection: [keep-alive]
-      Content-Length: ['61']
-      Content-Type: [application/json]
-      User-Agent: [python-requests/2.25.1]
+    body: '{"challenge": "eOpFQ+TSR1DJ0FthfhhFNi+KvO5gMcvnPREF4aMYLHA="}'
+    headers:
+      Accept:
+      - '*/*'
+      Accept-Encoding:
+      - gzip, deflate
+      Connection:
+      - keep-alive
+      Content-Length:
+      - '61'
+      Content-Type:
+      - application/json
+      User-Agent:
+      - python-requests/2.26.0

Hmm, odd. It is right that it is not factory reset, but the test does a delete-movies right before, so the movie list should be "as if" reset. I can not immediately see that this would make a difference.

This turned out to be my bad. I set number of bytes to 4 event though I was testing this against a device which uses only RGB. It passes now.

The test_playlist error is most likely a consequential error: If there are no movies uploaded there is no playlist, and no playlist entry with the requested number.

Exactly. There was no playlist, a script uploaded just one playlist but then requested playlist with number 1 whereas playlists are numbered from 0. Simple change of set_playlist_current from 1 to 0 made that test pass.

Snip of original communication and responses:

POST /xled/v1/playlist HTTP/1.1
X-Auth-Token: g9Pww2VcuRY=
Content-Length: 83
Content-Type: application/json

{"entries": [{"unique_id": "00000000-0000-0000-000A-000000000001", "duration": 5}]}

HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 13
Connection: close

{"code":1000}

GET /xled/v1/playlist HTTP/1.1
X-Auth-Token: g9Pww2VcuRY=

HTTP/1.1 200 OK
Content-Type: application/json
Transfer-Encoding: chunked
Connection: close

{"unique_id":"00000000-0000-0000-0000-000000000000","name":"","entries":[{"id":0,"handle":1,"name":"green","unique_id":"00000000-0000-0000-000A-000000000001","duration":5}],"code":1000}

POST /xled/v1/playlist/current HTTP/1.1
X-Auth-Token: g9Pww2VcuRY=
Content-Length: 9
Content-Type: application/json

{"id": 1}

HTTP/1.1 416 Requested Range Not Satisfiable
Content-Type: application/json
Content-Length: 13
Connection: close

{"code":1103}

Note however that "test_network_scan" can be run from station mode too, and does not change the mode, so this should be harmless to run and straightforward to keep.

Right. That works. I ended up skipping more than needed.

Yes, I know. In my original "manual tests" I had omitted them too, but then I though "Damn it Anders, you need to be thorough, even if it takes more effort", so I added them. Once recorded though, they are unproblematic to run, so the "unit test" aspect of these is still valuable. We can use my cassettes for those cases.

From what I can tell the issue with vcr is that (with its current configuration?) it expects either all cassettes or none. So I have removed all of them and it got stuck once it switched a device to AP mode to which it wasn't able to connect. Maybe it could be determined better than my current decorator @pytest.mark.skip(). I can think of one easy to use right now.

(Do you think I should manually edit the cassettes to remove details from my network environment? Are these things sensitive?)

I don't think your private IP addresses needs to be redacted. On my end I don't prefer to publish MAC addresses, HW IDs, serial numbers of my devices. I know that they are being sent through MQTT and in the past it was possible to control them by others.

[Anders-Holst]

Ok, I will make new recordings with vcrpy-unittest installed. My mistake to miss it. And at the same time maybe I will carefully edit out at least MAC and HW numbers from the device and network data. I think it is wise to be a little paranoid about these things.

It is a relief that test_movie_newif works - I started to fear that the protocol had changed again or that I had failed to make it sufficiently device independent. By the way, I think that set_playlist_current should work now with 1 too, once test_movie_newif works. There are two uploaded movies in the playlist, and the index is the movie index within the playlist, so the last movie will have index 1. And when starting the playlist it always starts at movie 0, so unless we change it to something else we can not tell for sure that the call has any effect.

Regarding recordings, I again realize I do things differently - I have not figured out how to run all tests automatically at once. Instead I manually call setUp, then the specific test I want to run, and finally tearDown. Then there is no problem to, in between the tests disconnect from the wifi and connect to the Twinkly AP, and after that test connect back again. But of course, if you run through them automatically while recording, then it will be a problem.

So I am not sure what you want to do with this. One attitude is that the primary purpose after all is unit testing, not testing additional devices, although that is a nice extra. I can record the station and ap cassettes again, and then it will work in the testing phase. Maybe it is acceptable that recording requires some extra and manual work.

Another attitude is to say that perfect coverage is difficult to achieve anyway - then every single option for every call has to be tested, and it is not the case for now. So the easiest is to skip those two tests entirely, without much loss.

But suddenly it dawns on me that the obvious solution, which gives up neither rigidity nor convenience while recording, is of course to break these two tests out to separate classes ! Then each can be run in turn, and network connections changed in between. I'm already on to it!

[Anders-Holst]

Hi @scrool
I have now reformatted the cassette files, remove sensitive numbers, and broken out the two network mode tests into separate classes and files, so they can be run separately during recording.

[scrool]

This version looks great! Split into separate modules was great idea - I was easily able to skip those, that I wasn't interested in. I have squashed them and merged all those changes.

Thanks! 🥳