From a4fbb861850b37818e87864ec9809bc8f58ae34e Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 26 Mar 2026 17:54:02 +1100 Subject: [PATCH 1/6] docker: use container naming consistent across projects --- docker-compose.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index ed2a45f..20aa18b 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -1,7 +1,7 @@ services: sync_init: user: "${UID}:${GID}" - image: git_hg_sync + image: git-hg-sync entrypoint: ["/app/create_clones.sh", "/clones"] environment: # Let the script create .ssh somewhere writable @@ -9,9 +9,9 @@ services: volumes: - .:/app - ./clones:/clones:z - sync: &sync_config + git-hg-sync: &sync_config user: "${UID}:${GID}" - image: git_hg_sync + image: git-hg-sync build: . command: ["--config", "config-docker.toml", "--log-raw-level", "debug"] volumes: From e81aae10affc675a2a4f30f132e72aef33d19942 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 27 Mar 2026 19:12:38 +1100 Subject: [PATCH 2/6] tests: improve pulse integration tests Add tests to exercise routing_key mismatches, and fix tests for successful delivery that were passing by luck. One key learning here is that each binding needs to be attached to a separate queue, otherwise any worker listening to a given queue may receive all messages matching the bindings. --- docker-compose.yaml | 3 ++ git_hg_sync/__main__.py | 2 +- rabbitmq/definitions.json | 2 +- tests/conftest.py | 36 ++++++++++------ tests/pulse_utils.py | 10 ++++- tests/test_integration.py | 89 +++++++++++++++++++++++++++++++-------- 6 files changed, 110 insertions(+), 32 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index 20aa18b..20affbc 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -53,6 +53,9 @@ services: - ./tests_output:/app/tests_output:z profiles: - test + depends_on: + pulse: + condition: service_healthy networks: - pulse_network diff --git a/git_hg_sync/__main__.py b/git_hg_sync/__main__.py index d3140af..cd598d9 100644 --- a/git_hg_sync/__main__.py +++ b/git_hg_sync/__main__.py @@ -36,7 +36,7 @@ def get_connection(config: PulseConfig) -> Connection: ) -def get_queue(config: Config | PulseConfig) -> Queue: +def get_queue(config: PulseConfig) -> Queue: exchange = Exchange(config.exchange, type="topic") return Queue( name=config.queue, diff --git a/rabbitmq/definitions.json b/rabbitmq/definitions.json index f92494b..0d2c6c0 100644 --- a/rabbitmq/definitions.json +++ b/rabbitmq/definitions.json @@ -4,7 +4,7 @@ "arguments": {}, "destination": "queue/guest/test", "destination_type": "queue", - "routing_key": "#", + "routing_key": "default", "source": "exchange/guest/test", "vhost": "/" } diff --git a/tests/conftest.py b/tests/conftest.py index 700a535..cf17cfc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -27,38 +27,50 @@ def pulse_config() -> PulseConfig: host="pulse", port=5672, exchange="exchange/guest/test", - routing_key="#", + routing_key="default", queue="queue/guest/test", password="guest", heartbeat=30, ssl=False, ) + @pytest.fixture def test_config(pulse_config: PulseConfig) -> Config: return Config( pulse=pulse_config, clones=ClonesConfig(directory=Path("clones")), tracked_repositories=[ - TrackedRepository(name="mozilla-central", url="https://github.com/mozilla-firefox/firefox.git"), + TrackedRepository( + name="mozilla-central", + url="https://github.com/mozilla-firefox/firefox.git", + ), + ], + branch_mappings=[ + BranchMapping( + branch_pattern=".*", + source_url="https://github.com/mozilla-firefox/firefox.git", + destination_url="destination_url", + destination_branch="destination_branch", + ) ], - branch_mappings=[BranchMapping( - branch_pattern = '.*', - source_url = "https://github.com/mozilla-firefox/firefox.git", - destination_url = 'destination_url', - destination_branch = 'destination_branch', - )], ) + @pytest.fixture def get_payload() -> Callable: - - def get_payload(**kwargs: dict) -> dict: + def get_payload( + request: pytest.FixtureRequest | None = None, **kwargs: dict + ) -> dict: """Return a default payload, with override via kwargs.""" + repo_url = "repo.git" + if request: + repo_url = request.node.name + payload = { "type": "push", - "repo_url": "repo.git", - "branches": { "main": 40 * "0"}, + "repo_url": repo_url, + "branches": {"main": 40 * "0"}, "tags": {}, "time": 0, "push_id": 0, diff --git a/tests/pulse_utils.py b/tests/pulse_utils.py index 250e868..4a9dff3 100644 --- a/tests/pulse_utils.py +++ b/tests/pulse_utils.py @@ -13,11 +13,14 @@ def send_pulse_message( pulse_config: PulseConfig, payload: Any, purge: bool = False -) -> None: +) -> tuple[kombu.Connection, kombu.Queue]: """Send a pulse message The routing key will be constructed from the repository URL. The Pulse message will be constructed from the specified payload and sent to the requested exchange. + + This function takes care of declaring and binding a Queue, so it can purge it before + the start of the test. It returns the Connectiond and Queue for use by the caller. """ userid = pulse_config.userid password = pulse_config.password @@ -40,6 +43,9 @@ def send_pulse_message( with connection: ex = kombu.Exchange(exchange, type="topic") + + # Declare the queue prior to sending, so we can purge it of potential spurious + # messages from previous tests. queue = kombu.Queue( name=queue_name, exchange=exchange, @@ -70,6 +76,8 @@ def send_pulse_message( print(f"publishing message to {exchange}") producer.publish(data) + return (connection, queue) + if __name__ == "__main__": config = Config.from_file(HERE.parent / "config.toml") diff --git a/tests/test_integration.py b/tests/test_integration.py index 446ed00..f09c9af 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -22,22 +22,73 @@ HERE = Path(__file__).parent -@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without rabbitMq") -def test_send_and_receive(pulse_config: PulseConfig, get_payload: Callable) -> None: - payload = get_payload() +@pytest.mark.parametrize( + "queue_key", + ( + (""), # Use the default from the config. + ("one"), + ("two.three"), + ), +) +@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without RabbitMQ") +def test_send_and_receive( + request: pytest.FixtureRequest, + pulse_config: PulseConfig, + get_payload: Callable, + queue_key: str, +) -> None: + payload = get_payload(request=request) + + if queue_key: + pulse_config.routing_key = queue_key + pulse_config.queue = f"{pulse_config.queue}-{queue_key}" def callback(body: Any, message: kombu.Message) -> None: message.ack() assert body["payload"] == payload - pulse_utils.send_pulse_message(pulse_config, payload, purge=True) - connection = get_connection(pulse_config) - queue = get_queue(pulse_config) + connection, queue = pulse_utils.send_pulse_message( + pulse_config, payload, purge=True + ) + with connection.Consumer(queue, auto_declare=False, callbacks=[callback]): connection.drain_events(timeout=5) -@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without rabbitMq") +@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without RabbitMQ") +@pytest.mark.parametrize( + "send_key,queue_key", + ( + ("three", "three.four"), + ("five.six", "five"), + ), +) +def test_send_and_receive_routing_key_mismatch( + pulse_config: PulseConfig, get_payload: Callable, send_key: str, queue_key: str +) -> None: + payload = get_payload() + + def callback(_body: Any, _message: kombu.Message) -> None: + raise AssertionError("No message should be received") + + pulse_config.routing_key = queue_key + # We need to create a unique queue for this binding. + pulse_config.queue = f"{pulse_config.queue}-{send_key}-{queue_key}" + + pulse_config_sender = pulse_config.model_copy() + pulse_config_sender.routing_key = send_key + + connection, queue = _setup_connection_and_queue(pulse_config) + pulse_utils.send_pulse_message(pulse_config_sender, payload, purge=True) + + with ( + connection.Consumer(queue, auto_declare=False, callbacks=[callback]), + pytest.raises(TimeoutError), + ): + connection.drain_events(timeout=5) + + +@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without RabbitMQ") def test_full_app( tmp_path: Path, get_payload: Callable, @@ -112,7 +163,7 @@ def test_full_app( assert hg_rev(hg_remote_repo_path, destination_branch) in tag_log -@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without rabbitMq") +@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without RabbitMQ") def test_no_duplicated_ack_messages( test_config: Config, get_payload: Callable, @@ -125,10 +176,7 @@ def test_no_duplicated_ack_messages( wait = 30 - connection = get_connection(test_config.pulse) - queue = get_queue(test_config.pulse) - queue(connection).queue_declare() - queue(connection).queue_bind() + connection, queue = _setup_connection_and_queue(test_config.pulse) worker = PulseWorker(connection, queue, one_shot=True) @@ -142,7 +190,7 @@ def test_no_duplicated_ack_messages( callback.assert_called_once() -@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without rabbitMq") +@pytest.mark.skipif(NO_RABBITMQ, reason="This test doesn't work without RabbitMQ") def test_messages_in_order( test_config: Config, get_payload: Callable, @@ -151,10 +199,7 @@ def test_messages_in_order( It may also timeout, which is likely indicative of the same issue. """ - connection = get_connection(test_config.pulse) - queue = get_queue(test_config.pulse) - queue(connection).queue_declare() - queue(connection).queue_bind() + connection, queue = _setup_connection_and_queue(test_config.pulse) worker = PulseWorker(connection, queue, one_shot=False) @@ -184,3 +229,13 @@ def event_handler(event: Event) -> None: worker.run() assert events_log == [0, 0, 1, 1] + + +def _setup_connection_and_queue( + pulse_config: PulseConfig, +) -> tuple[kombu.Connection, kombu.Queue]: + connection = get_connection(pulse_config) + queue = get_queue(pulse_config) + queue(connection).queue_declare() + queue(connection).queue_bind() + return connection, queue From 3a1bcc06e2f7d176cc47e363987b990b2bbd9ffe Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 27 Mar 2026 19:13:37 +1100 Subject: [PATCH 3/6] lint: format test_config_files with ruff --- tests/test_config_files.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/test_config_files.py b/tests/test_config_files.py index ebf3c90..f155a44 100644 --- a/tests/test_config_files.py +++ b/tests/test_config_files.py @@ -8,10 +8,7 @@ BASEDIR = Path(__file__).parent.parent -@pytest.mark.parametrize( - "config_file", - list(BASEDIR.glob("config-*.toml")) -) +@pytest.mark.parametrize("config_file", list(BASEDIR.glob("config-*.toml"))) def test_config_files(config_file: Path) -> None: try: config = Config.from_file(config_file) @@ -21,6 +18,8 @@ def test_config_files(config_file: Path) -> None: # We just do a shallow verification. What we really care is that the file could be # loaded correctly. assert config.pulse, f"`pulse` section missing in {config_file}" - assert config.tracked_repositories, f"`tracked_repositories` section missing in {config_file}" + assert config.tracked_repositories, ( + f"`tracked_repositories` section missing in {config_file}" + ) assert config.branch_mappings, f"`branch_mappings` section missing in {config_file}" assert config.tag_mappings, f"`tag_mappings` section missing in {config_file}" From 1b27c0754af55f4522d01ba0de87c77199c1093e Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 27 Mar 2026 19:16:40 +1100 Subject: [PATCH 4/6] Apply suggestion from @shtrom --- tests/pulse_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pulse_utils.py b/tests/pulse_utils.py index 4a9dff3..1779a67 100644 --- a/tests/pulse_utils.py +++ b/tests/pulse_utils.py @@ -20,7 +20,7 @@ def send_pulse_message( and sent to the requested exchange. This function takes care of declaring and binding a Queue, so it can purge it before - the start of the test. It returns the Connectiond and Queue for use by the caller. + the start of the test. It returns the Connection and Queue for use by the caller. """ userid = pulse_config.userid password = pulse_config.password From be28ad6134900a7dcbf86a66d6bbb476239e321c Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 30 Mar 2026 17:59:46 +1100 Subject: [PATCH 5/6] tests: skip suite configuration placeholder --- git_hg_sync/config.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git_hg_sync/config.py b/git_hg_sync/config.py index dfac6df..5d10a20 100644 --- a/git_hg_sync/config.py +++ b/git_hg_sync/config.py @@ -3,6 +3,7 @@ from typing import Self import pydantic +import pytest import tomllib from mozlog import get_proxy_logger @@ -84,6 +85,8 @@ def _update_config_from_env( @staticmethod def from_file(file_path: pathlib.Path) -> "Config": assert file_path.exists(), f"config file {file_path} doesn't exists" + if "config-suite.toml" in str(file_path): + pytest.skip("ignoring placeholder file created by suite") with file_path.open("rb") as config_file: config = tomllib.load(config_file) return Config(**config) From 09a81f991614af9bc2fc8bd2ff8072f46e402918 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 30 Mar 2026 18:00:16 +1100 Subject: [PATCH 6/6] build: add rudimentary Makefile --- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 Makefile diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..f9a56f0 --- /dev/null +++ b/Makefile @@ -0,0 +1,15 @@ +DOCKER := $(shell which docker) +DOCKER_COMPOSE := ${DOCKER} compose +ARGS_TESTS ?= + +build: + ${DOCKER_COMPOSE} build + +format: + ${DOCKER_COMPOSE} run --rm -it --entrypoint bash test -c 'ruff format /app && ruff check --fix /app' + +requirements: + ${DOCKER_COMPOSE} run --rm pip-compile + +test: + ${DOCKER_COMPOSE} run --rm test ${ARGS_TESTS}