-
-
Notifications
You must be signed in to change notification settings - Fork 29
Fix tests for PR #282 mompow field - update property counts, add test assertions, convert values to Watts, implement power state logic, remove architectural redundancy, and add changelog entry #292
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
base: master
Are you sure you want to change the base?
Conversation
@Apollon77 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: Apollon77 <[email protected]>
…est assertions Co-authored-by: Apollon77 <[email protected]>
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.
@copilot please also check the details of the original or comments on how the value needs to be read. I would like to expose the value in Watt so we need to multiply it depending on the value
…#282 discussion Co-authored-by: Apollon77 <[email protected]>
I've updated the Changes made in commit e3fd5c1:
All 26 tests pass successfully with the new conversion logic. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot The initial PR includes an info that also value 1 was reported when "pow=0" which seems odd. Please add logic that considers the "pow" flag and sets the mompow field to 0 if pow=0 |
Co-authored-by: Apollon77 <[email protected]>
I've added logic to consider the "pow" flag as requested. When pow=0 (AC is off), the mompow field is now set to 0 regardless of the raw sensor value. This addresses the odd behavior where value 1 was reported when pow=0. The implementation checks for the pow field in the sensor response and applies the conversion logic accordingly. All tests pass successfully including new test cases that verify this behavior. Commit: d9a3dcc Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copiolot The updateData() method in DaikinAc class could be a good place to add the high level logic and set mompow to 0 when pow=0, please check if there is even a better place down the code flow. And please add the logic |
@copilot The updateData() method in DaikinAc class could be a good place to add the high level logic and set mompow to 0 when pow=0, please check if there is even a better place down the code flow. And please add the logic |
…t expectations Co-authored-by: Apollon77 <[email protected]>
I've added the high-level logic to set Changes made in commit 3c65cd2:
The implementation correctly handles the architectural separation where Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…te logic when control info is available Co-authored-by: Apollon77 <[email protected]>
Co-authored-by: Apollon77 <[email protected]>
…ompow=1 Co-authored-by: Apollon77 <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@lleonis sooo .... now? :-) |
Now it looks OK. Thanks. Test data is a bit unrealistic with inputs like |
LOL. Yeeaahh but I agree. Testing is testing ;-) |
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.
@copilot now again the changelog entry is missing. Please add.
Co-authored-by: Apollon77 <[email protected]>
Added changelog entry for version 2.3.0 documenting the mompow field support feature in commit 76764bb. The changelog includes the main features: mompow field support, unit conversion to Watts, and power state logic. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This PR fixes the failing tests that were preventing PR #282 from being merged. PR #282 adds support for the
mompow
(momentary power) sensor property to theSensorInfoResponse
class, but the existing tests had hardcoded expectations that broke when the new field was added.Problem
When PR #282 added the
mompow
field toSensorInfoResponse
, it increased the number of properties from 5 to 6, causing several test assertions to fail:Object.keys(response).length === 5
but now there are 6 propertiesmompow
field functionalitySolution
mompow?: number
property and parsing logic toSensorInfoResponse.ts
mompow
field to mock sensor data with different values (150, 200, 300)mompow
field is properly parsed and accessiblegetACSensorInfo()
method to setmompow=0
when the AC is powered off (pow=0
)demand_control
mocks to prevent test hanging issuesupdateData()
andgetACSensorInfo()
methodsChanges Made
mompow
property and parsing with unit conversion (×100)getACSensorInfo()
method only (removed duplicate fromupdateData()
)Test Results
✅ All 27 tests now pass successfully (increased from 26 due to new test case)
✅ SensorInfoResponse.ts maintains high test coverage
✅ New mompow field is properly tested with various values
✅ Backward compatibility preserved - existing functionality unchanged
✅ Power state logic works correctly and is properly tested
✅ Architectural redundancy eliminated - cleaner, more maintainable code
Unit Conversion Details
The
mompow
field now represents momentary power consumption in Watts:mompow=15
→ Library exposes:mompow=1500
(Watts)Power State Logic
Implemented at the
getACSensorInfo()
method level:power=false
in control_info),mompow
is set to 0 regardless of raw sensor valuemompow=1
butpow=0
Architecture Note
Power state logic is correctly implemented at the
DaikinAC.getACSensorInfo()
method level where bothget_control_info
andget_sensor_info
data can be made available together. Previous duplicate implementation inupdateData()
was redundant sinceupdateData()
callsgetACSensorInfo()
which already applies the logic.Changelog
Added version 2.3.0 entry to README.md changelog documenting:
Fixes #291.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.