From 0469b70539bf26b7eafc6ef1b737ff99e8083c42 Mon Sep 17 00:00:00 2001 From: bkfox Date: Fri, 26 May 2023 11:56:52 +0200 Subject: [PATCH] review code with comments --- aircox/tests/conftest.py | 16 ++++ aircox/tests/models/test_page.py | 139 +++++++++++++++++++++++++------ 2 files changed, 131 insertions(+), 24 deletions(-) diff --git a/aircox/tests/conftest.py b/aircox/tests/conftest.py index d1a2be6..bbae4a7 100644 --- a/aircox/tests/conftest.py +++ b/aircox/tests/conftest.py @@ -11,22 +11,38 @@ from aircox import models def stations(): return baker.make(models.Station, _quantity=2) + @pytest.fixture def category(): return baker.make(models.Category) + +# this should be renamed "static_pages" (use underscore to ease reading +# -> snake_case convention) @pytest.fixture def staticpages(): + # regarding the different test over status related method, you must + # provide the different values for status, such as: + # return [baker.make(models.StaticPage, status=StaticPage.STATUS_DRAFT), + # baker.make(..., status=StaticPage.STATUS_PUBLISHED), + # ...] + # + # in the test, you also need a single static page, so you can compose + # your fixture with another one (for example, a published static page) return baker.make(models.StaticPage, _quantity=2) + @pytest.fixture def pages(): return baker.make(models.Page, _quantity=2) + +# same for this one eventually nav_item @pytest.fixture def navitem(): return baker.make(models.NavItem) + @pytest.fixture def programs(stations): items = list( diff --git a/aircox/tests/models/test_page.py b/aircox/tests/models/test_page.py index 2466e5f..1a3191b 100644 --- a/aircox/tests/models/test_page.py +++ b/aircox/tests/models/test_page.py @@ -1,8 +1,6 @@ import pytest -from model_bakery import baker - -from aircox.models import Category, Page, StaticPage, NavItem +from aircox.models import Page, StaticPage @pytest.mark.django_db @@ -10,11 +8,26 @@ class TestCategory: def test__str__(self, category): assert category.__str__() == category.title -# BasePAgeQuerySet is queryset, we use StaticPage in the test. + +# BasePAgeQuerySet is queryset, we use StaticPage in the test. # We do not use instances of the Page Model because it's use as abstract. class TestBasePageQuerySet: @pytest.mark.django_db def test_draft(self, staticpages): + # you can also do this (i think, you might check this is right: + # query = StaticPage.objects.draft().distinct() + # assert query.values_list("status") == [StaticPage.STATUS_DRAFT] + # + # note: a loop does not ensure it iterate as is (empty queryset), + # so test might pass because it does not loop so don't assert. + # Regarding this, different solutions, as the one upper, but also + # using for-loop python syntax with keyword else + # ( https://book.pythontips.com/en/latest/for_-_else.html ). + # This last solution is however less readable/pythonic. + # + # Another solution is to check the count of result queryset. + # In this, case you assign a variable to the queryset in order to + # reuse cached data instead of making multiple db calls for page in StaticPage.objects.draft(): assert page.status == StaticPage.STATUS_DRAFT @@ -30,7 +43,16 @@ class TestBasePageQuerySet: @pytest.mark.django_db def test_parent_return_childs_from_parent_object(self, staticpages): + # note: object must be saved to ensure the queryset returns what you + # want. + # you can directly provide a parent page in fixture for static_pages + # like `static_page_root`, `static_page_children` (with one of each + # status will help with other tests, as in display_title ones) staticpages[0].parent = staticpages[1] + + # same story for loops as upper. Still, if you expect to have only one + # item returned, use `first()` on the queryset. Otherwise, use python + # set comparison (check comments for `station` tests) for child in StaticPage.objects.parent(staticpages[1]): assert child.parent == staticpages[1] assert child != staticpages[1] @@ -42,25 +64,48 @@ class TestBasePageQuerySet: assert child.parent == staticpages[1] assert child != staticpages[1] + # use snake_case naming: test_search_with_search_content @pytest.mark.django_db def test_search_with_searchcontent(self, staticpages): + # tuple notation (aka parameter destructuring) should be used carefully + # since it is less readable, and details slip over reading (e.g. + # indices here are 0 and 1, fields are different too. + # Usually, use parameter destructuring on variable itself, not their + # content, otherwise less readable + # + # also, if you provide in fixture the content and title for objects + # this avoids to reassign values over all tests (e.g. here, slug + # generation, etc.) staticpages[0].title, staticpages[1].content = "test", "test" + # avoid tuple notation when you execute methods, this is less + # readable and not pythonic staticpages[0].save(), staticpages[1].save() + # assert that returned objects are exactly those you want assert StaticPage.objects.search(q="test").count() == 2 @pytest.mark.django_db def test_search_without_searchcontent(self, staticpages): staticpages[0].title, staticpages[1].content = "test", "test" staticpages[0].save(), staticpages[1].save() - assert StaticPage.objects.search(q="test", search_content=False).count() == 1 - assert staticpages[0] in StaticPage.objects.search(q="test", search_content=False) + # assign queryset to a variable in order to reuse the cache + # generated by the queryset (also, less readable asserting this + # way here) + assert ( + StaticPage.objects.search(q="test", search_content=False).count() + == 1 + ) + assert staticpages[0] in StaticPage.objects.search( + q="test", search_content=False + ) -# BasePage is abstract, we use StaticPage in the test. +# BasePage is abstract, we use StaticPage in the test. # We do not use instances of the Page Model because it's use as abstract. class TestBasePage: @pytest.mark.django_db def test__str__(self, staticpages): + # if you only need one object, just provide/use a fixture for a single + # static page. Same for all methods below. assert staticpages[0].title == staticpages[0].__str__() @pytest.mark.django_db @@ -71,13 +116,20 @@ class TestBasePage: @pytest.mark.django_db def test_save_generate_slug_that_already_exist(self, staticpages): - staticpages[0].slug, staticpages[1].slug, staticpages[1].title = "title", None, "title" + # argh... I know sometime is boring to write it on three lines, + # but still, it is the moment where rigorous code helps in the future. + # (always think of the future) + staticpages[0].slug, staticpages[1].slug, staticpages[1].title = ( + "title", + None, + "title", + ) staticpages[0].save(), staticpages[1].save() assert staticpages[1].slug == "title-1" @pytest.mark.django_db def test_save_without_cover_with_parent(self, staticpages): - staticpages[0].cover, staticpages[0].parent= None, staticpages[1] + staticpages[0].cover, staticpages[0].parent = None, staticpages[1] staticpages[0].save() assert staticpages[0].cover == staticpages[1].cover @@ -91,53 +143,77 @@ class TestBasePage: def test_get_absolute_url_from_published_page(self, staticpages): staticpages[0].status = StaticPage.STATUS_PUBLISHED staticpages[0].save() - assert staticpages[0].get_absolute_url() == "/pages/" + staticpages[0].slug + "/" + assert ( + staticpages[0].get_absolute_url() + == "/pages/" + staticpages[0].slug + "/" + ) + # for the following methods testing status, see comment upper on providing + # a fixture for each status. @pytest.mark.django_db def test_is_draft(self, staticpages): staticpages[0].status = StaticPage.STATUS_DRAFT staticpages[0].save() - assert staticpages[0].is_draft == True + # test on true-falseness: + # assert staticpages[0].is_draft == True + assert staticpages[0].is_draft @pytest.mark.django_db def test_is_published(self, staticpages): staticpages[0].status = StaticPage.STATUS_PUBLISHED staticpages[0].save() - assert staticpages[0].is_published == True + # assert staticpages[0].is_published == True + assert staticpages[0].is_published @pytest.mark.django_db def test_is_trash(self, staticpages): staticpages[0].status = StaticPage.STATUS_TRASH staticpages[0].save() - assert staticpages[0].is_trash == True + # assert staticpages[0].is_trash == True + assert staticpages[0].is_trash @pytest.mark.django_db def test_display_title_from_parent_ifdraft(self, staticpages): + # again fixture :) + # if you have a static_page_root, baker.make generate values for the + # title, so no need to assign it. + # no need to set status with the right child fixture. staticpages[0].title = "Parent page title" staticpages[0].status = StaticPage.STATUS_PUBLISHED + # no need to save since display_title is a property and we don't + # need to fetch values from db staticpages[0].save() staticpages[1].title = "Child page title" staticpages[1].parent = staticpages[0] staticpages[1].status = StaticPage.STATUS_DRAFT staticpages[1].save() + + # you can even do a single assert using variable instead of const + # string (this ease further updates): + # assert staticpages[0].display_title == staticpages[1].display_title assert staticpages[0].display_title == "Parent page title" assert staticpages[1].display_title == "Parent page title" @pytest.mark.django_db def test_headline(self, staticpages): - staticpages[0].content="

