jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
329 stars 35 forks source link

Mean power Update #150

Closed adi90x closed 1 year ago

adi90x commented 1 year ago

Hello, Reference Issue #146 Mean Power shouldn't multiply device_power by number of heater registered. Also add a small information on that in the documentation.

jmcollin78 commented 1 year ago

Hello @adi90x ,

Thank you for your PR. I wonder why no unit tests have changed. It seems not normal if the rule changed.

adi90x commented 1 year ago

@jmcollin78 I guess there is no unit test for that 😁

jmcollin78 commented 1 year ago

@jmcollin78 I guess there is no unit test for that 😁

Does test_power.py pass ? If you change something, something must break or please add a test that shows what you have done works.

EDIT : I try with your change and all pass without error. So can you please add a test that shows you change works ? In long terms, it is essential to have a good coverage of the code. Thank you in advance. You have the test_power.py in example if you want.

adi90x commented 1 year ago

@jmcollin78 I guess there is no unit test for that 😁

Does test_power.py pass ? If you change something, something must break or please add a test that shows what you have done works.

EDIT : I try with your change and all pass without error. So can you please add a test that shows you change works ? In long terms, it is essential to have a good coverage of the code. Thank you in advance. You have the test_power.py in example if you want.

I think that it pass because test is done with only one heater so there is no difference in the mean_cycle_power to I update. Will try to switch test_power_management_energy_over_switch to use 2 heater.

adi90x commented 1 year ago

Hello, Was quick in fact previous test was using mean_power_cycle attribute instead of device_power. Test is OK with the commit and NOK before.

jmcollin78 commented 1 year ago

Thank you @adi90x !