[...]
> Some say that the pattern of “comment announcing block of code, block of code”
> is an indicator that you're probably better off extracting separate methods.
> I think in this case the terminate-and-wait loop is a bit mechanical and
> potentially worth extracting.  It also gives you a bit more freedom to do
> things like a direct “return” on success, and to unit test the loop's corner
> cases.  The while / if / sleep / else / break / else / raise loop, while
> small, may be just a bit too easy to break in maintenance.

I've factored some of it out into a new countdown() function. I've
stuck with break instead of return, just because. I don't have strong
feelings about early returns, but here I marginally prefer the break.

> Oh, and while you're at it, could you capitalize and punctuate that error
> message?  It helps confirm to the hurried reader that the message is an
> independent sentence, rather than a continuation of an earlier statement (or
> of a traceback line) that the eye should skip over.

Done.

[...]
> Very similar to what you were doing in stop().  Once again I think this is
> easier to deal with if you extract it.  There's probably a reusable thingy
> somewhere for this basic structure, but I don't suppose it's worth digging up.

Yep, changed as for stop().

> === modified file 'pgbouncer/tests.py'
> --- pgbouncer/tests.py  2011-09-05 15:01:52 +0000
> +++ pgbouncer/tests.py  2011-10-25 20:23:23 +0000
> @@ -15,7 +15,7 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> 
>  import os
> -from unittest import main, TestLoader
> +import unittest
> 
>  import fixtures
>  import psycopg2
> @@ -28,10 +28,12 @@
>  # A 'just-works' workaround for Ubuntu not exposing initdb to the main PATH.
>  os.environ['PATH'] = os.environ['PATH'] + ':/usr/lib/postgresql/8.4/bin'
> 
> +
> +test_loader = testresources.TestLoader()
> +
> +
>  def test_suite():
> -    result = testresources.OptimisingTestSuite()
> -    result.addTest(TestLoader().loadTestsFromName(__name__))
> -    return result
> +    return test_loader.loadTestsFromName(__name__)
> 
> 
> Not that I have any idea what I'm talking about, but is there any risk that
> this global initialization might cause trouble for “make lint” checks (which I
> believe import the test as a module)?

TestLoader.__init__() is not overridden from object() and
loadTestsFromName() does not mutate anything, so this is safe.
However, test_loader.discover() does mutate self so I've changed this
to create a new TestLoader as needed, Just In Case.

> 
> 
> @@ -49,15 +51,22 @@
> 
>      resources = [('db', DatabaseManager(initialize_sql=setup_user))]
> 
> +    def setUp(self):
> +        super(TestFixture, self).setUp()
> +        self.bouncer = PGBouncerFixture()
> +        self.bouncer.databases[self.db.database] = 'host=' + self.db.host
> +        self.bouncer.users['user1'] = ''
> 
> You're doing this test a world of good.  Still, it's a shame to jump through
> the “super” hoop *and* create implicit state *and* make an explicit call
> whenever you use the fixture.
> 
> Why not fold all of it into a single explicit call?  What I mean is:
> 
>     def makeBouncerFixture(self, unix_socket_dir=None):
>         bouncer = PGBouncerFixture()
>         bouncer.databases[self.db.database] = 'hosts=' + self.db.host
>         bouncer.users['user1'] = ''
>         if unix_socket_dir is not None:
>             bouncer.unix_socket_dir = unix_socket_dir
>         return self.useFixture(bouncer)
> 
> Then replace every “self.useFixture(self.bouncer)” with
> “self.makeBouncerFixture()” and done.  It's shorter, even.  And if you need
> the bouncer object in a test, just capture the method's return value.
> 
> In “connect” of course you'd need to pass the bouncer as an extra argument.
> Not sure if that negates the winnings.

It gets a little ugly because now the connect() method can no longer
assume that it should connect as "user1". Well, it _can_ assume that,
but it's less concrete than having it defined every time by
setUp(). There are ways around that... but I'm inclined to stick with
what's there. I thing setUp() can easily be overused, but I think it's
fine here because every test uses self.bouncer and self.connect.

I have simplified the connect() method.

[...]
> I like Unix domain sockets.  Why mess with global system state when you don't
> have to?

Indeed :)

>  if __name__ == "__main__":
> -    main(testLoader=TestLoader())
> +    unittest.main(testLoader=test_loader)
> 
> 
> I'll be honest: I have no idea how this boilerplate differs from the usual
> test_suite boilerplate.

This probably isn't needed, but I'll leave it in for now. It does no
harm, I hope.

> Now to read your answers to the questions I posted earlier.  But code-wise,
> this looks good.

Now to respond to your response :)

Thanks for the review.