My headline

My content.

" - staticpages[1].content="" + # provide content on fixture, may be on static_page_root + staticpages[0].content = "

My headline

My content.

" + staticpages[1].content = "" + # no need to save indeed, since headline is a property staticpages[0].save(), staticpages[1].save() assert staticpages[0].headline == "My headline\n" assert staticpages[1].headline == "" - #for Page only, StaticPage don't have category argument. + # for Page only, StaticPage don't have category argument. @pytest.mark.django_db def test_get_init_kwargs_from(self, pages): kwargs = pages[0].get_init_kwargs_from(pages[0]) - assert kwargs == {"cover": pages[0].cover, "category": pages[0].category} + assert kwargs == { + "cover": pages[0].cover, + "category": pages[0].category, + } - #for Page only, StaticPage don't have category argument. + # for Page only, StaticPage don't have category argument. @pytest.mark.django_db def test_from_page(self, pages): assert Page.from_page(pages[0]).cover == pages[0].cover @@ -147,21 +223,28 @@ class TestBasePage: class TestPageQuerySet: @pytest.mark.django_db def test_published(self, staticpages): + # same as in fixture & loop comments upper staticpages[0].status = Page.STATUS_PUBLISHED staticpages[0].save() for page in Page.objects.published(): assert page.status == Page.STATUS_PUBLISHED + class TestPage: @pytest.mark.django_db def test_save_published_page_pubdate(self, pages): - pages[0].status, pages[0].pub_date= Page.STATUS_PUBLISHED, None + # if you use only one object, take a single page as input + pages[0].status, pages[0].pub_date = Page.STATUS_PUBLISHED, None pages[0].save() + # to ensure the value regarding to a datetime, you can do: + # now = tz.now() + # pages[0].save() + # assert pages[0].pub_date >= now assert pages[0].pub_date is not None @pytest.mark.django_db def test_save_unpublished_page_pubdate(self, pages): - pages[0].status, pages[0].pub_date= Page.STATUS_DRAFT, None + pages[0].status, pages[0].pub_date = Page.STATUS_DRAFT, None pages[0].save() assert pages[0].pub_date is None @@ -175,22 +258,30 @@ class TestPage: class TestStaticPage: @pytest.mark.django_db def test_get_absolute_url(self, staticpages): - staticpages[0].attach_to=StaticPage.ATTACH_TO_DIFFUSIONS + staticpages[0].attach_to = StaticPage.ATTACH_TO_DIFFUSIONS + # value may depends on user configuration, which might imply + # to retrieve url using django `reverse` method (urls are translated) + # but if it is too problematic let it as is for the moment assert staticpages[0].get_absolute_url() == "/week/" + class TestNavItem: + # snake_case / maybe cleaner as test_get_url_from_raw_url @pytest.mark.django_db def test_get_url_from_selfurl(self, navitem): assert navitem.url == navitem.get_url() - #for staticpage only. + # comment not required: for staticpage only. + # test_get_url_from_page @pytest.mark.django_db def test_get_url_from_pageurl(self, navitem, staticpages): - navitem.url, navitem.page = None, staticpages[0] + navitem.url, navitem.page = None, staticpages[0] navitem.save() assert navitem.get_url() == staticpages[0].get_absolute_url() + # test_get_url_none @pytest.mark.django_db def test_get_url_without_selfurl_page(self, navitem): navitem.url, navitem.page = None, None - assert navitem.get_url() == None + # assert navitem.get_url() == None + assert navitem.get_url() is None