Separate concerns #11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the pytest-isolated plugin by splitting a monolithic 708-line plugin.py file into focused, single-responsibility modules and reorganizing tests from a single file with 32 tests into feature-based test files.
Changes:
- Split monolithic plugin file into 6 modules:
config.py,utils.py,grouping.py,reporting.py,execution.py, and a minimalplugin.pyentry point - Reorganized 32 tests from
test_plugin.pyinto 6 feature-based test files - Removed
test_k_filtering.pyand consolidated filtering tests intotest_filtering.py
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pytest_isolated/plugin.py | Reduced to 19-line entry point that imports from new modules |
| src/pytest_isolated/config.py | CLI options and constants extracted from plugin.py |
| src/pytest_isolated/utils.py | Helper functions and type definitions extracted from plugin.py |
| src/pytest_isolated/grouping.py | Test collection and grouping logic extracted from plugin.py |
| src/pytest_isolated/reporting.py | Test result reporting extracted from plugin.py |
| src/pytest_isolated/execution.py | Subprocess execution logic extracted from plugin.py |
| tests/test_basic.py | Core isolation tests (4 tests) extracted from test_plugin.py |
| tests/test_grouping.py | Grouping logic tests (6 tests) extracted from test_plugin.py |
| tests/test_execution.py | Subprocess management tests (11 tests) extracted from test_plugin.py |
| tests/test_reporting.py | Output and result handling tests (6 tests) extracted from test_plugin.py |
| tests/test_options.py | CLI options tests (5 tests) extracted from test_plugin.py |
| tests/test_filtering.py | Consolidated filtering tests (8 tests) from test_k_filtering.py |
| tests/test_plugin.py | Deleted (tests moved to feature-based files) |
| tests/test_k_filtering.py | Deleted (tests consolidated into test_filtering.py) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pcrespov
left a comment
pcrespov
left a comment
There was a problem hiding this comment.
Super nice!
I think now it is far more maintainable.!
Left some minor comments and suggestions!
thx so much!
| help="Timeout in seconds for isolated test groups (default: 300)", | ||
| ) | ||
| group.addoption( | ||
| "--no-isolation", |
There was a problem hiding this comment.
some CLI designs use these options called: boolean flag negation or positive/negative flag pairs.
It consist on a optional flag e.g. --isolated and then negation of it as --no-isolated
Its a small detail but it is very neat to use for flags, for instance: git commit -m "x" --verify and git commit -m "x" --no-verify
Seems to be very common becuase apparently in python311 is even implemented implemented in argparse: https://docs.python.org/3/library/argparse.html#argparse.BooleanOptionalAction
import argparse
parser = argparse.ArgumentParser()
parser.add_argument(
"--verify",
action=argparse.BooleanOptionalAction,
default=True,
help="Enable or disable verification",
)
args = parser.parse_args()
print(args.verify)
| "isolated_capture_passed", | ||
| type="bool", | ||
| default=False, | ||
| help="Capture output for passed tests (default: False)", |
There was a problem hiding this comment.
This option i still do not fully understand.
When I run a test that is successful I should still be able to see the output if i use -s. why adding a new option and not using the ones already in pytest? i.e.
--capture=method Per-test capturing method: one of fd|sys|no|tee-sys
-s Shortcut for --capture=no
There was a problem hiding this comment.
this only only available in ini file? and not in CLI
| return None # child runs the normal loop | ||
|
|
||
| config = session.config | ||
| groups = getattr(config, "_subprocess_groups", OrderedDict()) |
There was a problem hiding this comment.
MINOR: perhaps you can keep these customized attrs like _subprocess_groups in a separate constants.py module
| if not isinstance(groups, OrderedDict): | ||
| groups = OrderedDict() | ||
| group_timeouts: dict[str, int | None] = getattr( | ||
| config, "_subprocess_group_timeouts", {} |
| config = session.config | ||
| groups = getattr(config, "_subprocess_groups", OrderedDict()) | ||
| if not isinstance(groups, OrderedDict): | ||
| groups = OrderedDict() |
There was a problem hiding this comment.
this logic is a bit strange, right?
if config._subprocess_groups is not an OrderDict create an empty one?
I thought grouping is the one reponsible of createing this entry.. what else would it be ccreated?
| # Get default timeout configuration | ||
| timeout_opt = config.getoption("isolated_timeout", None) | ||
| timeout_ini = config.getini("isolated_timeout") | ||
| default_timeout = timeout_opt or (int(timeout_ini) if timeout_ini else 300) |
There was a problem hiding this comment.
move 300 as a constant on the top or to the constants.py i mentioned before
| f"Subprocess group={group_name!r} timed out after {group_timeout} " | ||
| f"seconds (execution time: {execution_time:.2f}s). " | ||
| f"Increase timeout with --isolated-timeout, isolated_timeout ini, " | ||
| f"or @pytest.mark.isolated(timeout=N)." |
There was a problem hiding this comment.
THOUGHT: that timeout on pytest.mark.isoated is cool but it would be even cooler if one could compose different plugins e.g. pytest-timeout (
@pytest.mark.isolated
@pytest.mark.timeout(300)
@pytest.mark.flaky
def test_it():
...
Actually, i think this should also work.
| tests_dir = pytester.mkdir("tests") | ||
| test_file = tests_dir / "test_isolated.py" | ||
| test_file.write_text(""" | ||
| import pytest |
There was a problem hiding this comment.
MINOR: aline this text like the others for easier reading
| """ | ||
| import pytest | ||
|
|
||
| pytestmark = pytest.mark.isolated |
There was a problem hiding this comment.
Oh nice. I now recall you told me you also implemented thisone
| subdir = pytester.mkdir("tests") | ||
| test_file = subdir / "test_nested.py" | ||
| test_file.write_text(""" | ||
| import pytest |
There was a problem hiding this comment.
smae here aline the string. It can be done with textwrap
There was a problem hiding this comment.
instea of test_file, cannot pytester.makepyfile helper be used as you do elsewhere?
2 participants
Refactoring: Separate concerns and reorganize codebase
Strategy
Code was split by moving functions verbatim using a script. Only imports were updated to reflect the reorganization.
Plugin Structure
Split monolithic
plugin.py(708 lines) into focused modules:config.py- CLI options and constantsutils.py- Helper functions and type definitionsgrouping.py- Test collection and grouping logicreporting.py- Test result reportingexecution.py- Subprocess executionplugin.py- Entry point (19 lines, imports only)Test Organization
Reorganized tests from single
test_plugin.py(32 tests) into feature-based files:test_basic.py- Core isolation functionality (4 tests)test_grouping.py- Test grouping logic (6 tests)test_execution.py- Subprocess management (11 tests)test_reporting.py- Output and result handling (6 tests)test_options.py- CLI options (5 tests)test_filtering.py- Consolidated filtering tests (8 tests)