Refactor `QueryCount` usage to use `assertNumQueries` instead

[timjklein36]

The existing QueryCount test utility class was implemented without the knowledge of an existing django built-in way of asserting that a certain number of database queries were performed.

The following code snippet is an example of an existing use of QueryCount:

def test_bulk_create_multiple_one_to_one(self):
    queries = QueryCount()

    with queries.start_count():
        baker.make(models.LonelyPerson, _quantity=5, _bulk_create=True)
        assert queries.count == 6

    assert models.LonelyPerson.objects.all().count() == 5
    assert models.Person.objects.all().count() == 5

The next code snippet is what it could look like using the provided assertNumQueries:

def test_bulk_create_multiple_one_to_one(self):
    with self.assertNumQueries(6):
        baker.make(models.LonelyPerson, _quantity=5, _bulk_create=True)

    assert models.LonelyPerson.objects.all().count() == 5
    assert models.Person.objects.all().count() == 5

Then the QueryCount class can be removed entirely.

[Edald123]

Hi! may I work on this issue? If yes, do I need to know anything else to start working on it?

[timjklein36]

@Edald123 By all means, feel free to work on this issue. That would be great!

You shouldn't need any knowledge beyond the information from my original description.

Basically replace the following pattern:

queries = QueryCount()
with queries.start_count():
    ...
    assert queries.count == 6

With this:

with self.assertNumQueries(6):
    ...

[Edald123]

Thank you! I'm going to start working on it then, can you assign me?

[timjklein36]

@Edald123 One thing to note: the test classes will have to subclass Django TestCase to use the assertion method.

[Edald123]

Thanks for the information, I will pay attention to that as well. I will be sharing my work soon!

[berinhard]

Yay! Go ahead @Edald123 =) We'll be happy to review your PR! 🍰 🥮 🍰

[Edald123]

Hi! I've made the requested changes, I just have 2 questions.

1. When I run the tests with make test I'm receiving that 3 tests failed. I already analyzed where these failures are occurring and they appear when the code looks like this:

with self.assertNumQueries(5):
            baker.make(models.Person, _quantity=5)
            assert models.Person.objects.count() == 5

• The 2nd error occurs exactly when this same line appears: assert models.Person.objects.count() == some number
• The 3rd error occurs due to this line inside the with statement: assert models.NonStandardManager.manager.count() == 3
• All 3 failures raise AssertionError, which indicates that 1 more query than expected is being made.
• The part of code that I present raises: AssertionError: 6 != 5 : 6 queries executed, 5 expected
• The solution I found was to move that line of code in the 3 parts of code out of the with statement, like this:

with self.assertNumQueries(5):
            baker.make(models.Person, _quantity=5)
assert models.Person.objects.count() == 5

• I also found that, since it is receiving 1 more query we can change the number in self.assertNumQueries() from 5 to 6 and leave assert models.Person.objects.count() == 5 inside the with statement. This also solves the failures.

• My question is, which of those 2 solutions would be better? Is it necessary to look for another approach?

2. And my second question is: Since the class QueryCount is alone in the file model_bakery/tests/utils.py is it ok if I delete this file to remove the class?

Thank you!

[timjklein36]

@Edald123 I think your initial suggestion makes the most sense to me: move the .count() assertion outside of the with statement. That way, the intent of the assertNumQueries is very obvious to what is being asserted.

Second, yes, feel free to delete the entire tests/utils.py file since it would be empty otherwise. It was created when adding the QueryCount class there anyway, so it was not needed for another reason.

[Edald123]

Hi! I made the changes already and my PR is ready to be reviewed, please let me know if anything needs to be changed. Thank you!

[amureki]

@timjklein36 @Edald123 great work here 🤝