Repository: jwodder/pytest-fail-slow (language: python)

Issue:
Opt-in to failing slow tests when `pytest-fail-slow` is installed?
Thanks for writing `pytest-fail-slow`! We're trying it out on two of SciPy's CI jobs to identify very slow in PRs before they are merged.

Initially, I thought that slow tests would only fail if `fail_slow` were provided as a command line argument. However, now that I've added some exceptions via the `@pytest.mark.fail_slow` mark, marked tests are _always_ subject to the specified time limit if `pytest-fail-slow` is installed. Because of variability in test times on different platforms, this is undesirable; we really only want to use `pytest-fail-slow` on certain CI jobs or when a developer manually opts in - whether `pytest-fail-slow` is installed or not. 

I tried using `pytest_configure` in `conftest.py` to replace the `fail_slow` mark with a dummy unless a command line argument is present, but I couldn't get that to work - the mark is not even present in `config.getini('markers')`, so I can't really replace it. 

Do I need to learn more about how to configure `pytest`, or can you suggest another way to use `pytest-fail-slow` only if a command line option is present? Would this be a feature you'd be willing to support/document?


Discussion / hints:
> Initially, I thought that slow tests would only fail if `fail_slow` were provided as a command line argument.

Yes, as you've discovered, this is not the case.  The effect of the `--fail-slow` option is essentially to add `@pytest.mark.fail_slow(...)` to every test that doesn't already have it, and the marker always does its thing when the plugin is installed.  Can you suggest a way the documentation could have made this clearer?

> Would this be a feature you'd be willing to support/document?

How about this: I add an `enabled: bool = True` keyword argument to the `pytest-fail-slow` markers; when `enabled` is set to `False` on a marker, the marker will do nothing for the current test run.  Would that meet your needs?

EDIT: Though now I'm wondering what the best behavior would be if a marker has `enabled=False` and `--fail-slow[-setup]` is given on the command line; which one should take precedence?
> Can you suggest a way the documentation could have made this clearer?

It is not surprising in retrospect; it's just not what I had in mind. But if there would be additional documentation for the new opt-in feature we're discussing, that would be an easy place to make the default behavior very explicit.

> Would that meet your needs?

I'm not sure I understand how I'd set the marker `enabled=False` - where would that go? 

In case it helps, the solution i had in mind was an option in some test configuration file that we could set to not use `pytest-fail-slow`, even if installed, unless a keyword argument is found when running pytest. For instance, we have it so that tests marked with `pytest.mark.slow` don't run unless `--slow` is passed as an argument to `pytest`.

BTW, right now we register a placeholder mark for `fail_slow` if `pytest-fail-slow` is not installed so that we don't get failures due to an unrecognized mark. (Just thought I'd make that explicit, in case it helps.)
> I'm not sure I understand how I'd set the marker `enabled=False` - where would that go?

It's not a marker; it's an argument to the `fail_slow` and `fail_slow_setup` markers.  You'd use it like this:

```python
@pytest.mark.fail_slow(5, enabled=some_condition())
def test_thingy():
    ...
```

where `some_condition()` is a boolean expression, just like you'd pass to a `skipif` or `xfail` marker.  (Note that I'm not planning to support [condition strings](https://docs.pytest.org/en/8.2.x/historical-notes.html#conditions-as-strings-instead-of-booleans)) at this time.)  If you want `fail_slow` markers to only take effect if `--slow` is given on the command line, you should be able to just do:

```python
@pytest.mark.fail_slow(5, enabled=pytest.config.getvalue("slow"))
def test_thingy():
    ...
```

EDIT: I've just realized that, despite what the documentation I linked above says, `pytest.config` no longer exists.  Looks like I'll have to add support for condition strings, which means you'd actually write:

```python
@pytest.mark.fail_slow(5, enabled="config.getvalue('slow')")
def test_thingy():
    ...
```
I see. I'd rather not *have* to specify the condition as an argument of every marker,  but that would work, and it would be a more general solution than what I had in mind. 

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 08e08a3..b605ad6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,7 @@
+v0.6.0 (in development)
+-----------------------
+- Add `enabled` keyword argument to markers
+
 v0.5.0 (2024-02-11)
 -------------------
 - Migrated from setuptools to hatch
diff --git a/README.rst b/README.rst
index 6b97cae..db67a51 100644
--- a/README.rst
+++ b/README.rst
@@ -66,17 +66,51 @@ To cause a specific test to fail if it takes too long to run, apply the
     def test_something_sluggish():
         ...
 
-In addition, the ``--fail-slow DURATION`` option can be passed to the
-``pytest`` command to affect all tests in that run.  If ``--fail-slow`` is
-given and a test has the ``fail_slow`` marker, the duration given by the marker
-takes precedence for that test.
-
 If a test fails due to being slow, pytest's output will include the test's
 duration and the duration threshold, like so::
 
     ________________________________ test_func ________________________________
     Test passed but took too long to run: Duration 123.0s > 5.0s
 
+(*New in version 0.6.0*) If you only want a given test to fail for being slow
+under certain conditions — say, when running under CI or on a certain platform
+— supply the ``enabled`` keyword argument to the marker.  The value of
+``enabled`` can be either a boolean expression or a `condition string`_.  When
+the ``enabled`` value evaluates to ``True`` (the default), the test will fail
+if its runtime exceeds the given duration; if it evaluates to ``False``, a
+lengthy runtime will not cause the test to fail.  Example usage:
+
+.. code:: python
+
+    import os
+    import pytest
+
+    @pytest.mark.fail_slow("5s", enabled="CI" in os.environ)
+    def test_something_that_needs_to_be_fast_in_ci():
+        ...
+
+An an alternative or in addition to the marker, the ``--fail-slow DURATION``
+option can be passed to the ``pytest`` command to, in essence, apply the
+``fail_slow`` marker with the given cutoff to all tests that don't already have
+the marker.  (As far as ``pytest`` is concerned, the option does not actually
+cause any markers to be added to any tests, in case your code cares about
+that.)  If a test already has the ``fail_slow`` marker, the ``--fail-slow``
+option will have no effect on it.
+
+If you want a test to fail for being slow only if the ``--fail-slow`` option is
+given, but you also want a different cutoff for the test than that passed to
+the option, you can give the test a ``fail_slow`` marker that sets the desired
+cutoff and also sets ``enabled`` to a condition string that checks whether
+``--fail-slow`` has been given, like so:
+
+.. code:: python
+
+    import pytest
+
+    @pytest.mark.fail_slow(3, enabled="config.getoption('--fail-slow') is not None")
+    def test_something_sometimes_sluggish():
+        ...
+
 **Note:** This feature only takes the durations for tests themselves into
 consideration.  If a test passes in less than the specified duration, but one
 or more fixture setups/teardowns take longer than the duration, the test will
@@ -104,11 +138,6 @@ the desired cutoff time as the argument:
 Do not apply the marker to the test's fixtures; markers have no effect on
 fixtures.
 
-In addition, the ``--fail-slow-setup DURATION`` option can be passed to the
-``pytest`` command to affect all tests in that run.  If ``--fail-slow-setup``
-is given and a test has the ``fail_slow_setup`` marker, the duration given by
-the marker takes precedence for that test.
-
 If the setup for a test takes too long to run, the test will be marked as
 "errored," the test itself will not be run, and pytest's output will include
 the setup stage's duration and the duration threshold, like so::
@@ -116,6 +145,12 @@ the setup stage's duration and the duration threshold, like so::
     _______________________ ERROR at setup of test_func _______________________
     Setup passed but took too long to run: Duration 123.0s > 5.0s
 
+Like ``fail_slow``, the ``fail_slow_setup`` marker takes an optional
+``enabled`` keyword argument that can be used to conditionally enable or
+disable failure for slow setups.  There is also a ``--fail-slow-setup
+DURATION`` option that can be passed to ``pytest`` to, in essence, apply the
+marker to all tests that don't already have it.
+
 **Note:** If a test depends on multiple fixtures and just one of them exceeds
 the given duration on its own, the remaining fixtures will still have their
 setup steps run.  Also, all fixture teardowns will still be run after the test
@@ -134,3 +169,6 @@ following units (case insensitive):
 - ``s``, ``sec``, ``secs``, ``second``, ``seconds``
 - ``ms``, ``milli``, ``millisec``, ``milliseconds``
 - ``us``, ``μs``, ``micro``, ``microsec``, ``microseconds``
+
+.. _condition string: https://docs.pytest.org/en/8.2.x/historical-notes.html
+                      #conditions-as-strings-instead-of-booleans
diff --git a/tox.ini b/tox.ini
index 5d0c458..9147d67 100644
--- a/tox.ini
+++ b/tox.ini
@@ -65,6 +65,9 @@ unused-arguments-ignore-stub-functions = True
 extend-select = B901,B902,B950
 ignore = A003,A005,B005,E203,E262,E266,E501,E704,U101,W503
 
+per-file-ignores =
+    test/*.py:B950
+
 [isort]
 atomic = True
 force_sort_within_sections = True


Test patch:
diff --git a/src/pytest_fail_slow/__init__.py b/src/pytest_fail_slow/__init__.py
index a4d65e4..b913a46 100644
--- a/src/pytest_fail_slow/__init__.py
+++ b/src/pytest_fail_slow/__init__.py
@@ -16,11 +16,16 @@
 """
 
 from __future__ import annotations
+from collections.abc import Generator, Mapping
+import os
+import platform
 import re
-from typing import Generator, Union
+import sys
+import traceback
+from typing import Union
 import pytest
 
-__version__ = "0.5.0"
+__version__ = "0.6.0.dev1"
 __author__ = "John Thorvald Wodder II"
 __author_email__ = "pytest-fail-slow@varonathe.org"
 __license__ = "MIT"
@@ -94,27 +99,74 @@ def pytest_addoption(parser: pytest.Parser) -> None:
 
 
 def pytest_runtest_setup(item: pytest.Item) -> None:
-    setup_mark = item.get_closest_marker("fail_slow_setup")
-    if setup_mark is not None:
-        if len(setup_mark.args) != 1:
-            raise pytest.UsageError(
-                "@pytest.mark.fail_slow_setup() takes exactly one argument"
-            )
-        setup_timeout = parse_duration(setup_mark.args[0])
-    else:
-        setup_timeout = item.config.getoption("--fail-slow-setup")
-    item.stash[setup_timeout_key] = setup_timeout
-
-    call_mark = item.get_closest_marker("fail_slow")
-    if call_mark is not None:
-        if len(call_mark.args) != 1:
-            raise pytest.UsageError(
-                "@pytest.mark.fail_slow() takes exactly one argument"
+    item.stash[setup_timeout_key] = get_fail_slow_timeout(
+        item, "fail_slow_setup", "--fail-slow-setup"
+    )
+    item.stash[call_timeout_key] = get_fail_slow_timeout(
+        item, "fail_slow", "--fail-slow"
+    )
+
+
+def get_fail_slow_timeout(
+    item: pytest.Item, mark_name: str, option_name: str
+) -> int | float | None:
+    m = item.get_closest_marker(mark_name)
+    if m is None:
+        timeout = item.config.getoption(option_name)
+        assert isinstance(timeout, (int, float)) or timeout is None
+        return timeout
+    try:
+        (duration,) = m.args
+    except ValueError:
+        raise pytest.UsageError(
+            f"@pytest.mark.{mark_name}() takes exactly one positional argument"
+        )
+    enabled = m.kwargs.get("enabled", True)
+    if isinstance(enabled, str):
+        enabled = evaluate_enabled(item, mark_name, enabled)
+    if not enabled:
+        return None
+    return parse_duration(duration)
+
+
+def evaluate_enabled(item: pytest.Item, mark_name: str, condition: str) -> bool:
+    # Based on evaluate_condition() in _pytest/skipping.py
+    ctx = {
+        "os": os,
+        "sys": sys,
+        "platform": platform,
+        "config": item.config,
+    }
+    for dictionary in reversed(
+        item.ihook.pytest_markeval_namespace(config=item.config)
+    ):
+        if not isinstance(dictionary, Mapping):  # pragma: no cover
+            raise ValueError(
+                "pytest_markeval_namespace() needs to return a dict, got"
+                f" {dictionary!r}"
             )
-        call_timeout = parse_duration(call_mark.args[0])
-    else:
-        call_timeout = item.config.getoption("--fail-slow")
-    item.stash[call_timeout_key] = call_timeout
+        ctx.update(dictionary)
+    if hasattr(item, "obj"):
+        ctx.update(item.obj.__globals__)
+    try:
+        filename = f"<{mark_name} enabled>"
+        code = compile(condition, filename, "eval")
+        return bool(eval(code, ctx))
+    except SyntaxError as exc:
+        msglines = [
+            f"Error evaluating {mark_name!r} condition",
+            "    " + condition,
+            "    " + " " * (exc.offset or 0) + "^",
+            "SyntaxError: invalid syntax",
+        ]
+        pytest.fail("\n".join(msglines), pytrace=False)
+    except Exception as exc:
+        msglines = [
+            f"Error evaluating {mark_name!r} condition",
+            "    " + condition,
+            *traceback.format_exception_only(type(exc), exc),
+        ]
+        pytest.fail("\n".join(msglines), pytrace=False)
 
 
 @pytest.hookimpl(wrapper=True)
diff --git a/test/test_fail_slow.py b/test/test_fail_slow.py
index 535351f..da71aae 100644
--- a/test/test_fail_slow.py
+++ b/test/test_fail_slow.py
@@ -47,10 +47,22 @@
 
 
 @pytest.mark.parametrize("src,success,slow", CASES)
+@pytest.mark.parametrize(
+    "decor",
+    [
+        "",
+        "import pytest\n@pytest.mark.fail_slow(2, enabled=False)\n",
+        "import pytest\n@pytest.mark.fail_slow(2, enabled='1 == 2')\n",
+    ],
+)
 def test_fail_slow_no_threshold(
-    pytester: pytest.Pytester, src: str, success: bool, slow: bool  # noqa: U100
+    pytester: pytest.Pytester,
+    src: str,
+    success: bool,
+    slow: bool,  # noqa: U100
+    decor: str,
 ) -> None:
-    pytester.makepyfile(test_func=src.format(decor=""))
+    pytester.makepyfile(test_func=src.format(decor=decor))
     result = pytester.runpytest()
     if success:
         result.assert_outcomes(passed=1)
@@ -69,6 +81,8 @@ def test_fail_slow_no_threshold(
         ([], "import pytest\n@pytest.mark.fail_slow(2)\n", r"2s"),
         ([], "import pytest\n@pytest.mark.fail_slow(2.0)\n", r"2\.\d+s"),
         ([], "import pytest\n@pytest.mark.fail_slow('0.0333m')\n", r"1\.9\d+s"),
+        ([], "import pytest\n@pytest.mark.fail_slow(2, enabled=True)\n", r"2s"),
+        ([], "import pytest\n@pytest.mark.fail_slow(2, enabled='1 == 1')\n", r"2s"),
         (["--fail-slow=30"], "import pytest\n@pytest.mark.fail_slow(2)\n", r"2s"),
         (["--fail-slow=0.01"], "import pytest\n@pytest.mark.fail_slow(2)\n", r"2s"),
     ],
@@ -101,7 +115,9 @@ def test_fail_slow_threshold(
         result.stdout.no_fnmatch_line("*Test passed but took too long to run*")
 
 
-@pytest.mark.parametrize("args", ["", "42, 'foo'"])
+@pytest.mark.parametrize(
+    "args", ["", "42, 'foo'", "enabled=False", "42, 'foo', enabled=False"]
+)
 def test_fail_slow_marker_bad_args(pytester: pytest.Pytester, args: str) -> None:
     pytester.makepyfile(
         test_func=(
@@ -115,5 +131,115 @@ def test_fail_slow_marker_bad_args(pytester: pytest.Pytester, args: str) -> None
     result = pytester.runpytest()
     result.assert_outcomes(errors=1)
     result.stdout.fnmatch_lines(
-        ["*UsageError: @pytest.mark.fail_slow() takes exactly one argument"]
+        ["*UsageError: @pytest.mark.fail_slow() takes exactly one positional argument"]
+    )
+
+
+def test_fail_slow_disabled_plus_option(pytester: pytest.Pytester) -> None:
+    pytester.makepyfile(
+        test_func=(
+            "from time import sleep\n"
+            "import pytest\n"
+            "\n"
+            "@pytest.mark.fail_slow(2, enabled=False)\n"
+            "def test_func():\n"
+            "    sleep(5)\n"
+            "    assert 2 + 2 == 4\n"
+        )
+    )
+    result = pytester.runpytest("--fail-slow=2")
+    result.assert_outcomes(passed=1)
+    result.stdout.no_fnmatch_line("*Test passed but took too long to run*")
+
+
+@pytest.mark.parametrize(
+    "args,limitrgx",
+    [
+        ([], None),
+        (["--fail-slow=0"], "2s"),
+        (["--fail-slow=3"], "2s"),
+    ],
+)
+def test_fail_slow_enabled_iff_option(
+    pytester: pytest.Pytester, args: list[str], limitrgx: str | None
+) -> None:
+    pytester.makepyfile(
+        test_func=(
+            "from time import sleep\n"
+            "import pytest\n"
+            "\n"
+            "@pytest.mark.fail_slow(2, enabled='config.getoption(\"--fail-slow\") is not None')\n"
+            "def test_func():\n"
+            "    sleep(5)\n"
+            "    assert 2 + 2 == 4\n"
+        )
+    )
+    result = pytester.runpytest(*args)
+    if limitrgx is None:
+        result.assert_outcomes(passed=1)
+        result.stdout.no_fnmatch_line("*Test passed but took too long to run*")
+    else:
+        result.assert_outcomes(failed=1)
+        result.stdout.re_match_lines(
+            [
+                r"_+ test_func _+$",
+                "Test passed but took too long to run:"
+                rf" Duration \d+\.\d+s > {limitrgx}$",
+            ],
+            consecutive=True,
+        )
+
+
+def test_fail_slow_bad_condition_syntax(pytester: pytest.Pytester) -> None:
+    pytester.makepyfile(
+        test_func=(
+            "import pytest\n"
+            "\n"
+            "@pytest.mark.fail_slow(2, enabled='bad syntax')\n"
+            "def test_func():\n"
+            '    Path("test.txt").write_text("Tested\\n")\n'
+        )
+    )
+    result = pytester.runpytest("--fail-slow=2")
+    result.assert_outcomes(errors=1)
+    result.stdout.re_match_lines(
+        [
+            r"_+ ERROR at setup of test_func _+$",
+            r"(invalid syntax(?:\. Perhaps you forgot a comma\?)?|unexpected EOF while parsing) \(<fail_slow enabled>, line 1\)",
+            "",
+            "During handling of the above exception, another exception occurred:",
+            "Error evaluating 'fail_slow' condition",
+            "    bad syntax",
+            r" +\^",
+            "SyntaxError: invalid syntax",
+        ],
+        consecutive=True,
+    )
+    assert not (pytester.path / "test.txt").exists()
+
+
+def test_fail_slow_bad_condition(pytester: pytest.Pytester) -> None:
+    pytester.makepyfile(
+        test_func=(
+            "import pytest\n"
+            "\n"
+            "@pytest.mark.fail_slow(2, enabled='yes')\n"
+            "def test_func():\n"
+            '    Path("test.txt").write_text("Tested\\n")\n'
+        )
+    )
+    result = pytester.runpytest("--fail-slow=2")
+    result.assert_outcomes(errors=1)
+    result.stdout.re_match_lines(
+        [
+            r"_+ ERROR at setup of test_func _+$",
+            "name 'yes' is not defined",
+            "",
+            "During handling of the above exception, another exception occurred:",
+            "Error evaluating 'fail_slow' condition",
+            "    yes",
+            "NameError: name 'yes' is not defined",
+        ],
+        consecutive=True,
     )
+    assert not (pytester.path / "test.txt").exists()
diff --git a/test/test_fail_slow_setup.py b/test/test_fail_slow_setup.py
index 4cb6837..524ecfd 100644
--- a/test/test_fail_slow_setup.py
+++ b/test/test_fail_slow_setup.py
@@ -12,6 +12,30 @@
         ([], "@pytest.mark.fail_slow_setup(2)\n", r"2s"),
         ([], "@pytest.mark.fail_slow_setup(2.0)\n", r"2\.\d+s"),
         ([], "@pytest.mark.fail_slow_setup('0.0333m')\n", r"1\.9\d+s"),
+        ([], "@pytest.mark.fail_slow_setup(2, enabled=True)\n", r"2s"),
+        ([], "@pytest.mark.fail_slow_setup(2, enabled='1 == 1')\n", r"2s"),
+        ([], "@pytest.mark.fail_slow_setup(2, enabled=False)\n", None),
+        ([], "@pytest.mark.fail_slow_setup(2, enabled='1 == 2')\n", None),
+        (
+            ["--fail-slow-setup=2"],
+            "@pytest.mark.fail_slow_setup(2, enabled=False)\n",
+            None,
+        ),
+        (
+            [],
+            "@pytest.mark.fail_slow_setup(2, enabled='config.getoption(\"--fail-slow-setup\") is not None')\n",
+            None,
+        ),
+        (
+            ["--fail-slow-setup=0"],
+            "@pytest.mark.fail_slow_setup(2, enabled='config.getoption(\"--fail-slow-setup\") is not None')\n",
+            r"2s",
+        ),
+        (
+            ["--fail-slow-setup=3"],
+            "@pytest.mark.fail_slow_setup(2, enabled='config.getoption(\"--fail-slow-setup\") is not None')\n",
+            r"2s",
+        ),
         ([], "@pytest.mark.fail_slow_setup(10)\n", None),
         (["--fail-slow-setup=30"], "@pytest.mark.fail_slow_setup(2)\n", r"2s"),
         (["--fail-slow-setup=0.01"], "@pytest.mark.fail_slow_setup(2)\n", r"2s"),
@@ -166,7 +190,9 @@ def test_fail_slow_setup_all_run(pytester: pytest.Pytester) -> None:
     assert (pytester.path / "quick.txt").read_text() == "Set up\n"
 
 
-@pytest.mark.parametrize("args", ["", "42, 'foo'"])
+@pytest.mark.parametrize(
+    "args", ["", "42, 'foo'", "enabled=False", "42, 'foo', enabled=False"]
+)
 def test_fail_slow_setup_marker_bad_args(pytester: pytest.Pytester, args: str) -> None:
     pytester.makepyfile(
         test_func=(
@@ -180,5 +206,8 @@ def test_fail_slow_setup_marker_bad_args(pytester: pytest.Pytester, args: str) -
     result = pytester.runpytest()
     result.assert_outcomes(errors=1)
     result.stdout.fnmatch_lines(
-        ["*UsageError: @pytest.mark.fail_slow_setup() takes exactly one argument"]
+        [
+            "*UsageError: @pytest.mark.fail_slow_setup() takes exactly one"
+            " positional argument"
+        ]
     )