Skip to content
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

Wrong integration is loaded when overriding an existing one #64

Closed
tetienne opened this issue May 20, 2021 · 18 comments
Closed

Wrong integration is loaded when overriding an existing one #64

tetienne opened this issue May 20, 2021 · 18 comments

Comments

@tetienne
Copy link

tetienne commented May 20, 2021

Hi,

I want to thank you for this really great package. It really ease the tests for our custom components.

In particular, we use it for this component: https://github.com/iMicknl/ha-tahoma

The purpose of this component is to totally rewrite an existing one within HA core (Tahoma). Sadly, when we run our tests the legacy tahoma component is loaded instead of our. And so, our library is not found: https://github.com/iMicknl/ha-tahoma/pull/444/checks?check_run_id=2606320918#step:7:967

Normally, we should see in the log, this warning:

WARNING homeassistant.loader:loader.py:791 You are using a custom integration tahoma which has not been tested by Home Assistant. This component might cause stability problems, be sure to disable it if you experience issues with Home Assistant

like here

This last PR was still using https://github.com/boralyl/pytest-homeassistant.

Have you any idea on how to fix this?

@MatthewFlamm
Copy link
Owner

First your current errors on the current failing PR that you linked is due to missing requirements. Hopefully the PR I submitted fixed it. I need to see the real error related to this if it is still happening.

@MatthewFlamm
Copy link
Owner

MatthewFlamm commented May 20, 2021

My PR passed, so it was simple a requirements ommission?

GitHub being weird today. The test CI started way later. Will update this when it finishes

@MatthewFlamm
Copy link
Owner

Ok it does look like it is loading from the default homeassistant component. This a bit of a niche use case. Why is this not simply put as a PR to core or using a separate domain name?

@MatthewFlamm
Copy link
Owner

MatthewFlamm commented May 20, 2021

One thought to why this is occurring is that homeassistant does some voodoo to load the custom_components namespace package, whereas here, it needs to be a regular package if you want to import the package (or you have to implement the voodoo yourself). I wonder if that messes with the overriding of the existing component?

@tetienne
Copy link
Author

tetienne commented May 20, 2021

We have already opened several PRs on the Core repo to integrate our component update. But there is so many changes, it takes a lot of time.
Meanwhile we continue improvement and so the tests.

If you look at the previous pytest home assistant plug-in, can you guess if the developer mimic the voodoo?

@tetienne
Copy link
Author

Link to our main pr: home-assistant/core#49403

@MatthewFlamm
Copy link
Owner

I assume you had used the previous plug-in with a previous version of homeasisstant? If so, it is hard to rule out that it is a change in homeassistant that causes this difference. In fact, I suspect it is a homeassistant version difference if this is the case, i.e. you are using a newer homeassistant version in conjunction with this package.

My quick look through https://github.com/boralyl/pytest-homeassistant, it looks like it should work the same. The loading of integrations is done inside homeassistant, not the testing fixtures.

@MatthewFlamm
Copy link
Owner

If not too much work, it would be very valuable to know whether you have this issue by changing the domain and folder name to something else. This would pinpoint whether the title of this issue is truly correct (Wrong integration is loaded when overriding an existing one), or whether there is another issue.

@tetienne
Copy link
Author

I already tried before opening this issue. And with another domain it works fine.

@MatthewFlamm
Copy link
Owner

If the PR I submitted succeeds, I wonder if this fixture should be changed in this package to use autouse=True, so that all tests have it. I still havent wrapped my head around why all other uses of this package are unaffected.

@MatthewFlamm
Copy link
Owner

See home-assistant/core#43692

@tetienne
Copy link
Author

It will probably make sense indeed to use autouse=True. But, you have to be sure that's a patch to hide a kind of bug.

@MatthewFlamm
Copy link
Owner

I tried removing this fixture from some of the tests in core and it does indeed stop loading custom integrations that do not override existing integrations. The common usage with this package seems to be unaffected however.

I think the best course of action is to put this info into the README. I don't want to force to use this fixture without fully understanding it.

@MatthewFlamm
Copy link
Owner

What a coincidence but the newest version of homeassistant fixed the loophole. Now all custom components require this.

@tetienne
Copy link
Author

So you mean the fixture is required without no doubt?

@MatthewFlamm
Copy link
Owner

Correct and using autouse=True in this package doesn't work due to some interaction between pytest and homeassistant's thread testing. I recommend creating your own autoused fixture.

@MatthewFlamm
Copy link
Owner

I suspect this PR closed the loop hole that we were all using home-assistant/core#49916. A lot of core tests had this fixture added in this PR.

@MatthewFlamm
Copy link
Owner

I think this is fixed now, so I'm going to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants