ENH: Continue running doctests on failure #197

effigies · 2023-04-03T14:48:01Z

Closes #44.

May address #188 at least somewhat.

This may need to be hooked up with an option. Interested in more test cases to check the desired behavior.

bsipocz

Could you please add a changelog entry for it, too?

This works beautifully, thank you!

However, I feel the behaviour may not be always desired, so having an option to opt-in/out would be nice (I'm somewhat undecided what the default should be, inclined towards making it opt-out as doctest is running everything by default rather than stopping at the first failure, but that being said it maybe a very unexpected behaviour change for the existing doctestplus users)

bsipocz · 2023-04-04T23:27:12Z

pytest_doctestplus/plugin.py

+    def __init__(
+        self, checker=None, verbose=None, optionflags=0, continue_on_failure=True
+    ):


nitpick: we allow 100 long lines here, please make black behave and put things on one line :)

bsipocz · 2023-04-05T00:12:05Z

@pllim @saimn - would you like opt-in or opt-out for this? I've tested it with astroquery and have to say very much like what I see, but that being said I very much think we need the option to give users the ability to opt-out (or opt-in).

pllim · 2023-04-06T19:35:41Z

For backward compatibility, maybe opt-in since that would introduce no behavior change?

Unless no one likes the current behavior at all?

bsipocz · 2023-04-06T19:42:33Z

For backward compatibility, maybe opt-in since that would introduce no behavior change?
Unless no one likes the current behavior at all?

It doesn't add more failures, just longer tracebacks, but ideally, people maintaining packages (and looking at doctest failures) are used to the idea of looking at the top of the traceback rather than the occasional red herring at the bottom.
(even if it's opt-in, I expect we need to do explain this concept occasionally for less regular contributors).
Anyway, I'm super stoked about having this available, and being able to fix more doctest failures at once rather than doing it iteratively step-by-step.

pllim · 2023-04-07T03:10:41Z

Thinking about this more, I am also okay with opt-out as personally I also like to see all failures in one go and try to fix as many of them as possible in the next commit, so shall we go for "opt-out" (i.e., have this automatically be enabled) unless @saimn objects?

bsipocz · 2023-04-07T03:15:27Z

Either opt-in or opt-out we need to have the option which is currently not yet included in this PR.

However, I would be happy to merge this as is, and start using the dev version for astroquery, which would help smoking out issues and test the actual behaviour in practice.

saimn · 2023-04-07T09:04:16Z

opt-out seems OK (especially if doctest does that by default).

effigies · 2023-04-07T13:24:20Z

Sounds good. I'll add in an option. Hopefully I can reuse the one that's in vanilla pytest so that people don't need to re-write an option because they added --doctest-plus.

I do think some more unit tests to make sure the effects of turning it on/off would be good. I should definitely add some data dependency like:

>>> import nonexistentmodule as nem
>>> a = nem.calculate_something()

Anything else spring to mind?

I can't promise I'll get back to this before next week.

bsipocz · 2023-05-09T13:05:45Z

@effigies - gentle ping to see whether adding the option is still on your radar? My motivations are rather selfish as I would like to use this as soon as possible for astroquery.

…lure"

effigies · 2023-05-09T14:01:41Z

pytest_doctestplus/plugin.py

+            raise ValueError(
+                "DocTestFinder.find: name must be given when obj.__name__ doesn't exist: "
+                f"{type(obj)!r}"
+            )


Flake8 was complaining in tox.

effigies · 2023-05-09T14:02:10Z

Thanks for the bump. This should be ready for review.

bsipocz · 2023-05-09T15:20:24Z

I'm afraid this is not exactly what I thought we were discussing above, or at least I didn't find the way to disable the feature in e.g. the [tool:pytest] section of setup.cfg.

effigies · 2023-05-09T15:54:17Z

I see. I was aiming to reuse the flag from regular doctests. As a flag, I believe the only way to enable/disable is with addopts, as seen in: https://docs.pytest.org/en/7.1.x/reference/customize.html#pytest-ini

I could create a new ini flag (I think I saw how to do that), but then I'm not sure if it's configurable by the CLI. Either way, this is your project, so happy to do things the way you want...

bsipocz · 2023-05-09T16:51:45Z

Ah, I see. addopts would work, too, I just didn't manage to figure out how to turn it off. Would it mean we should make the feature opt-in as opposed to opt-out?

effigies · 2023-05-10T17:15:08Z

@bsipocz As written, it is opt-in instead of opt-out.

bsipocz

two nitpicks.

I would also like to ask someone to have a double check on, as according to the tests, this indeed opt-in, but if I test it with astroquery and on a narrative docs rst file, I get the behaviour by default.

Branch is here: https://github.com/bsipocz/astroquery/tree/hacking
command: pytest docs/imcce/imcce.rst -R

traceback:

docs/imcce/imcce.rst F                                                                                           [100%]

======================================================= FAILURES =======================================================
_________________________________________________ [doctest] imcce.rst __________________________________________________
176 between the individual ephemerides. By default, ``epoch_nsteps = 1``,
177 which means that only a single epoch ``epoch`` will be queried.
178 
179 Consider the following example, which queries ephemerides for asteroid
180 Pallas over an entire year with a time step of 1 day:
181 
182 .. doctest-remote-data::
183 
184    >>> from astroquery.imcce import Miriade
185    >>> Miriade.get_ephemerides('Pallas', epoch='2019-01-01',
Differences (unified diff with -expected +actual):
    @@ -1,16 +1,25 @@
     <Table length=365>
    -target        epoch                 RA         ...   DEC_rate    delta_rate
    -                d                  deg         ... arcsec / min    km / s
    -str20        float64             float64       ...   float64      float64
    +target        epoch                 RA         ...   DEC_rate    delta_rate 
    +                d                  deg         ... arcsec / min    km / s   
    +str20        float64             float64       ...   float64      float64   
     ------ -------------------- ------------------ ... ------------ ------------
    -Pallas            2458484.5 200.58653041666665 ...      0.15854  -19.3678426
    -Pallas            2458485.5 200.92696041666662 ...      0.16727  -19.4137911
    -Pallas            2458486.5  201.2641308333333 ...      0.17613  -19.4552654
    -Pallas            2458487.5 201.59797541666663 ...      0.18511  -19.4921119
    -Pallas            2458488.5 201.92842624999997 ...      0.19421  -19.5241979
    +Pallas            2458484.5 200.58652916666665 ...      0.15854  -19.3678428
    +Pallas            2458485.5 200.92695916666662 ...      0.16727  -19.4137914
    +Pallas            2458486.5  201.2641295833333 ...      0.17613  -19.4552657
    +Pallas            2458487.5 201.59797416666663 ...      0.18511  -19.4921121
    +Pallas            2458488.5 201.92842499999998 ...      0.19421  -19.5241982
    +Pallas            2458489.5 202.25541541666664 ...      0.20344  -19.5514112
    +Pallas            2458490.5 202.57887916666664 ...      0.21278  -19.5736573
    +Pallas            2458491.5          202.89875 ...      0.22224  -19.5908596
    +Pallas            2458492.5 203.21496166666662 ...      0.23181  -19.6029552
        ...                  ...                ... ...          ...          ...
    -Pallas            2458844.5  261.5083995833333 ...     0.029542   -2.5107101
    -Pallas            2458845.5  261.8853333333333 ...     0.034077   -2.7290984
    -Pallas            2458846.5       262.26158625 ...     0.038612   -2.9467484
    -Pallas            2458847.5 262.63713083333334 ...     0.043144   -3.1635878
    -Pallas            2458848.5       263.01193875 ...     0.047672   -3.3795661
    +Pallas            2458839.5 259.61455874999996 ...    0.0069183   -1.4120195
    +Pallas            2458840.5 259.99445124999994 ...     0.011429   -1.6321409
    +Pallas            2458841.5       260.37381125 ...     0.015948   -1.8522381
    +Pallas            2458842.5 260.75260958333325 ...     0.020475   -2.0721405
    +Pallas            2458843.5  261.1308166666666 ...     0.025007   -2.2916812
    +Pallas            2458844.5 261.50840124999996 ...     0.029542    -2.510709
    +Pallas            2458845.5         261.885335 ...     0.034077   -2.7290973
    +Pallas            2458846.5 262.26158833333335 ...     0.038612   -2.9467473
    +Pallas            2458847.5 262.63713249999995 ...     0.043144   -3.1635867
    +Pallas            2458848.5  263.0119404166667 ...     0.047672    -3.379565

/Users/bsipocz/munka/devel/astroquery/docs/imcce/imcce.rst:185: DocTestFailure
448 * ``ephtype``: switch between J2000 ephemerides (default) and other
449   coordinates
450 * ``refplane``: switch from equatorial coordinates (default) to
451   ecliptical coordinates
452 * ``elements``: switch to MPCORB ephemerides instead of ASTORB
453 * ``radial_velocity``: provides additional information on target's radial
454   velocity
455 
456 
457 >>> 1 + 1
Expected:
    3
Got:
    2

/Users/bsipocz/munka/devel/astroquery/docs/imcce/imcce.rst:457: DocTestFailure

tests/test_doctestplus.py

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

effigies · 2023-05-21T17:34:51Z

I forgot that I used DebugRunnerPlus twice in #196. Updated the other. I realized I used bare config in one and self.config in the other. I'm not positive which is right, or if they're effectively equivalent.

bsipocz

This works now as expected, thank you for the fix!

The two more minor things that are missing are a changelog entry, and mentioning the feature in the docs. I may just add them quickly, so we can use this feature straight away when trying this plugin for some of the core libraries.

effigies · 2023-05-23T00:59:38Z

Changelog and README updated.

bsipocz · 2023-05-23T06:45:05Z

Thank you so much @effigies! 🎆

pllim · 2023-05-23T17:32:34Z

Thank you, all!

TEST: Check that multiple failures are recorded

6a0fb86

bsipocz reviewed Apr 5, 2023

View reviewed changes

bsipocz mentioned this pull request Apr 28, 2023

BUG: fix parsing by filtering out junk lines astropy/astroquery#2719

Merged

effigies added 3 commits May 9, 2023 09:24

ENH: Add continue_on_failure to DebugRunnerPlus

0fb979a

TEST: Add data dependency unit test, both lines fail in one "test fai…

5590f00

…lure"

ENH: Respect --doctest-continue-on-failure flag

0c12016

effigies force-pushed the enh/continue-on-failure branch from 2f9f0f2 to 0c12016 Compare May 9, 2023 14:01

effigies commented May 9, 2023

View reviewed changes

bsipocz modified the milestones: 0.12.2, 0.13 May 9, 2023

bsipocz added the enhancement label May 10, 2023

bsipocz reviewed May 19, 2023

View reviewed changes

tests/test_doctestplus.py Outdated Show resolved Hide resolved

tests/test_doctestplus.py Outdated Show resolved Hide resolved

effigies and others added 2 commits May 21, 2023 13:15

Apply suggestions from code review

b7c6ad3

Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>

FIX: Apply --doctest-continue-on-failure when running on RST files

4389ea7

bsipocz approved these changes May 22, 2023

View reviewed changes

DOC: Update changelog and note feature in README

36db79f

bsipocz merged commit deff8fd into scientific-python:main May 23, 2023

bsipocz mentioned this pull request May 23, 2023

ENH: add --keep-going option (ala sphinx) #188

Closed

effigies deleted the enh/continue-on-failure branch March 4, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Continue running doctests on failure #197

ENH: Continue running doctests on failure #197

effigies commented Apr 3, 2023

bsipocz left a comment

bsipocz Apr 4, 2023

bsipocz commented Apr 5, 2023

pllim commented Apr 6, 2023

bsipocz commented Apr 6, 2023

pllim commented Apr 7, 2023

bsipocz commented Apr 7, 2023

saimn commented Apr 7, 2023

effigies commented Apr 7, 2023

bsipocz commented May 9, 2023

effigies May 9, 2023

effigies commented May 9, 2023

bsipocz commented May 9, 2023

effigies commented May 9, 2023

bsipocz commented May 9, 2023 •

edited

Loading

effigies commented May 10, 2023

bsipocz left a comment

effigies commented May 21, 2023

bsipocz left a comment

effigies commented May 23, 2023

bsipocz commented May 23, 2023

pllim commented May 23, 2023

ENH: Continue running doctests on failure #197

ENH: Continue running doctests on failure #197

Conversation

effigies commented Apr 3, 2023

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz Apr 4, 2023

Choose a reason for hiding this comment

bsipocz commented Apr 5, 2023

pllim commented Apr 6, 2023

bsipocz commented Apr 6, 2023

pllim commented Apr 7, 2023

bsipocz commented Apr 7, 2023

saimn commented Apr 7, 2023

effigies commented Apr 7, 2023

bsipocz commented May 9, 2023

effigies May 9, 2023

Choose a reason for hiding this comment

effigies commented May 9, 2023

bsipocz commented May 9, 2023

effigies commented May 9, 2023

bsipocz commented May 9, 2023 • edited Loading

effigies commented May 10, 2023

bsipocz left a comment

Choose a reason for hiding this comment

effigies commented May 21, 2023

bsipocz left a comment

Choose a reason for hiding this comment

effigies commented May 23, 2023

bsipocz commented May 23, 2023

pllim commented May 23, 2023

bsipocz commented May 9, 2023 •

edited

Loading