gh-111726: Explicitly close database connections in sqlite3 doctests by sobolevn · Pull Request #111730 · python/cpython · GitHub

My logic behind this was:

Never add .close() to .. doctest::, because it distracts people from the core example (except for cases, where it was already present)
Always add .close() to .. testcode::, because one code part is better than two

Issue: Doc/library/sqlite3.rst has multiple doctest warnings #111726

📚 Documentation preview 📚: https://cpython-previews--111730.org.readthedocs.build/

There are still some left:

 Document: library/sqlite3
-------------------------
Exception ignored in: <sqlite3.Connection object at 0x7f4d99de1590>
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/functools.py", line 36, in update_wrapper
    def update_wrapper(wrapper,
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x7f4d99de1590>
Exception ignored in: <sqlite3.Connection object at 0x7f4d99de3ce0>
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/functools.py", line 36, in update_wrapper
    def update_wrapper(wrapper,
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x7f4d99de3ce0>
Exception ignored in: <sqlite3.Connection object at 0x7f4d9dc24160>
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/functools.py", line 36, in update_wrapper
    def update_wrapper(wrapper,
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x7f4d9dc24160>
Exception ignored in: <sqlite3.Connection object at 0x7f4d9dc246b0>
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/functools.py", line 36, in update_wrapper
    def update_wrapper(wrapper,
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x7f4d9dc246b0>
4 items passed all tests:
  72 tests in default
   3 tests in sqlite3.cursor
   3 tests in sqlite3.limits
   8 tests in sqlite3.trace
86 tests in 4 items.
86 passed and 0 failed.
Test passed.
1 items passed all tests:
   9 tests in default (cleanup code)
9 tests in 1 items.
9 passed and 0 failed.
Test passed.
Exception ignored in sys.unraisablehook: <function debug at 0x7f4da10aec90>
Traceback (most recent call last):
  File "<doctest sqlite3.trace[4]>", line 2, in debug
AttributeError: 'sqlite3.Connection' object has no attribute '__name__'
Exception ignored in sys.unraisablehook: <function debug at 0x7f4da10aec90>
Traceback (most recent call last):
  File "<doctest sqlite3.trace[4]>", line 2, in debug
AttributeError: 'sqlite3.Connection' object has no attribute '__name__'

One more thing: when running locally I got these files in my Doc/ folder:

» git status
On branch issue-111735
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        new file:   Doc/backup.db
        new file:   Doc/dump.sql
        new file:   Doc/example.db
        modified:   Doc/library/turtle.rst
        new file:   Doc/tutorial.db

One more thing: when running locally I got these files in my Doc/ folder:
» git status

On branch issue-111735

Changes to be committed:

Yeah, we don't clean up temporary files in the doctests. We should, though.

@erlend-aasland do you have any idea why do we still have ResourceWarnings? I've looked through the code multiple times, but I cannot find leaking resources.

It is a shame that we cannot debug it :(

@erlend-aasland do you have any idea why do we still have ResourceWarnings? I've looked through the code multiple time, but I cannot find leaking resources.

I'll have a look tomorrow.

Ok, looks like I understood the problem.

Here's the sample code that does this (it is in sqlite3.rst):

      >>> sqlite3.enable_callback_tracebacks(True)
      >>> con = sqlite3.connect(":memory:")
      >>> def evil_trace(stmt):
      ...     5/0
      ...
      >>> con.set_trace_callback(evil_trace)
      >>> def debug(unraisable):
      ...     print(f"{unraisable.exc_value!r} in callback {unraisable.object.__name__}")
      ...     print(f"Error message: {unraisable.err_msg}")
      >>> import sys
      >>> sys.unraisablehook = debug
      >>> cur = con.execute("SELECT 1")
      ZeroDivisionError('division by zero') in callback evil_trace
      Error message: None

There are still lots of warnings to fix:

Document: library/sqlite3
-------------------------
4 items passed all tests:
  71 tests in default
   3 tests in sqlite3.cursor
   3 tests in sqlite3.limits
   7 tests in sqlite3.trace
84 tests in 4 items.
84 passed and 0 failed.
Test passed.
2 items passed all tests:
  12 tests in default (cleanup code)
   1 tests in sqlite3.trace (cleanup code)
13 tests in 2 items.
13 passed and 0 failed.
Test passed.
Exception ignored in: <sqlite3.Connection object at 0x127adbf00>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x127adbf00>
Exception ignored in: <sqlite3.Connection object at 0x12740ce20>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x12740ce20>
Exception ignored in: <sqlite3.Connection object at 0x12740ef10>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x12740ef10>
Exception ignored in: <sqlite3.Connection object at 0x1242f9bf0>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x1242f9bf0>
Exception ignored in: <sqlite3.Connection object at 0x123539590>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x123539590>

Document: library/hashlib
-------------------------
1 items passed all tests:
  58 tests in default
58 tests in 1 items.
58 passed and 0 failed.
Test passed.

Document: library/binascii
--------------------------
1 items passed all tests:
   5 tests in default
5 tests in 1 items.
5 passed and 0 failed.
Test passed.
Exception ignored in: <sqlite3.Connection object at 0x1242faad0>
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Doc/venv/lib/python3.13/site-packages/docutils/nodes.py", line 387, in __new__
    def __new__(cls, data, rawsource=None):
ResourceWarning: unclosed database in <sqlite3.Connection object at 0x1242faad0>

Now I give up :)
No ideas left.

@erlend-aasland do you have any idea why do we still have ResourceWarnings?

You can run tests with tracemalloc to see where the object triggering ResourceWarnings was created: https://docs.python.org/dev/library/devmode.html#resourcewarning-example

Never add .close() to .. doctest::, because it distracts people from the core example (except for cases, where it was already present)

I would prefer to make it explicit that calling close() is recommended. That's why ResourceWarning is emitted if it's not called.

I would prefer to make it explicit that calling close() is recommended. That's why ResourceWarning is emitted if it's not called.

We should indeed put more emphasis on that in the docs, but there is no reason to do it in every example. There's a lot of examples in the sqlite3 docs, and filling each and everyone of them with best practises is IMO not a good idea for well written docs :)

LGTM. But I don't get the two :skipif: True. Does it mean that the cleanup is never executed? If yes, just remove it, no?

@erlend-aasland: Are you ok with this PR?

This reverts commit f6414d8.

LGTM

I'm not sure what's going on with this PR. Once I saw "os.unlink()" calls, then they disappears. Does @sobolevn or @erlend-aasland plan to propose a following PR to call "os.unlink()"? I proposed using support.os_helper.unlink() which is more reliable when used in tests.

I'm not sure what's going on with this PR. Once I saw "os.unlink()" calls, then they disappears. Does @sobolevn or @erlend-aasland plan to propose a following PR to call "os.unlink()"? I proposed using support.os_helper.unlink() which is more reliable when used in tests.

They were fixed by Hugo in #117604, as I already said in #111730 (comment).

support.os_helper.unlink is not used in any doctests at the moment. Perhaps we should rewrite all relevant doctests to use support.os_helper.unlink?

@vstinner: Sphinx recommends using its own machinery (doctest_global_cleanup and testcleanup) for cleaning up temporary doctest files:

You can use this to e.g. remove any temporary files that the tests leave behind.

We cannot land this yet, there are still warnings to be fixed, as Nikita said in #111730 (comment).

They were fixed by Hugo in #117604,

Great!

as I already said in #111730 (comment).

Oh, I'm lost in GitHub UI which hides comments when a comment is "solved".

support.os_helper.unlink is not used in any doctests at the moment. Perhaps we should rewrite all relevant doctests to use support.os_helper.unlink?

Nah, it's not worth it, os.unlink() is fine. It was just a comment on a review.

We cannot land this yet, there are still warnings to be fixed, as Nikita said in #111730 (comment).

I suggest to merge these fixes to make the situation clearer, since I'm lost in the lost history of this PR. You can, just remove "all" from the PR title :-)

But it's up to you, if you want to first really fix all warnings.

LGTM

…stency with the doctest and testcleanup directives

@sobolevn, @vstinner: I fixed the remaining issues with f170f7c. It seems Sphinx has a problem with doctest and following testcleanup. I chose to fix the issues by adding more explicit .close()s to doctests. It is not bad to promote good behaviour more explicitly.

cc. @AA-Turner

Sorry it took so long, Nikita!

…tests (pythonGH-111730) (cherry picked from commit a770266) Co-authored-by: Nikita Sobolev <mail@sobolevn.me> Co-authored-by: Erlend E. Aasland <erlend@python.org>

GH-117630 is a backport of this pull request to the 3.12 branch.

@erlend-aasland thanks a lot for your help and for taking this over! 🤝

…ctests (GH-111730) (#117630) (cherry picked from commit a770266) Co-authored-by: Nikita Sobolev <mail@sobolevn.me> Co-authored-by: Erlend E. Aasland <erlend@python.org>

…tests (python#111730) Co-authored-by: Erlend E. Aasland <erlend@python.org>

pythongh-111726 : Fix doctest warnings in Doc/library/sqlite3.rst

3fbdbc7

bedevere-app Bot added docs Documentation in the Doc dir skip news labels Nov 4, 2023

bedevere-app Bot mentioned this pull request Nov 4, 2023

Doc/library/sqlite3.rst has multiple doctest warnings #111726

Closed

sobolevn marked this pull request as ready for review November 4, 2023 09:47

sobolevn requested review from berkerpeksag and erlend-aasland as code owners November 4, 2023 09:47

bedevere-app Bot added the awaiting review label Nov 4, 2023

sobolevn added 2 commits November 4, 2023 13:04

Typo

2bdd8b2

More closes

faea5b2

More closes

89cd0ca

sobolevn mentioned this pull request Nov 7, 2023

Decide what should we do with doctests in CI #111704

Open

Cleanup temp files

6df8ec5

Restore unraisable hook

9113b01

Use correct test names

9c119d5

vstinner reviewed Nov 10, 2023

View reviewed changes

Comment thread Doc/library/sqlite3.rst Outdated

Merge branch 'main' into issue-111726

c13460f

vstinner reviewed Dec 7, 2023

View reviewed changes

Comment thread Doc/library/sqlite3.rst Outdated

vstinner approved these changes Dec 7, 2023

View reviewed changes

erlend-aasland added 2 commits April 8, 2024 09:06

Revert "Don't test load ext stuff since we cannot skip it conditionally"

9cef674

This reverts commit f6414d8.

Remove changes done by pythongh-117623

fbf5c6c

vstinner approved these changes Apr 8, 2024

View reviewed changes

erlend-aasland marked this pull request as draft April 8, 2024 07:51

bedevere-app Bot removed the awaiting merge label Apr 8, 2024

erlend-aasland changed the title gh-111726: Explicitly close all database connections in sqlite3 doctests gh-111726: Explicitly close database connections in sqlite3 doctests Apr 8, 2024

Pull in main

3a0e72b

erlend-aasland marked this pull request as ready for review April 8, 2024 08:13

bedevere-app Bot added the awaiting core review label Apr 8, 2024

vstinner approved these changes Apr 8, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 8, 2024

Fix the remaining resource warnings; looks like Sphinx has an inconsi…

f170f7c

…stency with the doctest and testcleanup directives

erlend-aasland enabled auto-merge (squash) April 8, 2024 09:14

erlend-aasland merged commit a770266 into python:main Apr 8, 2024

This comment was marked as outdated.

Sign in to view

bedevere-app Bot removed the awaiting merge label Apr 8, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Apr 8, 2024

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-111726 : Explicitly close database connections in sqlite3 doc…

6f60713

…tests (python#111730) Co-authored-by: Erlend E. Aasland <erlend@python.org>

gh-111726: Explicitly close database connections in sqlite3 doctests by sobolevn · Pull Request #111730 · python/cpython · GitHub

Repository navigation

gh-111726: Explicitly close database connections in sqlite3 doctests - #111730

gh-111726: Explicitly close database connections in sqlite3 doctests#111730erlend-aasland merged 18 commits intopython:mainpython/cpython:mainfrom sobolevn:issue-111726sobolevn/cpython:issue-111726Copy head branch name to clipboard

Conversation

sobolevn commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Nov 4, 2023

Uh oh!

sobolevn commented Nov 4, 2023

Uh oh!

erlend-aasland commented Nov 4, 2023

Uh oh!

sobolevn commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Nov 4, 2023

Uh oh!

sobolevn commented Nov 7, 2023

Uh oh!

sobolevn commented Nov 7, 2023

Uh oh!

sobolevn commented Nov 7, 2023

Uh oh!

vstinner commented Nov 10, 2023

Uh oh!

Uh oh!

vstinner commented Nov 10, 2023

Uh oh!

erlend-aasland commented Dec 6, 2023

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 8, 2024

Uh oh!

erlend-aasland commented Apr 8, 2024

Uh oh!

erlend-aasland commented Apr 8, 2024

Uh oh!

erlend-aasland commented Apr 8, 2024

Uh oh!

vstinner commented Apr 8, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Apr 8, 2024

Uh oh!

erlend-aasland commented Apr 8, 2024

Uh oh!

This comment was marked as outdated.

bedevere-app Bot commented Apr 8, 2024

Uh oh!

sobolevn commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-111726: Explicitly close database connections in sqlite3 doctests#111730
erlend-aasland merged 18 commits into
python:mainfrom
sobolevn:issue-111726

sobolevn commented Nov 4, 2023 •
edited
Loading

sobolevn commented Nov 4, 2023 •
edited
Loading

sobolevn commented Apr 8, 2024 •
edited
Loading