Skip to content

Fix Monoprice robustness#33869

Merged
springstan merged 5 commits intohome-assistant:devfrom
OnFreund:monoprice_fixes
Apr 9, 2020
Merged

Fix Monoprice robustness#33869
springstan merged 5 commits intohome-assistant:devfrom
OnFreund:monoprice_fixes

Conversation

@OnFreund
Copy link
Copy Markdown
Contributor

@OnFreund OnFreund commented Apr 9, 2020

Proposed change

A couple of Monoprice robustness fixes:

  1. Limit parallel updates - there is only one serial connection.
  2. Silently handle failed zone updates

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)
  • 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
  • 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:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant Bot commented Apr 9, 2020

Hey there @etsinko, mind taking a look at this pull request as its been labeled with a integration (monoprice) you are listed as a codeowner for? Thanks!

OnFreund
OnFreund commented Apr 9, 2020
View reviewed changes
Comment thread homeassistant/components/monoprice/media_player.py Outdated
state = self._monoprice.zone_status(self._zone_id)
except SerialException:
_LOGGER.warning("Could not update zone %d", self._zone_id)
return False
Copy link
Copy Markdown
Contributor Author

@OnFreund OnFreund Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy with this - ideally I would have raised an UpdateFailed here. However, since this is not actually an error for Zones that aren't connected, there are two negative side effects to raising here:

  1. The logs contain an error
  2. The entities are never created (instead of created and disabled)

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is checking the return value of update. Please remove those and just return where needed.

@MartinHjelmare MartinHjelmare changed the title Monoprice robustness fixes Fix Monoprice robustness Apr 9, 2020
@OnFreund OnFreund requested a review from MartinHjelmare April 9, 2020 08:56
MartinHjelmare
MartinHjelmare approved these changes Apr 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 9, 2020

Should we tag it for a patch release?

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented Apr 9, 2020

I believe we should

@OnFreund
Copy link
Copy Markdown
Contributor Author

OnFreund commented Apr 9, 2020

Not sure why codecov is showing a diff hit - I added a test for the new code, and the "Coverage Changes" tab is showing a file from a different integration (Yamaha)

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 9, 2020

If we add a test for this case, codecov will pass:
https://codecov.io/gh/home-assistant/core/compare/b46eee04e475a1211e10252bca30c4a46f1d474e...5e0ca3475c20c240371f6fd60e3f5da40ad60f6d/diff#D2-141

@MartinHjelmare MartinHjelmare added this to the 0.108.2 milestone Apr 9, 2020
MartinHjelmare
MartinHjelmare approved these changes Apr 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@springstan springstan merged commit a4c9446 into home-assistant:dev Apr 9, 2020
balloob pushed a commit that referenced this pull request Apr 10, 2020
@OnFreund @balloob
Fix Monoprice robustness (#33869)
2ff255d 
* Silently handle update failures

* Limite parallel updates

* Remove return values

* Remove trailing return

* Add test for empty update
@balloob balloob mentioned this pull request Apr 10, 2020
Merged
@lock lock Bot locked and limited conversation to collaborators Apr 15, 2020
@OnFreund OnFreund deleted the monoprice_fixes branch April 25, 2020 15:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

No one assigned

Labels

cherry-picked cla-signed has-tests integration: monoprice small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

0.108.2

Development

Successfully merging this pull request may close these issues.

5 participants

@OnFreund @MartinHjelmare @balloob @homeassistant @springstan