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

Doctest 2 to 3 #15

Merged
merged 12 commits into from Sep 30, 2016

Conversation

Projects
None yet
2 participants
@janwijbrand
Copy link
Member

janwijbrand commented Sep 29, 2016

Introduce optionflag EXCEPTION_2TO3 to normalize exception class names in traceback output. In Python 3 they are displayed as the full dotted name. In Python 2 they are displayed as "just" the class name. When running doctests in Python 3, the optionflag will not have any effect, however when running the same test in Python 2, the segments in the full dotted name leading up to the class name are stripped away from the "expected" string.

@@ -39,6 +53,10 @@ def check_output(self, want, got, optionflags):
want = transformer(want)
got = transformer(got)

if sys.version < '3':

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

I would prefer if sys.version_info[0] < 3:

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

We'll update this

@@ -62,6 +80,12 @@ def output_difference(self, example, got, optionflags):

# temporarily hack example with normalized want:
example.want = want

if sys.version < '3':

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

Here too.

Who knows, there may be a Python 10 at some point.

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

We'll update this



def strip_dottedname_from_traceback(string):
# We might want to confirm more strictly were dealing with a traceback.

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

I'd prefer that. See doctest.py for the sigh private regexp. I think a simpler version that checks just the first line would suffice.

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

We'll look in to this.

... else:
... expected = False
>>> result == expected
True

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

Ugh.

Maybe just do it always, on Python 2 and Python 3? Stripping a non-existent prefix is a no-op anyway.

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

We're not sure we understand what you mean.

We took the conscious decision to use Python 3 as the "baseline" so to say. Meaning, when running the tests in Python 3 we do not touch the output. This means, when fixing tests, you "move forward" so to say instead of sticking to Python 2.

... HINT:
... You seem to test traceback output.
... The optionflag EXCEPTION_2TO3 is set.
... Do you use the full dotted name for the exception class name?

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

This hint is not helpful. Is it saying that I should use the full dotted name, or that I shouldn't?

I'm starting to think that instead of # doctest: +EXCEPTION2TO3 I want #doctest: +IGNORE_EXCEPTION_MODULE, and then do the stripping on both the want and the got texts, irrespective of the current Python version.

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

We'll try to update the hint to more helpful.

We'll update the name of the flag to IGNORE_EXCEPTION_MODULE_IN_PYTHON2 due to the aforementioned "conscious decision".

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

That's uncomfortably long :(

foo.bar.FooBarError: requires at least one argument."""
expected = """\
Traceback (most recent call last):
FooBarError: requires at least one argument."""

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

Please use textwrap.dedent() here and in all the other tests in this file, to make this readable.

This comment has been minimized.

@janwijbrand

janwijbrand Sep 30, 2016

Member

Will look in to that.

@janwijbrand

This comment has been minimized.

Copy link
Member

janwijbrand commented Sep 30, 2016

@mgedmin thank you for the comment. We processed them. Perhaps you could have another look? TIA.

FooBarError: requires at least one argument."""
Traceback (most recent call last):
FooBarError: requires at least one argument."""
expected = textwrap.dedent(expected)
self.assertEqual(expected, strip_dottedname_from_traceback(string))

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

This works, but what I had in mind was

        def test_strip_dottedname(self):
            string = textwrap.dedent("""\
                Traceback (most recent call last):
                  ...
                foo.bar.FooBarError: requires at least one argument""")
            expected = textwrap.dedent("""\
                Traceback (most recent call last):
                  ...
                FooBarError: requires at least one argument""")
            self.assertEqual(strip_dottedname_from_traceback(string), expected)

Anyway, a minor matter, and while it's still a bit hard to see where a statement begins and ends, the most important thing is that now it's easy to see where a test method begins and ends. Good enough.

@mgedmin
Copy link
Member

mgedmin left a comment

lgtm

CHANGES.rst Outdated
@@ -10,6 +10,13 @@ Changes

- Cleaned up useless 2to3 conversion.

- Introduce optionflag ``EXCEPTION_2TO3`` to normalize exception class names

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

This has now been renamed, yes?

Also, I think "option flag" as two words works better.

This comment has been minimized.

@mgedmin

mgedmin Sep 30, 2016

Member

please fix this before merging! ;)

janwijbrand added some commits Sep 30, 2016

We wanted to confirm more strictly we're dealing with a traceback here.
However, doctest will preprocess exception output. It gets rid of the
the stack trace and the "Traceback (most recent call last)"-part. It
passes only the exception message to the checker.
@janwijbrand

This comment has been minimized.

Copy link
Member

janwijbrand commented Sep 30, 2016

We found out by actually using zope.testing "in the field" that being smart about detecting traceback ouput worked only in the tests of zope.testing. We found that doctest will preprocess the exception output. See commit message and comment in renormalizing.py.

@mgedmin
Copy link
Member

mgedmin left a comment

:shipit:

@janwijbrand janwijbrand merged commit 527ce11 into master Sep 30, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@janwijbrand janwijbrand deleted the doctest-2-to-3 branch Sep 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment