Remove modbus pragma no cover and solve nan #99221

janiversen · 2023-08-28T15:15:26Z

Breaking change

Proposed change

Added tests to be able to remove the last "pragma: no cover"

While removing the tests found a bug in the "nan" implementation, which is also corrected, since it in the same lines.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-08-28T15:15:35Z

Hey there @adamchengtkc, @vzahradnik, mind taking a look at this pull request as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of modbus can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign modbus Removes the current integration label and assignees on the pull request, add the integration domain after the command.

janiversen · 2023-09-06T18:22:41Z

@MartinHjelmare You reviewed the last PR with nan, this is a followup.

MartinHjelmare · 2023-09-07T08:33:38Z

homeassistant/components/modbus/base_platform.py

@@ -230,14 +234,20 @@ def unpack_structure_result(self, registers: list[int]) -> str | None:
                if isinstance(v_temp, int) and self._precision == 0:
                    v_result.append(str(v_temp))
                elif v_temp is None:
-                    v_result.append("")  # pragma: no cover
+                    v_result.append("nan")


Where is the nan result handled in the sensor entity?

the short answer is "the same way as line 240 is handled, which is not changed in this PR" 😬

The long answer is "it does not"

And I correct myself. This is not the normal sensor code, this is the part, where the user have requested a comma separated list of multiple values...this is returned as a string in the sensor code.

using "nan" in the string (as already done in line 240) secures that the string returned will look like:
"123,nan,17" and not "123,,17"

And how will the sensor parse that string when converting to a sensor state?

It will not, it is returned as a string and the user must have some automation that splits it.

Remark, this code is only active if the user configured "structure:" indicating he wants a comma separated string.

For normal sensors this code is bypassed (see "if len" a couple of lines earlier).

The comma separated list is actually some very old code I never touched...modern configurations use "slave_count:" which generates n normal sensors, so they can be used normally.

It looks like we'll raise a ValueError here if we either try to make an integer from "nan" or if we try to make a float we'll raise in the base sensor class since we don't allow infinite numbers:

core/homeassistant/components/modbus/sensor.py

Lines 124 to 130 in aff49cb

result = self.unpack_structure_result(raw_result.registers)

if self._coordinator:

if result:

result_array = list(

map(float if self._precision else int, result.split(","))

)

self._attr_native_value = result_array[0]

core/homeassistant/components/sensor/__init__.py

Lines 603 to 610 in aff49cb

if not isfinite(numerical_value):

raise ValueError(

f"Sensor {self.entity_id} has device class '{device_class}', "

f"state class '{state_class}' unit '{unit_of_measurement}' and "

f"suggested precision '{suggested_precision}' thus indicating it "

f"has a numeric value; however, it has the non-finite value: "

f"'{numerical_value}'"

)

Is there some guard for that happening that I'm not seeing?

hmmm you got a point, let me change it to a zero.

Actually a very good catch, thanks for that, I wonder why none of the people who use this have reported a problem....probably they have not had a "nan".

Changed !!

What I still do not understand, is why the test I made worked

Thanks again for catching this.

@MartinHjelmare a polite ping.

Regarding the tests, it's the slave sensors that have the coordinator which will use the problematic code path.

MartinHjelmare · 2023-09-12T10:55:29Z

homeassistant/components/modbus/base_platform.py

                elif v_temp != v_temp:  # noqa: PLR0124
                    # NaN float detection replace with None
-                    v_result.append("nan")  # pragma: no cover
+                    v_result.append("nan")


Don't we have the same problem here?

yup, I did change that too, but seems I forgot to press "save".

Done.

MartinHjelmare · 2023-09-12T11:00:05Z

Do you want to update the slave tests to cover the cases?

janiversen · 2023-09-12T11:07:35Z

It does not affect the slave sensors, but I missed one general sensor test ! corrected.

MartinHjelmare · 2023-09-12T11:23:39Z

Isn't this code path relevant for the slave sensors?

core/homeassistant/components/modbus/sensor.py

Lines 125 to 134 in 71207e1

    
           if self._coordinator: 
        
               if result: 
        
                   result_array = list( 
        
                       map(float if self._precision else int, result.split(",")) 
        
                   ) 
        
                   self._attr_native_value = result_array[0] 
        
                   self._coordinator.async_set_updated_data(result_array) 
        
               else: 
        
                   self._attr_native_value = None 
        
                   self._coordinator.async_set_updated_data(None)

janiversen · 2023-09-12T11:28:41Z

Yes that is slave_sensors, but the nan was in the receiving part, which is tested in async def test_struct_sensorin test_sensor.py.

this is the distribution, so no need to repeat the nan test here. But lets see if cov claims its missing.

MartinHjelmare · 2023-09-12T11:48:09Z

I don't believe we're covering the sensor coordinator code path in the test_struct_sensor test.

janiversen · 2023-09-12T11:51:46Z

no test_struct_sensor covers receiving data via struct....that is the nan changes.

