Fix test typing

[florian-wagner-frequenz]

Previously there was a test which violated type-hints. As we can not keep having the test and type hints on that test, the test itself was rewritten to test a semantically slightly different but similar edge case.
The original test case is of course prevented when using type-hints (i.e not passing None to a function that doesn't accept None)

[Copilot]

Pull Request Overview

This PR updates a test to avoid passing None and instead validates behavior with an empty config dict, aligning with current type hints.

• Renames test from test_load_config_load_None to test_load_config_load_empty
• Updates the docstring to describe the empty-config edge case
• Changes the load_config call to pass the entire config dict instead of config.get("loggers", None)

Comments suppressed due to low confidence (1)

tests/config/test_util.py:57

• [nitpick] The test name test_load_config_load_empty repeats 'load'. Consider renaming it to test_load_config_empty for clarity.

def test_load_config_load_empty(

[florian-wagner-frequenz]

Fixes the underlying problem preventing #1234 from proceeding

[llucax]

The suggested improvements to the tests are completely optional, not approving only because I think we should not add stuff that users don't care about in the release notes.

[Marenz]

removed this pull request from the merge queue due to no response for status checks 1 hour ago

hmm maybe my fix 45c4ad7
wasn't a fix?

[florian-wagner-frequenz]

Trying once more in the hope that this is spurious. Otherwise I would leave it to @Marenz to figure out what is going on

[florian-wagner-frequenz]

Yup, your problem now @Marenz

[llucax]

It looks to me that we have a more fundamental problem if this fixes the infinite loop issue. Both the actor and the battery status tracker loops should exit when there is a CancelledError, so it looks like we are forgetting to cancel at some point, or worse, someone is eating up the CancelledError in the middle.

This hack will uncover this more fundamental issue, which could come to bite us in other ways, so I would try to find the real root cause for this instead of patching it just to pass the tests.

[llucax]

Also probably not the best idea to hijack this PR to fix the CI, why didn't you create a new PR like last time?

[Marenz]

Also probably not the best idea to hijack this PR to fix the CI, why didn't you create a new PR like last time?

Because this was easier to test, and well, it's working now.. it wasn't reliably reproducable with the other PR which is how we ended up in this situation in the first place ;)

so I would try to find the real root cause for this

This is fixing the real root cause.
I wrote it on slack last week, but here is the analysis:

I think I got it. So the run loop in _batter_status_tracker will never quit except for a cancelled exception. On shutdown, it will get the eventloop closed error and will try again..and again..and again.. and because the eventloop is closed, it's never giving up control to let other tasks properly clean up, including the task that would trigger the cancelexception for it (componentpool_status_tracker.py the _run method with the async exit stack context lib thing).

I didn't write it explicitly, but the exact same thing is also happening in the actor-restart part. The actor stops because of the RuntimeError: Eventloop is closed error makes it try to restart before any chancel exception can be done.

[llucax]

Because this was easier to test, and well, it's working now.. it wasn't reliably reproducable with the other PR which is how we ended up in this situation in the first place ;)

You could have created another PR with the same commits as this PR for testing. Now is like the original PR got completely lost in the noise, and we are also spamming @florian-wagner-frequenz unnecessarily :P

Also discussing via slack, but for the records, what I mean about root cause is that these loops should finish, before the event loop is closed, via a CancelledError. If this is not happening it means that something of these is happening:

• We are missing a cancel()
• We are missing an await on the cancelled task
• We are doing the above, but somehow after the loop was closed (in __del__ for example).

We should find and fix when the loop is being closed without every task being properly stopped and awaited, instead of just hiding when it happens pretending that nothing is wrong... 😱

[Marenz]

and we are also spamming florian-wagner-frequenz unnecessarily :P

Nah, he announced that he unsubscribed and gave over ownership to me :) (that is, until you highlighted him again now :P )
His part is reviewed and approved, nothing to get lost.

But yes, I suppose for future historians this is a less optimal conflation of things that get fixed.

[llucax]

Looks good to me in general, but:

1. After you verified it works, please open a new PR with these fixes, these fixes are too important to be sneaked in a PR that was trying to fix something much more trivial.
2. It would be good to verify that this works even without handling the case where the loop is gone in run_forever() and Actor._run_loop(), but I guess if we make them crash instead of continuing as if nothing happened, that also works, as pytest will fail/error the tests that crashed.

[llucax]

This is actually wrong, get_event_loop() is deprecated in Python 3.12, get_running_loop() is recommended but then this function will raise if there is no running event loop. Apparently there is no reliable way to query if an event loop is running in the current context that is not calling get_running_loop() and catch the RuntimeError, so we should probably do that here too, or unify it with except RuntimeError, as catching `RuntimeError in particular doesn't help in any way if we are not inspecting the message (which I agree is less reliable).

All this said, and since this is just a safety net to avoid infinite loop in the presence of a HUGE bug (async code should never run outside of the context of an event loop), instead of returning I would try to crash the application with a backtrace. Maybe log the exception and then just simply assert False, or even sys.exit(-1) to ensure this assert is not simply swallowed by some dead task that didn't propagated the exception because it wasn't awaited. For me in this case crashing the complete thing makes sense because once there is no more event loop, we can't really do any work, it doesn't make sense to try to continue after knowing there is no loop anymore.

I think this could help us figuring out exactly which part of the code is entering an infinite loop.

[llucax]

Same here, assert False instead of break.