gh-70280: allow using custom sys.displayhook's with doctest #25651

skirpichev · 2021-04-27T05:39:04Z

This reverts patch from gh-52296.

Issue: doctest should allow custom sys.displayhook #70280

skirpichev · 2021-04-27T07:05:49Z

test_ttk_guionly() failure unrelated to the PR.

skirpichev · 2021-05-17T10:02:03Z

@tim-one, you are listed as an expert for this module, thus I'm ask you for review. The author of the reverted patch is ok with the change, see the bug thread. I think, it's an easy thing to decide if this can be accepted or not. I would prefer this in 3.10, if possible.

github-actions · 2021-06-17T00:04:19Z

This PR is stale because it has been open for 30 days with no activity.

skirpichev · 2022-03-20T07:49:21Z

@ezio-melotti, you are listed as an expert for testing in the devguide, could you review this little PR? (It seems the doctest module lacks a maintainer.)

MaxwellDupre

Good call.

erlend-aasland · 2022-07-21T18:05:42Z

(FYI, I've updated the PR description with gh numbers iso. bpo numbers.)

erlend-aasland · 2022-07-21T18:08:00Z

It isn't obviously clear to me if Tim's #70280 (comment) approved or disapproved of this change. I have no opinion about it, but I'd appreciate a +1 from one or two other core devs before accepting this.

Lib/test/test_doctest.py

ezio-melotti · 2022-08-05T02:21:51Z

Lib/test/test_doctest.py

+    >>> with patch('sys.displayhook', sys.__displayhook__):
+    ...     r = doctest.DocTestRunner(verbose=False).run(test)


Instead of "fixing" the displayhook here with unittest.mock.patch, the behavior and this workaround should be documented:

describe that doctest now honors custom displayhooks

add a .. versionchanged to indicate the change in behavior

suggest unittest.mock.patch as a way to restore the displayhook while running the doctests

suggest testsetup/testcleanup as another option

In addition, since this is a change in behavior, it might be better to add a FutureWarning and actually implement the change after 1-2 releases.

@ezio-melotti , I have made the requested changes; please review again.

I'm not so sure which is a right place for the versionchanged stuff. Right now it's the changed method. Let me know it it's ok or suggesting patch/testsetup/etc is required (I find this too trivial to add, after all - changing the sys.displayhook is very well documented).

Let me also know if the two-step process with the FutureWarning is required: I'll create another PR with a warning (actual change will be postponed for 3.13, it's correct?). So far, the current state wasn't documented and it seems to be rather a bug then a compatibility breaker...

bedevere-bot · 2022-08-05T02:22:01Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

skirpichev · 2022-08-06T11:25:17Z

I have made the requested changes; please review again

bedevere-bot · 2022-08-06T11:25:21Z

Thanks for making the requested changes!

@ezio-melotti: please review the changes made to this pull request.

serhiy-storchaka

It does not add support for testing examples with a customized sys.displayhook value, as claimed. It unconditionally changes the current behavior. It breaks the code that sets sys.displayhook outside of tests, and it leaks sys.displayhook explicitly set in a doctest.

Doc/library/doctest.rst

skirpichev · 2022-08-07T10:11:12Z

It unconditionally changes the current behavior.

@serhiy-storchaka, that's the point.

It breaks the code that sets sys.displayhook outside of tests, and it leaks sys.displayhook explicitly set in a doctest.

In fact, it tests the code that sets sys.displayhook outside of tests, unless I misunderstood you. In last commits I've added also a test for changing the sys.displayhook value inside of doctests.

skirpichev · 2022-09-12T05:11:31Z

@erlend-aasland, maybe you could redo your review after recent changes? Core devs seems to be inactive for month and I would appreciate any feedback on how well I addressed their review comments.

erlend-aasland · 2022-09-12T08:41:39Z

erlend-aasland, maybe you could redo your review after recent changes? Core devs seems to be inactive for month and I would appreciate any feedback on how well I addressed their review comments.

My stance is still the same as before:

It isn't obviously clear to me if Tim's #70280 (comment) approved or disapproved of this change. I have no opinion about it, but I'd appreciate a +1 from one or two other core devs before accepting this.

Although I have the power to approve and merge this, I of course won't do such a thing. I don't see any loud and clear +1 from any other core dev, hence I'm more aligned to close both this PR and the linked issue.

I'll unsubscribe from this and leave it for Ezio and Serhiy (or any other core dev who want to pick this up).

This reverts patch from https://bugs.python.org/issue8048

skirpichev · 2023-01-19T07:49:52Z

Feel free to take this over.

the-knights-who-say-ni added the CLA signed label Apr 27, 2021

bedevere-bot added the awaiting review label Apr 27, 2021

github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 17, 2021

MaxwellDupre approved these changes Jul 13, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 13, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

erlend-aasland changed the title ~~bpo-26092: allow using custom sys.displayhook's with doctest~~ gh-70280: allow using custom sys.displayhook's with doctest Jul 21, 2022

erlend-aasland added the DO-NOT-MERGE label Jul 21, 2022

ezio-melotti requested changes Aug 5, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Aug 5, 2022

ezio-melotti mentioned this pull request Aug 5, 2022

doctest should allow custom sys.displayhook #70280

Closed

skirpichev requested a review from ezio-melotti August 5, 2022 10:22

ezio-melotti requested a review from serhiy-storchaka August 5, 2022 22:52

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 6, 2022

skirpichev mentioned this pull request Aug 6, 2022

Accept "I have made the requested changes..." in PR review comments python/bedevere#499

Open

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 7, 2022

serhiy-storchaka reviewed Aug 7, 2022

View reviewed changes

Doc/library/doctest.rst Outdated Show resolved Hide resolved

skirpichev requested review from ezio-melotti and serhiy-storchaka and removed request for ezio-melotti and serhiy-storchaka September 11, 2022 04:01

bpo-26092: allow using custom sys.displayhook's with doctest

b877680

This reverts patch from https://bugs.python.org/issue8048

skirpichev closed this Jan 19, 2023

skirpichev deleted the fix-26092 branch January 19, 2023 07:49

skirpichev mentioned this pull request Nov 9, 2023

gh-70280: allow using custom sys.displayhook's with doctest #111871

Closed

		>>> with patch('sys.displayhook', sys.__displayhook__):
		... r = doctest.DocTestRunner(verbose=False).run(test)

Uh oh!

gh-70280: allow using custom sys.displayhook's with doctest #25651

gh-70280: allow using custom sys.displayhook's with doctest #25651

Uh oh!

Conversation

skirpichev commented Apr 27, 2021 • edited by erlend-aasland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Apr 27, 2021

Uh oh!

skirpichev commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 17, 2021

Uh oh!

skirpichev commented Mar 20, 2022

Uh oh!

MaxwellDupre left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Jul 21, 2022

Uh oh!

Uh oh!

ezio-melotti Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 5, 2022

Uh oh!

skirpichev commented Aug 6, 2022

Uh oh!

bedevere-bot commented Aug 6, 2022

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skirpichev commented Aug 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Sep 12, 2022

Uh oh!

erlend-aasland commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

skirpichev commented Apr 27, 2021 •

edited by erlend-aasland

Loading

skirpichev commented May 17, 2021 •

edited

Loading

erlend-aasland commented Jul 21, 2022 •

edited

Loading

ezio-melotti Aug 5, 2022 •

edited

Loading

skirpichev Aug 5, 2022 •

edited

Loading

skirpichev commented Aug 7, 2022 •

edited

Loading

erlend-aasland commented Sep 12, 2022 •

edited

Loading