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

Async integration tests #2001

Merged
merged 15 commits into from
Sep 19, 2024
Merged

Conversation

salty-ivy
Copy link
Member

@salty-ivy salty-ivy commented Sep 2, 2024

Description

integrations tests to test the async toolbar.

Fixes # (issue)

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@salty-ivy salty-ivy marked this pull request as ready for review September 8, 2024 18:27
Copy link
Member

@matthiask matthiask left a 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.

tests/base.py Show resolved Hide resolved
tests/views.py Outdated Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved

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

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.

tests/test_integration_async.py Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved
@salty-ivy salty-ivy changed the title Asyn integration tests Async integration tests Sep 9, 2024
Copy link
Contributor

@tim-schilling tim-schilling left a 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 Show resolved Hide resolved
tests/test_integration_async.py Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved
self.assertContains(response, "ASCII") # template
self.assertContains(response, "djDebug") # toolbar

response = self.client.get("/regular/LÀTÍN/")
Copy link
Contributor

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?

Copy link
Member Author

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 ; )

Copy link
Member Author

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.

tests/test_integration_async.py Outdated Show resolved Hide resolved
tests/test_integration_async.py Outdated Show resolved Hide resolved
Copy link
Member

@matthiask matthiask left a 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 ?

@tim-schilling
Copy link
Contributor

@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

Copy link
Contributor

@tim-schilling tim-schilling left a 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)

@matthiask matthiask merged commit 7290f9c into jazzband:main Sep 19, 2024
25 checks passed
@matthiask
Copy link
Member

Let's do this then :) Thanks!

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.

4 participants