tests: download kafka via pytest

- no need to keep the kafka version in multiple places (Makefile and conftest) - this is more natural for people used to python developement (running pytest directly will work) - the download now is controlled by pytest, makes it easier to change the download location for caching via CI - because the download is done using fixtures, and it may be slow, to avoid test failures due to timeout the pytest-timeout plugin is configured to only time the test itself
Aiven-Open · Apr 1, 2022 · 74ea4ac · 74ea4ac
1 parent 7549fcb
commit 74ea4ac
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 99 deletions.
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -21,6 +21,10 @@ jobs:
       - name: Install dependencies
         run: pip install -r requirements-dev.txt
 
+      # required for pylint
+      - name: Generate version.py
+        run: make karapace/version.py
+
       - name: Run all pre-commit hooks
         run: pre-commit run --all-files
 

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -22,9 +22,6 @@ jobs:
       with:
         python-version: ${{ matrix.python-version }}
 
-    - name: Fetch kafka
-      run: make fetch-kafka
-
     - name: Install dependencies
       run: python -m pip install -r requirements-dev.txt
 
@@ -33,8 +30,25 @@ jobs:
       with:
          version: '3.13.0'
 
+    # needed by both unit and integation tests
+    - name: Generate version.py
+      run: make karapace/version.py
+
     - name: Execute unit-tests
-      run: make unittest
+      run: python3 -m pytest -s -vvv tests/unit/
 
     - name: Execute integration-tests
-      run: make integrationtest
+      run: python3 -m pytest -s -vvv tests/integration/ --log-dir=/tmp/ci-logs --log-file=/tmp/ci-logs/pytest.log
+
+    - name: Archive logs
+      uses: actions/upload-artifact@v2
+      if: ${{ always() }}
+      with:
+          name: logs
+          # This directory is created by the fixture session_tmppath. The path is composed of a
+          # prefix defined by --basepath, followed by a folder name, in this case it is "karapace".
+          #
+          # See:
+          # - https://docs.pytest.org/en/6.2.x/tmpdir.html#base-temporary-directory
+          # - fixture session_tmppath in tests/integration/conftest.py
+          path: /tmp/pytest*
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -47,24 +47,20 @@ In Fedora® distributions you can install it using:
 dnf install protobuf-compiler
 ```
 
-To run the tests use `make`. It will download Kafka to be used in the tests for you:
+To run the tests use the binary `pytest` available in the virtualenv. It will download Kafka to be
+used in the tests for you:
 
 ```sh
