search

django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.47k stars 209 forks source link

Test asyncio.shield with sync and async middlware #432

Closed ttys0dev closed 9 months ago

ttys0dev commented 9 months ago

I noticed asyncio.shield doesn't work as expected when using sync middleware, I've wrote two tests, one which passes for async middleware and one which fails for sync middleware.

andrewgodwin commented 9 months ago

The code looks reasonable and I'll always take more tests - could you add a docstring to each test that describes exactly the situation it's testing?

ttys0dev commented 9 months ago

could you add a docstring to each test that describes exactly the situation it's testing?

Added some more details/comments, how does that look?

andrewgodwin commented 9 months ago

They're better, but I'm a little bit on the fence about including Django-specific tests in asgiref - I know they're not actually that Django specific as they're testing sync_to_async and async_to_sync, but the way they're phrased is a bit specific - that said, they're still good tests. I think I'll merge them in and then just modify the docstring a little to say "django style" or something.

Thanks for putting these together!

ttys0dev commented 9 months ago

the way they're phrased is a bit specific

I mostly just tried to copy the style of the existing django style tests.

By the way should I open a tracking issue for the disabled deadlocking test?