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

Test asyncio.shield with sync and async middlware #432

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

ttys0dev
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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!

@andrewgodwin andrewgodwin merged commit 19e14e7 into django:main Jan 14, 2024
6 checks passed
@ttys0dev
Copy link
Contributor Author

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?

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.

None yet

2 participants