Skip to content
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 report format option (#1749) #1754

Merged
merged 12 commits into from
Jul 23, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ Punyashloka Biswal
Quentin Pradet
Ralf Schmitt
Raphael Pierzina
Romain Dorgueil
Roman Bolshakov
Ronny Pfannschmidt
Ross Lawley
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ time or change existing behaviors in order to make them less surprising/more use
namespace in which doctests run.
Thanks `@milliams`_ for the complete PR (`#1428`_).

* New ``--doctest-report`` option available to change the output format of diffs
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick: "when displaying failed doctests (#1749). Thanks @hartym for the PR." 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean I should thank myself in the changelog? also removed the ticket reference, as it was making lint fail.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sounds weird but we like to have people acknowledged in the CHANGELOG. A maintainer could do it before/after merging, but then we couldn't just use GitHub's merge button so we ask people to prepare the final CHANGELOG message so we can speed things up. 😁

About the missing link, you just have to add the proper link reference... scroll down in the CHANGELOG and you will see a long list of links. Just add your link there and linting should now work.

when running (failing) doctests.

* New ``name`` argument to ``pytest.fixture`` decorator which allows a custom name
for a fixture (to solve the funcarg-shadowing-fixture problem).
Thanks `@novas0x2a`_ for the complete PR (`#1444`_).
Expand Down
22 changes: 19 additions & 3 deletions _pytest/doctest.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import traceback

import pytest
from _pytest._code.code import TerminalRepr, ReprFileLocation, ExceptionInfo
from _pytest._code.code import ExceptionInfo, ReprFileLocation, TerminalRepr
from _pytest.fixtures import FixtureRequest


Expand All @@ -17,6 +17,11 @@ def pytest_addoption(parser):
action="store_true", default=False,
help="run doctests in all .py modules",
dest="doctestmodules")
group.addoption("--doctest-report",
type=str.lower, default="udiff",
help="choose another output format for diffs on doctest failure",
choices=sorted(_get_report_choices_keys()),
dest="doctestreport")
group.addoption("--doctest-glob",
action="append", default=[], metavar="pat",
help="doctests file matching pattern, default: test*.txt",
Expand Down Expand Up @@ -59,7 +64,6 @@ def toterminal(self, tw):


class DoctestItem(pytest.Item):

def __init__(self, name, parent, runner=None, dtest=None):
super(DoctestItem, self).__init__(name, parent)
self.runner = runner
Expand Down Expand Up @@ -94,7 +98,7 @@ def repr_failure(self, excinfo):
message = excinfo.type.__name__
reprlocation = ReprFileLocation(filename, lineno, message)
checker = _get_checker()
REPORT_UDIFF = doctest.REPORT_UDIFF
REPORT_UDIFF = _get_report_choices().get(self.config.getoption("doctestreport"))
if lineno is not None:
lines = doctestfailure.test.docstring.splitlines(False)
# add line numbers to the left of the error message
Expand Down Expand Up @@ -291,6 +295,18 @@ def _get_allow_bytes_flag():
return doctest.register_optionflag('ALLOW_BYTES')


def _get_report_choices():
Copy link
Member

Choose a reason for hiding this comment

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

Excellent! 👍

import doctest
return dict(
zip(
_get_report_choices_keys(),
(doctest.REPORT_UDIFF, doctest.REPORT_CDIFF, doctest.REPORT_NDIFF, doctest.REPORT_ONLY_FIRST_FAILURE, 0, )
)
)
Copy link
Member

@The-Compiler The-Compiler Jul 23, 2016

Choose a reason for hiding this comment

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

What about something like:

options = {
    'udiff': doctest.REPORT_UDIFF,
    ...
}
assert set(options) == set(_get_report_choices_keys())
return options

While being a slight duplication, that would make it easier to read IMHO, would make it work regardless of the order, and would fail if the keys diverge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try something else about this, hold on. Will opt for the roof also, will get fresher code :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote the implementation in 51fa244


def _get_report_choices_keys():
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick here: wouldn't be simpler to just have _get_report_choices() and use _get_report_choices().keys() when you need the keys instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was the first implementation, but then it required to import doctest at the option parsing time, and it was making test suite fail (because i guess some doctest dependency is importing logging, and there's a test that checks it is not the case.)

See discussion at #1749 (and first impl at 625b603 that broke the tests). Let me know if you want me to change back but then I have no idea how to avoid this import related failure.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. OK that makes sense, thanks!

Could you add a little explanation to get_report_choices_keys() like "return report choices keys. separate function because this is used to declare options and we can't import doctest at that time"? Just so the next person who stumbles on this doesn't wonder the same thing. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ec7695e

return ('udiff', 'cdiff', 'ndiff', 'only_first_failure', 'none', )

@pytest.fixture(scope='session')
def doctest_namespace():
"""
Expand Down
19 changes: 18 additions & 1 deletion doc/en/doctest.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ from docstrings in all python modules (including regular
python test modules)::

pytest --doctest-modules

You can make these changes permanent in your project by
putting them into a pytest.ini file like this:

Expand Down Expand Up @@ -102,6 +101,7 @@ itself::
>>> get_unicode_greeting() # doctest: +ALLOW_UNICODE
'Hello'


The 'doctest_namespace' fixture
-------------------------------

Expand Down Expand Up @@ -130,3 +130,20 @@ which can then be used in your doctests directly::
10
"""
pass


Output format
-------------

Copy link
Member

Choose a reason for hiding this comment

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

Sorry realized one last thing: please add a .. versionadded:: 3.0 here to make it clear this option was introduced in that version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was wondering also ... But not familiar with pytest release process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In f8f690d

You can change the diff output format on failure for your doctests
by using one of standard doctest modules format in options
(see :data:`python:doctest.REPORT_UDIFF`, :data:`python:doctest.REPORT_CDIFF`,
Copy link
Member

Choose a reason for hiding this comment

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

Did you generate the docs locally to make sure those links work? Sorry, I'm a very casual Sphinx user and rarely remember the proper syntax for references. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, indeed. Never used intersphinx before so I had to find the syntax first too ^^

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! 👍

:data:`python:doctest.REPORT_NDIFF`, :data:`python:doctest.REPORT_ONLY_FIRST_FAILURE`)::

pytest --doctest-modules --doctest-report none
pytest --doctest-modules --doctest-report udiff
pytest --doctest-modules --doctest-report cdiff
pytest --doctest-modules --doctest-report ndiff
pytest --doctest-modules --doctest-report only_first_failure


79 changes: 79 additions & 0 deletions testing/test_doctest.py
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,85 @@ def foo():
"--junit-xml=junit.xml")
reprec.assertoutcome(failed=1)

def _run_doctest_report(self, testdir, format):
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the well written tests!

I think we could move all those tests to a separate test class (TestReportOptions maybe?) to better group them and this utility function together. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 1a33025

testdir.makepyfile("""
def foo():
'''
>>> foo()
a b
0 1 4
1 2 4
2 3 6
'''
print(' a b\\n'
'0 1 4\\n'
'1 2 5\\n'
'2 3 6')
""")
return testdir.runpytest("--doctest-modules", "--doctest-report", format)

def test_doctest_report_udiff(self, testdir, format='udiff'):
Copy link
Member

Choose a reason for hiding this comment

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

Is this default parameter intended?

Copy link
Member

Choose a reason for hiding this comment

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

Oh now I see what you intended in test_doctest_report_case_insensitive. In this case I suggest using parametrize here instead:

@pytest.mark.parametrize('format', ['udiff', 'UDIFF', 'uDiFf'])
def test_doctest_report_udiff(self, testdir, format):

And removing test_doctest_report_case_insensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In e229a27

result = self._run_doctest_report(testdir, format)
result.stdout.fnmatch_lines([
' 0 1 4',
' -1 2 4',
' +1 2 5',
' 2 3 6',
])

def test_doctest_report_cdiff(self, testdir):
result = self._run_doctest_report(testdir, 'cdiff')
result.stdout.fnmatch_lines([
' a b',
' 0 1 4',
' ! 1 2 4',
' 2 3 6',
' --- 1,4 ----',
' a b',
' 0 1 4',
' ! 1 2 5',
' 2 3 6',
])

def test_doctest_report_ndiff(self, testdir):
result = self._run_doctest_report(testdir, 'ndiff')
result.stdout.fnmatch_lines([
' a b',
' 0 1 4',
' - 1 2 4',
' ? ^',
' + 1 2 5',
' ? ^',
' 2 3 6',
])

def test_doctest_report_none_or_only_first_failure(self, testdir):
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using parametrize here as well:

@pytest.mark.parametrize('format', ['none', 'only_first_failure'])
def test_doctest_report_none_or_only_first_failure(self, testdir, format):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In e229a27

Copy link
Member

Choose a reason for hiding this comment

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

e229a27 was about test_doctest_report_udiff correct? I mean here that you can avoid the inner loop and use parametrize in here as well:

@pytest.mark.parametrize('format', ['none', 'only_first_failure'])
def test_doctest_report_none_or_only_first_failure(self, testdir):    
    result = self._run_doctest_report(testdir, format)
    result.stdout.fnmatch_lines([
        'Expected:',
        '       a  b',
        '    0  1  4',
        '    1  2  4',
        '    2  3  6',
        'Got:',
        '       a  b',
        '    0  1  4',
        '    1  2  5',
        '    2  3  6',
    ])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, kind of applied dumbly your comment, did not remember about this other loop.

for format in 'none', 'only_first_failure':
result = self._run_doctest_report(testdir, format)
result.stdout.fnmatch_lines([
'Expected:',
' a b',
' 0 1 4',
' 1 2 4',
' 2 3 6',
'Got:',
' a b',
' 0 1 4',
' 1 2 5',
' 2 3 6',
])

def test_doctest_report_case_insensitive(self, testdir):
for format in 'udiff', 'UDIFF', 'uDiFf':
self.test_doctest_report_udiff(testdir, format)

def test_doctest_report_invalid(self, testdir):
result = self._run_doctest_report(testdir, 'obviously_invalid_format')
result.stderr.fnmatch_lines([
"*error: argument --doctest-report: invalid choice: 'obviously_invalid_format' (choose from*"
])



class TestLiterals:

Expand Down