Would AssertMaxQueries (similar to AssertNumQueries) be a useful addition

[Roger Hunwicks]

I'm doing some refactoring of a Django app and I want to make sure that I don't introduce any really poorly performing pages as a result of poor queries.

I want to write a test that tries every registered URL and makes sure that none of them has an excessive number of queries.

AssertNumQueries doesn't seem very helpful to me, because it requires you to know when you write the test exactly how many queries the function should require.

I'd like to create an AssertMaxQueries that passes as long as the number of queries is less than or equal to the parameter provided to the context manager. You could also do this my adding a parameter to AssertNumQueries to tell it whether to test for equality or "less than or equal to": that would involve less duplicated code in testcases.py, but is possibly less discoverable for a test author.

Would that be a useful contribution to Django proper, or should I just create it in my own tests/utils.py.

Roger

[Michael Manfre]

AssertNumQueries is often a problem for 3rd party backends. AssertMaxQueries would become the same and likely result in poorly written tests because of the inherent slop factor in the arbitrarily chosen max value.

I'm not opposed to Django adding this. I'm opposed to Django using it in its own test suite.

Regards,

Michael Manfre

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4d8d120a-8819-4a7d-977f-4acae670130f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[James Bennett]

I like the idea -- if nothing else, it would provide an easy way to have a test suite discover N+1 problems, especially in templates.

Not sure about the feasibility, though; would you be willing to work up a rough implementation of it?

[Shai Berger]

On Sunday 17 August 2014 17:36:12 Michael Manfre wrote:
> AssertNumQueries is often a problem for 3rd party backends.
> AssertMaxQueries would become the same and likely result in poorly written
> tests because of the inherent slop factor in the arbitrarily chosen max
> value.
>

I agree with the concern, but I also note the intended use:

> On Sun, Aug 17, 2014 at 2:52 AM, Roger Hunwicks <roger.h...@gmail.com>

> > I want to write a test that tries every registered URL and makes sure
> > that none of them has an excessive number of queries.
> >

This implies that the limit chosen should not be tight; also, it sounds like
it is meant mostly for DML, where the differences in number of queries between
backends are relatively small (unlike DDL). I think with documentation noting
these points, AssertMaxQueries could be a welcome addition and maybe even
useful for Django's own test-suite.

That said,

> > Would that be a useful contribution to Django proper, or should I just
> > create it in my own tests/utils.py.
> >

I think you should first have it working for yourself, then, with some
experience in its use, offer it to the community. Don't put too much into that
"some experience" -- just make sure you write some tests with it and it really
works as well as you expect.

HTH,
Shai.

[Florian Apolloner]

I am not so convinced, what would you put in as the upper limit? While preventing n+1, it still requires you to know what n in this testcase is and changing n can lead to funny errors. Currently we are documenting (hopefully) how those query counts come together, so it's clear what's happening when it breaks…

[Jeremy Dunck]

I use this method on my own test subclasses, and I find it useful as a tripwire: a cause for review and consideration, more than a hard error. Did the number of queries go up on this change? Is that reasonable or a mistake? Have we blown the perf budget so we should refactor? Or maybe the number should just be raised to something that seems like a reasonable next tripwire.

These limits aren't flaky or false positives, so they seem useful supports given a reasonable test suite otherwise.

--

You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/41676693-480d-4dfc-82a6-377da0f54057%40googlegroups.com.

[Josh Smeaton]

+1 - useful for user code that guards against accidental poor ORM usage.

[Roger Hunwicks]

On Sunday, August 17, 2014 8:39:17 PM UTC+6, James Bennett wrote:

Not sure about the feasibility, though; would you be willing to work up a rough implementation of it?

Implementation of the assertion is straightforward, because it can be a direct copy of AssertNumQueries with:

self.test_case.assertLessEqual(

          executed, self.num, "%d queries executed, %d or less expected" % (

              executed, self.num

          )

instead of the the assertEqual in AssertNumQueries.

Regarding it's use, I run it with a low default and then make exceptions for specific urls that I know have more queries:

        max_queries = {'admin:app1_model1_change': 50,

                     'admin:app1_model2_add': 100,

                     'admin:app2_model1_changelist': 100}

<snip>

        for model in get_models():

          if model in admin.site._registry:

              try:

                 if admin.site._registry[model].has_add_permission(type("request", (), {"user": user})):

                        route = "admin:%s_%s_add" % (model._meta.app_label, model._meta.module_name)

                      url = reverse(route)

                      with self.assertMaxQueries(max_queries.get(route, 10)):

                          response = self.client.get(url, follow=True)

I'm happy to create a GitHub pull request for the Assertion and the required documentation changes