Skip to content

Decide what should we do with doctests in CI #111704

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

Open
sobolevn opened this issue Nov 3, 2023 · 8 comments
Open

Decide what should we do with doctests in CI #111704

sobolevn opened this issue Nov 3, 2023 · 8 comments
Assignees
Labels
build The build process and cross-build docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

sobolevn commented Nov 3, 2023

Bug report

Initial PR and discussion: #111682

So, here's the problem:

Example 1::

>>> 1 + 2
3

Example 2:

.. doctest::

>>> 1 + 2
3

We have two tools: doctest (builtin module) and sphinx (3rd party) that can run these samples as doctests. The problem is that they define "doctest" term differently:

  • doctest module says that both of these examples are doctests
  • sphinx says that only example with .. doctest:: directive is a doctest

This is problematic, because not all example are actually executed in CI, right now we have a CI job that uses Sphinx. So, in the result of this we have broken doctest (1st meaning) examples in the docs.

Examples:

What can we do?

Options:

  1. Add .. doctest:: directives everywhere, but I think it is too much work and people will forget to do this
  2. Try to use doctest_test_doctest_blocks, see docs https://www.sphinx-doc.org/en/master/usage/extensions/doctest.html This is what I plan to do locally and I will send a PR with the results. I fear that quite a lot of doctests might be broken
  3. Add doctests to test_* files using something like
def load_tests(loader, tests, pattern):
    tests.addTests(doctest.DocFileSuite(
        os.path.join(REPO_ROOT, 'Doc/library/LIBNAME.rst'),
        module_relative=False,
    ))
    return tests

The last option is also problematic, because right now we can only run doctests in CI, when PR only touches Doc/ folder, I am not sure that it will be easy with this setup.

CC @AlexWaygood @AA-Turner

Linked PRs

@sobolevn sobolevn added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir docs Documentation in the Doc dir build The build process and cross-build labels Nov 3, 2023
@sobolevn sobolevn self-assigned this Nov 3, 2023
@sobolevn sobolevn changed the title Decide what should we do with doctests Decide what should we do with doctests in CI Nov 3, 2023
@sobolevn
Copy link
Member Author

sobolevn commented Nov 3, 2023

I hope that I will send the first PR today / tomorrow, but I am a bit sick :(

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 3, 2023

Option (2) is definitely attractive. Just two concerns from me:

  1. How long will this make the Doctest CI job? It already takes 10 minutes to run all CPython's doctests via Sphinx; I worry that this option could make it significantly slower, by dramatically increasing the number of doctests run in CI. This is hypothetical, though; I might be wrong!
  2. I agree with you that there'll probably be quite a few that fail at first. If we could figure out a way to tackle the errors incrementally, like what we've been doing with the Sphinx nitpicks, that would be ideal. Would it be possible to apply doctest_test_doctest_blocks only to certain files? (Cc. @AA-Turner again as the Sphinx expert here!)

@AlexWaygood
Copy link
Member

Also, rest up and get well soon!

@hugovk
Copy link
Member

hugovk commented Nov 3, 2023

Option 4: Modify Sphinx to run both types?

But I'm also concerned about the doctest taking longer. Of the three docs jobs, doctest is the longest at 10 minutes (= 3m setup/building + 7m doctest) compared to 2.5 minutes each for the other two:

image

Can we do them in parallel instead of sequential? Can the doctest module run them quicker?

@sobolevn
Copy link
Member Author

sobolevn commented Nov 3, 2023

Modify Sphinx to run both types?

This is option 2 :)

@sobolevn
Copy link
Member Author

sobolevn commented Nov 7, 2023

After working with make doctest for a while, here are my thoughts:

  1. It does not allow to even run a single file (or maybe I cannot find the docs about it). This causes a lot of problems when debugging tests
  2. It uses stuff that is fundamentally incompatible with -m doctest, like testsetup
  3. It is hard to clean things up after usage (see gh-111726: Explicitly close database connections in sqlite3 doctests #111730), because you have to separate cleanup from the test itself (to not show it to user):
.. testcleanup::

   import os
   os.remove("tutorial.db")
  1. If you want to skip some file, you have to:

I didn't like this tool at all.
It might be useful for small projects, but not for big ones.

@AlexWaygood
Copy link
Member

  • Add :skipIf: TO ALL DOCTESTS in a file (see

Can't you just not add .. doctest to the REPL snippets in the documentation that you don't want to be treated as doctests? (Though I know we'd need to change that if we were to switch to a system where sphinx automatically treated all REPL snippets as doctests, regardless of whether they have the .. doctest directive above them.)

@sobolevn
Copy link
Member Author

sobolevn commented Nov 7, 2023

Can't you just not add .. doctest

Turns out that the answer is "no".
See #111735
It is a bit of a mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants