Skip to content
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

Fix missing state class for reactive power sensors after #1417 #1451

Merged
merged 8 commits into from
Jan 5, 2025

Conversation

N3rdix
Copy link
Contributor

@N3rdix N3rdix commented Dec 28, 2024

After explicitly switching to reactive power device class I missed the state_class also depending on this device_class (#1417).

Should fix #1449 (the raised repair regarding the changed unit of measurement is expected nevertheless)

@N3rdix N3rdix temporarily deployed to continuous-integration December 28, 2024 07:50 — with GitHub Actions Inactive
Copy link

codecov bot commented Dec 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.69%. Comparing base (d7d71a9) to head (3a5a193).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1451   +/-   ##
=======================================
  Coverage   94.69%   94.69%           
=======================================
  Files          12       12           
  Lines        1943     1943           
=======================================
  Hits         1840     1840           
  Misses        103      103           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drc38
Copy link
Collaborator

drc38 commented Dec 28, 2024

@N3rdix it'd be useful to include an assert in the tests to confirm the unit returned for this device class is what is expected.

@N3rdix N3rdix temporarily deployed to continuous-integration December 28, 2024 15:30 — with GitHub Actions Inactive
@N3rdix
Copy link
Contributor Author

N3rdix commented Dec 28, 2024

@drc38 although I added an assert in the test_init.py it does not feel to be the best place. As we do not have a test_sensor.py yet and I am not confident enough yet with pytest to build such a test from scratch easily I thought an experienced view might help.
Happy for any kind of help 😄

@drc38
Copy link
Collaborator

drc38 commented Dec 28, 2024

@drc38 although I added an assert in the test_init.py it does not feel to be the best place. As we do not have a test_sensor.py yet and I am not confident enough yet with pytest to build such a test from scratch easily I thought an experienced view might help. Happy for any kind of help 😄

Best to put the assert statements here

assert cs.get_unit("test_cpid", "Energy.Active.Import.Register") == "kWh"

Using mock config creates the integration's sensors so no need to do so manually. Thanks for contributing hope that helps.

@drc38
Copy link
Collaborator

drc38 commented Dec 28, 2024

Also add the device class here

DEFAULT_CLASS_UNITS_HA = {

And update reactive units with HA values

UnitOfMeasure.var: UnitOfMeasure.var,

@N3rdix N3rdix temporarily deployed to continuous-integration December 29, 2024 16:09 — with GitHub Actions Inactive
@N3rdix N3rdix temporarily deployed to continuous-integration December 29, 2024 16:15 — with GitHub Actions Inactive
@N3rdix N3rdix temporarily deployed to continuous-integration December 29, 2024 16:19 — with GitHub Actions Inactive
@N3rdix N3rdix marked this pull request as draft December 29, 2024 16:49
@N3rdix N3rdix temporarily deployed to continuous-integration December 30, 2024 09:01 — with GitHub Actions Inactive
@N3rdix N3rdix marked this pull request as ready for review December 30, 2024 09:04
@N3rdix
Copy link
Contributor Author

N3rdix commented Dec 30, 2024

Hi @drc38,
thanks for your hints, that helped to understand the tests better 👍

I've added the asserts and adjusted the mock data accordingly. Do you think this is enough/ok?

tests/test_charge_point_v16.py Show resolved Hide resolved
@N3rdix N3rdix temporarily deployed to continuous-integration December 30, 2024 16:02 — with GitHub Actions Inactive
@lbbrhzn lbbrhzn merged commit 22d09ea into lbbrhzn:main Jan 5, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charger power reactive export/import have lost their state classes and units after update to v0.6.3
3 participants