Merge lp://qastaging/~rvb/maas/maas-improve-tests into lp://qastaging/~maas-committers/maas/trunk

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_ge...

Ahem. "They typically initially objects that they don't need" should be: "They typically initialize objects that they don't need"

Thanks for the review.

> 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?

Okay, fixed.

> Tip: "too long" * 100

Nice!

> === 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.
[...]
> 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.

Okay, I was ready to stand firm, but the performance argument "fait mouche".

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

;)

R.

Thanks for making the changes! It makes the tests a bit longer, but it's usually worth it.