Thanks for the review Jeroen, this will be landing mid next week I
suspect - its the third of a three deep stack of branches.

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

mmm, OTOH they couldn't land [much] earlier as the code being improved
is an older iteration of the overall theme; seeing it makes the code
using the fixtures easier to understand I think - because its both
new, and not in devel yet anyway.

I really appreciate you finding my half done comments, I think I was
getting fatigued with the branch.

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

yea, so I probably should write that up somewhere - it originates in
the now playing uniqueconfig branch; that idiom is the contract, we
could wrap it, but I think its unlikely to change anytime soon, and we
can easily grep if we do change it.

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

Note that the (moved) code in the else: block reused webPort and
uploadPort, so you'd need upload_port, restricted_upload_port and so
on; and it would be arguably less clear that one side is ephemeral.
OTOH this is a pretty standard refactoring. I could go either way; I'm
inclined to leave it now, because this changes very rarely and its not
clearly better one way or the other.

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

easily

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

Yeah, this is fugly shit man. So the long term arc is to make all the
fixtures ephemeral-capable, specialise separately for persistent test
services and eliminate the conditional code. Need to do memcache,
buildd master, codehosting sftp etc first.

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

mmm, thats actually often less clear when its not a tuple. Its cute
that (foo) == foo in python, but you have to think harder about it.


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

Personal pet peeve: VWS in functions. If you need it, you need
extract-method; which I'll do, I was nearly going to add a new top
level templateMethod anyway, so I'll just go ahead and do that.

> 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

sure, again, was on my threshold; I think doing it is probably worth
it on tachandler as we may have more in the future.
> @@ -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 and is deliberate... How about this?
# An attempt to read the pidfile before this fixture was setUp, with
dynamic configuration
# the pidfile location is undefined at this point in time.
> 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?

That was the one doctest, yes.

> +                # And it exposes a config fragment
> +                # (Which is not activated by it.
>
> Looks like this comment was never completed.

Just a missing ), fwiw.

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

Well, this is the unit test for the creation of that string; its true
that if the creation logic changes the test will too - but thats
deliberate at this layer.
What would change the creation logic? Changes to the config schema for
this module - we'd want to test that. And changes to lazr-config -
we'd want to deal with that too.

What would be really nice would be lazr config supporting a 'now give
me a fragment' idiom. E.g. - magical sketching here:
config.push()
config.librarian.upload_port = 2134
...
self.service_config = config.pop_changes()

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

good point, will change.

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

In this case I want to be sure that that message isn't split between
chunks, because other code depends on it - so this is testing the
desired contract.

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

oversight in moved code.

> Two more candidates for self.assertIn?  There's another one at the very bottom here:

I wasn't aware of assertIn for LP; will update.

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

Nah, brain fail.

> +        with nested(
> +            LayerFixture(BaseLayer),
> +            LayerFixture(DatabaseLayer),
> +            ):
> +            with LayerFixture(LibrarianLayer):
>
>
> Can't the LibraranLayer fixture go into the nested()?

No, because we need to make assertions after the librarian is cleaned
up, before the database is cleaned up.

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

Whats the motivation here? Its easily readable, visually distinctive
(the ): line is a clear marker).

> +            # 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.

Thanks, I seem to be having ) trouble. And other things ;).

> +            # self.assertEqual(default_root, config.librarian_server.root)
>
> What's this for?  Is it related to the unfinished comment somehow?

Oh, TDD recursion I hadn't quite popped out. It should work now, so
I'll uncomment it.


> +    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".

No, because that would evaluate late, which the comment in the code
block addresses.


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

No idea, I'd WAG at circular imports. will fiddle a little and see whats up.

-Rob