Fix flaky light group test

[balloob]

Description:

https://travis-ci.org/home-assistant/home-assistant/jobs/348247842#L994
#12229 (comment)

The new light.group test introduced a flaky test. It's caused by having an async state change listener that is a callback. Callbacks are not tracked by the core when tracking jobs and thus will not be waited for when calling hass.async_block_till_done(). They can usually be flushed out by doing a sleep(0) which async_block_till_done does, but if a callback calls another callback, that won't work.

So for this case, let's switch to calling async_block_till_done twice.

Checklist:

If the code does not interact with devices:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• Tests have been added to verify that the new code works.

[OttoWinter]

Oh noes :/ I've tried your patch locally, but the tests still seems to be flaky. I went ahead and added the await hass.async_block_till_done() statement 6 times to your proposed places but still the test_service_calls test fails.

Now comes the confusing part: If I add a print("async_added_to_hass") or a _LOGGER.debug statement here, all tests work fine 🤔 (tested about 10 times each). With another print statement inside the async_state_changed_listener I was able to verify that our state changed listener is never called without having some I/O related statement (like print) before. I now too little about the async_added_to_hass and async_track_state_change functions, but I think we could do one of these things:

• Make async_state_changed_listener a coroutine
• Have our async_update method be called manually on async_added_to_hass. Just a self.async_schedule_update_ha_state(True) after the state change listener is initialized.
  • Benefit: When we get added to hass, all our child lights might have already been initialized and we don't receive a state changed callback.
• Alternatively, passing True to async_add_devices should also do the same.

Using the "await hass.async_block_till_done() twice" method we'd also have to update all other test cases, I believe.

Edit:
Douh! I think I now why this is happening now:

When we get added to hass, all our child lights might have already been initialized and we don't receive a state changed callback.

[balloob]

We should not make changes to the code to make it slower to fight a flaky test

[OttoWinter]

@balloob All with you on that, but I think the error doesn't lie in the async_block_till_done, but in us not receiving the initial states of all children if we're initialized after them. See edit from before:

When we get added to hass, all our child lights might have already been initialized and we don't receive a state changed callback.

[balloob]

oh, good point. In that case we should collect all existing states when getting added to HASS and process them.

[balloob]

So I guess I'll close this PR then as it is not the right solution.

[OttoWinter]

Exactly, but which one of these do you prefer?

As far as I see they pretty much have the same result, but there might be some differences between the two.

1.

# in async_setup_platform()
async_add_devices([GroupLight(config.get(CONF_NAME),
                              config[CONF_ENTITIES])], True) # True <-------

2.

async def async_added_to_hass(self) -> None:
    """Register callbacks."""
    @callback
    def async_state_changed_listener(entity_id: str, old_state: State,
                                     new_state: State):
        """Handle child updates."""
        self.async_schedule_update_ha_state(True)

    self._async_unsub_state_changed = async_track_state_change(
        self.hass, self._entity_ids, async_state_changed_listener)
    self.async_schedule_update_ha_state(True) # <-----

[balloob]

The first one.