Code review comment for lp://qastaging/~rvb/maas/maas-improve-tests

Hi,

Lots of progress but there's one thing I feel very strongly needs fixing. See rant inline:

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py 2012-01-24 13:21:41 +0000
+++ src/maasserver/middleware.py 2012-01-25 11:34:39 +0000
@@ -43,7 +43,8 @@

> def process_request(self, request):
> # API views.
> - if self.api_url.match(request.path):
> + if (self.api_url.match(request.path)
> + and not self.public_urls.match(request.path)):
> if request.user.is_anonymous():
> return rc.FORBIDDEN
> else:

Line-breaking conditionals often makes it harder to grok the code at speed. It also raises the "where does the 'and' go" controversy in an awkward place. Could you rephrase this so that the "if" is a single line?

@@ -125,6 +131,19 @@
         self.assertEqual(0, Node.objects.filter(hostname='diane').count())
         self.assertEqual(1, Node.objects.filter(hostname='francis').count())

+ def test_node_PUT_invalid(self):
+ """
+ If the data provided to update a node is invalid, a 'Bad request'
+ response is returned.
+
+ """
+ node = factory.make_node(hostname='diane')
+ response = self.client.put(
+ '/api/nodes/%s/' % node.system_id,
+ {'hostname': ''.join(['too long' for x in xrange(100)])})

Tip: "too long" * 100

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-01-23 12:43:28 +0000
+++ src/maasserver/tests/test_models.py 2012-01-25 11:34:39 +0000
@@ -34,6 +39,9 @@
> self.assertEqual(len(self.node.system_id), 41)
> self.assertTrue(self.node.system_id.startswith('node-'))
>
> + def test_display_status(self):
> + self.assertEqual('New', self.node.display_status())
> +

Waitwaitwait — where does self.node come from? What are you testing and proving exactly when you say its display_status is 'New'? Yes, I can guess, but making people guess is a bad habit. Tests should be as self-evident and independent as possible.

@@ -48,6 +56,50 @@

+class NodeManagerTest(TestCase):
+
+ def setUp(self):
+ super(NodeManagerTest, self).setUp()
+ self.user = factory.make_user()
+ self.admin = factory.make_admin()
+ self.anon_node = factory.make_node()
+ self.user_node = factory.make_node(
+ set_hostname=True, status=NODE_STATUS.DEPLOYED,
+ owner=self.user)
+ self.admin_node = factory.make_node(
+ set_hostname=True, status=NODE_STATUS.DEPLOYED,
+ owner=self.admin)

This is more of the bad habit: for every test I need to look at, I have to bear in mind how setUp initialized these objects.

It's also usually bad for performance, since setUp() gets called for each test in the test case. When test cases are set up in this way, they typically initially objects that not all of the tests need.

And in the longer run, there's always the temptation to extend the setup, which makes the problems worse. If you need an object created for a test, create it in a helper method that you call explicitly.

+ def test_get_visible_nodes_user(self):
+ """get_visible_nodes returns the nodes a user has access to."""
+ visible_nodes = Node.objects.get_visible_nodes(self.user)
+
+ self.assertSequenceEqual(
+ [self.anon_node, self.user_node], visible_nodes)

See? This is intimately entwined with the setup. If you ever need to extend setUp further, or move some object creation into explicit methods, or optimize away database restore between test runs, or change the ordering in setUp, chances are you'll break this test for no good reason.

+ def test_get_visible_nodes_admin(self):
+ """get_visible_nodes returns all the nodes if the user used for the
+ permission check is an admin."""
+ visible_nodes = Node.objects.get_visible_nodes(self.admin)
+
+ self.assertSequenceEqual(
+ [self.anon_node, self.user_node, self.admin_node], visible_nodes)

And this one as well. When I need to update this test for some other code change, I need to trace back to setUp why each node was or wasn't supposed to be in the result, as opposed to the other test.

It isn't anywhere near problematic yet, but that just means that there's no excuse for not fixing it! As it is, the maintenance burden of adding complexity here is O(n²) for very small n.

Expect n to grow. In the longer run we'll add other selection criteria, and the temptation will be to add more kinds of nodes into setUp, as well as tests to the test case. Each node will show up in some of the test outputs but not in others. Eventually it becomes an arbitrary soup of sample data that people won't want to touch, but making things worse will be slightly easier than cleaning things up.

I feel strongly about this antipattern. You wouldn't believe the suffering it caused Soyuz and Rosetta!

Jeroen