MatthewFlamm / pytest-homeassistant-custom-component

Package to automatically extract testing plugins from Home Assistant for custom component testing
MIT License
67 stars 11 forks source link

Wrong integration is loaded when overriding an existing one #64

Closed tetienne closed 3 years ago

tetienne commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Link to our main pr: https://github.com/home-assistant/core/pull/49403

MatthewFlamm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

MatthewFlamm commented 3 years ago

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 commented 3 years ago

See https://github.com/home-assistant/core/pull/43692

tetienne commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

tetienne commented 3 years ago

So you mean the fixture is required without no doubt?

MatthewFlamm commented 3 years ago

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 commented 3 years ago

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

MatthewFlamm commented 3 years ago

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