Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1387 +/- ##
==========================================
- Coverage 95.13% 94.55% -0.59%
==========================================
Files 2 2
Lines 473 514 +41
Branches 57 62 +5
==========================================
+ Hits 450 486 +36
- Misses 17 22 +5
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for picking this up so quickly @tjkuson 🙏🏻 # conftest.py
import asyncio
import pytest_asyncio
def pytest_asyncio_loop_factories(config, item):
return {
"asyncio": asyncio.new_event_loop,
}
@pytest_asyncio.fixture(autouse=True) # function scope
def enable_event_loop_debug() -> None:
event_loop = asyncio.get_event_loop()
event_loop.set_debug(True)
print(f"enable_event_loop_debug - {id(event_loop)}")
@pytest_asyncio.fixture(autouse=True, scope="session", loop_scope="session")
async def some_fixture():
event_loop = asyncio.get_event_loop()
print(f"\nsome_fixture - {id(event_loop)}")
yield
event_loop = asyncio.get_event_loop()
print(f"\nsome_fixture (after yield) - {id(event_loop)}")# test_file.py
import asyncio
async def test_some_function():
event_loop = asyncio.get_event_loop()
print(f"test_some_function - {id(event_loop)}")
assert 2 == 2test_folder/test_file.py::test_some_function[asyncio]
some_fixture - 4478765968
enable_event_loop_debug - 4478765968
test_some_function - 4478734016
PASSED
some_fixture (after yield) - 4478765968Here I'd expect the loop in |
tjkuson
force-pushed
the
fix-loop-factory-lifecycle
branch
from
2b08c50 to
7506d9f
Compare
tjkuson
force-pushed
the
fix-loop-factory-lifecycle
branch
from
7506d9f to
adfd7fd
Compare
|
@cdce8p That should be fixed now in the most recent commit. This complexity seems to be that when loop factories are provided, we have to manage everything the policy system used to manage implicitly, whilst supporting both systems, which gets tricky. The coverage miss is a false positive, perhaps due to the fact the path is exercised in a subprocess. Without the allegedly uncovered lines, the tests fail. |
|
Hmm... I just tested your lasted changes. While it works fine on Python 3.14, it now crashes on 3.13. Not entirely sure what's going on there. Below the full traceback for 3.13 with the example code I posted earlier. Full traceback=================================================================================================================================== test session starts ===================================================================================================================================
platform darwin -- Python 3.13.12, pytest-9.0.2, pluggy-1.6.0
rootdir: /.../pytest-asyncio
configfile: pyproject.toml
plugins: asyncio-1.2.1.dev135
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 1 item
test_folder/test_file.py
some_fixture - 4413564048
enable_event_loop_debug - 4413566288
test_some_function - 4413566288
.
some_fixture (after yield) - 4413564048
==================================================================================================================================== 1 passed in 0.01s ====================================================================================================================================
+ Exception Group Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/bin/pytest", line 7, in <module>
| sys.exit(console_main())
| ~~~~~~~~~~~~^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/config/__init__.py", line 223, in console_main
| code = main()
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/config/__init__.py", line 199, in main
| ret: ExitCode | int = config.hook.pytest_cmdline_main(config=config)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/pluggy/_hooks.py", line 512, in __call__
| return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
| ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/pluggy/_manager.py", line 120, in _hookexec
| return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
| ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/pluggy/_callers.py", line 167, in _multicall
| raise exception
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/pluggy/_callers.py", line 121, in _multicall
| res = hook_impl.function(*args)
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/main.py", line 365, in pytest_cmdline_main
| return wrap_session(config, _main)
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/main.py", line 360, in wrap_session
| config._ensure_unconfigure()
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/config/__init__.py", line 1171, in _ensure_unconfigure
| self._cleanup_stack.close()
| ~~~~~~~~~~~~~~~~~~~~~~~~~^^
| File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/contextlib.py", line 627, in close
| self.__exit__(None, None, None)
| ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
| File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/contextlib.py", line 619, in __exit__
| raise exc
| File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/contextlib.py", line 604, in __exit__
| if cb(*exc_details):
| ~~^^^^^^^^^^^^^^
| File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/contextlib.py", line 482, in _exit_wrapper
| callback(*args, **kwds)
| ~~~~~~~~^^^^^^^^^^^^^^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 95, in cleanup
| collect_unraisable(config)
| ~~~~~~~~~~~~~~~~~~^^^^^^^^
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 81, in collect_unraisable
| raise ExceptionGroup("multiple unraisable exception warnings", errors)
| ExceptionGroup: multiple unraisable exception warnings (3 sub-exceptions)
+-+---------------- 1 ----------------
| Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 33, in gc_collect_harder
| gc.collect()
| ~~~~~~~~~~^^
| ResourceWarning: unclosed <socket.socket fd=7, family=1, type=1, proto=0>
|
| The above exception was the direct cause of the following exception:
|
| Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 67, in collect_unraisable
| warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
| ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| pytest.PytestUnraisableExceptionWarning: Exception ignored in: <socket.socket fd=7, family=1, type=1, proto=0>
| Enable tracemalloc to get traceback where the object was allocated.
| See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
+---------------- 2 ----------------
| Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 33, in gc_collect_harder
| gc.collect()
| ~~~~~~~~~~^^
| ResourceWarning: unclosed <socket.socket fd=6, family=1, type=1, proto=0>
|
| The above exception was the direct cause of the following exception:
|
| Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 67, in collect_unraisable
| warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
| ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| pytest.PytestUnraisableExceptionWarning: Exception ignored in: <socket.socket fd=6, family=1, type=1, proto=0>
| Enable tracemalloc to get traceback where the object was allocated.
| See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
+---------------- 3 ----------------
| Traceback (most recent call last):
| File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/base_events.py", line 764, in __del__
| _warn(f"unclosed event loop {self!r}", ResourceWarning, source=self)
| ~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ResourceWarning: unclosed event loop <_UnixSelectorEventLoop running=False closed=False debug=False>
|
| The above exception was the direct cause of the following exception:
|
| Traceback (most recent call last):
| File "/.../pytest-asyncio/venv-313/lib/python3.13/site-packages/_pytest/unraisableexception.py", line 67, in collect_unraisable
| warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
| ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| pytest.PytestUnraisableExceptionWarning: Exception ignored in: <function BaseEventLoop.__del__ at 0x106f02200>
| Enable tracemalloc to get traceback where the object was allocated.
| See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
+------------------------------------ |
|
That warning is reproducible on |
|
Okay @cdce8p, I think the latest commit should fix the leak. Essentially, I think the fix I pushed is better: just skip |
|
Thanks for all your work here @tjkuson, really appreciated! I only had time to run some tests but those look quite promising. There is only one minor issue left: The order in which the runners / event loops are created seems to be the position of the fixture in source code. This could cause some (minor) duplicate work if the session one is placed after a function fixture. Previously pytest-asyncio would first create the session event loop and run all session fixtures, and only then create the function event loop and run the function fixtures before running the test case. I've modified my example by wrapping # conftest.py
import asyncio
import pytest_asyncio
def wrap_new_event_loop(*args, **kwargs):
loop = asyncio.new_event_loop(*args, **kwargs)
print(f"\n=> New event loop {id(loop)}")
return loop
def pytest_asyncio_loop_factories(config, item):
return {
"asyncio": wrap_new_event_loop,
}
@pytest_asyncio.fixture(autouse=True) # function scope
def enable_event_loop_debug() -> None:
event_loop = asyncio.get_event_loop()
event_loop.set_debug(True)
print(f"enable_event_loop_debug - {id(event_loop)}")
@pytest_asyncio.fixture(autouse=True, scope="session", loop_scope="session")
async def some_fixture():
event_loop = asyncio.get_event_loop()
print(f"\nsome_fixture - {id(event_loop)}")
yield
event_loop = asyncio.get_event_loop()
print(f"\nsome_fixture (after yield) - {id(event_loop)}")# test_file.py
import asyncio
async def test_some_function():
event_loop = asyncio.get_event_loop()
print(f"test_some_function - {id(event_loop)}")
assert 2 == 2-- |
|
No problem, @cdce8p! Async fixtures depend on the runner at setup time, but pytest's fixture cache doesn't know when the underlying loop changes. The PR was fixing this by inserting the loop factory fixture at the front of the fixtures list, which invalidated the cache for subsequent fixtures, but meant it could create loops for before they were strictly necessary. pytest does not seem to natively support dynamic fixture dependencies, so I don't think there's a fix that isn't somewhat hacky (though would love to be proven wrong)! However, resolving the fixture early seemed to fix the caching issue whilst preserving the desired order: dbe0009 |
|
Can confirm that all seems to work as expected now. AFAICT with that we can replace our custom event loop policy in Home Assistant. From a usability perspective the only (minor) annoyance IMO is that after #1373 every async test function has the parametrized name of our "default" loop factory name added. This would make sense for two or more different factories but for just one it's a bit redundant. Is there a way, maybe as an option, to hide the parameter if only one loop factory is specified? Just the loop factory in the test case name would cause some churn as a lot of our tests use snapshots (with |
|
@cdce8p What version of pytest are you running? The ability to hide a parameter via Also, @seifertm, this PR does quite a lot now. Would you prefer me to split up the PR into separate PRs? Each commit is its own fix, the only nuance being that dbe0009 fixes and issue with a commit in this very PR (7499c3d). |
We're on 9.0, so that's not an issue. Based on the documentation it seems I've got it down to just 3 failing test cases for the entire Home Assistant test suite. One is flaky and the other two seem to be genuine issues with the test itself. Will look into it some more later just to be sure. So overall it's looking great! |
tjkuson
force-pushed
the
fix-loop-factory-lifecycle
branch
from
ba9ab3e to
f819c49
Compare
Oops, I must have got confused reading the changelog/made a typo! I've amended comment in the commit, thanks for pointing that out. However, the point remains that it's not supported the minimum version of pytest supported by this plugin, which is 8.2 (I noticed the issue due to a failing test). |
It looks like you already addressed this, but it would be totally fine to bump the minimum supported pytest version to a newer 8.* release. I'll create a follow-up issue to track this. |
No need to split up the PR. Reviewing each commit individually is absolutely fine. |
seifertm
left a comment
seifertm
left a comment
There was a problem hiding this comment.
Thanks @tjkuson for following up on your initial loop factory PR! Also thank to @cdce8p for testing this. User feedback is extremely helpful for pytest-asyncio.
The code looks great. If we add a news fragment for the robustness improvements (calling e.g. asyncio.run in a sync fixture) and mention the changed test IDs for pytest <8.4, we can merge and create another prerelease.
| ), | ||
| ], | ||
| ) | ||
| def test_sync_fixture_sees_correct_loop_after_loop_broken_with_factory( |
There was a problem hiding this comment.
Up to this point, pytest-asyncio didn't support this. The argument was that loop control should be left to pytest-asyncio and calling any of those loop breaking actions is basically a user error. Unfortunately, these calls are sometimes made in third-party libraries leaving the user with a broken event loop.
The idea of wrapping sync fixtures and temporarily substituting the loop is quite nice. I think this is a step in the right direction and pytest-asyncio needs to reconsider the "loop control" argument in the future.
Let's just make sure we have a news fragment that mentions this as a feature.
|
|
||
| @pytest.mark.skipif( | ||
| not hasattr(pytest, "HIDDEN_PARAM"), | ||
| reason="pytest.HIDDEN_PARAM requires pytest 9.0+", |
There was a problem hiding this comment.
| reason="pytest.HIDDEN_PARAM requires pytest 9.0+", | |
| reason="pytest.HIDDEN_PARAM requires pytest 8.4+", |
| not hasattr(pytest, "HIDDEN_PARAM"), | ||
| reason="pytest.HIDDEN_PARAM requires pytest 9.0+", | ||
| ) | ||
| def test_single_factory_does_not_add_suffix_to_test_name( |
There was a problem hiding this comment.
This would also be my expectation. I didn't consider that this could break snapshot tests. We should mention in the existing news fragment that test IDs change when using a pytest version before 8.4.
| @@ -0,0 +1 @@ | |||
| Fixed ``pytest_asyncio_loop_factories`` not installing the custom event loop as the current loop, async fixture teardown/cache invalidation not being tied to the runner lifecycle, sync ``@pytest_asyncio.fixture`` seeing the wrong event loop when multiple loop scopes are active, and an event loop leak on Python 3.10-3.13. | |||
There was a problem hiding this comment.
The current release process keeps all news fragments between prereleases and only removes them on a stable release. That means the final changelog will reflect the changes between stable releases.
This PR addresses an issue introduced in a prerelease, so it doesn't necessarily need a news fragment.
Feel free to keep it, if you want to highlight changes between the current prerelease and the upcoming one, but I will probably edit it out before the stable release.
4 participants
Fixes issues in #1373 (ref).
Added regression tests.