Merge pull request #9670 from LabNConsulting/chopps/fix-valgrind-fail-check

Chopps/fix valgrind fail check

Author: donaldsharp

tests/topotests/conftest.py

@@ -21,13 +21,6 @@
 from lib.topotest import g_extra_config as topotest_extra_config
 from lib.topotest import json_cmp_result
 
-try:
-    from _pytest._code.code import ExceptionInfo
-
-    leak_check_ok = True
-except ImportError:
-    leak_check_ok = False
-
 
 def pytest_addoption(parser):
     """
@@ -138,8 +131,7 @@ def pytest_addoption(parser):
 
 
 def check_for_memleaks():
-    if not topotest_extra_config["valgrind_memleaks"]:
-        return
+    assert topotest_extra_config["valgrind_memleaks"]
 
     leaks = []
     tgen = get_topogen()
@@ -151,21 +143,25 @@ def check_for_memleaks():
             existing = tgen.valgrind_existing_files
         latest = glob.glob(os.path.join(logdir, "*.valgrind.*"))
 
+    daemons = set()
     for vfile in latest:
         if vfile in existing:
             continue
-        with open(vfile) as vf:
+        existing.append(vfile)
+        with open(vfile, encoding="ascii") as vf:
             vfcontent = vf.read()
             match = re.search(r"ERROR SUMMARY: (\d+) errors", vfcontent)
             if match and match.group(1) != "0":
                 emsg = "{} in {}".format(match.group(1), vfile)
                 leaks.append(emsg)
+                daemons.add(re.match(r".*\.valgrind\.(.*)\.\d+", vfile).group(1))
+
+    if tgen is not None:
+        tgen.valgrind_existing_files = existing
 
     if leaks:
-        if leak_check_ok:
-            pytest.fail("Memleaks found:\n\t" + "\n\t".join(leaks))
-        else:
-            logger.error("Memleaks found:\n\t" + "\n\t".join(leaks))
+        logger.error("valgrind memleaks found:\n\t%s", "\n\t".join(leaks))
+        pytest.fail("valgrind memleaks found for daemons: " + " ".join(daemons))
 
 
 def pytest_runtest_logstart(nodeid, location):
@@ -178,18 +174,21 @@ def pytest_runtest_logfinish(nodeid, location):
     topolog.logfinish(nodeid, location)
 
 
-def pytest_runtest_call():
-    """
-    This function must be run after setup_module(), it does standarized post
-    setup routines. It is only being used for the 'topology-only' option.
-    """
+@pytest.hookimpl(hookwrapper=True)
+def pytest_runtest_call(item: pytest.Item) -> None:
+    "Hook the function that is called to execute the test."
+
+    # For topology only run the CLI then exit
     if topotest_extra_config["topology_only"]:
-        tgen = get_topogen()
-        if tgen is not None:
-            # Allow user to play with the setup.
-            tgen.cli()
+        get_topogen().cli()
+        pytest.exit("exiting after --topology-only")
+
+    # Let the default pytest_runtest_call execute the test function
+    yield
 
-        pytest.exit("the topology executed successfully")
+    # Check for leaks if requested
+    if topotest_extra_config["valgrind_memleaks"]:
+        check_for_memleaks()
 
 
 def pytest_assertrepr_compare(op, left, right):
@@ -333,7 +332,10 @@ def assert_feature_windows(b, feature):
     topotest_extra_config["pause"] = pause
     assert_feature_windows(pause, "--pause")
 
-    topotest_extra_config["topology_only"] = config.getoption("--topology-only")
+    topology_only = config.getoption("--topology-only")
+    if topology_only and is_xdist:
+        pytest.exit("Cannot use --topology-only with distributed test mode")
+    topotest_extra_config["topology_only"] = topology_only
 
     # Check environment now that we have config
     if not diagnose_env(rundir):
@@ -373,12 +375,6 @@ def pytest_runtest_makereport(item, call):
     else:
         pause = False
 
-    if call.excinfo is None and call.when == "call":
-        try:
-            check_for_memleaks()
-        except:
-            call.excinfo = ExceptionInfo()
-
     title = "unset"
 
     if call.excinfo is None:

tests/topotests/lib/topotest.py

@@ -1725,7 +1725,7 @@ def start_daemon(daemon, extra_opts=None):
                     )
                     if valgrind_extra:
                         cmdenv += (
-                            "--gen-suppressions=all --expensive-definedness-checks=yes"
+                            " --gen-suppressions=all --expensive-definedness-checks=yes"
                         )
                 elif daemon in strace_daemons or "all" in strace_daemons:
                     cmdenv = "strace -f -D -o {1}/{2}.strace.{0} ".format(

tests/topotests/pytest.ini

@@ -81,4 +81,3 @@ markers =
 # memleak_path = /tmp/memleak_
 # Output files will be named after the testname:
 # /tmp/memleak_test_ospf_topo1.txt
-memleak_path = /tmp/memleak_