Skip to content

Tidy tests #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joncotton wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Tidy tests #22

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bfeb979
Add a test for all() on GFKQuerySet and MergeQuerySet.
joncotton Feb 28, 2012
7b173b8
Clean up _utils.TestCase.
joncotton Mar 9, 2012
eeddab1
Tidy imports. Remove unused imports. Some PEP8 spacing.
joncotton Mar 9, 2012
099b266
Include a sanity test that was orphaned.
joncotton Mar 9, 2012
331b1ab
Feedback from the PR. Restoring significant import whitespace, using …
joncotton Mar 12, 2012
0f81a07
Moving `assertInContext` to armstrong.dev in armstrong.dev PR #9.
joncotton Mar 12, 2012
f052230
Updating required versions of `armstrong.dev` and `django-reversion`.
joncotton Mar 13, 2012
70934ff
Separate tests for `all()` and `exclude()`
joncotton Mar 13, 2012
5fde6e7
Removing the requirements/base.txt file.
joncotton Mar 13, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions armstrong/core/arm_wells/tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
from .query import *
from .querysets import *
from .views import *
from .arm_wells_support.tests import *
Copy link
Contributor

@tswicegood tswicegood Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be done -- arm_wells.tests is the actual tests of armstrong.core.arm_wells while the arm_wells_support tests are for the functionality that provides. They're a sanity check, but should be tested as a separate app. They won't always be in the INSTALLED_APPS which means their models wouldn't be installed, which would cause an issue if arm_wells was tested inside another project.

Copy link
Member Author

@joncotton joncotton Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the explanation. I'll drop the import.

24 changes: 6 additions & 18 deletions armstrong/core/arm_wells/tests/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import random

from django.test import TestCase as DjangoTestCase
from django.test.client import RequestFactory

from armstrong.dev.tests.utils.base import ArmstrongTestCase
from .arm_wells_support.models import Story, StoryChild, Image
from ..models import Node
from ..models import Well
from ..models import WellType
from ..models import WellType, Well, Node


class TestCase(ArmstrongTestCase):
pass


def generate_random_image():
Expand Down Expand Up @@ -52,15 +52,3 @@ def generate_random_welltype():
title = "Random Well %d" % r
slug = "random-well-%d" % r
return WellType.objects.create(title=title, slug=slug)


class TestCase(DjangoTestCase):
def setUp(self):
self.factory = RequestFactory()

def assertInContext(self, var_name, other, template_or_context):
# TODO: support passing in a straight "context" (i.e., dict)
context = template_or_context.context_data
self.assertTrue(var_name in context,
msg="`%s` not in provided context" % var_name)
self.assertEqual(context[var_name], other)
2 changes: 2 additions & 0 deletions armstrong/core/arm_wells/tests/arm_wells_support/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
from django.contrib.contenttypes import generic
from django.contrib.contenttypes.models import ContentType


class Content(models.Model):
title = models.CharField(max_length=100)

def __unicode__(self):
return self.title

Expand Down
5 changes: 2 additions & 3 deletions armstrong/core/arm_wells/tests/arm_wells_support/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import random

from django.test import TestCase

from .._utils import TestCase
Copy link
Contributor

@tswicegood tswicegood Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests inside *_support modules should not rely on the behavior in any of Armstrong's TestCase classes. These should be straight Django.

from .models import Story


Expand All @@ -10,4 +9,4 @@ def test_uses_title_when_converted_to_string(self):
title = "Some Random Title %d" % random.randint(100, 200)
story = Story(title=title)

self.assertEqual(title, str(story))
self.assertEqual(title, str(story), msg="sanity check")
4 changes: 1 addition & 3 deletions armstrong/core/arm_wells/tests/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import datetime

from ._utils import TestCase

from ..models import Well
from ..models import WellType
from ..models import WellType, Well
Copy link
Contributor

@tswicegood tswicegood Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is significant. Importing in Python is broken down into three groups. System level, apps that require installation, and finally any specific to this package. In testing, I try to break them into a fourth -- parts of the system under test.



