review code with comments

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(

aircox/tests/models/test_page.py

@@ -1,8 +1,6 @@
 import pytest
 
-from model_bakery import baker
+from aircox.models import Page, StaticPage
-
-from aircox.models import Category, Page, StaticPage, NavItem
 
 
 @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
+        # assign queryset to a variable in order to reuse the cache
-        assert staticpages[0] in StaticPage.objects.search(q="test", search_content=False)
+        # 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="<h2>My headline</h2><p>My content.</p>"
+        # provide content on fixture, may be on static_page_root
-        staticpages[1].content=""
+        staticpages[0].content = "<h2>My headline</h2><p>My content.</p>"
+        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