add subquery tests for EmbeddedModelField #226
Conversation
WaVEV
force-pushed
the
EMF-subquery-tests
branch
from
27ec33c to
77d6286
Compare
|
|
||
|
|
||
| class SubqueryExistsTest(TestCase): | ||
| def setUp(self): |
| @@ -123,3 +129,62 @@ class MyModel(models.Model): | |||
| self.assertEqual( | |||
| msg, "Embedded models cannot have relational fields (Target.key is a ForeignKey)." | |||
| ) | |||
|
|
|||
|
|
|||
| class SubqueryExistsTest(TestCase): | |||
| self.assertEqual(queryset.count(), 3) | ||
| self.assertQuerySetEqual(queryset, ["Book B", "Book C", "Book D"], lambda book: book.name) | ||
|
|
||
| def test_range_query(self): |
There was a problem hiding this comment.
Why in this class? Is it because the count() does an aggregation?
There was a problem hiding this comment.
this test could be put in a better place. Will move
There was a problem hiding this comment.
Probably should be in a different commit with this one
There was a problem hiding this comment.
It would need to be rewritten because it relies on this class's test data, so I didn't include it there. Probably range could be tested with Holder in QueryingTests, but I'm not sure it's a pressing need considering all the other lookups that aren't explicitly tested.
There was a problem hiding this comment.
Ok, I will rewrite it. I think is a good to have, we also need to test if the range is covered.
| author__name=OuterRef("name"), author__address__city="Boston" | ||
| ) | ||
| queryset = Author.objects.filter(Exists(subquery)) | ||
|
|
There was a problem hiding this comment.
Please omit the blank lines.
|
|
||
| def test_in_subquery(self): | ||
| subquery = Author.objects.filter(age__gt=35).values("name") | ||
| queryset = Book.objects.filter(author__name__in=Subquery(subquery)).order_by("name") |
There was a problem hiding this comment.
As far as I tested, the Subquery wrapping of subquery is unnecessary.
There was a problem hiding this comment.
Mmh yes, don't remember why i added that
| address2 = Address.objects.create(city="Boston", state="MA", zip_code=20002) | ||
| author1 = Author.objects.create(name="Alice", age=30, address=address1) | ||
| author2 = Author.objects.create(name="Bob", age=40, address=address2) | ||
| book1 = Book.objects.create(name="Book A", author=author1) |
There was a problem hiding this comment.
It would be nice to title it "Book 1" rather than switching between letters and numbers.
| class SubqueryExistsTest(TestCase): | ||
| def setUp(self): | ||
| # Create test data | ||
| address1 = Address.objects.create(city="New York", state="NY", zip_code=10001) |
There was a problem hiding this comment.
Embedded models should be saved to the database in their own collection (i.e. no create()). #216 will prohibit it.
| author__name=OuterRef("author__name"), author__address__city="Boston" | ||
| ) | ||
| queryset = Book.objects.filter(Exists(subquery)).order_by("name") | ||
| self.assertEqual(queryset.count(), 3) |
There was a problem hiding this comment.
Are the .count() to check the length or to check that count works correctly? It's just that assertQuerySetEqual will give a more helpful message about any extra or missing elements.
|
|
||
| class SubqueryExistsTests(TestCase): | ||
| def setUpTestData(self): | ||
| # Create test data |
| library2 = Library.objects.create( | ||
| name="Community Library", location="Suburbs", best_seller="Book 1" | ||
| ) | ||
| # Add books to libraries |
| self.assertEqual(queryset.count(), 3) | ||
| self.assertQuerySetEqual(queryset, ["Book B", "Book C", "Book D"], lambda book: book.name) | ||
|
|
||
| def test_range_query(self): |
There was a problem hiding this comment.
It would need to be rewritten because it relies on this class's test data, so I didn't include it there. Probably range could be tested with Holder in QueryingTests, but I'm not sure it's a pressing need considering all the other lookups that aren't explicitly tested.
timgraham
changed the title
WaVEV
force-pushed
the
EMF-subquery-tests
branch
from
10dbdc3 to
6f383ee
Compare
| def test_range_query(self): | ||
| sorted_obj = sorted(self.objs, key=lambda x: x.data.auto_now) | ||
| queryset = Holder.objects.filter( | ||
| data__auto_now__range=(sorted_obj[2].data.auto_now, sorted_obj[-1].data.auto_now) |
There was a problem hiding this comment.
Any reason not to do range on data__integer? To me, this is similar to the other lookup tests (e.g. group under test_gte) and those look so much simpler.
There was a problem hiding this comment.
None, I want to try if the dates works fine. I know that is not the place to do that, but is not wrong.
There was a problem hiding this comment.
If the test worked locally and failed on CI, I'd guess it's due to milliseconds (fixable with truncate_ms() perhaps). I think we wouldn't have these truncate_ms() issues if we had a way to truncate milliseconds at https://github.com/django/django/blob/f133285a9a7f3647fd55abc2e57b8a9a2c11ac94/django/db/models/fields/__init__.py#L1649-L1655. It might be possible with a change in Django.
There was a problem hiding this comment.
Yeap, is a flaky test. give that sometimes two elements has the same time. (due to precision). Will fix it
There was a problem hiding this comment.
range in integers in order to keep the test simple
There was a problem hiding this comment.
Is a one-liner similar to the other lookup tests insufficient? What's the important part of the test? That range works? That ordering works? That count() works? Everything together? Just trying to understand your thinking about why you made it part of this PR. Not sure if I got the indices right, but I had something like this in mind:
self.assertCountEqual(Holder.objects.filter(data__integer__range=(2, 5)), self.objs[2:5])
There was a problem hiding this comment.
🤔 the order by is just to comparing results. It has nothing more to do with the test. So It is good to have one liner test
WaVEV
force-pushed
the
EMF-subquery-tests
branch
from
3f2dd51 to
b7d5f95
Compare
timgraham
force-pushed
the
EMF-subquery-tests
branch
from
6a34c4a to
a7458c2
Compare
timgraham
force-pushed
the
EMF-subquery-tests
branch
from
a7458c2 to
9b4083f
Compare
No description provided.