async def test_slave_sensor in test_sensor should cover all the coordinator stuff....if I missed something coverage will tell.

MartinHjelmare · 2023-09-12T11:56:18Z

Right, so my suggestion is that we add a test that covers a coordinator sensor needing to handle the changed NaN cases in unpack_structure_result, as that's the case that would fail unless we included the review comments.

janiversen · 2023-09-12T12:09:10Z

Got you, done.

More test is always good, I am proud of having coverage report 100% coverage so most changes without a proper test is caught.

MartinHjelmare · 2023-09-12T14:05:36Z

Thanks!

* Remove pragma no cover. * Ruff ! * Review comments. * update test. * Review. * review. * Add slave test.

home-assistant bot added bugfix cla-signed has-tests integration: modbus small-pr PRs with less than 30 lines. by-code-owner Quality Scale: gold labels Aug 28, 2023

janiversen force-pushed the solve_no_cover branch 2 times, most recently from 3007261 to 926ed1e Compare September 3, 2023 19:38

janiversen changed the title ~~Remove modbus pragma no cover~~ Remove modbus pragma no cover and solve nan Sep 4, 2023

janiversen added this to the 2023.9.1 milestone Sep 6, 2023

janiversen requested a review from MartinHjelmare September 6, 2023 18:21

MartinHjelmare reviewed Sep 7, 2023

View reviewed changes

janiversen requested a review from MartinHjelmare September 7, 2023 19:40

bramkragten modified the milestones: 2023.9.1, 2023.9.2 Sep 8, 2023

janiversen force-pushed the solve_no_cover branch 2 times, most recently from 9081bc6 to 90cfbf6 Compare September 9, 2023 19:39

janiversen added 4 commits September 12, 2023 11:03

Remove pragma no cover.

169da59

Ruff !

fef8540

Review comments.

bc7f72f

update test.

fa5402c

MartinHjelmare reviewed Sep 12, 2023

View reviewed changes

Review.

ab8491f

janiversen force-pushed the solve_no_cover branch from cd36e39 to ab8491f Compare September 12, 2023 10:57

janiversen requested a review from MartinHjelmare September 12, 2023 10:58

review.

8510071

Add slave test.

400e814

MartinHjelmare approved these changes Sep 12, 2023

View reviewed changes

MartinHjelmare merged commit c178388 into home-assistant:dev Sep 12, 2023
31 checks passed

janiversen deleted the solve_no_cover branch September 12, 2023 14:53

balloob added the cherry-picked label Sep 12, 2023

balloob pushed a commit that referenced this pull request Sep 12, 2023

Remove modbus pragma no cover and solve nan (#99221)

d5ff05b

* Remove pragma no cover. * Ruff ! * Review comments. * update test. * Review. * review. * Add slave test.

balloob mentioned this pull request Sep 12, 2023

2023.9.2 #100223

Merged

github-actions bot locked and limited conversation to collaborators Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove modbus pragma no cover and solve nan #99221

Remove modbus pragma no cover and solve nan #99221

janiversen commented Aug 28, 2023

home-assistant bot commented Aug 28, 2023

janiversen commented Sep 6, 2023

MartinHjelmare Sep 7, 2023

janiversen Sep 7, 2023 •

edited

Loading

janiversen Sep 7, 2023

MartinHjelmare Sep 7, 2023

janiversen Sep 7, 2023 •

edited

Loading

MartinHjelmare Sep 9, 2023

janiversen Sep 9, 2023 •

edited

Loading

janiversen Sep 9, 2023

janiversen Sep 11, 2023

MartinHjelmare Sep 12, 2023

MartinHjelmare Sep 12, 2023

janiversen Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023 •

edited

Loading

MartinHjelmare commented Sep 12, 2023

	result = self.unpack_structure_result(raw_result.registers)
	if self._coordinator:
	if result:
	result_array = list(
	map(float if self._precision else int, result.split(","))
	)
	self._attr_native_value = result_array[0]

	if not isfinite(numerical_value):
	raise ValueError(
	f"Sensor {self.entity_id} has device class '{device_class}', "
	f"state class '{state_class}' unit '{unit_of_measurement}' and "
	f"suggested precision '{suggested_precision}' thus indicating it "
	f"has a numeric value; however, it has the non-finite value: "
	f"'{numerical_value}'"
	)

Remove modbus pragma no cover and solve nan #99221

Remove modbus pragma no cover and solve nan #99221

Conversation

janiversen commented Aug 28, 2023

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Aug 28, 2023

janiversen commented Sep 6, 2023

Choose a reason for hiding this comment

janiversen Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023

MartinHjelmare commented Sep 12, 2023

janiversen commented Sep 12, 2023 • edited Loading

MartinHjelmare commented Sep 12, 2023

janiversen Sep 7, 2023 •

edited

Loading

janiversen Sep 7, 2023 •

edited

Loading

janiversen Sep 9, 2023 •

edited

Loading

janiversen commented Sep 12, 2023 •

edited

Loading