cleanup for #18586 (modeltests.basic.model test.test object creation) #2671

Closed
wants to merge 1 commit into from

2 participants

@zsoldosp

https://code.djangoproject.com/ticket/18586

Split up 300 line test method (inherited from doctest days)

  • Extracted method's relevant test assertions into multiple test methods on new class ModelInstanceCreationTests
  • moved the extra assertions from the test method as separate test methods to the relevant existing testclasses where possible, created new ones where couldn't find an appropriate one
  • Test data was changed to make the new tests more readable in isolation and to accommodate the various different Article models across the various test folders

I left the PR as multiple commits to be able to find refactoring mistakes while the PR is being reviewed, can be squashed later.

@MarkusH MarkusH commented on an outdated diff May 16, 2014
tests/basic/tests.py
+ def test_object_is_not_written_to_database_until_save_was_called(self):
+ a = Article(
+ id=None,
+ headline='Area man programs in Python',
+ pub_date=datetime(2005, 7, 28),
+ )
+ self.assertIsNone(a.id)
+ self.assertEquals(0, Article.objects.all().count())
+
+ # Save it into the database. You have to call save() explicitly.
+ a.save()
+ self.assertIsNotNone(a.id)
+ self.assertEquals(1, Article.objects.all().count())
+
+ def test_can_initialize_model_instance_using_positional_arguments(self):
+ """You can initialize a model instance using positional arguments,
@MarkusH
Django member
MarkusH added a note May 16, 2014

Can you please format the multi line comments like

def foo():
    """
    My very long comment that
    spans multiple lines
    """

and short comments like

def foo():
    """Short comment"""

At least the indention of the 2nd+x line is not recommended in PEP-257. Additionally, putting the first line of the comment on a separate line looks prettier to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MarkusH MarkusH commented on an outdated diff May 16, 2014
tests/basic/tests.py
+ id=None,
+ headline='Area man programs in Python',
+ pub_date=datetime(2005, 7, 28),
+ )
+ self.assertIsNone(a.id)
+ self.assertEquals(0, Article.objects.all().count())
+
+ # Save it into the database. You have to call save() explicitly.
+ a.save()
+ self.assertIsNotNone(a.id)
+ self.assertEquals(1, Article.objects.all().count())
+
+ def test_can_initialize_model_instance_using_positional_arguments(self):
+ """You can initialize a model instance using positional arguments,
+ which should match the field order as defined in the model."""
+ a2 = Article(None, 'Second article', datetime(2005, 7, 29))
@MarkusH
Django member
MarkusH added a note May 16, 2014

you can use a here again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff May 20, 2014
tests/basic/tests.py
@@ -16,7 +16,167 @@
from .models import Article, SelfRef, ArticleSelectOnSave
+class ModelInstanceCreationTests(TestCase):
+
+ def test_object_is_not_written_to_database_until_save_was_called(self):
+ a = Article(
+ id=None,
+ headline='Area man programs in Python',
+ pub_date=datetime(2005, 7, 28),
+ )
+ self.assertIsNone(a.id)
+ self.assertEquals(0, Article.objects.all().count())
@timgraham
Django member

assertEquals (deprecated alias) -> assertEqual
I would also reverse the order of the arguments and use self.assertEqual(Article.objects.all().count(), 0). That is what I have seen most often in tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff May 20, 2014
tests/basic/tests.py
+ pub_date=datetime(2005, 7, 28),
+ )
+ self.assertIsNone(a.id)
+ self.assertEquals(0, Article.objects.all().count())
+
+ # Save it into the database. You have to call save() explicitly.
+ a.save()
+ self.assertIsNotNone(a.id)
+ self.assertEquals(1, Article.objects.all().count())
+
+ def test_can_initialize_model_instance_using_positional_arguments(self):
+ """
+ You can initialize a model instance using positional arguments,
+ which should match the field order as defined in the model.
+ """
+ a2 = Article(None, 'Second article', datetime(2005, 7, 29))
@timgraham
Django member

can simply use "a" rather than "a#" in this, the next, and some of the other tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff May 20, 2014
tests/queries/tests.py
@@ -2145,6 +2145,130 @@ def test_flat_extra_values_list(self):
self.assertQuerysetEqual(qs, [72], self.identity)
+class QuerysetSupportsPythonIdioms(BaseQuerysetTest):
@timgraham
Django member

looks like plain TestCase will be sufficient here and we can remove the super call to setUp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff May 20, 2014
tests/datetimes/tests.py
+ Article(pub_date=pub_date, title='title #{}'.format(i)).save()
+ # Use iterator() with datetimes() to return a generator that lazily
+ # requests each result one at a time, to save memory.
+ dates = []
+ with self.assertNumQueries(0):
+ article_datetimes_iterator = Article.objects.datetimes('pub_date', 'day', order='DESC').iterator()
+
+ with self.assertNumQueries(1):
+ for article in article_datetimes_iterator:
+ dates.append(article)
+ self.assertEqual(dates, [
+ datetime.datetime(2005, 7, 31, 0, 0),
+ datetime.datetime(2005, 7, 30, 0, 0),
+ datetime.datetime(2005, 7, 29, 0, 0),
+ datetime.datetime(2005, 7, 28, 0, 0)])
+
@timgraham
Django member

chop blank line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

Please go ahead and squash, thanks.

@zsoldosp zsoldosp cleanup for #18586 (modeltests.basic.model test.test object creation)
https://code.djangoproject.com/ticket/18586

Split up 300 line test method (inherited from doctest days)

* Extracted method's relevant test assertions into multiple test
  methods on new class ModelInstanceCreationTests
* moved the extra assertions from the test method as separate test
  methods to the relevant existing testclasses where possible,
  created new ones where couldn't find an appropriate one
* Test data was changed to make the new tests more readable in
  isolation and to accommodate the various different Article
  models across the various test folders
f9edb7e
@zsoldosp

@timgraham squashed - sorry for the delay, I thought I've done it a long time ago!

@timgraham
Django member

I left some comments for improvement above, can you incorporate them?

Could you also amend the commit message to follow our guidelines?

See also our patch review checklist.

@zsoldosp

oops, overlooked those. will do - but I'll be offline now for a week, so only around June 16

@timgraham
Django member

merged in 7e2c804.

@timgraham timgraham closed this Jun 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment