Support using flag --skipslow instead of -m "not slow" for pytest#1421
Support using flag --skipslow instead of -m "not slow" for pytest#1421mhucka wants to merge 7 commits into
--skipslow instead of -m "not slow" for pytest#1421Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new --skipslow flag for pytest to improve test execution control, updating the CONTRIBUTING.md documentation, pyproject.toml configuration, and conftest.py setup. The reviewer recommended using more specific type hints (pytest.Parser and pytest.Item) and the getoption API instead of getvalue for better type safety and correctness.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a custom --skipslow command-line option for pytest to skip slow tests, replacing the previous -m "not slow" marker-based filtering. It updates the documentation, pytest configuration, and adds unit tests for the new conftest hooks. The review feedback points out that checking item.keywords for "slow" is a pytest anti-pattern because it can match test names or module names containing the word "slow", and suggests using item.get_closest_marker("slow") instead, along with updating the corresponding unit tests to mock this method.
| def pytest_runtest_setup(item: pytest.Item) -> None: | ||
| if "slow" in item.keywords and item.config.getoption("skipslow"): | ||
| pytest.skip("skipped because of --skipslow option") |
There was a problem hiding this comment.
Using "slow" in item.keywords is a pytest anti-pattern. item.keywords contains not only the markers applied to a test, but also the names of the test function, class, and module (and their components split by underscores). This means any test with the word slow in its name, class, or file name (e.g., test_slow_algorithm) will be incorrectly skipped when --skipslow is passed, even if it is not marked with @pytest.mark.slow.
Instead, use item.get_closest_marker("slow") to safely and precisely check for the presence of the marker.
| def pytest_runtest_setup(item: pytest.Item) -> None: | |
| if "slow" in item.keywords and item.config.getoption("skipslow"): | |
| pytest.skip("skipped because of --skipslow option") | |
| def pytest_runtest_setup(item: pytest.Item) -> None: | |
| if item.get_closest_marker("slow") is not None and item.config.getoption("skipslow"): | |
| pytest.skip("skipped because of --skipslow option") |
| def test_pytest_runtest_setup_skips(self): | ||
| import conftest | ||
| import pytest | ||
| from unittest.mock import MagicMock | ||
|
|
||
| # Create mock item representing a slow test when skipslow is True. | ||
| item = MagicMock() | ||
| item.keywords = {"slow"} | ||
| item.config.getoption.return_value = True | ||
|
|
||
| with self.assertRaises(pytest.skip.Exception): | ||
| conftest.pytest_runtest_setup(item) | ||
|
|
||
| item.config.getoption.assert_called_once_with("skipslow") | ||
|
|
||
| def test_pytest_runtest_setup_does_not_skip_if_not_slow(self): | ||
| import conftest | ||
| from unittest.mock import MagicMock | ||
|
|
||
| # Test case 1: Not marked as 'slow', skipslow is True. | ||
| item = MagicMock() | ||
| item.keywords = set() | ||
| item.config.getoption.return_value = True | ||
|
|
||
| # Should not raise an exception. | ||
| conftest.pytest_runtest_setup(item) | ||
|
|
||
| # Test case 2: Marked as 'slow', skipslow is False. | ||
| item = MagicMock() | ||
| item.keywords = {"slow"} | ||
| item.config.getoption.return_value = False | ||
|
|
||
| conftest.pytest_runtest_setup(item) |
There was a problem hiding this comment.
Since we are updating pytest_runtest_setup to use item.get_closest_marker("slow") instead of checking item.keywords, we should update the unit tests to mock get_closest_marker accordingly.
def test_pytest_runtest_setup_skips(self):
import conftest
import pytest
from unittest.mock import MagicMock
# Create mock item representing a slow test when skipslow is True.
item = MagicMock()
item.get_closest_marker.return_value = MagicMock()
item.config.getoption.return_value = True
with self.assertRaises(pytest.skip.Exception):
conftest.pytest_runtest_setup(item)
item.get_closest_marker.assert_called_once_with("slow")
item.config.getoption.assert_called_once_with("skipslow")
def test_pytest_runtest_setup_does_not_skip_if_not_slow(self):
import conftest
from unittest.mock import MagicMock
# Test case 1: Not marked as 'slow', skipslow is True.
item = MagicMock()
item.get_closest_marker.return_value = None
item.config.getoption.return_value = True
# Should not raise an exception.
conftest.pytest_runtest_setup(item)
# Test case 2: Marked as 'slow', skipslow is False.
item = MagicMock()
item.get_closest_marker.return_value = MagicMock()
item.config.getoption.return_value = False
conftest.pytest_runtest_setup(item)
The flag
-m "not slow"was not only annoying to type: it invited misspelling and waste of time.This PR changes the flag to be
--skipslow, following the same thing done in the ReCirq project.