revert changes to helpers, documented refactor request instead

Signed-off-by: Ganyu (Bruce) Xu <[email protected]>

Author: xuganyu96

tests/helpers.py

@@ -142,29 +142,31 @@ def is_sig_stfl_enabled_by_name(name):
                     return True
     return False
 
+# TODO: this function needs a refactor because the scan logic currently applies
+# re.findall to all args/kwargs in the wrapped function. If the function takes
+# a non-string argument, re.findall will raise a TypeError, which is
+# undesirable. It can also cause incorrect skipping if the wrapped function
+# takes some string argument that unintentionally matches the regex.
+# Instead, I prefer replacing this decorator with a regular function that returns
+# a boolean indicating whether a KEM/SIG/STFL_SIG name matches the env var
+# SKIP_ALGS, then let the caller decide whether to call pytest.skip.
 def filtered_test(func):
-    funcname = func.__name__[len("test_") :]
+    funcname = func.__name__[len("test_"):]
 
     @functools.wraps(func)
     def wrapper(*args, **kwargs):
-        if ("SKIP_ALGS" in os.environ) and len(os.environ["SKIP_ALGS"]) > 0:
-            for algexp in os.environ["SKIP_ALGS"].split(","):
+        if ('SKIP_ALGS' in os.environ) and len(os.environ['SKIP_ALGS'])>0:
+            for algexp in os.environ['SKIP_ALGS'].split(','):
                 for arg in args:
-                    if isinstance(arg, str) and len(re.findall(algexp, arg)) > 0:
+                    if len(re.findall(algexp, arg))>0:
                         pytest.skip("Test disabled by alg filter")
                 for arg in kwargs:
-                    if (
-                        isinstance(kwargs[arg], str)
-                        and len(re.findall(algexp, kwargs[arg])) > 0
-                    ):
+                    if len(re.findall(algexp, kwargs[arg]))>0:
                         pytest.skip("Test disabled by alg filter")
-        if ("SKIP_TESTS" in os.environ) and (
-            funcname in os.environ["SKIP_TESTS"].lower().split(",")
-        ):
+        if ('SKIP_TESTS' in os.environ) and (funcname in os.environ['SKIP_TESTS'].lower().split(',')):
             pytest.skip("Test disabled by filter")
         else:
             return func(*args, **kwargs)
-
     return wrapper
 
 # So far, build dir name has been hard coded to "build".

tests/test_acvp_vectors.py

@@ -38,7 +38,10 @@
 @helpers.filtered_test
 @pytest.mark.parametrize("kem_name", helpers.available_kems_by_name())
 def test_acvp_vec_kem_keygen(kem_name):
-    # TODO: this logic feels awkward
+    # TODO: this logic feels backwards. It will instantiate kem_name with all
+    # possible names, then filter out the overwhelming majority of them that are
+    # "not ML-KEM". This creates a lot of skipped tests that, despite not having
+    # much performance penalty, pollute the pytest output.
     if not (helpers.is_kem_enabled_by_name(kem_name)):
         pytest.skip("Not enabled")
     if not (kem_name in ml_kem):