Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed import error when exceptiongroup isn't available #2231

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

agronholm
Copy link
Contributor

Fixes #2227. Closes #2229. This is an alternate version of the linked PR.

Summary

Last minute changes to the AnyIO 4.0 compatibility PR inadvertently caused an ImportError on Pythons earlier than 3.11 when the exceptiongroup back-port wasn't installed. This makes the import optional and thus restores compatibility with AnyIO 3.x.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@agronholm
Copy link
Contributor Author

I also went ahead and updated the pinned AnyIO version as v4.0.0rc1 is out now.

@fluix-dev
Copy link

This would also need something like the following patch to fix the tests for anyio < 4 which #2228 raises.

From 917d1c135635d6d9e5aec83633673ec21a529933 Mon Sep 17 00:00:00 2001
From: Steven Guikal <void@fluix.one>
Date: Sun, 30 Jul 2023 17:18:40 -0400
Subject: [PATCH] Fix wsgi test for AnyIO versions before 4.0.0

---
 tests/middleware/test_wsgi.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/middleware/test_wsgi.py b/tests/middleware/test_wsgi.py
index ad39754..5b2508c 100644
--- a/tests/middleware/test_wsgi.py
+++ b/tests/middleware/test_wsgi.py
@@ -2,6 +2,7 @@ import sys
 
 import pytest
 
+from importlib.metadata import version
 from starlette.middleware.wsgi import WSGIMiddleware, build_environ
 
 if sys.version_info < (3, 11):  # pragma: no cover
@@ -69,11 +70,15 @@ def test_wsgi_exception(test_client_factory):
     # The HTTP protocol implementations would catch this error and return 500.
     app = WSGIMiddleware(raise_exception)
     client = test_client_factory(app)
-    with pytest.raises(ExceptionGroup) as exc:
-        client.get("/")
-
-    assert len(exc.value.exceptions) == 1
-    assert isinstance(exc.value.exceptions[0], RuntimeError)
+    if version("anyio") < "4":
+        with pytest.raises(RuntimeError):
+            client.get("/")
+    else:
+        with pytest.raises(ExceptionGroup) as exc:
+            client.get("/")
+
+        assert len(exc.value.exceptions) == 1
+        assert isinstance(exc.value.exceptions[0], RuntimeError)
 
 
 def test_wsgi_exc_info(test_client_factory):
-- 
2.41.0

@agronholm
Copy link
Contributor Author

So do you want me to keep 4.0.0rc1 as the pinned AnyIO version? If so, how do I prove that it also passes on 3.x?

@fluix-dev
Copy link

I'm not a contributor so I'm not sure what compatibility guarantees are made or how tests are run. Applying my patch means distributions that have AnyIO < 4 in their repositories (aka everyone since 4.0.0 is only a pre-release) don't need to modify tests.

requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
agronholm and others added 2 commits August 23, 2023 09:57
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@agronholm
Copy link
Contributor Author

Curious – the coverage fails even on AnyIO 3.7.1. Maybe the test changes themselves caused this?

@Kludex Kludex closed this Aug 23, 2023
@Kludex Kludex reopened this Aug 23, 2023
@Kludex
Copy link
Member

Kludex commented Aug 23, 2023

Closed by mistake.

@agronholm
Copy link
Contributor Author

agronholm commented Aug 23, 2023

The only reason coverage fails here is that the code base is only tested against AnyIO 3 now, and tripping the missing line requires AnyIO 4. Should I add a # pragma: no cover there?

Ideally we would test against both but that may complicate the test suite too much.

@Kludex
Copy link
Member

Kludex commented Aug 24, 2023

Yes, add the pragma, please.

@agronholm agronholm requested a review from Kludex August 24, 2023 20:31
@Kludex Kludex merged commit 7349c60 into encode:master Aug 25, 2023
5 checks passed
@agronholm agronholm deleted the fix-exceptiongroup-importerror branch August 25, 2023 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No module named 'exceptiongroup' when upgraded to starlette 0.31.0
3 participants