Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add capteesys capture fixture to bubble up output to --capture handler #12854

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ayjayt
Copy link

@ayjayt ayjayt commented Oct 5, 2024

potentially closes #12081(and more!)

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Add text like closes #XYZW to the PR description and/or commits (where XYZW is the issue number). See the github docs for more information.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Add yourself to AUTHORS in alphabetical order.

Explanation

To set global capture behavior, we set --capture=tee-sys so that pytest can a) capture output for printing reports and b) dump output to our terminals. Both of these things are useful.

Passing capture-type fixtures in our tests will block global capture behavior, but allow us to analyze captured output inside the test.

Unfortunately, capture fixtures don't support anything like --capture=tee-sys- until now. This PR adds a new fixture: capteesys which captures output for analysis in tests but also pushes the output to whatever is set by --capture=. It allows the output to "bubble up" to the next capture handler.

Docs build, tests pass.

My Approach (Engineering)

How the system works right now:

Right now, when you include a fixture (syscap, fdcap, etc), that fixture initializes the CaptureFixture wrapper and passes it the class of the implementation it wants to use, one of:

class NoCapture(...): ... # eg. __init__ takes file descriptor #
class SysCapture(...): ... # eg. __init__ takes file descriptor # + several options
class SysCaptureBinary(...): ...
class SysCapture(...): ...
class FDCapture(...): ...
class FDCaptureBinary(...): ...

self.captureclass = ... # whatever was passed

which the wrapper will instantiate as an object later (depending on fixture scope) as such:

def _start(...):
    ...
    out = self.captureclass(1) # captureclass set to one of above classes
    err = self.captureclass(2)

There is no current way to pass other options to the constructor, which would enable other features, such as tee.

Solution:

Discarded Solutions

  • Make that each Capture class accept a tee argument- most will discard it (FDCapture probably could accept it). Most of the time it would be nonsensical and maybe misleading to developers or users.

  • Create a conditional switch for the CaptureFixture wrapper which changes the initialization process based on the actual constructor- But it seems bad to create individual branches in base/containing classes for all possible children/attributes.

Chosen Solution

Since the CaptureFixture class is already initialized by passing the class of the implementation, you just add a new configure dictionary:

capture_fixture = CaptureFixture(SysCapture, request, _ispytest=True)
# to
capture_fixture = CaptureFixture(SysCapture, request, config=dict(tee=True), _ispytest=True)

Which is then expanded when initializing the implementation:

out = self.captureclass(1, **self._config)

It requires very few code changes. This explanation was longer.

To create the capteesys fixture, I just copied the capsys fixture but added the config dictionary as part of its setup.

The config dict is passed alongside the class that the fixture will
eventually initialize. It can use the config dict for optional
arguments to the implementation's constructor.
@ayjayt ayjayt force-pushed the andrew/add_capteesys_fixture branch from 2227ade to 11578a9 Compare October 5, 2024 13:20
@ayjayt
Copy link
Author

ayjayt commented Oct 5, 2024

It may be better to use "impl_kwargs" instead of "config" or something, in the future maybe have a "impl_args" that expands with * to allow positional arguments and a "impl_kwargs" that expands with **.

@ayjayt ayjayt force-pushed the andrew/add_capteesys_fixture branch from 11578a9 to e859f40 Compare October 5, 2024 13:35
ayjayt added 4 commits October 5, 2024 09:39
When I started this feature, I expected passing the "tee" flag
to "syscap" would copy output directly to the calling terminal, to the
original ORIGINAL sys.stdout/sys.stderr. That's not how it works-
it passes output to whatever capture was set earlier! This actually
makes the feature more flexible because it enables tee-like behavior as
well as pytests reporting behavior. Awesome!
@ayjayt ayjayt force-pushed the andrew/add_capteesys_fixture branch from e859f40 to 82afa8f Compare October 5, 2024 13:39
@@ -1004,6 +1004,40 @@ def test_output(capsys):
capman.unset_fixture()


@fixture
def capteesys(request: SubRequest) -> Generator[CaptureFixture[str]]:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixture is almost 100% a copy of the capsys one above it, since a tee-sys is just a sys w/ an extra argument (tree=True)

@ayjayt
Copy link
Author

ayjayt commented Oct 5, 2024

I've created a polyfill to add to conftest.py to use this feature and warn if/when it is finally released:

@pytest.fixture(scope="function")
def capteesys(request):
    from _pytest import capture
    import warnings
    if hasattr(capture, "capteesys"):
        warnings.warn(( "You are using a polyfill for capteesys, but this"
                        " version of pytest supports it natively- you may"
                        f" remove the polyfill from your {__file__}"),
                        DeprecationWarning)
        # Remove next two lines if you don't want to ever switch to native version
        yield request.getfixturevalue("capteesys")
        return
    capman = request.config.pluginmanager.getplugin("capturemanager")
    capture_fixture = capture.CaptureFixture(capture.SysCapture, request, _ispytest=True)
    def _inject_start():
        self = capture_fixture # closure seems easier than importing Type or Partial
        if self._capture is None:
            self._capture = capture.MultiCapture(
                in_ = None,
                out = self.captureclass(1, tee=True),
                err = self.captureclass(2, tee=True)
            )
            self._capture.start_capturing()
    capture_fixture._start = _inject_start
    capman.set_fixture(capture_fixture)
    capture_fixture._start()
    yield capture_fixture
    capture_fixture.close()
    capman.unset_fixture()

@ayjayt ayjayt force-pushed the andrew/add_capteesys_fixture branch from eaa4036 to 9cd57b1 Compare October 7, 2024 04:46
@gvwilson
Copy link

gvwilson commented Oct 7, 2024

👍 this would be very useful (albeit in rather esoteric contexts :-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Capture: copy output when using readouterr() instead of consuming it
2 participants