Add a python testing framework

[JelteF]

This changes our testing infrastructure to use python instead of bash.
Both test/test.sh and test/ssl/test.sh and have been completely
coverted to python.

The main reason I want to introduce this change is because Bash
is one of the worst languages to write tests in. One of the main goals
of tests is to find unexpected errors. And bash makes it very easy not
to notice such errors. This made it quite hard for me to write correct
tests and review newly introduced tests in PRs for correctness.

Another big reason I want this change is because writing tests
for #666 requires running multiple pgbouncer processes at the
same time. The bash infrastructure did not allow for this. The
new python testing infrastructure.

Finally there's some other advantages that this new test
infrastructure brings:

1. Tests can be run in parallel, which makes them go much
   faster. The duration of the test step in CI is now ~1 minute
   for non valgrind runs and ~2 minutes for valgrind runs.
   With the bash testing infrastructure these timings were
   5.5 minutes and ~8 minutes respectively.
2. Running tests with valgrind locally is now as simple as
   ENABLE_VALGRIND=1 pytest -n auto
3. Some tests would not run on some OSes (e.g. ssl tests on
   windows). I made sure almost all are now fully cross platform
   (for the platforms we support in CI).
4. USE_SUDO=1 tests are now run in CI on MacOS and FreeBSD.
   (I confirmed USE_SUDO=1 works with Linux on my local machine,
   but was unable to get it working in CI due to iptables not being
   allowed in the CI containers).

How to run tests can be found in the updated test/README.md file.

PS. I haven't tested with OpenBSD, since I don't have access to such a
machine. I'm fairly confident that the tests will run fine though, since
they pass on all CI plaftorms (including MacOS and FreeBSD). But if
anyone else has access to an OpenBSD box, it would be great if you
could test that tests pass there as well.

[JelteF]

This PR is now ready for review.

For reference this is an example of the time it takes to run tests now:

And before (with #793) it was:

[JelteF]

This was the only bug that this new testing infrastructure uncovered. It seems that the previous bash tests did not allow many bugs through so far.

[eulerto]

Impressive!

My Debian 11 reported:

72 passed, 4 skipped, 9 warnings in 34.95s

Previous tests ~ 2 min 10 secs.

One of the things that annoys me was:

conftest.py:14: in create_certs
    os.rename(TEST_DIR / "ssl" / "TestCA1", cert_dir / "TestCA1")
E   OSError: [Errno 18] Invalid cross-device link: '/a/pgbouncer/test/ssl/TestCA1' -> '/tmp/pytest-of-euler/pytest-3/certs/TestCA1'

Maybe you can replace os.rename with shutil.move. I tried it and it worked just fine.

Are you planning to convert the ssl shell scripts to python as well?

I noticed that you adjusted the code. I'm still checking but it seems it should be a separate commit.

I didn't inspect the tests yet.

[JelteF]

72 passed, 4 skipped, 9 warnings in 34.95s

Previous tests ~ 2 min 10 secs.

Good to hear that you're seeing the speed improvement locally too. I had similar improvements on my 10c/20t machine (make check going from 3.0 minutes to 22 seconds)

Maybe you can replace os.rename with shutil.move. I tried it and it worked just fine.

Good catch! Both my machine and CI don't have multiple disks, so I didn't run into this. I'll make that change.

Are you planning to convert the ssl shell scripts to python as well?

So I did convert the ssl tests, but I indeed kept the scripts to create the certificates that are used during those tests. I thought they did the job pretty well, and the advantage of shelling out to the openssl command from python vs from bash seemed perytty negligible. I added one small script new bash script, but that's really just a the few first line from the original test/ssl/test.sh file copied to a new script.

Honestly this PR already contains many more changes than I normally like in one PR. Normally I'd want to do this in smaller chunks. But I figured that I needed to convert a bunch of tests to prove (also to myself) that the testing framework was designed well. And once I converted more than half of the tests, I thought I might as well convert all. But that's why I'd like to postpone any other changes to some later PR.

[JelteF]

This is new btw, but it changed test runtime from ~33s to ~22s on my machine. And it seemed like a small enough addition to bundle it in here.

[JelteF]

Bonus points for this python infrastructure. It found this issue in CI and allowed me to easily run the same test with VALGRIND multiple times in parallel by installing pytest-repeat: #796

[JelteF]

@eulerto @petere Does any of you want to look at this closer? I'd like to merge this PR soonish, since any other feature/bugfix PR that adds tests will conflict with this.

[eulerto]

I spent an hour on it. I have some cosmetic changes. No code changes. I noticed that make clean does not remove test/__pycache__/. I plan to continue on it tomorrow.

[eulerto]

This version looks good to me.

Tests look like

$ make check USE_SUDO=1 HAVE_IPV6_LOCALHOST=1
etc/optscan.sh
PYTHONIOENCODING=utf8 python3 -m pytest -n auto
============================================ test session starts =============================================
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /a/pgbouncer, configfile: pyproject.toml
plugins: xdist-2.2.0, asyncio-0.14.0, forked-1.3.0, timeout-1.4.1
timeout: 30.0s
timeout method: signal
timeout func_only: False
gw0 [80] / gw1 [80] / gw2 [80] / gw3 [80] / gw4 [80] / gw5 [80] / gw6 [80] / gw7 [80]
................................................................................                       [100%]
============================================== warnings summary ==============================================
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
  /usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148: PytestConfigWarning: Unknown config ini key: asyncio_mode
  
    self._warn_or_fail_if_strict("Unknown config ini key: {}\n".format(key))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
====================================== 80 passed, 9 warnings in 37.19s =======================================
make -C test check
./hba_test
HBA test OK

and if I enable valgrind

$ make check ENABLE_VALGRIND=1 USE_SUDO=1 HAVE_IPV6_LOCALHOST=1
etc/optscan.sh
PYTHONIOENCODING=utf8 python3 -m pytest -n auto
============================================================================================================================= test session starts ==============================================================================================================================
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /a/pgbouncer, configfile: pyproject.toml
plugins: xdist-2.2.0, asyncio-0.14.0, forked-1.3.0, timeout-1.4.1
timeout: 30.0s
timeout method: signal
timeout func_only: False
gw0 [80] / gw1 [80] / gw2 [80] / gw3 [80] / gw4 [80] / gw5 [80] / gw6 [80] / gw7 [80]
................................................................................                                                                                                                                                                                         [100%]
=============================================================================================================================== warnings summary ===============================================================================================================================
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
/usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148
  /usr/lib/python3/dist-packages/_pytest/config/__init__.py:1148: PytestConfigWarning: Unknown config ini key: asyncio_mode
  
    self._warn_or_fail_if_strict("Unknown config ini key: {}\n".format(key))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================================================================================================================= 80 passed, 9 warnings in 50.05s ========================================================================================================================
make -C test check
./hba_test
HBA test OK

I didn't investigate why I'm getting the referred warning. Old pytest?

I noticed that if you enable valgrind there is a leak report. Nothing to worry right now.

==45571== VALGRIND-ERROR-BEGIN
==45571== 304 bytes in 1 blocks are possibly lost in loss record 3 of 3
==45571==    at 0x483AB65: calloc (vg_replace_malloc.c:760)
==45571==    by 0x4012D16: allocate_dtv (dl-tls.c:343)
==45571==    by 0x4012D16: _dl_allocate_tls (dl-tls.c:589)
==45571==    by 0x4D01B81: allocate_stack (allocatestack.c:622)
==45571==    by 0x4D01B81: pthread_create@@GLIBC_2.2.5 (pthread_create.c:660)
==45571==    by 0x1263E2: pam_init (pam.c:129)
==45571==    by 0x1148C0: main (main.c:1011)
==45571== 
==45571== VALGRIND-ERROR-END

Cosmetic changes that I mentioned in the last message are below. (I could have asked you to enable editing this PR but I decided to include it here.)

diff --git a/test/conftest.py b/test/conftest.py
index 4433cc4..b2bf1cc 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -41,7 +41,7 @@ def cert_dir(tmp_path_factory, worker_id):
 
 @pytest.fixture(autouse=True, scope="session")
 def pg(tmp_path_factory, cert_dir):
-    """Starts a new postgres db that is shared for tests in this process"""
+    """Starts a new Postgres db that is shared for tests in this process"""
     pg = Postgres(tmp_path_factory.getbasetemp() / "pgdata")
     pg.initdb()
     os.truncate(pg.hba_path, 0)
@@ -97,7 +97,7 @@ def pg(tmp_path_factory, cert_dir):
 @pytest.mark.asyncio
 @pytest.fixture
 async def bouncer(pg, tmp_path):
-    """Starts a new pgbouncer process"""
+    """Starts a new PgBouncer process"""
     bouncer = Bouncer(pg, tmp_path / "bouncer")
 
     await bouncer.start()
@@ -109,7 +109,7 @@ async def bouncer(pg, tmp_path):
 
 @pytest.fixture(autouse=True)
 def pg_log(pg):
-    """Prints the postgres logs that were created during the test
+    """Prints the Postgres logs that were created during the test
 
     This can be useful for debugging a failure.
     """
@@ -122,7 +122,7 @@ def pg_log(pg):
 
 @pytest.fixture(autouse=True)
 def pg_reset(pg):
-    """Resets any changes to postgres settings from previous tests"""
+    """Resets any changes to Postgres settings from previous tests"""
     pg.reset_hba()
     os.truncate(pg.pgdata / "postgresql.auto.conf", 0)
     pg.reload()
diff --git a/test/test_admin.py b/test/test_admin.py
index 17e60ea..8845ebe 100644
--- a/test/test_admin.py
+++ b/test/test_admin.py
@@ -8,7 +8,7 @@ def test_show(bouncer):
         "clients",
         "config",
         "databases",
-        # Calling show fds on MacOS leaks the returned file descriptors to the
+        # Calling SHOW FDS on MacOS leaks the returned file descriptors to the
         # python test runner. So we don't test this one directly. SHOW FDS is
         # still tested indirectly by the takeover tests.
         # "fds",
diff --git a/test/test_auth.py b/test/test_auth.py
index b424b5d..491b43c 100644
--- a/test/test_auth.py
+++ b/test/test_auth.py
@@ -87,7 +87,7 @@ def run_server_auth_test(bouncer, dbname):
         bouncer.test(dbname=f"{dbname}z")
 
 
-# test plain-text password authentication from PgBouncer to PostgreSQL server
+# Test plain-text password authentication from PgBouncer to PostgreSQL server
 #
 # The PostgreSQL server no longer supports storing plain-text
 # passwords, so the server-side user actually uses md5 passwords in
@@ -111,7 +111,7 @@ def test_scram_server(bouncer):
         psycopg.OperationalError, match="password authentication failed"
     ):
         bouncer.test(dbname="p6x")
-    # good password from auth_file, but is not supported with SCRAM
+    # good password from auth_file, but it is not supported with SCRAM
     with pytest.raises(psycopg.OperationalError, match="wrong password type"):
         bouncer.test(dbname="p6y")
     # bad password from auth_file
@@ -140,7 +140,7 @@ def connect_with_md5_client_users(bouncer):
 
 
 def connect_with_scram_client_users(bouncer):
-    # users with a stored scram password
+    # users with a stored SCRAM password
     bouncer.test(user="scramuser1", password="foo")
     # bad password
     with pytest.raises(
@@ -149,7 +149,7 @@ def connect_with_scram_client_users(bouncer):
         bouncer.test(user="scramuser1", password="wrong")
 
 
-# test plain-text password authentication from client to PgBouncer
+# Test plain-text password authentication from client to PgBouncer
 def test_password_client(bouncer):
     bouncer.admin(f"set auth_type='plain'")
     connect_with_password_client_users(bouncer)
@@ -158,7 +158,7 @@ def test_password_client(bouncer):
 
     # long password
     bouncer.test(user="longpass", password=LONG_PASSWORD)
-    # Too long password
+    # too long password
     with pytest.raises(
         psycopg.OperationalError, match="password authentication failed"
     ):
@@ -177,7 +177,7 @@ def test_scram_client(bouncer):
     connect_with_password_client_users(bouncer)
     connect_with_scram_client_users(bouncer)
 
-    # cannot authenticate to MD5 stored passwords with scram auth
+    # cannot authenticate to MD5 stored passwords with SCRAM auth
     # good password
     with pytest.raises(
         psycopg.OperationalError, match="(password|SASL) authentication failed"
@@ -197,5 +197,5 @@ def test_scram_both(bouncer):
     # plain-text password in userlist.txt
     bouncer.test(dbname="p61", user="scramuser3", password="baz")
 
-    # scram password in userlist.txt
+    # SCRAM password in userlist.txt
     bouncer.test(dbname="p62", user="scramuser1", password="foo")
diff --git a/test/test_limits.py b/test/test_limits.py
index ad6004e..106d1c0 100644
--- a/test/test_limits.py
+++ b/test/test_limits.py
@@ -56,8 +56,8 @@ async def test_min_pool_size(pg, bouncer):
 
 def test_min_pool_size_with_lower_max_user_connections(bouncer):
     # The p0x in test.init has min_pool_size set to 5. This should make
-    # the bouncer try to create a pool for maxedout2 user of size 5 after a
-    # client connects to the bouncer. However maxedout2 user has
+    # the PgBouncer try to create a pool for maxedout2 user of size 5 after a
+    # client connects to the PgBouncer. However maxedout2 user has
     # max_user_connections set to 2, so the final pool size should be only 2.
 
     # Running a query for sufficient time for us to reach the final
@@ -69,8 +69,8 @@ def test_min_pool_size_with_lower_max_user_connections(bouncer):
 
 def test_min_pool_size_with_lower_max_db_connections(bouncer):
     # The p0x in test.init has min_pool_size set to 5. This should make
-    # the bouncer try to create a pool for puser1 user of size 5 after a client
-    # connects to the bouncer. However the db also has max_db_connections set
+    # the PgBouncer try to create a pool for puser1 user of size 5 after a client
+    # connects to the PgBouncer. However the db also has max_db_connections set
     # to 2, so the final pool size should be only 2.
 
     # Running a query for sufficient time for us to reach the final
@@ -103,7 +103,7 @@ async def test_max_db_connections(pg, bouncer):
         *[bouncer.asleep(0.5, dbname="p2", user=u, times=2) for u in users]
     )
 
-    # p2 in pgbouncer maps to p0 in postgres
+    # p2 in PgBouncer maps to p0 in Postgres
     assert pg.connection_count("p0", users=users) == 4
 
 
diff --git a/test/test_no_database.py b/test/test_no_database.py
index 0025f76..e858071 100644
--- a/test/test_no_database.py
+++ b/test/test_no_database.py
@@ -30,10 +30,10 @@ def test_no_database_auth_user(bouncer):
 
 
 def test_no_database_md5_auth_scram_pw_success(bouncer):
-    # Testing what happens on successful SCRAM auth connection to non-existent DB
-    # Segfaults have been seen after mock authentication was put in place
-    # with md5 auth and a scram PW when saving SCRAM credentials. Including this test to check for the
-    # condition repeating.
+    # Testing what happens on successful SCRAM auth connection to non-existent
+    # DB Segfaults have been seen after mock authentication was put in place
+    # with md5 auth and a scram PW when saving SCRAM credentials. Including
+    # this test to check for the condition repeating.
     bouncer.admin(f"set auth_type='md5'")
     with bouncer.log_contains(r"closing because: no such database: nosuchdb"):
         with pytest.raises(
@@ -43,9 +43,10 @@ def test_no_database_md5_auth_scram_pw_success(bouncer):
 
 
 def test_no_database_scram_auth_scram_pw_success(bouncer):
-    # Testing what happens on successful SCRAM auth with a SCRAM PW connection to non-existent DB
-    # Segfaults have been seen after mock authentication was put in place
-    # with md5 auth and a scram PW. Including this test for completeness
+    # Testing what happens on successful SCRAM auth with a SCRAM PW connection
+    # to non-existent DB Segfaults have been seen after mock authentication was
+    # put in place with md5 auth and a scram PW. Including this test for
+    # completeness.
     bouncer.admin(f"set auth_type='scram-sha-256'")
     with bouncer.log_contains(r"closing because: no such database: nosuchdb"):
         with pytest.raises(
@@ -55,9 +56,10 @@ def test_no_database_scram_auth_scram_pw_success(bouncer):
 
 
 def test_no_database_md5_auth_md5_pw_success(bouncer):
-    # Testing what happens on successful MD5 auth with a MD5 pw connection to non-existent DB
-    # Segfaults have been seen after mock authentication was put in place
-    # with md5 auth and a scram PW. Including this test for completeness
+    # Testing what happens on successful MD5 auth with a MD5 pw connection to
+    # non-existent DB Segfaults have been seen after mock authentication was
+    # put in place with md5 auth and a scram PW. Including this test for
+    # completeness.
     bouncer.admin(f"set auth_type='md5'")
     with bouncer.log_contains(r"closing because: no such database: nosuchdb"):
         with pytest.raises(
diff --git a/test/test_ssl.py b/test/test_ssl.py
index 33dbaa0..508999a 100644
--- a/test/test_ssl.py
+++ b/test/test_ssl.py
@@ -12,7 +12,7 @@ if not TLS_SUPPORT:
 # silicon when enabling SSL: https://github.com/psycopg/psycopg/discussions/270
 
 
-# override regular bouncer fixture with one that uses the special ssl config
+# override regular bouncer fixture with one that uses the special SSL config
 @pytest.mark.asyncio
 @pytest.fixture
 async def bouncer(pg, tmp_path):
@@ -197,7 +197,7 @@ def test_client_ssl_set_change_ca(bouncer, cert_dir):
     bouncer.psql_test(host="localhost", sslmode="verify-full", sslrootcert=new_root)
 
 
-@pytest.mark.skipif("WINDOWS", reason="windows does not have SIGHUP")
+@pytest.mark.skipif("WINDOWS", reason="Windows does not have SIGHUP")
 def test_client_ssl_sighup_enable_disable(bouncer, cert_dir):
     root = cert_dir / "TestCA1" / "ca.crt"
     key = cert_dir / "TestCA1" / "sites" / "01-localhost.key"
@@ -215,7 +215,7 @@ def test_client_ssl_sighup_enable_disable(bouncer, cert_dir):
     bouncer.test(sslmode="disable")
 
 
-@pytest.mark.skipif("WINDOWS", reason="windows does not have SIGHUP")
+@pytest.mark.skipif("WINDOWS", reason="Windows does not have SIGHUP")
 def test_client_ssl_sighup_change_ca(bouncer, cert_dir):
     root = cert_dir / "TestCA1" / "ca.crt"
     key = cert_dir / "TestCA1" / "sites" / "01-localhost.key"
diff --git a/test/test_timeouts.py b/test/test_timeouts.py
index b907379..488652a 100644
--- a/test/test_timeouts.py
+++ b/test/test_timeouts.py
@@ -122,7 +122,7 @@ def test_server_connect_timeout_drop_traffic(pg, bouncer):
 def test_tcp_user_timeout(pg, bouncer):
     bouncer.admin("set tcp_user_timeout=1000")
     bouncer.admin("set query_timeout=5")
-    # Make pgbouncer cache a connection to postgres
+    # Make PgBouncer cache a connection to Postgres
     bouncer.test()
     # without tcp_user_timeout, you get a different error message
     # about "query timeout" instead
diff --git a/test/utils.py b/test/utils.py
index 81f2486..cccf7fb 100644
--- a/test/utils.py
+++ b/test/utils.py
@@ -152,7 +152,7 @@ TLS_SUPPORT = get_tls_support()
 # it is a lower change that it will conflict with "in-use" ports
 PORT_LOWER_BOUND = 10200
 
-# ephemeral port start on many linux systems
+# ephemeral port start on many Linux systems
 PORT_UPPER_BOUND = 32768
 
 next_port = PORT_LOWER_BOUND
@@ -195,7 +195,7 @@ class QueryRunner:
         options.setdefault("dbname", self.default_db)
         options.setdefault("user", self.default_user)
         if ENABLE_VALGRIND:
-            # If valgrind is enabled pgbouncer is a significantly slower to
+            # If valgrind is enabled PgBouncer is a significantly slower to
             # respond to connection requests, so we wait a little longer.
             options.setdefault("connect_timeout", 20)
         else:
@@ -227,7 +227,7 @@ class QueryRunner:
     def cur(self, autocommit=True, **kwargs):
         """Open an psycopg cursor to this server
 
-        The connection an the cursors automatically close once you leave the
+        The connection and the cursors automatically close once you leave the
         "with" block
         """
         with self.conn(
@@ -241,7 +241,7 @@ class QueryRunner:
     async def acur(self, **kwargs):
         """Open an asynchronous psycopg cursor to this server
 
-        The connection an the cursors automatically close once you leave the
+        The connection and the cursors automatically close once you leave the
         "async with" block
         """
         async with await self.aconn(**kwargs) as conn:
@@ -249,7 +249,7 @@ class QueryRunner:
                 yield cur
 
     def sql(self, query, params=None, **kwargs):
-        """Run a sql query
+        """Run an SQL query
 
         This opens a new connection and closes it once the query is done
         """
@@ -257,7 +257,7 @@ class QueryRunner:
             cur.execute(query, params=params)
 
     def sql_value(self, query, params=None, **kwargs):
-        """Run a sql query that returns a single cell and return this value
+        """Run an SQL query that returns a single cell and return this value
 
         This opens a new connection and closes it once the query is done
         """
@@ -270,7 +270,7 @@ class QueryRunner:
             return value
 
     def asql(self, query, **kwargs):
-        """Run a sql query in asynchronous task
+        """Run an SQL query in asynchronous task
 
         This opens a new connection and closes it once the query is done
         """
@@ -289,7 +289,7 @@ class QueryRunner:
                 raise
 
     def psql(self, query, **kwargs):
-        """Run a sql query using psql instead of psycopg
+        """Run an SQL query using psql instead of psycopg
 
         This opens a new connection and closes it once the query is done
         """
@@ -508,7 +508,7 @@ class Postgres(QueryRunner):
 
     def reload(self):
         if WINDOWS:
-            # SIGHUP and thus reload don't exist on windows
+            # SIGHUP and thus reload don't exist on Windows
             self.restart()
         else:
             self.pgctl("reload")
@@ -519,7 +519,7 @@ class Postgres(QueryRunner):
         await process.communicate()
 
     def nossl_access(self, dbname, auth_type):
-        """Prepends a local non-ssl access to the hba file"""
+        """Prepends a local non-SSL access to the HBA file"""
         with self.hba_path.open() as pghba:
             old_contents = pghba.read()
         with self.hba_path.open(mode="w") as pghba:
@@ -530,7 +530,7 @@ class Postgres(QueryRunner):
             pghba.write(old_contents)
 
     def ssl_access(self, dbname, auth_type):
-        """Prepends a local ssl access rule to the hba file"""
+        """Prepends a local SSL access rule to the HBA file"""
         with self.hba_path.open() as pghba:
             old_contents = pghba.read()
         with self.hba_path.open(mode="w") as pghba:
@@ -547,7 +547,7 @@ class Postgres(QueryRunner):
         return self.pgdata / "postgresql.conf"
 
     def commit_hba(self):
-        """Mark the current hba contents as non-resetable by reset_hba"""
+        """Mark the current HBA contents as non-resetable by reset_hba"""
         with self.hba_path.open() as pghba:
             old_contents = pghba.read()
         with self.hba_path.open(mode="w") as pghba:
@@ -555,7 +555,7 @@ class Postgres(QueryRunner):
             pghba.write(old_contents)
 
     def reset_hba(self):
-        """Remove any hba rules that were added after the last call to commit_hba"""
+        """Remove any HBA rules that were added after the last call to commit_hba"""
         with self.hba_path.open() as f:
             hba_contents = f.read()
         committed = hba_contents[hba_contents.find("# committed-rules\n") :]
@@ -577,9 +577,9 @@ class Postgres(QueryRunner):
         )
 
     async def delayed_start(self, delay=1):
-        """Start postgres after a delay
+        """Start Postgres after a delay
 
-        NOTE: The sleep is asynchronous, but while waiting for postgres to
+        NOTE: The sleep is asynchronous, but while waiting for Postgres to
         start the pg_ctl start command will block the event loop. This is
         currently acceptable for our usage of this method in the existing
         tests and this way it was easiest to implement. However, it seems
@@ -589,7 +589,7 @@ class Postgres(QueryRunner):
         self.start()
 
     def configure(self, config):
-        """Configure specific postgres settings using ALTER SYSTEM SET
+        """Configure specific Postgres settings using ALTER SYSTEM SET
 
         NOTE: after configuring a call to reload or restart is needed for the
         settings to become effective.
@@ -650,7 +650,7 @@ class Bouncer(QueryRunner):
                 ini.flush()
 
     def base_command(self):
-        """returns the basecommand that is used to run pgbouncer
+        """returns the basecommand that is used to run PgBouncer
 
         This includes valgrind and all its arguments when ENABLE_VALGRIND is
         set
@@ -698,16 +698,16 @@ class Bouncer(QueryRunner):
             break
 
     def admin(self, query, **kwargs):
-        """Run a sql query on the pgbouncer admin database"""
+        """Run an SQL query on the PgBouncer admin database"""
         return self.admin_runner.sql(query, **kwargs)
 
     def admin_value(self, query, **kwargs):
-        """Run a sql query on the pgbouncer admin database that returns only a
+        """Run an SQL query on the PgBouncer admin database that returns only a
         single cell and return this value"""
         return self.admin_runner.sql_value(query, **kwargs)
 
     def aadmin(self, query, **kwargs):
-        """Run a sql query on the pgbouncer admin database in an asynchronous
+        """Run an SQL query on the PgBouncer admin database in an asynchronous
         task"""
         return self.admin_runner.asql(query, **kwargs)
 
@@ -724,10 +724,10 @@ class Bouncer(QueryRunner):
         self.aprocess = None
 
     async def reboot(self):
-        """Starts a new pgbouncer with the --reboot flag
+        """Starts a new PgBouncer with the --reboot flag
 
-        This new pgbouncer process will replace the current process and take
-        over its non-ssl sockets.
+        This new PgBouncer process will replace the current process and take
+        over its non-SSL sockets.
         """
         assert self.aprocess is not None or self.process is not None
         if self.aprocess:
@@ -789,9 +789,9 @@ class Bouncer(QueryRunner):
         self.port_lock.release()
 
     def write_ini(self, config):
-        """Writes a config to the ini file of this pgbouncer
+        """Writes a config to the ini file of this PgBouncer
 
-        It appends a newline automatically. To apply these changes pgbouncer
+        It appends a newline automatically. To apply these changes PgBouncer
         still needs to be reloaded or restarted. To reload in a cross platform
         way you need can use admin("reload").
         """

[eulerto]

LGTM.

[JelteF]

I noticed that make clean does not remove test/pycache/

Doesn't seem super important I'll leave it as is.

I created #799 to track the valgrind issue.

I applied your cosmetic changes and I updated the comment in pyproject.toml to include some stuff about the warning you are seeing.