FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-105539: Explict resource management for connection objects in sqlite3 tests by erlend-aasland · Pull Request #108017 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-105539: Explict resource management for connection objects in sqlite3 tests - #108017

Merged
erlend-aasland merged 4 commits into
python:mainfrom
erlend-aasland:sqlite/explicit-resource-handling-in-tests
Aug 17, 2023
ConversationCommits (4)ChecksFiles changed
Merged

gh-105539: Explict resource management for connection objects in sqlite3 tests#108017
erlend-aasland merged 4 commits into
python:mainfrom
erlend-aasland:sqlite/explicit-resource-handling-in-tests

Conversation

erlend-aasland commented Aug 16, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-bot mentioned this pull request Aug 16, 2023
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
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.

erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Aug 16, 2023
vstinner reviewed Aug 16, 2023
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Lib/test/test_sqlite3/test_dbapi.py Outdated

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 :-(

sobolevn reviewed Aug 16, 2023
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_hooks.py Outdated
Comment thread Lib/test/test_sqlite3/test_userfunctions.py Outdated
Comment thread Lib/test/test_sqlite3/test_regression.py Outdated
Comment thread Lib/test/test_sqlite3/test_regression.py
Comment thread Lib/test/test_sqlite3/test_regression.py
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 :)

vstinner approved these changes Aug 16, 2023
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

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

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the review, Victor and Nikita!

sobolevn approved these changes Aug 17, 2023
View reviewed changes

sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My first sqlite3 PR review ✅

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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
sobolevn sobolevn approved these changes
berkerpeksag Awaiting requested review from berkerpeksag

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

4 participants

Back | FazBrowse Home | New Git URL