Create a Python package & add a test suite #493
Conversation
The Python scripts are now loosely coupled (independent scripts) but
they sometimes share code (e.g. pad/unpad/encrypt/decrypt), and other
times even import from each other (in the case of smartconfig). They
also often include code to make them importable (if __name__ ==
"__main__").
However, they currently are not structured as a package. Move them all
to a package called "smarthack" and into valid module filenames (no
dashes!) so that they can be importable.
This allows one to do e.g.
from smarthack import registration
This is the first step towards a larger codebase refactor.
|
Hi @paravoid, thanks for your PR! It looks like some pretty significant changes, it may take me some time to review this. In the future please open an issue to discuss any big changes before you invest too much time. There are a number of outstanding branches that may conflict with some of the changes you've presented here, and a larger refactor taking place behind the scenes. |
|
Thanks! The changes here are really not very significant - it's just an mv + ~15 lines of small code changes for the test commit. The tests are all new files, and thus should not conflict with anything else. That said... I have a ton of major changes queued up. Should I file a single new issue to capture all of them? |
Add a basic tox.ini file, that applies a battery of tests, namely: * pytest (test suite) against python3.6 and python3.7 * pytest-cov (code coverage) * flake8 (basic) plus the plugins import-order and docstrings (pep8, linting) * pylint (linting) * black (automatic code formatting) * mypy (type check), in semi-strict mode As the codebase is (obviously) far from being compliant, add a top-level "whitelist" with files/directories that we consider ready and that will be adding to with subsequent commits.
Initial effort of unit-testing the codebase, starting with a test for smarthack.discovery. It currently reveals a bunch of real errors, with encrypted transport failing and masked by "except: pass" (to be fixed in a subsequent commit).
Still rudimentary (e.g. doesn't check replies), but still covers 86% of the codebase and has found a real bug already (namely: encrypted packets are not being decrypted).
Rudamentary, as it only checks gen_psk() and not ssl.wrap_socket (and as such won't catch issues like sslpsk#16), but the best we can do for now and without modifying the code itself.
|
This looks great! Are there plans on merging/implementing something similar? I agree that the current code is in need of a refactor. |
|
@alexyao2015 tuya-convert is fairly stable at the moment, while not the most organized, it is functional. I believe the priorities of this project are with keeping up with Tuya firmware changes, and not necessarily rewriting or refactoring what is already working. That said, feel free to pitch specific improvements and why you believe they are needed. I would be happy to accept incremental improvements so long as they fit the rest of the project and provide a clear benefit. |
|
Totally makes sense! The main issue I had was with installing/using it. I wanted to use the docker image, but ran into pretty massive roadbumps on my raspberry pi zero w as the base image wasn't available. I would suggest potentially using debian as the base with the s6 overlay, preinstalling all requirements, and uploading to docker hub so there is no need to have the user build it. It would simplify the installation and usage a lot. I personally had to manually create a container and install all requirements, however I didn't have time to make a complete dockerfile for it. |
If you just need a quick Tuya Convert setup on your Pi without messing around with the system each time you need it, you can use Tuya Convert OS, an OS that I put together especially for Tuya Convert. I keep a dedicated SD card around, just for the purpose of running this OS and quickly flash my devices. |
|
Hmm that's a nifty project. I only happened to have a raspberry pi zero w laying around, so I wouldn't be able to use that. Anyways, with a proper docker image and the correct scripts to automatically disconnect from wifi automatically, it really isn't that difficult. |
@alexyao2015 it may actually work on a RPi Zero W; it's based on Raspbian after all. You could give it a try and let me know, so that I can update the hardware requirements. |
This does three principal things:
smarthack, based on filenames that I saw in the codebase).The test suite is written using (old and) modern Python standards - it's PEP8-formatted using black, and has docstrings as well as type hints everywhere. It passes through flake8, flake8-import-order, flake8-docstring, pylint, mypy and black. It also runs code coverage numbers using coverage.py.
The test suite currently achieves a 76% code coverage of the existing Pythong ocde package. (Code coverage isn't an absolute measure - it doesn't measure correctness by itself, e.g. in the registration server it doesn't check for the validity of the responses).
The primary motivation for this is larger refactors that are coming down the line, as well as code hygiene in general. (I thought it'd be best to start with tests :). Between the test suite, static typing, code coverage, and other linter errors, I've found a number of real bugs in the codebase -- a couple are in tests marked with "xfail" but there are a few others. More on that soon :)
After this (hopefully) gets merged, it'd be neat if we could also setup some kind of CI (GitHub Actions, Travis etc. - pick your poison!) to catch regressions in PRs going forward.