Skip to content

Fix unhandled exceptions for config, default_config, harmony #33731

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.

Sign up for GitHub

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

Jump to bottom
Merged
MartinHjelmare merged 3 commits into from
Apr 6, 2020
Merged

Fix unhandled exceptions for config, default_config, harmony #33731

MartinHjelmare merged 3 commits into from
Apr 6, 2020

Conversation

@ziv1234
Copy link
Contributor

@ziv1234 ziv1234 commented Apr 6, 2020 •
edited by MartinHjelmare
Loading

Breaking change

Proposed change

did one PR for all three since they are tiny and i got tired of opening too many PRs...

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

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

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

probot-home-assistant bot commented Apr 6, 2020

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (config) you are listed as a codeowner for? Thanks!

@probot-home-assistant
Copy link

probot-home-assistant bot commented Apr 6, 2020

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

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 6, 2020

Is there an issue for harmony that should be referenced?

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 6, 2020

I don’t think so. I guess that when it was added to the JSON exceptions it was considered benign.
I personally think both are exceptions that shouldn’t be there and will upload in a few hours a PR that solves the remaining ones. They are pretty straightforward to resolve

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 6, 2020

There's a merge conflict.

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 6, 2020

There's a merge conflict.

fixed

MartinHjelmare
MartinHjelmare approved these changes Apr 6, 2020
View reviewed changes
Copy link
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.

Test failure is unrelated.

@MartinHjelmare MartinHjelmare merged commit cedf7e3 into home-assistant:dev Apr 6, 2020
@bdraco bdraco added this to the 0.108.0 milestone Apr 7, 2020
@bdraco
Copy link
Member

bdraco commented Apr 7, 2020

This needs to be backported to 0.108.0 as it will change the unique id for harmony to a str and will make it unstable when upgrading to 0.109.0

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020

I thought unique id is always a string. Isn’t it?
I don’t want to change unique ids so let me find another fix for the exceptions

@bdraco
Copy link
Member

bdraco commented Apr 7, 2020

@ziv1234. The unique is is new for 0.108 so it’s fine as long as this ships with .0

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020

Ok, but since I did it with the wrong assumption that unique Ids are always strings, and I am not the owner of harmony, I may have hurt the functionality there. Will find a better way to avoid the exceptions today. It just means that unique ids cannot be mocks. Not a real problem

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020

Will push a fix today so I don’t interfere with the integration functionality at all

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 7, 2020

unique_id should always be a string or None.

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020

ok, so in that case should we keep this as is since it won't cause backwards compatibility issues?
this is a recurring problem in many tests. if unique_id is part of a mock, it throws an exception when it tries to serialize it to storage. usually it is not a problem to define it explicitly in the mock and then it works, but some tests don't do that and ignore the background exceptions

@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020

just noticed that the PR is closed. Will open a new PR. was a simple change and now the code itself is exactly the original (Without the explicit cast to str)

@ziv1234 ziv1234 mentioned this pull request Apr 7, 2020
Closed
20 tasks
@ziv1234
Copy link
Contributor Author

ziv1234 commented Apr 7, 2020 •
edited by MartinHjelmare
Loading

new PR is #33777

@balloob balloob removed this from the 0.108.0 milestone Apr 7, 2020
@balloob
Copy link
Member

balloob commented Apr 7, 2020

Untagging this from the milestone in favor of #33777 fixing it in dev.

@balloob
Copy link
Member

balloob commented Apr 7, 2020

Oh, reading 33777 now. Ok, let's keep this in.

@balloob balloob added this to the 0.108.0 milestone Apr 7, 2020
balloob pushed a commit that referenced this pull request Apr 7, 2020
@ziv1234 @balloob
Fix unhandled exceptions for config, default_config, harmony (#33731)
4901fa2 
* replaced MagicMock with CoroutineMock to avoid exception

* added conversion to str so mock returns unique-id that doesn't throw json exception

* added non-empty config since hass throws exception when config is empty
@lock lock bot locked and limited conversation to collaborators Apr 15, 2020
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

@bdraco bdraco

@bramkragten bramkragten

Labels

cherry-picked cla-signed core has-tests integration: config integration: default_config integration: harmony small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

0.108.0

Development

Successfully merging this pull request may close these issues.

Fix default_config tests that have uncaught exceptions Fix config tests that have uncaught exceptions

6 participants

@ziv1234 @MartinHjelmare @bdraco @balloob @bramkragten @homeassistant