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

Adds ability to capture all the db queries at once #1177

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

kingbuzzman
Copy link
Contributor

@kingbuzzman kingbuzzman commented Feb 10, 2025

When managing multiple databases—over six in our case—we don’t focus on tracking queries for each one individually (e.g., db1 vs. db2 vs. db3). Instead, our priority is the overall number of queries executed. If that total seems off, we then dive deeper to identify the specific issue.

Old approach:

@pytest.mark.django_db(databases=["default", "log", "backoffice", "cart"])
def test_models(django_assert_num_queries):
    with django_assert_num_queries(3), django_assert_num_queries(2, using="log"), django_assert_num_queries(3, using="backoffice"), django_assert_num_queries(1, using="cart"):
        Model.objects.create(...)
        ...

Since we use a django database router these model's data store can change location due to size at any moment. When this happens, all our tests will break.

New approach:

@pytest.mark.django_db(databases=["default", "log", "backoffice", "cart"])
def test_models(django_assert_num_queries_all_connections):
    with django_assert_num_queries_all_connections(9):
        Model.objects.create(...)
        ...

@kingbuzzman kingbuzzman marked this pull request as ready for review February 10, 2025 20:19
@kingbuzzman
Copy link
Contributor Author

@bluetech let me know if there is something you want me to change

@bluetech
Copy link
Member

This seems sensible to me.

One thing, pytest-django tries to mostly follow Django upstream, and just uses django.test underneath as you know. So ideally, this feature suggestion would go through Django upstream.

Would you consider sending your suggestion to the Django developer list (https://forum.djangoproject.com/c/internals/5), or open a Django ticket to discuss it?

@adamchainz
Copy link
Member

adamchainz commented Feb 14, 2025

Yes, this seems like a good feature to add upstream first. I would propose it extending assertNumQueries to allow using to accept both an iterable of database names, like using={"default", "other"}, and some special value for all databases, probably using="__all__". There is precedent for __all__ in TestCase.databases.

Adding another fixture with “all connections” part in the name (django_assert_num_queries_all_connections seems unnecessary to me.

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.

3 participants