class WellManagerTestCase(TestCase):
Expand Down
4 changes: 2 additions & 2 deletions armstrong/core/arm_wells/tests/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def test_well_supports_indexing(self):
for node in well.nodes.all():
self.assertEqual(node.content_object, well.items[i])
i = i + 1
self.assertRaises(IndexError, lambda:well.items[i])
self.assertRaises(IndexError, lambda: well.items[i])

def test_well_supports_indexing_with_merged_queryset(self):
number_of_stories = random.randint(1, 5)
Expand All @@ -135,7 +135,7 @@ def test_well_supports_indexing_with_merged_queryset(self):
continue
self.assertEqual(story, well.items[i])
i = i + 1
self.assertRaises(IndexError, lambda:well.items[i])
self.assertRaises(IndexError, lambda: well.items[i])


class NodeTestCase(TestCase):
Expand Down
13 changes: 6 additions & 7 deletions armstrong/core/arm_wells/tests/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
from django.core.paginator import Paginator
import random

from .arm_wells_support.models import Image, Story
from ._utils import (add_n_random_stories_to_well, add_n_random_images_to_well,
generate_random_image, generate_random_story, generate_random_well,
TestCase)

from ..models import Node
from .arm_wells_support.models import Story
from ._utils import (TestCase,
add_n_random_stories_to_well,
generate_random_story,
generate_random_well)


class SimpleMergedNodesAndQuerySetTests(TestCase):
Expand All @@ -18,7 +17,7 @@ def test_merge_when_sliced_across_the_well_content_with_slice_object(self):
add_n_random_stories_to_well(2, well)

queryset = well.merge_with(Story.objects.all())
sliced = [a for a in queryset[slice(1,3)]]
sliced = [a for a in queryset[slice(1, 3)]]
self.assertEqual(2, len(sliced))

def test_merges_correctly_when_sliced_across_the_well_content(self):
Expand Down
40 changes: 23 additions & 17 deletions armstrong/core/arm_wells/tests/querysets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
from django.core.paginator import Paginator
import random

from .arm_wells_support.models import *
from ._utils import (add_n_random_stories_to_well, add_n_random_images_to_well,
generate_random_image, generate_random_story, generate_random_well,
TestCase)
from ._utils import (TestCase,
add_n_random_stories_to_well,
add_n_random_images_to_well,
generate_random_image,
generate_random_story,
generate_random_well)
Copy link
Contributor

@niran niran Mar 19, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the previous style here, but whatever.

Copy link
Contributor

@tswicegood tswicegood Mar 20, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. I've reverted some of these in joncotton#1 to keep it consistent with imports elsewhere. They should really either be one import, one line; or multi-line imports with () like this was originally.


from ..querysets import (GenericForeignKeyQuerySet, MergeQuerySet,
FilterException)
Expand All @@ -24,10 +26,12 @@ def setUp(self):
for i in range(self.number_of_extra_stories):
self.extra_stories.append(generate_random_story())

def test_raises_NotImplementedError_on_exclude(self):
def test_raises_NotImplementedError_on_all_and_exclude(self):
gfk_qs = GenericForeignKeyQuerySet(self.well.nodes.all()\
.select_related())
with self.assertRaises(NotImplementedError):
gfk_qs.all()
with self.assertRaises(NotImplementedError):
gfk_qs = GenericForeignKeyQuerySet(self.well.nodes.all()\
.select_related())
gfk_qs.exclude()

def test_raises_NotImplementedError_on_misc_functions(self):
Expand All @@ -50,7 +54,6 @@ def test_raises_AttributeError_on_unknown_attribute(self):
with self.assertRaises(AttributeError):
getattr(gfk_qs, "unknown_and_unknowable")


def test_gathers_all_nodes_of_one_type_with_two_queries(self):
gfk_qs = GenericForeignKeyQuerySet(self.well.nodes.all()\
.select_related())
Expand Down Expand Up @@ -104,7 +107,7 @@ def test_does_not_ignore_same_ids_in_different_types(self):
self.assertEqual(2, len(queryset))

