fix: missing logs on pytest failures #3255 (#3272)

* fix: missing logs on pytest failures #3255

.github/actions/regression-tests/action.yml

@@ -67,7 +67,7 @@ runs:
           mkdir /tmp/failed
           # Copy over the logs of the test that timedout. We need this because the exception/failure
           # handlers do not run when the shell command TIMEOUT sends a SIGTERM to terminate the pytest process.
-          cat /tmp/last_test_log_files.txt | xargs -I {} cp {} /tmp/failed/
+          cat /tmp/last_test_log_dir.txt | xargs -I {} cp -r {}/ /tmp/failed/
         fi
 
     - name: Send notification on failure

.github/workflows/ci.yml

@@ -179,9 +179,7 @@ jobs:
         uses: actions/upload-artifact@v4
         with:
           name: regression_logs
-          path: |
-            /tmp/**/*
-            /tmp/*
+          path: /tmp/failed/*
 
   lint-test-chart:
     runs-on: ubuntu-latest

.github/workflows/regression-tests.yml

@@ -53,7 +53,7 @@ jobs:
         uses: actions/upload-artifact@v4
         with:
           name: logs
-          path: /tmp/dragonfly.*
+          path: /tmp/failed/*
 
   lint-test-chart:
     runs-on: ubuntu-latest

tests/dragonfly/conftest.py

@@ -28,6 +28,37 @@
 logging.getLogger("asyncio").setLevel(logging.WARNING)
 
 DATABASE_INDEX = 0
+BASE_LOG_DIR = "/tmp/dragonfly_logs/"
+FAILED_PATH = "/tmp/failed/"
+
+
+# runs on pytest start
+def pytest_configure(config):
+    # clean everything
+    if os.path.exists(FAILED_PATH):
+        shutil.rmtree(FAILED_PATH)
+    if os.path.exists(BASE_LOG_DIR):
+        shutil.rmtree(BASE_LOG_DIR)
+
+
+# runs per test case
+def pytest_runtest_setup(item):
+    # Generate a unique directory name for each test based on its nodeid
+    translator = str.maketrans(":[]{}/ ", "_______", "\"*'")
+    unique_dir = item.name.translate(translator)
+
+    test_dir = os.path.join(BASE_LOG_DIR, unique_dir)
+    if os.path.exists(test_dir):
+        shutil.rmtree(test_dir)
+    os.makedirs(test_dir)
+
+    # Attach the directory path to the item for later access
+    item.log_dir = test_dir
+
+    # needs for action.yml to get logs if timedout is happen for test
+    last_logs = open("/tmp/last_test_log_dir.txt", "w")
+    last_logs.write(test_dir)
+    last_logs.close()
 
 
 @pytest.fixture(scope="session")
@@ -89,6 +120,8 @@ def df_factory(request, tmp_dir, test_env) -> DflyInstanceFactory:
     scripts_dir = os.path.dirname(os.path.abspath(__file__))
     path = os.environ.get("DRAGONFLY_PATH", os.path.join(scripts_dir, "../../build-dbg/dragonfly"))
 
+    log_directory = getattr(request.node, "log_dir")
+
     args = request.param if request.param else {}
     existing = request.config.getoption("--existing-port")
     existing_admin = request.config.getoption("--existing-admin-port")
@@ -103,6 +136,7 @@ def df_factory(request, tmp_dir, test_env) -> DflyInstanceFactory:
         existing_admin_port=int(existing_admin) if existing_admin else None,
         existing_mc_port=int(existing_mc) if existing_mc else None,
         env=test_env,
+        log_dir=log_directory,
     )
 
     factory = DflyInstanceFactory(params, args)
@@ -330,39 +364,38 @@ def with_ca_tls_client_args(with_tls_client_args, with_tls_ca_cert_args):
     return args
 
 
-def copy_failed_logs_and_clean_tmp_folder(report):
-    return  # TODO: to fix it first and then enable it.
-    failed_path = "/tmp/failed"
-    path_exists = os.path.exists(failed_path)
-    if not path_exists:
-        os.makedirs(failed_path)
+def copy_failed_logs(log_dir, report):
+    test_failed_path = os.path.join(FAILED_PATH, os.path.basename(log_dir))
+    if not os.path.exists(test_failed_path):
+        os.makedirs(test_failed_path)
 
-    if os.path.isfile("/tmp/last_test_log_files.txt"):
-        last_log_file = open("/tmp/last_test_log_files.txt", "r")
-        files = last_log_file.readlines()
-        logging.error(f"Test failed {report.nodeid} with logs: ")
-        for file in files:
-            # copy to failed folder
+    logging.error(f"Test failed {report.nodeid} with logs: ")
+
+    for f in os.listdir(log_dir):
+        file = os.path.join(log_dir, f)
+        if os.path.isfile(file):
             file = file.rstrip("\n")
             logging.error(f"🪵🪵🪵🪵🪵🪵 {file} 🪵🪵🪵🪵🪵🪵")
-            shutil.copy(file, failed_path)
-
-
-def pytest_exception_interact(node, call, report):
-    if report.failed:
-        copy_failed_logs_and_clean_tmp_folder(report)
-
-
-@pytest.fixture(autouse=True)
-def run_before_and_after_test():
-    # Setup: logic before any of the test starts
-    # Empty the log on each run
-    last_log_file = open("/tmp/last_test_log_files.txt", "w")
-    last_log_file.close()
-
-    yield  # this is where the testing happens
-
-    # Teardown
+            shutil.copy(file, test_failed_path)
+
+
+# tests results we get on the "call" state
+# but we can not copy logs until "teardown" state because the server isn't stoped
+# so we save result of the "call" state and process it on the "teardown" when the server is stoped
+@pytest.hookimpl(hookwrapper=True)
+def pytest_runtest_makereport(item, call):
+    outcome = yield
+    report = outcome.get_result()
+
+    if report.when == "call":
+        # Store the result of the call phase in the item
+        item.call_outcome = report
+
+    if report.when == "teardown":
+        log_dir = getattr(item, "log_dir", None)
+        call_outcome = getattr(item, "call_outcome", None)
+        if call_outcome and call_outcome.failed:
+            copy_failed_logs(log_dir, call_outcome)
 
 
 @pytest.fixture(scope="function")

tests/dragonfly/instance.py

@@ -29,6 +29,7 @@ class DflyParams:
     existing_admin_port: int
     existing_mc_port: int
     env: any
+    log_dir: str
 
 
 class Colors:
@@ -143,13 +144,6 @@ def _wait_for_server(self):
 
         self.log_files = self.get_logs_from_psutil()
 
-        last_log_file = open("/tmp/last_test_log_files.txt", "a")
-
-        for log in self.log_files:
-            last_log_file.write(log + "\n")
-
-        last_log_file.close()
-
         # Remove first 6 lines - our default header with log locations (as it carries no useful information)
         # Next, replace log-level + date with port and colored arrow
         sed_format = f"1,6d;s/[^ ]*/{self.port}{Colors.next()}➜{Colors.CLEAR}/"
@@ -339,6 +333,7 @@ def create(self, existing_port=None, **kwargs) -> DflyInstance:
         vmod = "dragonfly_connection=1,accept_server=1,listener_interface=1,main_service=1,rdb_save=1,replica=1,cluster_family=1,dflycmd=1"
         args.setdefault("vmodule", vmod)
         args.setdefault("jsonpathv2")
+        args.setdefault("log_dir", self.params.log_dir)
 
         for k, v in args.items():
             args[k] = v.format(**self.params.env) if isinstance(v, str) else v

tests/dragonfly/search_test.py

@@ -270,7 +270,6 @@ async def test_knn(async_client: aioredis.Redis, index_type, algo_type):
     }
 
     assert await knn_query(i2, "@even:{yes} => [KNN 3 @pos $vec]", [10.0] == {"k8", "k10", "k12"})
-
     await i2.dropindex()