launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/ssh-html-test into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/ssh-html-test/+merge/100354

Occasionally test_display_cropped_key would break because of a nasty subtlety: the randomly-generated key comment might contain spaces, but if a space happened to be at the end, the real code would strip it off and the test's expected output would leave it in.

One proposed solution was to have the test strip the generated comment.  But that would mean that instead of a comment of length n, we'd really be testing with a stripped comment of length i such that 0 ≤ i ≤ n, with the probability skewed exponentially towards n.  That would be very easy to miss, and given the special cases in the code we're testing, the tests might include edge cases that really should stand alone.  If we want to test for ranges of string lengths, we ought to do it either exhaustively, or at boundary conditions, or with a linear probability distribution.

Arguably we should update factory.getRandomString such as never to put spaces at the beginning or end of the random text.  I'm just a tad worried that that might hide similar cases where the mistake is in the “real” code instead of in the tests.  If anything, we ought to make leading and trailing spaces more probable!

Yeah, that's frivolous.  But right after this I'm going to do an experimental test run with getRandomString sabotaged to return _only_ spaces if spaces=True.  We'll see if anything breaks.  :-)
-- 
https://code.launchpad.net/~jtv/maas/ssh-html-test/+merge/100354
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/ssh-html-test into lp:maas.

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-30 17:01:12 +0000
+++ src/maasserver/tests/test_models.py	2012-04-02 03:25:23 +0000
@@ -710,6 +710,18 @@
 class GetHTMLDisplayForKeyTest(TestCase):
     """Testing for the method `get_html_display_for_key`."""
 
+    def make_comment(self, length):
+        """Create a comment of the desired length.
+
+        The comment may contain spaces, but not begin or end in them.  It
+        will be of the desired length both before and after stripping.
+        """
+        return ''.join([
+            factory.getRandomString(1),
+            factory.getRandomString(max([length - 2, 0]), spaces=True),
+            factory.getRandomString(1),
+            ])[:length]
+
     def make_key(self, type_len=7, key_len=360, comment_len=None):
         """Produce a fake ssh public key containing arbitrary data.
 
@@ -726,7 +738,7 @@
             factory.getRandomString(key_len),
             ]
         if comment_len is not None:
-            fields.append(factory.getRandomString(comment_len, spaces=True))
+            fields.append(self.make_comment(comment_len))
         return " ".join(fields)
 
     def test_display_returns_unchanged_if_unknown_and_small(self):
@@ -819,16 +831,16 @@
         # If the key has a small key_type, a small comment and a large
         # key_string (which is the 'normal' case), the key_string part
         # gets cropped.
-        key_type = factory.getRandomString(10)
-        key_string = factory.getRandomString(100)
-        comment = factory.getRandomString(10, spaces=True)
-        key = '%s %s %s' % (key_type, key_string, comment)
+        type_len = 10
+        comment_len = 10
+        key = self.make_key(type_len, 100, comment_len)
+        key_type, key_string, comment = key.split(' ', 2)
         display = get_html_display_for_key(key, 50)
         self.assertEqual(50, len(display))
         self.assertEqual(
             '%s %.*s%s %s' % (
                 key_type,
-                50 - (len(key_type) + len(HELLIPSIS) + len(comment) + 2),
+                50 - (type_len + len(HELLIPSIS) + comment_len + 2),
                 key_string, HELLIPSIS, comment),
             display)