Improve error message when tests use closed stdout/stderr (capture)

[nicoddemus]

@nicoddemus @asottile

This is still an issue with the latest pytest across tests (e.g. when one tests change the logging config and add a stream handler pointing to stdout, a subsequent test that doesn't setup logging and tries to write to logging will fail).

I think the issue boils down to the fact that if you set up a Stream Handler and you enable capsys the actual output stream will be a pytest stream that will be closed and thrown away at the next test in the test suite. So when a subsequent test tries to write to it you get this error. Adding the following fixture fixed this:

LOGGER = logging.getLogger()

@pytest.fixture(autouse=True)
def ensure_logging_framework_not_altered():
    before_handlers = list(LOGGER.handlers)
    yield
    LOGGER.handlers = before_handlers

Should this be automatically done always by pytest? Granted for this to work ideally you would need to do it for all loggers, not just the root one. In this case, for me, this was enough as I was only set up a custom stream logger onto the root logger.

In an ideal world, I would expect though pytest to cleanup loggig configurations at the end tests that requested the caplog fixture.

Originally posted by @gaborbernat in #14 (comment)

[nicoddemus]

Thanks @gaborbernat for the detailed description.

Indeed seems like pytest should automatically cleanup the handlers somehow, not sure how to do it though ensuring we don't break existing things...

cc @Thisch

[asottile]

I don't think pytest should automatically clean up any of a user's resources as it's arguably a correctness problem with the test suite or user code

[gaborbernat]

It not cleanup we should alter the capsys stream to raise a more meaningful error on write than stream closed and maybe point out the test where that capsys was pulled from?

[asottile]

yeah I'm fine with a better error message -- somehow notify the user that they're reassigning globals to sys.std* streams as a test side-effect

[nicoddemus]

I don't think pytest should automatically clean up any of a user's resources as it's arguably a correctness problem with the test suite or user code
yeah I'm fine with a better error message -- somehow notify the user that they're reassigning globals to sys.std* streams as a test side-effect

Oh right, I agree. Initially I thought we were not cleaning our handlers correctly, but I misunderstood the issue.

[nicoddemus]

yeah I'm fine with a better error message -- somehow notify the user that they're reassigning globals to sys.std* streams as a test side-effect

Actually IIUC the tests are not reassigning sys.std*, but they are installing handlers that point to sys.stdout (which is a buffer installed by pytest) and not cleaning up the handlers afterwards, making the next test fail when its logging operations try to write to the now-closed-buffer.

[nicoddemus]

Needs more investigation, but if the message originates from here:

pytest/src/_pytest/capture.py

Lines 420 to 427 in 2d613a0

	def write(self, obj):
	if isinstance(obj, str):
	obj = obj.encode(self.encoding, "replace")
	else:
	raise TypeError(
	"write() argument must be str, not {}".format(type(obj).__name__)
	)
	self.buffer.write(obj)

We just need to raise a more meaningful message if the buffer is already closed at this point.