Use fake HTTP servers in tests (WIP)

We introduce a few fixtures, defined in tests/conftest.py:

- http_server_factory, a factory to create HTTP server serving files in
  specified directory
- rest_api_server, an HTTP server serving files in tests/json directory
- runner, a CliRunner, configured with stdout and stderr separated as
  logs of the HTTP server thread would appear in stderr, and if mixed
  with stdout, the assertions in test case will fail.

The direct advantage of this is that PatroniResource.rest_api() method
is now covered by the test suite.

In test_api.py, we either use the main 'rest_api_server' or build a
specific one to check 404 case.

In test_cluster_config_has_changed.py, we build a specific server
serving only one file.

The test suite is slower however now, as we start and stop an HTTP server
for each test case. This might be improved later by using another scope
(e.g. module or package).

tests/__init__.py

@@ -0,0 +1,12 @@
+from http.server import HTTPServer
+from pathlib import Path
+from typing import Callable
+
+
+class RESTAPIServer(HTTPServer):
+    @property
+    def endpoint(self) -> str:
+        return f"http://{self.server_name}:{self.server_port}"
+
+
+ServerFactory = Callable[[Path], RESTAPIServer]

tests/conftest.py

@@ -1,11 +1,21 @@
+import logging
+import pathlib
+from collections.abc import Callable, Iterator
 from functools import partial
-from typing import Any, Callable
+from http.server import SimpleHTTPRequestHandler
+from threading import Thread
+from typing import Any
 
 import pytest
+from click.testing import CliRunner
 from pytest_mock import MockerFixture
 
+from . import RESTAPIServer, ServerFactory
 from .tools import my_mock
 
