launchpad-reviewers team mailing list archive

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

More notes follow:

YUIUnitTestsRemote could also do with some documentation.  For example, what happens when I run tests without a connection to the SauceLabs site?

Copying dozens of port numbers into a comment there is error-prone.  The list is likely to change.  (Are they stark raving mad to just grab all these ports?)

By the way, according to Clarkson this should get you shot:

345	=== added file 'src/maastesting/httpd.py'
346	--- src/maastesting/httpd.py	1970-01-01 00:00:00 +0000
347	+++ src/maastesting/httpd.py	2012-05-17 16:13:19 +0000

373	+class ThreadingHTTPServer(ThreadingMixIn, HTTPServer):
374	+    """A simple HTTP Server that whill run in it's own thread."""


In src/maastesting/saucelabs.py, I beseech you to break down the methods of SauceConnectFixture further.  I find them hard to read: the intent of a given bit of code often becomes clear only after going through its details.  Things get much easier if intentions are stated up front and the code implemented them, e.g. through well-named functions.

In SauceOnDemandFixture.__init__ it's as if you're playing a shell game with self.capabilities.  If you're going to have an immutable property in the class, and a modified copy in the object, then why not call the class one default_capabilities?  It would be easy for someone in a hurry to break this in a way that's hard to debug.

In src/maastesting/tests/test_httpd.py:

736	+    def test_use(self):
737	+        filename = "setup.py"
738	+        self.assertThat(filename, FileExists())
739	+        with HTTPServerFixture() as httpd:
740	+            url = urljoin(httpd.url, filename)
741	+            with closing(urlopen(url)) as http_in:
742	+                with open(filename, "rb") as file_in:
743	+                    self.assertEqual(
744	+                        file_in.read(), http_in.read(),
745	+                        "The content of %s differs from %s." % (
746	+                            url, filename))

Lots more indentation.  I think this would be easier on the eyes if you only nested the extraction of the file contents, and moved the assertion back to the method's base indentation level.

In src/maastesting/tests/test_saucelabs.py, test_setUp_and_cleanUp tests the exact letter-for-letter contents of the command line that the fixture will run.  Isn't that unnecessarily brittle?  When the command line changes, what will the accompanying test failure tell us besides the change itself?

Also in that file, one_retry iterates once, but does that count as a retry?  Would a name like no_retries or one_attempt cover it more accurately?
-- 
https://code.launchpad.net/~allenap/maas/try-out-saucelabs/+merge/106217
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/try-out-saucelabs into lp:maas.