Rewrite the tests

[erwindouna]

I'd be very much in favor of rewriting the test for the following reasons:

• It doesn't cover or doesn't test vital areas. For instance:

    def test_home_cant_be_set_to_auto_when_home_does_not_support_geofencing(
        self,
    ):
        # Test that the Tado home can't be set to auto geofencing mode when it
        # is not supported.
        with mock.patch(
            "PyTado.http.Http.request",
            return_value=json.loads(
                common.load_fixture("tadov2.home_state.auto_not_supported.json")
            ),
        ):
            self.tado_client.get_home_state()

        with mock.patch("PyTado.http.Http.request"):
            with self.assertRaises(Exception):
                self.tado_client.set_auto()

It attempts to mock, yet it doesn't assert or check any of the new states. It's crucial we test thoroughly the features we offer.

• Update to modern standards: for instance using the syrupy and using snapshots to test entire states. This will overall increase test quality, but also save quite the hazzle of writing long and repetitive test cases, such as:

def test_tadox_heating_manual_mode(self):
        """Test general homes response."""

        self.set_fixture("home_1234/tadox.heating.manual_mode.json")
        mode = self.tado_client.get_zone_state(1)

        assert mode.ac_power is None
        assert mode.ac_power_timestamp is None
        assert mode.available is True
        assert mode.connection == "CONNECTED"
        assert mode.current_fan_speed is None
        assert mode.current_humidity == 38.00
        assert mode.current_humidity_timestamp is None
        assert mode.current_hvac_action == "HEATING"
        assert mode.current_hvac_mode == "HEAT"
        assert mode.current_swing_mode == "OFF"
        assert mode.current_temp == 24.07
        assert mode.current_temp_timestamp is None
        assert mode.heating_power is None
        assert mode.heating_power_percentage == 100.0
        assert mode.heating_power_timestamp is None
        assert mode.is_away is None
        assert mode.link is None
        assert mode.open_window is False
        assert not mode.open_window_attr
        assert mode.overlay_active is True
        assert mode.overlay_termination_type == "NEXT_TIME_BLOCK"
        assert mode.power == "ON"
        assert mode.precision == 0.01
        assert mode.preparation is False
        assert mode.tado_mode is None
        assert mode.target_temp == 25.5
        assert mode.zone_id == 1

This can be replaced, by mechanisms from asynctado:

@pytest.mark.parametrize(
    ("fixture_file"),
    fixtures_files,
)
async def test_get_zone_state(
    fixture_file: str,
    python_tado: Tado,
    responses: aioresponses,
    snapshot: SnapshotAssertion,
) -> None:
    """Test get zone states."""
    zone_id = 1
    responses.get(
        f"{TADO_API_URL}/homes/1/zones/{zone_id}/state",
        status=200,
        body=load_fixture(fixture_file, folder="zone_state"),
    )
    assert await python_tado.get_zone_state(zone_id) == snapshot

• Add paramaterization for testing, to get a better coverage, but also reducing redunant code in the test cases.
• Let's see how much we deem as a desired code coverage. At least the basics of the features we offers should be covered and where feasible a 100% code coverage. That's open to discussion, of course.

[chiefdragon]

I created test_home_cant_be_set_to_auto_when_home_does_not_support_geofencing()
and two other related tests a couple of years ago when I added the ability to reset geofencing to auto. At the time they all had explicit asserts, but it looks like someone, I’m sure with good intentions, has refactored them. This one does have an assert (via assertraises - it’s not how I did it, but presumably it works fine), the other two tests have been renamed and no longer appear to have any asserts, which renders them of no value…
In case you’re improving the tests, which would be a very good idea, the rationale I took for the change I made was to be very cautious and try to avoid breaking anyone’s installation. To that end, i only allowed geofencing to be set to auto if I could be sure it was supported (as I don’t believe it is/was for everyone and could only test with my personal setup). In the event I couldn’t be sure from the data returned from the Tado API, I threw an exception if someone tried to change it to auto - and specifically a new one I created, called TadoNotSupportedException().
My original commit has longer and more descriptive test names, for reference, so you can see intent.
I hope that helps / is useful background. Good luck with future changes!

[wmalgadey]

but it looks like someone, I’m sure with good intentions, has refactored them

I actually didn't check the latest PRs bevor integration, because I didn't put a lot of trust in my python knowledge. For the future I would like to learn more about python, than what was needed bevor. That will never happen again 🙄

[wmalgadey]

I do not like the my_tado tests neither, and the reasons are the same @erwindouna! That's the reason I introduced the responses dependency and mocked the actual responses instead of the result a method.

The downside of this, it tests too many thinks IMHO, for instance the _configure_url method or the request handling. It is no red flag, but it would be nice to abstract hat away too, and do explicit testing for this functionality too.

In my opinion tests should ensure, that the intended use cases of a method will work, and make the usage of the library visible (I do like TDD a lot :)).