Hi Robert, Even though the cover letter went completely over my head (well, until I'd read the code and no longer needed it), I'm so looking forward to seeing this work that I grabbed this MP off the review queue. Very exciting stuff. And I like the naming of "ephemeral" instances. Well-chosen term, very succinct. As they say, only two things are hard in computer science: naming things and cache invalidation, and off-by-one errors. (This is a recent change I believe. When I was a young programmer the only really difficult thing was getting a date.) One thing I note is that you've made a lot of little infrastructural improvements in this branch. All good, but some of it (like the fixturizing of test layers) would have been good candidates for splitting off into a preparatory branch for smaller, more focused reviews. My other notes are all superficial. I had the better part of my review typed out when a wrong keypress destroyed it all. I'll try to reconstruct it, but apologize in advance for a lack of polish in this emergency re-do: === modified file 'daemons/librarian.tac' --- daemons/librarian.tac 2010-10-20 18:43:29 +0000 +++ daemons/librarian.tac 2010-10-21 04:51:25 +0000 @@ -25,7 +26,12 @@ dbconfig.setConfigSection('librarian') execute_zcml_for_scripts() -path = config.librarian_server.root +if os.environ.get('LP_TEST_INSTANCE'): This comes up several times. Can you think of a good global place for an equivalent of _dynamic_config? If not, no biggie. If there are any relevant changes, a quick search will reveal all uses of LP_TEST_INSTANCE. -# Set up the public librarian. -uploadPort = config.librarian.upload_port -webPort = config.librarian.download_port -setUpListener(uploadPort, webPort, restricted=False) - -# Set up the restricted librarian. -webPort = config.librarian.restricted_download_port -uploadPort = config.librarian.restricted_upload_port -setUpListener(uploadPort, webPort, restricted=True) +if os.environ.get('LP_TEST_INSTANCE'): + # Running in ephemeral mode: allocate ports on demand. + setUpListener(0, 0, restricted=False) + setUpListener(0, 0, restricted=True) +else: + # Set up the public librarian. + uploadPort = config.librarian.upload_port + webPort = config.librarian.download_port + setUpListener(uploadPort, webPort, restricted=False) + # Set up the restricted librarian. + webPort = config.librarian.restricted_download_port + uploadPort = config.librarian.restricted_upload_port + setUpListener(uploadPort, webPort, restricted=True) Both the "if" and the "else" clause do what the latter's comments say. May I suggest setting uploadPort and webPort in both clauses, and then doing the pair of setUpListener calls afterwards with those parameters? That also makes it nice and clear what that pair of zeros in the first clause means. (And if these two variables are not used in too many places to make it convenient, can you also rename them to upload_port and web_port to match our style guide?) === modified file 'lib/canonical/librarian/testing/server.py' --- lib/canonical/librarian/testing/server.py 2010-09-27 02:08:32 +0000 +++ lib/canonical/librarian/testing/server.py 2010-10-21 04:51:25 +0000 @@ -32,74 +34,14 @@ class LibrarianServerFixture(TacTestSetup): """Librarian server fixture. - >>> from urllib import urlopen - >>> from canonical.config import config - - >>> librarian_url = "http://%s:%d" % ( - ... config.librarian.download_host, - ... config.librarian.download_port) I see you moved these checks to a more appropriate place in the unit tests. Down with >>>! Not that I usually remember to do it myself, but this sort of thing would be nice to mention in the cover letter. In this case it would have saved me the time of figuring out where the test went. - :ivar: pid pid of the external process. + :ivar service_config: A config fragment with the variables for this + service. + :ivar root: the root of the server storage area. + :ivar upload_port: the port to upload on. + :ivar download_port: the port to download from. + :ivar restricted_upload_port: the port to upload restricted files on. + :ivar restricted_download_port: the port to upload restricted files from. + :ivar pid: pid of the external process. Didn't know about :ivar: but it looks like something I've been wishing for. Must start using it. @@ -155,22 +96,87 @@ @property def root(self): """The root directory for the librarian file repository.""" - return config.librarian_server.root + if self._dynamic_config(): + return self._root + else: + return config.librarian_server.root From an exception string in the pidfile property I gather that this may be needed because of the setup order. Is there no good place to set this as a simple attribute? + def _waitForDaemonStartup(self): + super(LibrarianServerFixture, self)._waitForDaemonStartup() + # Expose the dynamically allocated ports, if we used them. + if not self._dynamic_config(): + self.download_port = config.librarian.download_port + self.upload_port = config.librarian.upload_port + self.restricted_download_port = \ + config.librarian.restricted_download_port + self.restricted_upload_port = \ + config.librarian.restricted_upload_port Our style guides wants us to break lines using parentheses rather than backslashes: + self.restricted_download_port = ( + config.librarian.restricted_download_port) For reference, search for "\" in https://dev.launchpad.net/PythonStyleGuide + return + chunks = self.logChunks() + # A typical startup: upload, download, restricted up, restricted down. + #2010-10-20 14:28:21+0530 [-] Log opened. + #2010-10-20 14:28:21+0530 [-] twistd 10.1.0 (/usr/bin/python 2.6.5) starting up. + #2010-10-20 14:28:21+0530 [-] reactor class: twisted.internet.selectreactor.SelectReactor. + #2010-10-20 14:28:21+0530 [-] canonical.librarian.libraryprotocol.FileUploadFactory starting on 59090 + #2010-10-20 14:28:21+0530 [-] Starting factory + #2010-10-20 14:28:21+0530 [-] twisted.web.server.Site starting on 58000 + #2010-10-20 14:28:21+0530 [-] Starting factory + #2010-10-20 14:28:21+0530 [-] canonical.librarian.libraryprotocol.FileUploadFactory starting on 59095 + #2010-10-20 14:28:21+0530 [-] Starting factory + #2010-10-20 14:28:21+0530 [-] twisted.web.server.Site starting on 58005 + self.upload_port = int(chunks[3].split()[-1]) + self.download_port = int(chunks[5].split()[-1]) + self.restricted_upload_port = int(chunks[7].split()[-1]) + self.restricted_download_port = int(chunks[9].split()[-1]) I find the example very useful. But this code is still a bit hard to read. Three things I can suggest: 1. Extract method. 2. A few blank lines would make things easier for me. That may be a matter of differences in editors and browsers though. 3. It'd be nice to avoid losing the list's shape inside string/tuple manipulation, so we're not too puzzled if it ever changes. Not the one and only true way by any means, but perhaps something like def get_int_from_log(log_chunk): """Extract an integer from a chunk of log line.""" return int(log_chunk.split()[-1]) upload_port = get_int_from_log(chunks[3]) ... @@ -186,21 +192,26 @@ @property def pidfile(self): - return os.path.join(self.root, 'librarian.pid') + try: + return os.path.join(self.root, 'librarian.pid') + except AttributeError: + # Not setup and using dynamic config: tachandler reads this early. + return '/tmp/unused/' I'm having trouble understanding the comment. Is the "setup" a verb? Is the "and" perhaps meant to be "when"? The same thing applies in the logfile property. -def cleanupLibrarianFiles(): - """Remove all librarian files present in disk.""" - _global_fixture.clear() Much nicer with fixtures. I'm hoping to use more of them myself. === modified file 'lib/canonical/librarian/testing/tests/test_server_fixture.py' --- lib/canonical/librarian/testing/tests/test_server_fixture.py 2010-09-26 21:22:04 +0000 +++ lib/canonical/librarian/testing/tests/test_server_fixture.py 2010-10-21 04:51:25 +0000 -def test_suite(): - result = unittest.TestLoader().loadTestsFromName(__name__) - result.addTest(doctest.DocTestSuite( - 'canonical.librarian.testing.server', - optionflags=doctest.NORMALIZE_WHITESPACE | doctest.ELLIPSIS - )) - return result I was afraid to touch cases like this when I cleaned out test_suite functions. Won't this stop doctests from running? Or was it there only for that docstring test you deleted? class TestLibrarianServerFixture(TestCase): - def test_on_init_no_pid(self): - fixture = LibrarianServerFixture() + layer = DatabaseLayer + + def skip_if_persistent(self, fixture): if fixture._persistent_servers(): self.skip('persistent server running.') + + def test_on_init_no_pid(self): + fixture = LibrarianServerFixture() + self.skip_if_persistent(fixture) self.assertEqual(None, fixture.pid) + + def test_setUp_allocates_resources(self): + fixture = LibrarianServerFixture() + self.skip_if_persistent(fixture) + with fixture: + try: + self.assertNotEqual(config.librarian_server.root, fixture.root) + self.assertNotEqual( + config.librarian.download_port, + fixture.download_port) + self.assertNotEqual( + config.librarian.upload_port, + fixture.upload_port) + self.assertNotEqual( + config.librarian.restricted_download_port, + fixture.restricted_download_port) + self.assertNotEqual( + config.librarian.restricted_upload_port, + fixture.restricted_upload_port) + # And it exposes a config fragment + # (Which is not activated by it. Looks like this comment was never completed. + expected_config = dedent("""\ + [librarian_server] + root: %s + [librarian] + download_port: %s + upload_port: %s + download_url: http://launchpad.dev:%s/ + restricted_download_port: %s + restricted_upload_port: %s + restricted_download_url: http://launchpad_dev:%s/ + """) % ( + fixture.root, + fixture.download_port, + fixture.upload_port, + fixture.download_port, + fixture.restricted_download_port, + fixture.restricted_upload_port, + fixture.restricted_download_port, + ) + self.assertEqual(expected_config, fixture.service_config) This looks a bit brittle. I'm not sure what you do and don't want this to break on (ordering changes, whitespace changes, added items) but I wonder: would it make more sense to parse fixture.service_config and compare individual values rather than the full text? + def test_logChunks(self): + fixture = LibrarianServerFixture() + with fixture: + chunks = fixture.logChunks() This looks confusing: "log" can be either a verb or a noun. I'm not sure it was ever written down and ported over to our current wikis but our dromedaryCase method names were specified as having to start with a verb. So this looks like you're saying: "fixture, log your chunks." Which sounds disturbingly biological. + self.assertIsInstance(chunks, list) + found_started = False + for chunk in chunks: + if 'daemon ready' in chunk: + found_started = True + self.assertTrue(found_started) How big is chunks? If it's not too big, it might be easier (both in coding and in debugging) to just assertIn that 'daemon ready' occurs in '\n'.join(chunks). + def test_smoke_test(self): + # Avoid indefinite hangs: + import socket Does that comment refer to the addCleanup and is the import just here to avoid a circular import? If so, an explanatory comment is required for the import. See the python style guide; look for "Circular imports". + self.addCleanup(socket.setdefaulttimeout, socket.getdefaulttimeout()) + socket.setdefaulttimeout(1) + fixture = LibrarianServerFixture() + with fixture: + librarian_url = "http://%s:%d" % ( + config.librarian.download_host, + fixture.download_port) + restricted_librarian_url = "http://%s:%d" % ( + config.librarian.restricted_download_host, + fixture.restricted_download_port) + # Both download ports work: + self.assertTrue('Copyright' in urlopen(librarian_url).read()) + self.assertTrue( + 'Copyright' in urlopen(restricted_librarian_url).read()) Two more candidates for self.assertIn? There's another one at the very bottom here: + os.path.isdir(fixture.root) + # Ports are closed on cleanUp. + self.assertRaises(IOError, urlopen, librarian_url) + self.assertRaises(IOError, urlopen, restricted_librarian_url) + self.assertFalse(os.path.exists(fixture.root)) + # We can use the fixture again (gets a new URL): + with fixture: + librarian_url = "http://%s:%d" % ( + config.librarian.download_host, + fixture.download_port) + self.assertTrue('Copyright' in urlopen(librarian_url).read()) ...That one. === modified file 'lib/canonical/librarian/tests/test_sigdumpmem.py' --- lib/canonical/librarian/tests/test_sigdumpmem.py 2010-09-27 01:03:03 +0000 +++ lib/canonical/librarian/tests/test_sigdumpmem.py 2010-10-21 04:51:25 +0000 @@ -23,10 +23,8 @@ - # Use the global instance used by the Layer machinery; it would - # be nice to be able to access those without globals / magical - # 'constructors'. - pid = LibrarianTestSetup().pid + # Use the global instance used by the Layer machinery + pid = LibrarianLayer.librarian_fixture.pid Nice indeed. === modified file 'lib/canonical/testing/ftests/test_layers.py' --- lib/canonical/testing/ftests/test_layers.py 2010-10-21 04:51:19 +0000 +++ lib/canonical/testing/ftests/test_layers.py 2010-10-21 04:51:25 +0000 @@ -48,30 +53,70 @@ from lp.services.memcache.client import memcache_client_factory +class BaseLayerIsolator(Fixture): + """A fixture for isolating BaseLayer. + + This is useful to test interactions with LP_PERSISTENT_TEST_SERVICES + which makes tests within layers unable to test that easily. + """ + + def __init__(self, with_persistent=False): + """Create a BaseLayerIsolator. + + :param with_persistent: If True LP_PERSISTENT_TEST_SERVICES will + be enabled during setUp. + """ + super(BaseLayerIsolator, self).__init__() + self.with_persistent = with_persistent + + def setUp(self): + super(BaseLayerIsolator, self).setUp() + if self.with_persistent: + env_value = '' + else: + env_value = None + self.useFixture( + EnvironmentVariableFixture('LP_PERSISTENT_TEST_SERVICES', + env_value)) Indentation went wrong here. Depending on your toolchain it may mean that an evil nasty filthy tab managed to sneak in. @@ -246,6 +283,35 @@ +class LibrarianLayerTest(testtools.TestCase, TestWithFixtures): + + def test_makes_unique_instance(self): + # Capture the original settings + default_root = config.librarian_server.root + download_port = config.librarian.download_port + restricted_download_port = config.librarian.restricted_download_port + self.useFixture(BaseLayerIsolator()) + with nested( + LayerFixture(BaseLayer), + LayerFixture(DatabaseLayer), + ): + with LayerFixture(LibrarianLayer): Can't the LibraranLayer fixture go into the nested()? Also, there's a small problem with indentation in these situations: the continuation of the "heading" of the code block (in this case the nested()) is indented at the same level as the body of the code block, with nothing inbetween. The review team didn't like that, so we came up with two alternative suggestions for getting around it: 1. Keep whatever you need for the "heading" in variables so it fits on one line. 2. Separate the "heading" and the block's body with a comment. + # This needs more sophistication in the config system (it needs to + # affect configs on disk and other perhaps other processes This comment was never completed. Also typo: one "other" too many. + # self.assertEqual(default_root, config.librarian_server.root) What's this for? Is it related to the unfinished comment somehow? === modified file 'lib/lp/testing/__init__.py' --- lib/lp/testing/__init__.py 2010-10-19 23:04:18 +0000 +++ lib/lp/testing/__init__.py 2010-10-21 04:51:25 +0000 @@ -490,14 +490,27 @@ + def attachLibrarianLog(self, fixture): + """Include the logChunks from fixture in the test details.""" + # Evaluate the log when called, not later, to permit the librarian to + # be shutdown before the detail is rendered. + chunks = fixture.logChunks() + content = Content(UTF8_TEXT, lambda:chunks) I don't suppose just passing fixture.logChunks instead of lambda:chunks would work? Then it would make me feel better if you could add a space after the colon: "lambda: chunks". def setUp(self): testtools.TestCase.setUp(self) from lp.testing.factory import ObjectFactory + from canonical.testing.layers import LibrarianLayer Since you seem to know and I don't, and someone is bound to want to know, could you add a comment explaining why these imports are inline here? Jeroen