Skip to content

bpo-27200: fix pathlib, ssl, turtle and weakref doctests #616

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

Merged
merged 11 commits into from
Apr 13, 2017
Merged

bpo-27200: fix pathlib, ssl, turtle and weakref doctests #616

merged 11 commits into from
Apr 13, 2017

Conversation

marco-buttu
Copy link
Contributor

@marco-buttu marco-buttu commented Mar 11, 2017

This PR is the last but one in the series bpo-27200. The first one was #240, the second one #401, and the third #604. To run the doctests (from the Doc directory):

$ sphinx-build -b doctest . build/doctest \
library/pathlib.rst \
library/ssl.rst \
library/turtle.rst \
library/weakref.rst

@mention-bot
Copy link

@marco-buttu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eliben, @birkenfeld, @vsajip, @tiran and @loewis to be potential reviewers.

@marco-buttu marco-buttu changed the title bpo-27200: fix bpo-27200: fix pathlib, shlex, ssl, turtle and weakref doctests Mar 11, 2017
The setcontext(Context()) in testsetup does not guarantee that
the tests outside this file have a clean context.  We need to
clean the context at the end of the tests.

# make sure other tests (also outside this file) get a fresh context
setcontext(Context())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The setcontext(Context()) in testsetup does not guarantee that the tests outside this file get a fresh context. We need to clean the context at the end of the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Please leave decimal.rst changes out of this PR and open a separate PR and then ping Stefan Krah.

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

I think we also need to following change to ssl examples:

diff --git a/Doc/library/ssl.rst b/Doc/library/ssl.rst
index 01dbecd..02a0c79 100644
--- a/Doc/library/ssl.rst
+++ b/Doc/library/ssl.rst
@@ -2262,7 +2262,7 @@ SSL versions 2 and 3 are considered insecure and are therefore dangerous to
 use.  If you want maximum compatibility between clients and servers, it is
 recommended to use :const:`PROTOCOL_TLS_CLIENT` or
 :const:`PROTOCOL_TLS_SERVER` as the protocol version. SSLv2 and SSLv3 are
-disabled by default.
+disabled by default. ::
 
    >>> client_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
    >>> client_context.options |= ssl.OP_NO_TLSv1

And there are the other failures:

**********************************************************************
File "library/ssl.rst", line 421, in newcontext
Failed example:
    timestamp
Expected:
    1515144883
Got:
    1515134083.0
**********************************************************************
File "library/ssl.rst", line 424, in newcontext
Failed example:
    print(datetime.utcfromtimestamp(timestamp))
Expected:
    2018-01-05 09:34:43
Got:
    2018-01-05 06:34:43


# make sure other tests (also outside this file) get a fresh context
setcontext(Context())

Copy link
Member

Choose a reason for hiding this comment

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

Please leave decimal.rst changes out of this PR and open a separate PR and then ping Stefan Krah.

@@ -43,15 +43,16 @@ The :mod:`shlex` module defines the following functions:
string that can safely be used as one token in a shell command line, for
cases where you cannot use a list.

This idiom would be unsafe::
This idiom would be unsafe:
Copy link
Member

Choose a reason for hiding this comment

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

You have already modified this file in #604.

@@ -46,7 +46,7 @@ Pure paths are useful in some special cases; for example:
Basic use
---------

Importing the main class::
Importing the main class:
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change. The current version looks correct to me and the tests passed when I revert it to use ::.

@marco-buttu marco-buttu changed the title bpo-27200: fix pathlib, shlex, ssl, turtle and weakref doctests bpo-27200: fix pathlib, ssl, turtle and weakref doctests Apr 12, 2017
@marco-buttu
Copy link
Contributor Author

marco-buttu commented Apr 12, 2017

Thank you very much for your time @berkerpeksag! I made all changes you requested me. Just one question: you got a float from ssl.cert_time_to_seconds(). Is it not a bug in the code? Is it supposed to return both floats and ints?

@berkerpeksag
Copy link
Member

I don't know, it's probably worth doing some VCS archaeology :) I built and ran tests in Python 3.4 with an ancient OpenSSL so this might be the culprit.

@@ -2257,7 +2262,7 @@ SSL versions 2 and 3 are considered insecure and are therefore dangerous to
use. If you want maximum compatibility between clients and servers, it is
recommended to use :const:`PROTOCOL_TLS_CLIENT` or
:const:`PROTOCOL_TLS_SERVER` as the protocol version. SSLv2 and SSLv3 are
disabled by default.
disabled by default. ::
Copy link
Member

Choose a reason for hiding this comment

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

I know I suggested this style in my earlier comment, but could you move :: to a new line so we don't lose git blame information? This also applies to the changes in Doc/library/pathlib.rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done :-) Thanks againt @berkerpeksag

@berkerpeksag berkerpeksag merged commit 7b2491a into python:master Apr 13, 2017
@berkerpeksag
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants