GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-105539: Explict resource management for connection objects in sqlite3 tests by erlend-aasland · Pull Request #108017 · python/cpython

Conversation

erlend-aasland commented Aug 16, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Issue: Emit resource warning if sqlite3 fails to close the database #105539

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-105539: Explict resource management for connection objects i…

6af3de4

…n sqlite3 tests

erlend-aasland requested a review from berkerpeksag as a code owner

August 16, 2023 13:09

erlend-aasland added the skip news label

Aug 16, 2023

bedevere-bot mentioned this pull request

Aug 16, 2023

Emit resource warning if sqlite3 fails to close the database #105539

Closed

bedevere-bot added awaiting core review tests Tests in the Lib/test dir labels

Aug 16, 2023

erlend-aasland mentioned this pull request

Aug 16, 2023

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly #108015

Merged

erlend-aasland commented Aug 16, 2023

Copy link

Copy Markdown

Contributor Author

This PR either uses the memory_database() resource management helper from Lib/test/test_sqlite3/test_dbapi.py, or introduces explicit resource management via setUp()/tearDown() test case methods.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Aug 16, 2023

erlend-aasland requested a review from sobolevn

August 16, 2023 13:46

vstinner reviewed

Aug 16, 2023

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I would suggest moving memory_database() to a new utils.py file. For me, it's surprising that tests import other tests. See for example Lib/test/test_asyncio/utils.py.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner approved these changes

Aug 16, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels

Aug 16, 2023

vstinner commented Aug 16, 2023

Copy link

Copy Markdown

Member

I don't know the DB API, but I'm surprised that with connect() as db: ... doesn't close the database :-(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn reviewed

Aug 16, 2023

View reviewed changes

Comment thread

Lib/test/test_sqlite3/test_hooks.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_sqlite3/test_userfunctions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_sqlite3/test_regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_sqlite3/test_regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_sqlite3/test_regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 2 commits

August 16, 2023 21:30


          Pull in main

ce62430


          Address review

a92c095

- Move test utility functions to util.py
- Use memory database mixin
- Add check() helper for closed connection tests
- Address other remarks by Nikita

erlend-aasland commented Aug 16, 2023

Copy link

Copy Markdown

Contributor Author

I think all your remarks are addressed now; PTAL :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner approved these changes

Aug 16, 2023

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

If you want, you can replace from test.test_sqlite3.util import with from .util import.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with rocket emoji

All reactions

🚀 1 reaction


          neater imports

353c58d

erlend-aasland commented Aug 16, 2023

Copy link

Copy Markdown

Contributor Author

Thanks for the review, Victor and Nikita!

vstinner reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn approved these changes

Aug 17, 2023

View reviewed changes

sobolevn left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first sqlite3 PR review ✅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with rocket emoji

All reactions

🚀 1 reaction

erlend-aasland removed needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Aug 17, 2023

erlend-aasland merged commit 1344cfa into python:main

Aug 17, 2023

bedevere-bot removed the awaiting merge label

Aug 17, 2023

erlend-aasland deleted the sqlite/explicit-resource-handling-in-tests branch

August 17, 2023 06:46

erlend-aasland linked an issue

Aug 23, 2023

that may be closed by this pull request

Emit resource warning if sqlite3 fails to close the database #105539

Closed

Reviewers

vstinner

vstinner approved these changes

sobolevn

sobolevn approved these changes

berkerpeksag

Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests

Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Emit resource warning if sqlite3 fails to close the database

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

erlend-aasland commented Aug 16, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Aug 16, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Aug 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Aug 16, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Aug 16, 2023

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erlend-aasland commented Aug 16, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.