Skip to content

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

Merged
merged 7 commits into from
Dec 7, 2023

Conversation

sbytnar
Copy link
Contributor

@sbytnar sbytnar commented Dec 5, 2023

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

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bd23d61) 79.56% compared to head (8c5400a) 79.56%.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@rytilahti rytilahti left a 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.

@rytilahti rytilahti changed the title Add KP125M fixture. Enable tapo auth in pytest. Add KP125M fixture and allow passing credentials for tests Dec 5, 2023
sbytnar and others added 3 commits December 5, 2023 19:29
Co-authored-by: Teemu R. <tpr@iki.fi>
Co-authored-by: Teemu R. <tpr@iki.fi>
@sbytnar
Copy link
Contributor Author

sbytnar commented Dec 6, 2023

pytest kasa/tests/test_emeter.py -v -k KP125M

@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:
...
kasa/tests/test_emeter.py::test_emeterstatus_missing_current ✓ 100% ██████████

Results (0.80s):
1 passed
205 skipped

It looks like I'm missing some magic to make WITH_EMETER work properly.

@sbytnar
Copy link
Contributor Author

sbytnar commented Dec 6, 2023

Related https://github.com/python-kasa/python-kasa/pull/566, but I think there's no need to recreate the file as it's just filename changes.
Updated nickname and ssid to "##MASKEDNAME##".
@rytilahti If you nudge me in the right direction, I'll try and get the KP125M emeter tests working. Otherwise, I'm not aiming to add any more to this PR.

@rytilahti
Copy link
Member

rytilahti commented Dec 6, 2023

@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 has_emeter filters only "IOT" devices. This patch will fix that (and cause the tests to fail :-():

diff --git a/kasa/tests/conftest.py b/kasa/tests/conftest.py
index 53e2c49..f77c13b 100644
--- a/kasa/tests/conftest.py
+++ b/kasa/tests/conftest.py
@@ -151,7 +151,7 @@ def parametrize(desc, devices, protocol_filter=None, ids=None):
     )
 
 
-has_emeter = parametrize("has emeter", WITH_EMETER)
+has_emeter = parametrize("has emeter", WITH_EMETER, protocol_filter={"SMART", "IOT"})
 no_emeter = parametrize("no emeter", ALL_DEVICES_IOT - WITH_EMETER)
 
 bulb = parametrize("bulbs", BULBS, protocol_filter={"SMART", "IOT"})

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 has_emeter_iot "group" to use for those tests.

@sdb9696
Copy link
Collaborator

sdb9696 commented Dec 7, 2023

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.

@rytilahti
Copy link
Member

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.

@rytilahti rytilahti added the enhancement New feature or request label Dec 7, 2023
@rytilahti rytilahti merged commit be289a5 into python-kasa:master Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants