Merge lp:~jtv/launchpad/bug-536769 into lp:launchpad

One minor comment:

19 === modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
20 --- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-06 04:57:40 +0000
21 +++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-10 18:05:30 +0000
38 @@ -81,7 +83,12 @@
39 class TestTranslationTemplatesBuildBehavior(TestCaseWithFactory):
40 """Test `TranslationTemplatesBuildBehavior`."""
41
42 - layer = ZopelessDatabaseLayer
43 + layer = LaunchpadZopelessLayer
44 +
45 + def _becomeBuilddMaster(self):
46 + """Log into the database as the buildd master."""
47 + transaction.commit()
^^^ Is the transaction.commit() really necessary here, would a Store.flush not work ?

48 + self.layer.switchDbUser(config.builddmaster.dbuser)
49
50 def _makeBehavior(self):
51 """Create a TranslationTemplatesBuildBehavior.

I've got nothing to add to Jelmer's review.

I did have one question for my own learning though: is this the type of thing you were *against* using the with statement for? ie. creating a context manager that also switched back so that you could do:

with dbUserAs(config.builddmaster.dbuser):
    ...

?

@jelmer: I remembered after our conversation, the review type should be code*. Thanks for the review! I nice easy one to start with :-)

Hi Jelmer,

Thanks for the review.

> 45 + def _becomeBuilddMaster(self):
> 46 + """Log into the database as the buildd master."""
> 47 + transaction.commit()
> ^^^ Is the transaction.commit() really necessary here, would a Store.flush not
> work ?

Good question to ask! In this case, yes, we're switching database identities. And you can't do that in the same transaction, so flushing wouldn't make the most recent data changes visible.

This is a pattern we come across sometimes in our test: you want to test what you do on the database under realistic permissions, but those permissions aren't enough to set up your test data. The only choice is to commit and switch identities. And not do it too often, of course. :-)

Jeroen

Hi Michael,

> I did have one question for my own learning though: is this the type of thing
> you were *against* using the with statement for? ie. creating a context
> manager that also switched back so that you could do:

No, actually. I do believe Robert noted that that's a bad thing to do when changing directories in production code, but of course that's a far cry from a temporary state in a unit test.

For the record: what I object to in "with" is the exception-handling design of the context-manager protocol. It mixes two completely different goals that I think should not be combined: a "conditional catch/re-raise" that invites clever, obscure, and brittle control flow; and a simple "enter/exit" mechanism that makes "finally" clauses a bit easier to write. (The latter is helpful, though I still have the separate and minor gripe that it puts burdens on the calling code that from a C++ standpoint should be contained in the class that gives rise to them. But "with" did become a real improvement over "finally" with the more recent support for multiple context managers in a single block.)

The justifying use-case for the IMHO dangerous mixing of "conditional catch/re-raise" and "enter/exit" was a transaction handler that would commit implicitly if no exception was raised. Bad idea! That kind of design can work in very limited cases, but it's brittle to the point of irresponsibility. If you want that sort of thing, separate the two: (1) a transaction manager that aborts if not explicitly committed by exit time—note: no need to meddle with exceptions. And (2) a control-flow construct that commits the transaction if the contained block exits normally—note: an explicit commit at the end of the block expresses this about as well, in less code, without breaking up the order of events; and if necessary a conventional function call can give you the same "if we get to the end of all this, commit in a single place" structure.

Once these two things are bound into one context manager, what happens when you discover you need different behavior with particular exception paths? (a) Go back to custom control flow, which is now the suspicious thing to do. (b) Do a little exception dance to present the context manager with the expected situations at block exit. (c) Have more transaction managers (a hierarchy?) for the same resource but with different exception behaviors. All of these are bad.

To me, it would have made more sense to start out with a simpler enter/exit mechanism and see what else is really needed for code instrumentation, customized control flow, or whatever. But that involves weaseling out of fun and challenging design discussions for cool features, and standing up to the snake who says "taste this, it will give you the knowledge of exceptions and God-like power over control flow."

Did someone mention that there's no single right way to do things unless you're Dutch? They say we have opinions on everything. :)

> with dbUserAs(config.builddmaster.dbuser):
> ...
>
> ?

Is there any need to switch back afterwards? I thought this was part of normal test isolation.

Jeroen