-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Async integration tests #2001
Async integration tests #2001
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the Django docs regarding async_client
and now it seems to me that the tests should do what I think they do.
I really wish the reviewing interface would show what actually changed compared to the sync integration tests. Maybe we could copy the file in a separate pull request and start changing it later the next time we do something similar so that the reviewing interface is more helpful.
|
||
@override_settings(ROOT_URLCONF="tests.urls_use_package_urls") | ||
async def test_include_package_urls(self): | ||
"""Test urlsconf that uses the debug_toolbar.urls in the include call""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that comment was already there, but I don't understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good! I've got a few requests for changes, but some of them are discussions.
tests/test_integration_async.py
Outdated
self.assertContains(response, "ASCII") # template | ||
self.assertContains(response, "djDebug") # toolbar | ||
|
||
response = self.client.get("/regular/LÀTÍN/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be async_client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed it, though it seems to fail at async_client
; )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why assert is failing cause manually it seems to work. for now I am changing LATIN
to ASCII
for this too.
…s over concurrent db queries
for more information, see https://pre-commit.ci
… with async client asserion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change and we should merge it, or do you have any reservations @tim-schilling ?
@matthiask I didn't review it thoroughly, but it looks like test changes. I'm good with merging them if you're happy. I'll be less busy in ~2 weeks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval based on Matthias' approval 😁 (I skimmed over the changes quickly)
Let's do this then :) Thanks! |
Description
integrations tests to test the async toolbar.
Fixes # (issue)
Checklist:
docs/changes.rst
.