Repository: pytest-dev/pytest
Pull request: #7938 - [Plugin: Stepwise][Enhancements] Refactoring, smarter registration & --sw-skip functionality
Hi team, hope you are all keeping well.  While trying to advance my knowledge of the project I have been understanding plugins a little bit.  Here is a small PR to tidy up some of the stepwise plugin code for consistency.  Some of the most important changes are:

 - Stop registering the plugin if stepwise itself is not active (rather than these constant is_active checks inside hooks).
 - Adding a --sw-skip shorthand option to mirror that of --sw itself
 - Added a pytest_sessionfinish hook to stepwise (this is because --cache-clear was always outputting the [] now, I wanted to keep --cache-clear accurate in such circumstances and claim the cache is 'empty' ? However (imo) this should be handled in pytest_unconfigure and when --sw is not present, stepwise is always writing the [] upfront.  - need advice here 

I moved some strings out into more constants/globals - curious on your thoughts on that & secondly I think we are permitted f string use now (that 3.5+ is no longer officially supported? - I think that is the case anyway).

closes #7938

As always - thanks for all the great contributions to this open source project everyone! I appreciate your own time to review this and welcome any feedback.

Problem statement:
[Feature] Allow a --sw-skip shorthand cli arg like --sw itself permits
The stepwise plugin exposes a shorthand option for the stepwise itself, however it requires a longer arg only for skip, I think these should be consistent and should offer shorthand versions for both.

```python
def pytest_addoption(parser: Parser) -> None:
    group = parser.getgroup("general")
    group.addoption(
        "--sw",
        "--stepwise",
        action="store_true",
        dest="stepwise",
        help="exit on test failure and continue from last failing test next time",
    )
    group.addoption(
        "--stepwise-skip",
        action="store_true",
        dest="stepwise_skip",
        help="ignore the first failing test but stop on the next failing test",
    )
```

Expected:
`pytest --sw-skip`

Hints from issue discussion:

Commit message: add changelog for #7938

Patch to review:
diff --git a/changelog/7938.improvement.rst b/changelog/7938.improvement.rst
new file mode 100644
index 00000000000..8aea0f2418e
--- /dev/null
+++ b/changelog/7938.improvement.rst
@@ -0,0 +1 @@
+Stepwise plugin improvements and a new ``--sw-skip`` shorthand argument is now available
diff --git a/src/_pytest/stepwise.py b/src/_pytest/stepwise.py
index 97eae18fd20..46d225266ef 100644
--- a/src/_pytest/stepwise.py
+++ b/src/_pytest/stepwise.py
@@ -1,5 +1,6 @@
 from typing import List
 from typing import Optional
+from typing import TYPE_CHECKING
 
 import pytest
 from _pytest import nodes
@@ -8,6 +9,14 @@
 from _pytest.main import Session
 from _pytest.reports import TestReport
 
+if TYPE_CHECKING:
+    from _pytest.cacheprovider import Cache
+
+STEPWISE_CACHE_DIR = "cache/stepwise"
+NO_PREVIOUSLY_FAILED_WONT_SKIP = "no previously failed tests, not skipping."
+PREVIOUSLY_FAILED_TEST_NOT_FOUND = "previously failed test not found, not skipping."
+SKIPPING_PASSED_ITEMS = "skipping {} already passed items."
+
 
 def pytest_addoption(parser: Parser) -> None:
     group = parser.getgroup("general")
@@ -15,12 +24,15 @@ def pytest_addoption(parser: Parser) -> None:
         "--sw",
         "--stepwise",
         action="store_true",
+        default=False,
         dest="stepwise",
         help="exit on test failure and continue from last failing test next time",
     )
     group.addoption(
         "--stepwise-skip",
+        "--sw-skip",
         action="store_true",
+        default=False,
         dest="stepwise_skip",
         help="ignore the first failing test but stop on the next failing test",
     )
@@ -28,33 +40,39 @@ def pytest_addoption(parser: Parser) -> None:
 
 @pytest.hookimpl
 def pytest_configure(config: Config) -> None:
-    config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+    # We should always have a cache as cache provider plugin uses tryfirst=True
+    if config.option.stepwise:
+        config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+
+
+def pytest_sessionfinish(session: Session) -> None:
+    config = session.config
+    assert config.cache is not None
+    if not config.option.stepwise:
+        # This hook exists so that --cache-show when empty will not output the empty [] for stepwise by default.
+        # Perhaps this is ok to set in the unconfigure hook?  But I added it here to avoid changing behaviour.
+        config.cache.set(STEPWISE_CACHE_DIR, [])
 
 
 class StepwisePlugin:
     def __init__(self, config: Config) -> None:
         self.config = config
-        self.active = config.getvalue("stepwise")
         self.session: Optional[Session] = None
         self.report_status = ""
-
-        if self.active:
-            assert config.cache is not None
-            self.lastfailed = config.cache.get("cache/stepwise", None)
-            self.skip = config.getvalue("stepwise_skip")
+        assert config.cache is not None
+        self.cache: Cache = config.cache
+        self.lastfailed: Optional[str] = self.cache.get(STEPWISE_CACHE_DIR, None)
+        self.skip: bool = config.option.stepwise_skip
 
     def pytest_sessionstart(self, session: Session) -> None:
         self.session = session
 
     def pytest_collection_modifyitems(
-        self, session: Session, config: Config, items: List[nodes.Item]
+        self, config: Config, items: List[nodes.Item]
     ) -> None:
-        if not self.active:
-            return
         if not self.lastfailed:
-            self.report_status = "no previously failed tests, not skipping."
+            self.report_status = NO_PREVIOUSLY_FAILED_WONT_SKIP
             return
-
         already_passed = []
         found = False
 
@@ -65,16 +83,13 @@ def pytest_collection_modifyitems(
                 break
             else:
                 already_passed.append(item)
-
         # If the previously failed test was not found among the test items,
         # do not skip any tests.
         if not found:
-            self.report_status = "previously failed test not found, not skipping."
+            self.report_status = PREVIOUSLY_FAILED_TEST_NOT_FOUND
             already_passed = []
         else:
-            self.report_status = "skipping {} already passed items.".format(
-                len(already_passed)
-            )
+            self.report_status = SKIPPING_PASSED_ITEMS.format(len(already_passed))
 
         for item in already_passed:
             items.remove(item)
@@ -82,9 +97,6 @@ def pytest_collection_modifyitems(
         config.hook.pytest_deselected(items=already_passed)
 
     def pytest_runtest_logreport(self, report: TestReport) -> None:
-        if not self.active:
-            return
-
         if report.failed:
             if self.skip:
                 # Remove test from the failed ones (if it exists) and unset the skip option
@@ -109,14 +121,9 @@ def pytest_runtest_logreport(self, report: TestReport) -> None:
                     self.lastfailed = None
 
     def pytest_report_collectionfinish(self) -> Optional[str]:
-        if self.active and self.config.getoption("verbose") >= 0 and self.report_status:
-            return "stepwise: %s" % self.report_status
+        if self.config.getoption("verbose") >= 0 and self.report_status:
+            return f"stepwise: {self.report_status}"
         return None
 
-    def pytest_sessionfinish(self, session: Session) -> None:
-        assert self.config.cache is not None
-        if self.active:
-            self.config.cache.set("cache/stepwise", self.lastfailed)
-        else:
-            # Clear the list of failing tests if the plugin is not active.
-            self.config.cache.set("cache/stepwise", [])
+    def pytest_sessionfinish(self) -> None:
+        self.cache.set(STEPWISE_CACHE_DIR, self.lastfailed)
diff --git a/testing/test_stepwise.py b/testing/test_stepwise.py
index df66d798bbe..11c53bcabe2 100644
--- a/testing/test_stepwise.py
+++ b/testing/test_stepwise.py
@@ -117,14 +117,10 @@ def test_fail_and_continue_with_stepwise(stepwise_testdir):
     assert "test_success_after_fail PASSED" in stdout
 
 
-def test_run_with_skip_option(stepwise_testdir):
+@pytest.mark.parametrize("sw_option", ["--stepwise-skip", "--sw-skip"])
+def test_run_with_skip_option(stepwise_testdir, sw_option):
     result = stepwise_testdir.runpytest(
-        "-v",
-        "--strict-markers",
-        "--stepwise",
-        "--stepwise-skip",
-        "--fail",
-        "--fail-last",
+        "-v", "--strict-markers", "--stepwise", f"{sw_option}", "--fail", "--fail-last",
     )
     assert _strip_resource_warnings(result.stderr.lines) == []
 


Code review comments:
diff_hunk: @@ -8,53 +9,70 @@
 from _pytest.main import Session
 from _pytest.reports import TestReport
 
+if TYPE_CHECKING:
+    from _pytest.cacheprovider import Cache
+
+STEPWISE_CACHE_DIR = "cache/stepwise"
+NO_PREVIOUSLY_FAILED_WONT_SKIP = "no previously failed tests, not skipping."
+PREVIOUSLY_FAILED_TEST_NOT_FOUND = "previously failed test not found, not skipping."
+SKIPPING_PASSED_ITEMS = "skipping {} already passed items."
+
 
 def pytest_addoption(parser: Parser) -> None:
     group = parser.getgroup("general")
     group.addoption(
         "--sw",
         "--stepwise",
         action="store_true",
+        default=False,
         dest="stepwise",
         help="exit on test failure and continue from last failing test next time",
     )
     group.addoption(
         "--stepwise-skip",
+        "--sw-skip",
         action="store_true",
+        default=False,
         dest="stepwise_skip",
         help="ignore the first failing test but stop on the next failing test",
     )
 
 
 @pytest.hookimpl
 def pytest_configure(config: Config) -> None:
-    config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+    # We should always have a cache as cache provider plugin uses tryfirst=True
+    if config.option.stepwise:
+        config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+
+
+def pytest_sessionfinish(session: Session) -> None:
+    config = session.config
+    assert config.cache is not None
path: src/_pytest/stepwise.py
text: @user1:
I'd move this to inside the `if`, so it's closer to its point of use.

diff_hunk: @@ -117,14 +117,10 @@ def test_fail_and_continue_with_stepwise(stepwise_testdir):
     assert "test_success_after_fail PASSED" in stdout
 
 
-def test_run_with_skip_option(stepwise_testdir):
+@pytest.mark.parametrize("sw_option", ["--stepwise-skip", "--sw-skip"])
+def test_run_with_skip_option(stepwise_testdir, sw_option):
     result = stepwise_testdir.runpytest(
-        "-v",
-        "--strict-markers",
-        "--stepwise",
-        "--stepwise-skip",
-        "--fail",
-        "--fail-last",
+        "-v", "--strict-markers", "--stepwise", f"{sw_option}", "--fail", "--fail-last",
path: testing/test_stepwise.py
text: @user1:
The format is not needed here :grin: 

```suggestion
        "-v", "--strict-markers", "--stepwise", sw_option, "--fail", "--fail-last",
```

diff_hunk: @@ -8,53 +9,70 @@
 from _pytest.main import Session
 from _pytest.reports import TestReport
 
+if TYPE_CHECKING:
+    from _pytest.cacheprovider import Cache
+
+STEPWISE_CACHE_DIR = "cache/stepwise"
+NO_PREVIOUSLY_FAILED_WONT_SKIP = "no previously failed tests, not skipping."
+PREVIOUSLY_FAILED_TEST_NOT_FOUND = "previously failed test not found, not skipping."
+SKIPPING_PASSED_ITEMS = "skipping {} already passed items."
+
 
 def pytest_addoption(parser: Parser) -> None:
     group = parser.getgroup("general")
     group.addoption(
         "--sw",
         "--stepwise",
         action="store_true",
+        default=False,
         dest="stepwise",
         help="exit on test failure and continue from last failing test next time",
     )
     group.addoption(
         "--stepwise-skip",
+        "--sw-skip",
         action="store_true",
+        default=False,
         dest="stepwise_skip",
         help="ignore the first failing test but stop on the next failing test",
     )
 
 
 @pytest.hookimpl
 def pytest_configure(config: Config) -> None:
-    config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+    # We should always have a cache as cache provider plugin uses tryfirst=True
+    if config.option.stepwise:
+        config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+
+
+def pytest_sessionfinish(session: Session) -> None:
+    config = session.config
+    assert config.cache is not None
+    if not config.option.stepwise:
+        # This hook exists so that --cache-show when empty will not output the empty [] for stepwise by default.
+        # Perhaps this is ok to set in the unconfigure hook?  But I added it here to avoid changing behaviour.
path: src/_pytest/stepwise.py
text: @user1:
I agree it is safer to also do it in `pytest_sessionfinish` instead of `pytest_unconfigure`. 👍 (So I think you might remove this comment).

diff_hunk: @@ -8,53 +9,70 @@
 from _pytest.main import Session
 from _pytest.reports import TestReport
 
+if TYPE_CHECKING:
+    from _pytest.cacheprovider import Cache
+
+STEPWISE_CACHE_DIR = "cache/stepwise"
+NO_PREVIOUSLY_FAILED_WONT_SKIP = "no previously failed tests, not skipping."
+PREVIOUSLY_FAILED_TEST_NOT_FOUND = "previously failed test not found, not skipping."
+SKIPPING_PASSED_ITEMS = "skipping {} already passed items."
+
 
 def pytest_addoption(parser: Parser) -> None:
     group = parser.getgroup("general")
     group.addoption(
         "--sw",
         "--stepwise",
         action="store_true",
+        default=False,
         dest="stepwise",
         help="exit on test failure and continue from last failing test next time",
     )
     group.addoption(
         "--stepwise-skip",
+        "--sw-skip",
         action="store_true",
+        default=False,
         dest="stepwise_skip",
         help="ignore the first failing test but stop on the next failing test",
     )
 
 
 @pytest.hookimpl
 def pytest_configure(config: Config) -> None:
-    config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+    # We should always have a cache as cache provider plugin uses tryfirst=True
+    if config.option.stepwise:
+        config.pluginmanager.register(StepwisePlugin(config), "stepwiseplugin")
+
+
+def pytest_sessionfinish(session: Session) -> None:
+    config = session.config
+    assert config.cache is not None
+    if not config.option.stepwise:
+        # This hook exists so that --cache-show when empty will not output the empty [] for stepwise by default.
+        # Perhaps this is ok to set in the unconfigure hook?  But I added it here to avoid changing behaviour.
+        config.cache.set(STEPWISE_CACHE_DIR, [])
 
 
 class StepwisePlugin:
     def __init__(self, config: Config) -> None:
         self.config = config
-        self.active = config.getvalue("stepwise")
         self.session: Optional[Session] = None
         self.report_status = ""
-
-        if self.active:
-            assert config.cache is not None
-            self.lastfailed = config.cache.get("cache/stepwise", None)
-            self.skip = config.getvalue("stepwise_skip")
+        assert config.cache is not None
+        self.cache: Cache = config.cache
+        self.lastfailed: Optional[str] = self.cache.get(STEPWISE_CACHE_DIR, None)
+        self.skip: bool = config.option.stepwise_skip
path: src/_pytest/stepwise.py
text: @user1:
```suggestion
        self.skip: bool = config.getoption("stepwise_skip")
```

We never deprecated, but the "attr-access" to options through the `option` object is not recommended anymore, with `getoption` the recommended approach (doesn't really make a difference right now, but thought I would mention).

diff_hunk: @@ -65,26 +83,20 @@ def pytest_collection_modifyitems(
                 break
             else:
                 already_passed.append(item)
-
         # If the previously failed test was not found among the test items,
         # do not skip any tests.
         if not found:
-            self.report_status = "previously failed test not found, not skipping."
+            self.report_status = PREVIOUSLY_FAILED_TEST_NOT_FOUND
             already_passed = []
         else:
-            self.report_status = "skipping {} already passed items.".format(
-                len(already_passed)
-            )
+            self.report_status = SKIPPING_PASSED_ITEMS.format(len(already_passed))
 
         for item in already_passed:
path: src/_pytest/stepwise.py
text: @user1:
This of course is not part of your change, but just noticed his is `O(n)`... @author if you are willing, you can also save the index of the found item in the `for item in items` loop, and use simple slicing here to discard the items which already passed (no need to keep them on a list either).

The code even is easier to understand IMHO (this is applied on `master`):

```python
        failed_index = None
        for index, item in enumerate(items):
            if item.nodeid == self.lastfailed:
                failed_index = index
                break
            
        # If the previously failed test was not found among the test items,
        # do not skip any tests.
        if failed_index is None:
            self.report_status = "previously failed test not found, not skipping."
        else:
            self.report_status = f"skipping {failed_index} already passed items."        

            already_passed = items[:failed_index]
            items[:] = items[failed_index:]
            config.hook.pytest_deselected(items=already_passed)
```

@user1:
If you prefer to keep this PR small don't worry, I can open a separate PR with that small improvement above. 👍 

@author:
sure @user1 I will look at enhancing the performance

@author:
tho the code above doesn't quite look right with passing already_passed to the `deselected` hook

@user1:
Ahh right, my bad, fixed that and improved it further a little bit.

diff_hunk: @@ -8,53 +9,70 @@
 from _pytest.main import Session
 from _pytest.reports import TestReport
 
+if TYPE_CHECKING:
+    from _pytest.cacheprovider import Cache
+
+STEPWISE_CACHE_DIR = "cache/stepwise"
+NO_PREVIOUSLY_FAILED_WONT_SKIP = "no previously failed tests, not skipping."
path: src/_pytest/stepwise.py
text: @user1:
For this and the next two constants, I think it is better to keep it as before. The strings are only used once, so I think the code is easier to read when they are just written where they are used.

diff_hunk: @@ -117,14 +117,10 @@ def test_fail_and_continue_with_stepwise(stepwise_testdir):
     assert "test_success_after_fail PASSED" in stdout
 
 
-def test_run_with_skip_option(stepwise_testdir):
+@pytest.mark.parametrize("sw_option", ["--stepwise-skip", "--sw-skip"])
path: testing/test_stepwise.py
text: @user1:
Calling this param `stepwise_skip` would be more clear IMO.