Run tests in context managers #87

Sheila-nk · 2023-08-11T14:59:35Z

plugged in the DebugDTRunner
overrode its run function to ensure tests are run in context managers

Closes #82

Sheila-nk · 2023-08-11T15:11:03Z

Bug
It seems the DebugDTRunner implementation is not raising the defined exceptions as it should. This is likely because of this part of the code in the DoctestRunner run function:

       if out is None:
            encoding = save_stdout.encoding
            if encoding is None or encoding.lower() == 'utf-8':
                out = save_stdout.write
            else:
                # Use backslashreplace error handling on write
                def out(s):
                    s = str(s.encode(encoding, 'backslashreplace'), encoding)
                    save_stdout.write(s)

From the code implementation here in the DTRunner

    def _report_item_name(self, out, item_name, new_line=False):
        if item_name is not None:
            out("\n " + item_name + "\n " + "-"*len(item_name))
            if new_line:
                out("\n")

out is used as a callable but the raised TypeError indicates that it is still a list object

ev-br · 2023-08-11T15:14:34Z

What does DebugDTRunner have to do with any of this? Its only job is to stop after the first failure, so it' pytest -x.

Sheila-nk · 2023-08-11T15:22:59Z

I opted to override its run function since it is a subclass of DTRunner and calls the run function of its base class.
Also I noticed pytest's doctest module uses the DebugRunner as well here.

Sheila-nk · 2023-08-11T15:25:03Z

I can override the DTRunner instead. I am still a bit unsure at which points either runner is called.

ev-br · 2023-08-11T15:30:50Z

Ah OK, this might make sense if pytest runs doctests one by one needs to override the reporting. Let's come back to this after the weekend.

EDIT: For usual doctests, DoctestRunner is the main thing which gets a list of doctests, runs them and reports the progress and results. DebugDoctestRunner stops after the first failure. DTRunner and DebugDTRunner follow this.

scpdt/impl.py

Sheila-nk · 2023-08-14T13:47:13Z

It would probably be a good idea to make continue_on_failure configurable through ini options.

Sheila-nk · 2023-08-14T14:03:01Z

@ev-br does nameerror_after_exception config attrbute serve the same purpose as the continue_on_failure variable? I can utilize that instead.

Edit: it actually does. So I will use nameerror_after_exception instead. Users can set it in conftest.py.

ev-br

LGTM.

Several comments in-line, none of them blocking. Please address those which are easy, and add TODO comments for the rest.

The pytest code style is a little baroque with local imports and local classes, can move them to the module level in a clean-up. We should at some point also look into minimizing copy-paste from pytest, this is also for some other day.

There are likely several bugs lurking, these should be smoked out with dogfooding on scipy documentation, which should be the next step IMO.

scpdt/impl.py

ev-br · 2023-08-14T14:01:04Z

scpdt/impl.py

@@ -375,7 +376,7 @@ class DebugDTRunner(DTRunner):
    Almost verbatim copy of `doctest.DebugRunner`.
    """
    def run(self, test, compileflags=None, out=None, clear_globs=True):
-        r = super().run(test, compileflags, out, False)
+        r = super().run(test, compileflags=compileflags, out=out, clear_globs=False)


kwargs do make it read much better, thank you!

Now that I look at this, why does it drop clear_globs argument? This is pre-existing of course, can we pass it here nonetheless? At this point, probably best to just add an TODO comment and move on though.

ev-br · 2023-08-14T14:01:36Z

scpdt/plugin.py

        optionflags = doctest.get_optionflags(self)
-        runner = doctest._get_runner(
+
+        # We plugin scpdt's `DebugDTRunner`


Suggested change

# We plugin scpdt's `DebugDTRunner`

# We plug in scpdt's `DebugDTRunner`

ev-br · 2023-08-14T14:04:16Z

scpdt/plugin.py

            verbose=False,
            optionflags=optionflags,
-            checker=_get_checker(),
-            continue_on_failure=doctest._get_continue_on_failure(self.config),


why are we dropping continue_on_failure?

continue_on_failure is a pytest ini option. Figured it would be best to use DTConfig's nameerror_after_exception attribute instead since both do the same thing.

#87 (comment) please make sure that this test passes with the plugin

ev-br · 2023-08-14T14:05:46Z

scpdt/plugin.py

+
+
+def _get_runner(checker, verbose, optionflags):
+    import doctest


Nit: there is probably no reason to hide imports; move it to the module level.

It's because of this error:

AttributeError: module '_pytest.doctest' has no attribute 'UnexpectedException'

ev-br · 2023-08-14T14:07:00Z

scpdt/plugin.py

+
+        """
+        Almost verbatim copy of `_pytest.doctest.PytestDoctestRunner`.


Please detail the differences.

scpdt/plugin.py

ev-br · 2023-08-14T14:11:41Z

scpdt/plugin.py

