Disable output capturing in doctest on macOS #3215
Conversation
|
By applying the fix, the test now passes (on macOS Python 3.6) and the behavior is also corrected in real-world testing. I didn't see any concerns about applying this patch. Are there any? |
|
I don't understand why the test is still failing on Linux. I thought the test was previously only failing on macOS. How is it adding the fix improves the behavior for macOS but breaks the behavior for Linux? I'm tempted to constrain the fix to macOS only (acknowledging it's a hack) until such a time that a robust solution can be devised. |
_pytest/doctest.py
Outdated
| self._disable_output_capturing() | ||
| self.runner.run(self.dtest) | ||
|
|
||
| def _disable_output_capturing(self): |
There was a problem hiding this comment.
please include the platform name in the method name since its a platform hack
i propose _disable_output_capturing_for_darwin
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
well done, thanks 👍
_pytest/doctest.py
Outdated
| if capman: | ||
| out, err = capman.suspend_global_capture(in_=True) | ||
| sys.stdout.write(out) | ||
| sys.stdout.write(err) |
There was a problem hiding this comment.
err might be undefinded here if bool(capman) is False.
There was a problem hiding this comment.
Hmm. I'd consider that contingency unlikely (negligible) unless capture managers are expected to present as __bool__() == False under some condition.
There was a problem hiding this comment.
if the capturemanager is disabled by a cli option the code would fail
please lets use consisted indentation for the related code
for overly pedantic correctness it might make sense to change the check to is None
There was a problem hiding this comment.
I agree, no harm being consistent. Btw, shouldn't the last line write to sys.stderr instead?
There was a problem hiding this comment.
if the capturemanager is disabled by a cli option the code would fail
I'm afraid I'm not following. Do you mean --capture=no? As I understand it, passing that parameter would mean that bool(capman) == True. And I tested it, and it works fine.
I think I see what you're saying - that on line 121, err won't have been defined if the capman was False. And that would happen by disabling the plugin entirely, such as with -p no:capturemanager. Testing that, I get this output. It's not quite what I expected. The code didn't fail anywhere near doctest.py.
please lets use consisted indentation for the related code
At first, I thought this was a stylistic comment, but I see now that you've recognized an error I introduced when I copied this code from the original.
for overly pedantic correctness it might make sense to change the check to is None
I'm not sure about that. It may be the case that if the capture manager defines __bool__ which returns False, that's also a condition that should be bypassed (correctly). Since it doesn't define it either way, the boolean test currently seems to me to be correct either way, at which point I prefer the shorter code.
Btw, shouldn't the last line write to sys.stderr instead?
Yes, that does seem incongruent. I'll test with this suggestion.
There was a problem hiding this comment.
Okay, I was eventually able to trigger the UnboundLocalError. I had to disable the capture manager and make sure to pass --doctest-modules to the test run as well (forgot about that before).
There was a problem hiding this comment.
err might be undefinded here if bool(capman) is False.
Now that I re-read the original comment, I see that was the sole concern, and I was on an unrelated tangent.
|
I honestly still don't understand why it's necessary to write out captured streams when suspending the capture. I also don't understand why adding this new suspension routine doesn't require a symmetric operation (to unsuspend). But honestly, it seems to work, and until someone can show a use-case that this breaks, I'd like to move forward with it, as the status-quo is pdb is often broken in the test suites where I use doctests (which is almost all of them). |
nicoddemus
deleted the
bugfix/985/disable-output-capturing-in-doctest
branch
twmr
changed the title
... to avoid losing reference to stdout. Fixes #985.
This patch implements the change proposed, but encapsulates the workaround and documents it with a docstring.
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.masterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.