+logger = logging.getLogger(__name__)
+logger.setLevel(logging.DEBUG)
+
 
 def pytest_addoption(parser: Any) -> None:
     """
@@ -26,3 +36,53 @@ def fake_restapi(
     mocker: MockerFixture, use_old_replica_state: bool
 ) -> Callable[..., Any]:
     return partial(my_mock, mocker, use_old_replica_state=use_old_replica_state)
+
+
+@pytest.fixture
+def datadir() -> pathlib.Path:
+    return pathlib.Path(__file__).parent / "json"
+
+
+@pytest.fixture
+def http_server_factory(
+    use_old_replica_state: bool,  # XXX
+) -> Iterator[Callable[[pathlib.Path], RESTAPIServer]]:
+    """Factory fixture to create HTTP server based on specified directory."""
+    servers_and_threads = []
+
+    class JSONHTTPRequestHandler(SimpleHTTPRequestHandler):
+        def translate_path(self, path: str) -> str:
+            # TODO: handle use_old_replica_state here
+            path = path + ".json"
+            logger.debug("serving %s", path)
+            return super().translate_path(path)
+
+    def create_server(directory: pathlib.Path) -> RESTAPIServer:
+        """Create and start an HTTP server from a request handler class."""
+        handler_cls = partial(JSONHTTPRequestHandler, directory=str(directory))
+        httpd = RESTAPIServer(("", 0), handler_cls)
+        logger.info("starting HTTP server at %s", httpd.endpoint)
+        t = Thread(target=httpd.serve_forever)
+        t.start()
+        servers_and_threads.append((httpd, t))
+        return httpd
+
+    yield create_server
+
+    for httpd, t in servers_and_threads:
+        httpd.shutdown()
+        t.join()
+
+
+@pytest.fixture
+def rest_api_server(
+    datadir: pathlib.Path, http_server_factory: ServerFactory
+) -> RESTAPIServer:
+    """An HTTP server using ./json directory."""
+    return http_server_factory(datadir)
+
+
+@pytest.fixture
+def runner() -> CliRunner:
+    """A CliRunner with stdout and stderr not mixed."""
+    return CliRunner(mix_stderr=False)

tests/json/patroni.json

@@ -0,0 +1,3 @@
+{
+    "pending_restart": false
+}

tests/test_api.py

@@ -1,23 +1,23 @@
+from pathlib import Path
+
 from click.testing import CliRunner
 
 from check_patroni.cli import main
 
+from . import RESTAPIServer, ServerFactory
 
-def test_api_status_code_200(fake_restapi) -> None:
-    runner = CliRunner()
 
-    fake_restapi("node_is_pending_restart_ok")
+def test_api_status_code_200(runner: CliRunner, rest_api_server: RESTAPIServer) -> None:
     result = runner.invoke(
-        main, ["-e", "https://10.20.199.3:8008", "node_is_pending_restart"]
+        main, ["-e", rest_api_server.endpoint, "node_is_pending_restart"]
     )
     assert result.exit_code == 0
 
 
-def test_api_status_code_404(fake_restapi) -> None:
-    runner = CliRunner()
-
-    fake_restapi("Fake test", status=404)
-    result = runner.invoke(
-        main, ["-e", "https://10.20.199.3:8008", "node_is_pending_restart"]
-    )
+def test_api_status_code_404(
+    tmp_path: Path, http_server_factory: ServerFactory, runner: CliRunner
+) -> None:
+    # HTTP server using an empty (tmp) directory, thus responding with 404s.
+    httpd = http_server_factory(tmp_path)
+    result = runner.invoke(main, ["-e", httpd.endpoint, "node_is_pending_restart"])
     assert result.exit_code == 3

tests/test_cluster_config_has_changed.py

@@ -1,20 +1,31 @@
+import shutil
 from pathlib import Path
 
 import nagiosplugin
+import pytest
 from click.testing import CliRunner
 
 from check_patroni.cli import main
 
+from . import ServerFactory
 
-def test_cluster_config_has_changed_ok_with_hash(fake_restapi) -> None:
-    runner = CliRunner()
 
-    fake_restapi("cluster_config_has_changed")
+@pytest.fixture
+def rest_api_endpoint(
+    http_server_factory: ServerFactory, tmp_path: Path, datadir: Path
+) -> str:
+    shutil.copy(datadir / "cluster_config_has_changed.json", tmp_path / "config.json")
+    return http_server_factory(tmp_path).endpoint
+
+
+def test_cluster_config_has_changed_ok_with_hash(
+    runner: CliRunner, rest_api_endpoint: str
+) -> None:
     result = runner.invoke(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--hash",
             "96b12d82571473d13e890b893734e731",
@@ -28,20 +39,17 @@ def test_cluster_config_has_changed_ok_with_hash(fake_restapi) -> None:
 
 
 def test_cluster_config_has_changed_ok_with_state_file(
-    fake_restapi, tmp_path: Path
+    runner: CliRunner, rest_api_endpoint: str, tmp_path: Path
 ) -> None:
-    runner = CliRunner()
-
     state_file = tmp_path / "cluster_config_has_changed.state_file"
     with state_file.open("w") as f:
         f.write('{"hash": "96b12d82571473d13e890b893734e731"}')
 
-    fake_restapi("cluster_config_has_changed")
     result = runner.invoke(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--state-file",
             str(state_file),
@@ -54,15 +62,14 @@ def test_cluster_config_has_changed_ok_with_state_file(
     )
 
 
-def test_cluster_config_has_changed_ko_with_hash(fake_restapi) -> None:
-    runner = CliRunner()
-
-    fake_restapi("cluster_config_has_changed")
+def test_cluster_config_has_changed_ko_with_hash(
+    runner: CliRunner, rest_api_endpoint: str
+) -> None:
     result = runner.invoke(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--hash",
             "96b12d82571473d13e890b8937ffffff",
@@ -76,21 +83,18 @@ def test_cluster_config_has_changed_ko_with_hash(fake_restapi) -> None:
 
 
 def test_cluster_config_has_changed_ko_with_state_file_and_save(
-    fake_restapi, tmp_path: Path
+    runner: CliRunner, tmp_path: Path, rest_api_endpoint: str
 ) -> None:
-    runner = CliRunner()
-
     state_file = tmp_path / "cluster_config_has_changed.state_file"
     with state_file.open("w") as f:
         f.write('{"hash": "96b12d82571473d13e890b8937ffffff"}')
 
-    fake_restapi("cluster_config_has_changed")
     # test without saving the new hash
     result = runner.invoke(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--state-file",
             str(state_file),
@@ -115,7 +119,7 @@ def test_cluster_config_has_changed_ko_with_state_file_and_save(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--state-file",
             str(state_file),
@@ -136,17 +140,16 @@ def test_cluster_config_has_changed_ko_with_state_file_and_save(
     assert new_config_hash == "96b12d82571473d13e890b893734e731"
 
 
-def test_cluster_config_has_changed_params(fake_restapi, tmp_path: Path) -> None:
+def test_cluster_config_has_changed_params(
+    runner: CliRunner, tmp_path: Path, rest_api_endpoint: str
+) -> None:
     # This one is placed last because it seems like the exceptions are not flushed from stderr for the next tests.
-    runner = CliRunner()
-
     fake_state_file = tmp_path / "fake_file_name.state_file"
-    fake_restapi("cluster_config_has_changed")
     result = runner.invoke(
         main,
         [
             "-e",
-            "https://10.20.199.3:8008",
+            rest_api_endpoint,
             "cluster_config_has_changed",
             "--hash",
             "640df9f0211c791723f18fc3ed9dbb95",