Doctest report format option (#1749) #1754

hartym · 2016-07-23T12:53:43Z

Here is a first pull request, as it is my first contribution, I may have done incorrect things according to pytest standards, please let me know whatever I should do more or differently.

Target: for bug or doc fixes, target master; for new features, target features
Make sure to include one or more tests for your change
Add yourself to AUTHORS
Add a new entry to the CHANGELOG (choose any open position to avoid merge conflicts with other PRs)

…ailure. (fixes pytest-dev#1749)

…ogress as a few checks fail (related to pytest-dev#1749)

…tested things like "logging") for argument parsing (fixes pytest-dev#1749)

coveralls · 2016-07-23T12:59:33Z

Coverage increased (+0.04%) to 93.08% when pulling 87ca4b9 on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

nicoddemus · 2016-07-23T13:02:42Z

CHANGELOG.rst

@@ -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


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

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

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.

nicoddemus · 2016-07-23T13:04:41Z

_pytest/doctest.py

+        )
+    )
+
+def _get_report_choices_keys():


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

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.

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. 😁

In ec7695e

coveralls · 2016-07-23T13:10:51Z

Coverage increased (+0.04%) to 93.08% when pulling 014ebc9 on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

nicoddemus · 2016-07-23T13:11:07Z

testing/test_doctest.py

+            '      2  3  6',
+        ])
+
+    def test_doctest_report_none_or_only_first_failure(self, testdir):


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):

In e229a27

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', ])

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

…ate functions.

@nicoddemus

… from another as suggested by @nicoddemus in pr pytest-dev#1754

coveralls · 2016-07-23T13:23:21Z

Coverage increased (+0.02%) to 93.057% when pulling e229a27 on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

nicoddemus · 2016-07-23T13:26:52Z

_pytest/doctest.py

+    """
+    return ('udiff', 'cdiff', 'ndiff', 'only_first_failure', 'none', )
+
+def _get_report_choices():


Excellent! 👍

coveralls · 2016-07-23T13:32:57Z

Coverage increased (+0.02%) to 93.057% when pulling f8f690d on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

coveralls · 2016-07-23T13:41:53Z

Coverage increased (+0.02%) to 93.057% when pulling f8f690d on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

hackebrot · 2016-07-23T13:49:56Z

Great job @hartym! 😃

nicoddemus · 2016-07-23T13:50:51Z

I second that, this a very well written PR, thanks @hartym! 😄

hackebrot · 2016-07-23T13:55:37Z

EuroPython sprint already paid out for me 😁

The-Compiler · 2016-07-23T13:57:28Z

_pytest/doctest.py

+            _get_report_choices_keys(),
+            (doctest.REPORT_UDIFF, doctest.REPORT_CDIFF, doctest.REPORT_NDIFF, doctest.REPORT_ONLY_FIRST_FAILURE, 0, )
+        )
+    )


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.

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

Rewrote the implementation in 51fa244

nicoddemus · 2016-07-23T13:58:15Z

@hartym just checking, did you see my last comment about parametrizing test_doctest_report_none_or_only_first_failure? If you don't have time to do it I would be happy to merge as it is. 👍

hartym · 2016-07-23T13:59:34Z

Fixed in d5a70ac ;)
The temperature in the room makes it easy to miss comments, sorry ...

nicoddemus · 2016-07-23T14:00:33Z

Thanks a lot! 😁

Merging as soon as the CI passes again.

The-Compiler · 2016-07-23T14:01:23Z

The temperature in the room makes it easy to miss comments, sorry ...

The temperature is more comfortable outside on the roof 😉

coveralls · 2016-07-23T14:01:56Z

Coverage increased (+0.02%) to 93.057% when pulling d5a70ac on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

hackebrot · 2016-07-23T14:07:43Z

@kvas-it and myself are sitting outside as well 😎

nicoddemus · 2016-07-23T14:12:29Z

hartym · 2016-07-23T14:19:30Z

Ok, I implemented it differently so we avoid this strange "double method" thing, as @The-Compiler was suggesting. A bit more verbose, but definitely less error prone imho, what do you think ? (51fa244)

hackebrot · 2016-07-23T14:19:33Z

Trust me @nicoddemus, we'd love to have you here! Maybe next year?

nicoddemus · 2016-07-23T14:23:19Z

_pytest/doctest.py

@@ -94,7 +111,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_choice(self.config.getoption("doctestreport"))


Heh we missed the variable name REPORT_UDIFF... should be probably report_choice or so.

In 94731fc

nicoddemus · 2016-07-23T14:23:56Z

Ok, I implemented it differently so we avoid this strange "double method" thing, as @The-Compiler was suggesting. A bit more verbose, but definitely less error prone imho, what do you think ?

I think it looks good, thanks!

nicoddemus · 2016-07-23T14:24:11Z

Trust me @nicoddemus, we'd love to have you here! Maybe next year?

Maybe! 😄

coveralls · 2016-07-23T14:28:35Z

Coverage increased (+0.02%) to 93.06% when pulling 94731fc on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

coveralls · 2016-07-23T14:35:22Z

Coverage increased (+0.02%) to 93.06% when pulling 94731fc on hartym:1749_doctest_report_format into ae07985 on pytest-dev:features.

nicoddemus · 2016-07-23T15:02:22Z

Just waiting for CI to go green before merging.

Just took the opportunity to update the "what's new in pytest 3.0" post. 😉

nicoddemus · 2016-07-23T15:52:52Z

Thanks again @hartym! 👍

hartym · 2016-07-23T15:58:35Z

✌️ /me happy ; I guess I won't do more today (kind of slow walk more than sprint) but I'll be there tomorrow if there is some easy stuff for me.

nicoddemus · 2016-07-23T16:06:26Z

We certainly appreciate it! 😄

tomviner · 2016-07-23T16:07:58Z

Great work @hartym!

(A hint that @nicoddemus gave me is you can close issues via commit messages. So, next time, if you add e.g. fixes #1749 to a commit message, it'll close when the PR is merged! See for details: https://help.github.com/articles/closing-issues-via-commit-messages/ )

nicoddemus · 2016-07-25T13:37:50Z

Just a heads up @tomviner: @hartym did in fact wrote "Fixes #1749" in one of the commits, but GitHub only closes issues when that commit is merged into master. 😁

danilobellini · 2016-07-26T03:12:24Z

I've finished a plugin recently that has something to do with this new reporting feature: https://github.com/danilobellini/pytest-doctest-custom

I still didn't test it together with this new doctest-report, but I think expressions like "output format" would remind the internal doctest representation formatter to many people (which is actually what my plugin is about).

nicoddemus · 2016-07-26T03:15:20Z

Cool 😎

nicoddemus · 2018-04-07T02:11:48Z

Hey @danilobellini, please don't hesitate to follow up with moving your plugin to pytest-dev as we discussed. 😁

hartym added 3 commits July 23, 2016 13:06

Implements an option to choose the doctest output format in case of f…

625b603

…ailure. (fixes pytest-dev#1749)

Choose the doctest output format in case of failure, still work in pr…

fd8e019

…ogress as a few checks fail (related to pytest-dev#1749)

Separate the option keys and value to avoid importing "doctest" (and …

87ca4b9

…tested things like "logging") for argument parsing (fixes pytest-dev#1749)

hartym mentioned this pull request Jul 23, 2016

Feature request: doctest report format as an option #1749

Closed

nicoddemus reviewed Jul 23, 2016
View reviewed changes

Removed bug reference in changelog because it makes the linting fail.

014ebc9

nicoddemus reviewed Jul 23, 2016
View reviewed changes

hartym added 4 commits July 23, 2016 15:15

fixes changelog, I think I got it

922a295

Refactors test to be in its own class, as discussed with @nicoddemus.

1a33025

adds a bit of doctest hint on why the key and value getters are separ…

ec7695e

…ate functions.

using @pytest.mark.parametrize instead of calling one test many times…

e229a27

… from another as suggested by @nicoddemus in pr pytest-dev#1754

nicoddemus reviewed Jul 23, 2016
View reviewed changes

adds the versionadded flag in docs (pytest-dev#1749)

f8f690d

The-Compiler reviewed Jul 23, 2016
View reviewed changes

Simplify test in test_doctest_report_none_or_only_first_failure.

d5a70ac

Cleaner implementation of pytest-dev#1749.

51fa244

nicoddemus reviewed Jul 23, 2016
View reviewed changes

Changes variable name so it better describes what it does now.

94731fc

nicoddemus merged commit 9891593 into pytest-dev:features Jul 23, 2016

hartym deleted the 1749_doctest_report_format branch July 23, 2016 15:58

Doctest report format option (#1749) #1754

Doctest report format option (#1749) #1754

Conversation

hartym commented Jul 23, 2016

coveralls commented Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 23, 2016 • edited Loading

coveralls commented Jul 23, 2016 • edited Loading

hackebrot commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

hackebrot commented Jul 23, 2016

The-Compiler Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jul 23, 2016

hartym commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

The-Compiler commented Jul 23, 2016

coveralls commented Jul 23, 2016 • edited Loading

hackebrot commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

hartym commented Jul 23, 2016

hackebrot commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

coveralls commented Jul 23, 2016 • edited Loading

coveralls commented Jul 23, 2016 • edited Loading

nicoddemus commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

hartym commented Jul 23, 2016

nicoddemus commented Jul 23, 2016

tomviner commented Jul 23, 2016

nicoddemus commented Jul 25, 2016

danilobellini commented Jul 26, 2016

nicoddemus commented Jul 26, 2016

nicoddemus commented Apr 7, 2018

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

The-Compiler Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading

coveralls commented Jul 23, 2016 •

edited

Loading