+            execution of the particular doctest
+            """
+            self.continue_on_failure = continue_on_failure


config argument is ignored, what is self.config below in self.config.user_context_mgr. This ties up to a question of where the global DTConfig lives.

ev-br · 2023-08-14T14:32:02Z

@ev-br does nameerror_after_exception config attrbute serve the same purpose as the continue_on_failure variable? I can utilize that instead.

Not sure actually. I don't know what continue_on_failure is;
The job of nameerror_after_exception originally comes from the scipy issue linked in the commit message of
88d8ed7 .

EDIT: the relevant test is
88d8ed7#diff-e8cfb69580bba75d7bb6f9acb8dc3bd180fb424fdf130732869e621004217492R101

Sheila-nk · 2023-08-14T18:17:02Z

@ev-br
I have made all the recommended changes except for:

the declaration of the PytestDTRunner class and import of doctest at the module level.
When I moved everything to the module level, it seems all references to doctest is from the _pytest module which brings up this error:

AttributeError: module '_pytest.doctest' has no attribute 'UnexpectedException'

I have moved the creation of a test-case for the Name Error after exception to a new issue. I am yet to figure out what the test case looks like but manual testing by setting the namerror_after_exception config attribute in conftest.py is working as expected.

ev-br · 2023-08-14T18:25:41Z

LGTM, thank you Sheila!

Let's not worry about cleanups at this stage, I'll do a holistic review later anyway.

Reviewed at scipy/scipy_doctest#87

run tests in context managers

4a71b67

Sheila-nk added enhancement New features w.r.t. the original refguide-check pytest-plugin labels Aug 11, 2023

ev-br reviewed Aug 14, 2023

View reviewed changes

scpdt/impl.py Outdated Show resolved Hide resolved

create special runner to run tests in contexts and handle reporting

2211869

Sheila-nk marked this pull request as ready for review August 14, 2023 13:47

Sheila-nk requested a review from ev-br August 14, 2023 13:47

use config attribute

eacfc08

ev-br reviewed Aug 14, 2023

View reviewed changes

Sheila-nk added 2 commits August 14, 2023 20:57

clean up

af12312

pass clear_globs as is

72b10b2

Sheila-nk mentioned this pull request Aug 14, 2023

Replace nameerror_after_exception by pytests's continue_on_failure #88

Open

ev-br merged commit b1a4b23 into scipy:main Aug 14, 2023

Sheila-nk mentioned this pull request Aug 14, 2023

configure pytest to use the DTRunner #84

Closed

cr313 added a commit to cr313/doctest-scipy that referenced this pull request Apr 19, 2024

Run tests in context managers (#87)

1cf3cdf

Reviewed at scipy/scipy_doctest#87

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests in context managers #87

Run tests in context managers #87

Sheila-nk commented Aug 11, 2023

Sheila-nk commented Aug 11, 2023

ev-br commented Aug 11, 2023 •

edited

Loading

Sheila-nk commented Aug 11, 2023

Sheila-nk commented Aug 11, 2023 •

edited

Loading

ev-br commented Aug 11, 2023 •

edited

Loading

Sheila-nk commented Aug 14, 2023

Sheila-nk commented Aug 14, 2023 •

edited

Loading

ev-br left a comment

ev-br Aug 14, 2023

ev-br Aug 14, 2023

ev-br Aug 14, 2023

Sheila-nk Aug 14, 2023

ev-br Aug 14, 2023

ev-br Aug 14, 2023

Sheila-nk Aug 14, 2023

ev-br Aug 14, 2023

ev-br Aug 14, 2023

ev-br commented Aug 14, 2023 •

edited

Loading

Sheila-nk commented Aug 14, 2023

ev-br commented Aug 14, 2023

	# We plugin scpdt's `DebugDTRunner`
	# We plug in scpdt's `DebugDTRunner`



		def _get_runner(checker, verbose, optionflags):
		import doctest


		"""
		Almost verbatim copy of `_pytest.doctest.PytestDoctestRunner`.

Run tests in context managers #87

Run tests in context managers #87

Conversation

Sheila-nk commented Aug 11, 2023

Sheila-nk commented Aug 11, 2023

ev-br commented Aug 11, 2023 • edited Loading

Sheila-nk commented Aug 11, 2023

Sheila-nk commented Aug 11, 2023 • edited Loading

ev-br commented Aug 11, 2023 • edited Loading

Sheila-nk commented Aug 14, 2023

Sheila-nk commented Aug 14, 2023 • edited Loading

ev-br left a 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

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

ev-br commented Aug 14, 2023 • edited Loading

Sheila-nk commented Aug 14, 2023

ev-br commented Aug 14, 2023

ev-br commented Aug 11, 2023 •

edited

Loading

Sheila-nk commented Aug 11, 2023 •

edited

Loading

ev-br commented Aug 11, 2023 •

edited

Loading

Sheila-nk commented Aug 14, 2023 •

edited

Loading

ev-br commented Aug 14, 2023 •

edited

Loading