-make unittest
-make integrationtest
+make karapace/version.py
+pytest tests/unit
+pytest tests/integration
 ```
 
-### PyCharm
-
-If you want to run the tests from within the IDE, first download Kafka using `make fetch-kafka`, and
-use the project root as the working directory.
-
 ## Static checking and Linting
 
 The code is statically checked and formatted using [a few
 tools](https://github.com/aiven/karapace/blob/master/requirements-dev.txt). To run these
 automatically on each commit please enable the [pre-commit](https://pre-commit.com) hooks.
-Alternatively you can run it manually with `make pre-commit`.
 
 ## Manual testing
 

diff --git a/Makefile b/Makefile
@@ -1,23 +1,6 @@
-SHORT_VER = $(shell git describe --tags --abbrev=0 | cut -f1-)
-LONG_VER = $(shell git describe --long 2>/dev/null || echo $(SHORT_VER)-0-unknown-g`git describe --always`)
-KAFKA_PATH = kafka_$(SCALA_VERSION)-$(KAFKA_VERSION)
-KAFKA_TAR = $(KAFKA_PATH).tgz
-PYTHON_SOURCE_DIRS = karapace/
-PYTHON_TEST_DIRS = tests/
-ALL_PYTHON_DIRS = $(PYTHON_SOURCE_DIRS) $(PYTHON_TEST_DIRS)
-GENERATED = karapace/version.py
 PYTHON = python3
-DNF_INSTALL = sudo dnf install -y
 
-# Keep these is sync with tests/integration/conftest.py
-KAFKA_VERSION=2.7.0
-SCALA_VERSION=2.13
-
-KAFKA_IMAGE = karapace-test-kafka
-ZK = 2181
-KAFKA = 9092
-
-default: $(GENERATED)
+default: karapace/version.py
 
 clean:
 	# remove all the versions of kafka
@@ -30,52 +13,9 @@ clean:
 	# delete generate files
 	rm karapace/version.py
 
-.PHONY: $(KAFKA_IMAGE)
-$(KAFKA_IMAGE):
-	podman build -t $(KAFKA_IMAGE) -f container/Dockerfile .
-
-.PHONY: start-$(KAFKA_IMAGE)
-start-$(KAFKA_IMAGE):
-	@podman run -d --rm -p $(ZK):$(ZK) -p $(KAFKA):$(KAFKA) -p $(REGISTRY):$(REGISTRY) -p $(REST):$(REST) $(KAFKA_IMAGE) "all"
-	@podman ps
-
 karapace/version.py: version.py
 	$(PYTHON) $^ $@
 
-$(KAFKA_TAR):
-	wget "https://archive.apache.org/dist/kafka/$(KAFKA_VERSION)/$(KAFKA_PATH).tgz"
-
-$(KAFKA_PATH): $(KAFKA_TAR)
-	tar zxf "$(KAFKA_TAR)"
-
-.PHONY: fetch-kafka
-fetch-kafka: $(KAFKA_PATH)
-
-.PHONY: start-kafka
-start-kafka: fetch-kafka
-	$(KAFKA_PATH)/bin/zookeeper-server-start.sh $(KAFKA_PATH)/config/zookeeper.properties &
-	$(KAFKA_PATH)/bin/kafka-server-start.sh $(KAFKA_PATH)/config/server.properties &
-
-.PHONY: stop-kafka
-stop-kafka:
-	$(KAFKA_PATH)/bin/kafka-server-stop.sh 9 || true
-	$(KAFKA_PATH)/bin/zookeeper-server-stop.sh 9 || true
-	rm -rf /tmp/kafka-logs /tmp/zookeeper
-
-.PHONY: kafka
-kafka: start-kafka
-
 .PHONY: copyright
 copyright:
 	grep -EL "Copyright \(c\) 20.* Aiven" $(shell git ls-files "*.py" | grep -v __init__.py)
-
-.PHONY: unittest
-unittest: $(GENERATED)
-	python3 -m pytest -s -vvv tests/unit/
-
-.PHONY: integrationtest
-integrationtest: fetch-kafka $(GENERATED)
-	python3 -m pytest -s -vvv tests/integration/
-
-.PHONY: test
-test: lint copyright unittest
diff --git a/pytest.ini b/pytest.ini
@@ -1,3 +1,4 @@
 [pytest]
 addopts = -ra -q --tb=short --showlocals --numprocesses auto
 timeout = 60
+timeout_func_only = true
diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py
@@ -29,18 +29,28 @@
 )
 from typing import AsyncIterator, Dict, Iterator, List, Optional, Tuple
 
+import logging
 import os
+import pathlib
 import pytest
+import requests
 import signal
 import socket
+import tarfile
 import time
 import ujson
 
-# Keep these in sync with the Makefile
-KAFKA_CURRENT_VERSION = "2.7"
-BASEDIR = "kafka_2.13-2.7.0"
+KAFKA_VERSION = "2.7.0"
+KAFKA_SCALA_VERSION = "2.13"
+KAFKA_FOLDER = f"kafka_{KAFKA_SCALA_VERSION}-{KAFKA_VERSION}"
+KAFKA_TGZ = f"{KAFKA_FOLDER}.tgz"
+KAFKA_URL = f"https://archive.apache.org/dist/kafka/{KAFKA_VERSION}/{KAFKA_TGZ}"
+KAFKA_PROTOCOL_VERSION = "2.7"
+
+REPOSITORY_DIR = pathlib.Path(__file__).parent.parent.parent
+RUNTIME_DIR = (REPOSITORY_DIR / "runtime").absolute()
+KAFKA_DIR = RUNTIME_DIR / KAFKA_FOLDER
 
-CLASSPATH = os.path.join(BASEDIR, "libs", "*")
 KAFKA_WAIT_TIMEOUT = 60
 
 
@@ -111,6 +121,7 @@ def wait_for_kafka(kafka_servers: KafkaServers, wait_time) -> None:
 
 def wait_for_port(
     port: int,
+    process: Popen,
     *,
     hostname: str = "127.0.0.1",
     wait_time: float = 20.0,
@@ -124,6 +135,7 @@ def wait_for_port(
             hostname=hostname,
             port=port,
         )
+        assert process.poll() is None, f"Process no longer running, exit_code: {process.returncode}"
         time.sleep(2.0)
 
     elapsed = expiration.elapsed
@@ -137,6 +149,16 @@ def lock_path_for(path: Path) -> Path:
     return path.with_suffix("".join(suffixes))
 
 
+def maybe_download_kafka() -> None:
+    """If necessary download kafka to run the tests."""
+    if not os.path.exists(KAFKA_DIR):
+        logging.info("Downloading Kafka {url}", url=KAFKA_URL)
+
+        download = requests.get(KAFKA_URL, stream=True)
+        with tarfile.open(mode="r:gz", fileobj=download.raw) as file:
+            file.extractall(str(RUNTIME_DIR))
+
+
 @pytest.fixture(scope="session", name="kafka_servers")
 def fixture_kafka_server(request, session_tmppath: Path) -> Iterator[KafkaServers]:
     bootstrap_servers = request.config.getoption("kafka_bootstrap_servers")
@@ -162,11 +184,13 @@ def fixture_kafka_server(request, session_tmppath: Path) -> Iterator[KafkaServer
                 zk_config = ZKConfig.from_dict(config_data["zookeeper"])
                 kafka_config = KafkaConfig.from_dict(config_data["kafka"])
             else:
+                maybe_download_kafka()
+
                 zk_config, zk_proc = configure_and_start_zk(zk_dir)
                 stack.callback(stop_process, zk_proc)
 
                 # Make sure zookeeper is running before trying to start Kafka
-                wait_for_port(zk_config.client_port, wait_time=20)
+                wait_for_port(zk_config.client_port, zk_proc, wait_time=20)
 
                 kafka_config, kafka_proc = configure_and_start_kafka(kafka_dir, zk_config)
                 stack.callback(stop_process, kafka_proc)
@@ -302,8 +326,8 @@ def fixture_registry_async_pair(tmp_path: Path, kafka_servers: KafkaServers):
         try:
             master_process = stack.enter_context(Popen(["python", "-m", "karapace.karapace_all", str(master_config_path)]))
             slave_process = stack.enter_context(Popen(["python", "-m", "karapace.karapace_all", str(slave_config_path)]))
-            wait_for_port(master_port)
-            wait_for_port(slave_port)
+            wait_for_port(master_port, master_process)
+            wait_for_port(slave_port, slave_process)
             yield f"http://127.0.0.1:{master_port}", f"http://127.0.0.1:{slave_port}"
         finally:
             if master_process:
@@ -488,35 +512,25 @@ async def get_client() -> ClientSession:
 
 
 def zk_java_args(cfg_path: Path) -> List[str]:
-    if not os.path.exists(BASEDIR):
-        raise RuntimeError(
-            f"Couldn't find kafka installation to run integration tests. The "
-            f"expected folder {BASEDIR} does not exist. Run `make fetch-kafka` "
-            f"to download and extract the release."
-        )
+    assert KAFKA_DIR.exists(), f"Couldn't find kafka installation at {KAFKA_DIR} to run integration tests."
     java_args = [
         "-cp",
-        CLASSPATH,
+        str(KAFKA_DIR / "libs" / "*"),
         "org.apache.zookeeper.server.quorum.QuorumPeerMain",
         str(cfg_path),
     ]
     return java_args
 
 
 def kafka_java_args(heap_mb, kafka_config_path, logs_dir, log4j_properties_path):
-    if not os.path.exists(BASEDIR):
-        raise RuntimeError(
-            f"Couldn't find kafka installation to run integration tests. The "
-            f"expected folder {BASEDIR} does not exist. Run `make fetch-kafka` "
-            f"to download and extract the release."
-        )
+    assert KAFKA_DIR.exists(), f"Couldn't find kafka installation at {KAFKA_DIR} to run integration tests."
     java_args = [
         "-Xmx{}M".format(heap_mb),
         "-Xms{}M".format(heap_mb),
         "-Dkafka.logs.dir={}/logs".format(logs_dir),
         "-Dlog4j.configuration=file:{}".format(log4j_properties_path),
         "-cp",
-        CLASSPATH,
+        str(KAFKA_DIR / "libs" / "*"),
         "kafka.Kafka",
         kafka_config_path,
     ]
@@ -578,11 +592,11 @@ def configure_and_start_kafka(kafka_dir: Path, zk: ZKConfig) -> Tuple[KafkaConfi
         "default.replication.factor": 1,
         "delete.topic.enable": "true",
         "inter.broker.listener.name": "PLAINTEXT",
-        "inter.broker.protocol.version": KAFKA_CURRENT_VERSION,
+        "inter.broker.protocol.version": KAFKA_PROTOCOL_VERSION,
         "listeners": listeners,
         "log.cleaner.enable": "true",
         "log.dirs": config.datadir,
-        "log.message.format.version": KAFKA_CURRENT_VERSION,
+        "log.message.format.version": KAFKA_PROTOCOL_VERSION,
         "log.retention.check.interval.ms": 300000,
         "log.segment.bytes": 200 * 1024 * 1024,  # 200 MiB
         "num.io.threads": 8,
@@ -605,7 +619,7 @@ def configure_and_start_kafka(kafka_dir: Path, zk: ZKConfig) -> Tuple[KafkaConfi
         for key, value in kafka_config.items():
             fp.write("{}={}\n".format(key, value))
 
-    log4j_properties_path = os.path.join(BASEDIR, "config/log4j.properties")
+    log4j_properties_path = str(KAFKA_DIR / "config" / "log4j.properties")
 
     kafka_cmd = get_java_process_configuration(
         java_args=kafka_java_args(