def test_non_standard_node(self):
num_nodes = random.randint(3,5)
num_nodes = random.randint(3, 5)
for i in range(num_nodes):
OddNode.objects.create(baz=generate_random_story())
gfk_qs = GenericForeignKeyQuerySet(
Expand All @@ -116,14 +119,14 @@ def test_non_standard_node(self):
self.assertEqual(obj.__class__, Story)

def test_non_standard_node_failure(self):
num_nodes = random.randint(3,5)
num_nodes = random.randint(3, 5)
for i in range(num_nodes):
OddNode.objects.create(baz=generate_random_story())
with self.assertRaises(ValueError):
gfk_qs = GenericForeignKeyQuerySet(
OddNode.objects.all().select_related(),
gfk='bad_field_name'
)
GenericForeignKeyQuerySet(
OddNode.objects.all().select_related(),
gfk='bad_field_name'
)

def test_works_with_duplicate_nodes(self):
well = generate_random_well()
Expand Down Expand Up @@ -162,7 +165,8 @@ def test_non_empty_filter(self):

queryset = well.items
with self.assertRaises(FilterException):
self.assertEqual(3, len(queryset.filter(title__in=['foo','bar'])))
self.assertEqual(3, len(queryset.filter(title__in=['foo', 'bar'])))


class MergeQuerySetTestCase(TestCase):
def setUp(self):
Expand All @@ -178,9 +182,11 @@ def setUp(self):
generate_random_image()
self.qs_b = Image.objects.all()

def test_raises_NotImplementedError_on_exclude(self):
def test_raises_NotImplementedError_on_all_and_exclude(self):
merge_qs = MergeQuerySet(self.qs_a, self.qs_b)
with self.assertRaises(NotImplementedError):
Copy link
Contributor

@tswicegood tswicegood Mar 13, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please break these out into multiple test methods? If this assertRaises failed, we wouldn't know that the next assertRaises also fails (or passes) until we fixed the first method. It's generally a good rule to keep one assertion per test method -- or at least one concern. Here, all() and exclude() are two different completely different methods that just happen to raise the same error.

merge_qs.all()
with self.assertRaises(NotImplementedError):
merge_qs = MergeQuerySet(self.qs_a, self.qs_b)
merge_qs.exclude()

def test_raises_NotImplementedError_on_misc_functions(self):
Expand Down
35 changes: 19 additions & 16 deletions armstrong/core/arm_wells/tests/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,38 @@
import random

from django.core.exceptions import ImproperlyConfigured
from django.db.models.query import QuerySet
from django.http import HttpRequest
from django.test.client import RequestFactory
import fudge
import random

from .arm_wells_support.models import Story
from ._utils import add_n_random_stories_to_well
from ._utils import generate_random_story
from ._utils import generate_random_well
from ._utils import TestCase
from ._utils import (TestCase,
add_n_random_stories_to_well,
generate_random_story,
generate_random_well)

from ..views import SimpleWellView
from ..views import QuerySetBackedWellView
from ..views import SimpleWellView, QuerySetBackedWellView


class WellViewTestCase(TestCase):
class SimpleWellViewTest(TestCase):
view_class = SimpleWellView

def setUp(self):
super(WellViewTestCase, self).setUp()
super(SimpleWellViewTest, self).setUp()
self.factory = RequestFactory()
self.well = generate_random_well()


class SimpleWellViewTest(WellViewTestCase):
view_class = SimpleWellView

def default_kwargs(self):
return {
"template_name": "index.html",
"well_title": self.well.title,
}

def assertInContext(self, var_name, other, template_or_context):
Copy link
Contributor

@tswicegood tswicegood Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to keep assert* methods like this outside of the test classes they're used. Invariably, you end up wanting to use them elsewhere if they're useful. If this is to be moved anywhere, it should be in to armstrong.dev so the method is available everywhere.

Copy link
Member Author

@joncotton joncotton Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now should I move it back into _utils.py.TestCase?

Copy link
Contributor

@tswicegood tswicegood Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one option, or if you'd like to move this into armstrong.dev entirely that's a good option as well.

Copy link
Member Author

@joncotton joncotton Mar 12, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #9

# TODO: support passing in a straight "context" (i.e., dict)
context = template_or_context.context_data
self.assertTrue(var_name in context,
msg="`%s` not in provided context" % var_name)
self.assertEqual(context[var_name], other)

def test_raises_exception_without_template_name_param(self):
kwargs = self.default_kwargs()
del kwargs["template_name"]
Expand Down