-
-
Notifications
You must be signed in to change notification settings - Fork 221
Add KP125M fixture and allow passing credentials for tests #567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #567 +/- ##
=======================================
Coverage 79.56% 79.56%
=======================================
Files 37 37
Lines 3083 3083
Branches 804 804
=======================================
Hits 2453 2453
Misses 548 548
Partials 82 82 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @sbytnar! 👍
To verify if the emeter is tested, you could try pytest kasa/tests/test_emeter.py -v -k KP125M
to see the tests containing KP125M.
Co-authored-by: Teemu R. <tpr@iki.fi>
Co-authored-by: Teemu R. <tpr@iki.fi>
@rytilahti If I run with -k KP125M, all tests are skipped. If I point it at a device --ip $IP --username "$USERNAME" --password "$PASSWORD", I get: Results (0.80s): It looks like I'm missing some magic to make WITH_EMETER work properly. |
|
@sbytnar ah, sorry, the test harness is currently in a bit of flux due to the new protocols being added. The new fixture is not picked as it's categorized as a "SMART" device while the
The problem is that the tests are written to test the internal structure which differs from what KP125M reports. We can leave it as-is (i.e., not testing these for the time being) for this PR and fix that later on. The tests that are failing are checking for the device responses which differ, so we would need to guard those checks by adding a new |
This LGTM and ready to merge. qq. you've masked nickname. Do you think we should mask all nicknames/aliases in dump_devinfo? We currently don't but maybe we should. Especially because if people try to mask nickname themselves they could put a non base64 value in there. |
Okay, let's merge this. @sdb9696, yeah, it probably makes sense to mask the name as it might be personalized & not having it masked doesn't bring any benefits that I can think of. |
Is this the right way to add KP125M to conftest.py? Does this correctly test emeter?
Tested with:
$ poetry run pytest
...
Results (61.71s (0:01:01)):
7984 passed
110 skipped
$ poetry run pytest --ip $IP --username $USERNAME --password $PASSWORD
...
Results (22.73s):
1276 passed
6818 skipped