Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
erlend-aasland merged 4 commits into from
Aug 17, 2023
Merged

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

erlend-aasland merged 4 commits into from
Aug 17, 2023

Conversation

@erlend-aasland
Copy link
Contributor

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

@bedevere-bot bedevere-bot mentioned this pull request Aug 16, 2023
Closed
@bedevere-bot bedevere-bot added awaiting core review tests Tests in the Lib/test dir labels Aug 16, 2023
@erlend-aasland erlend-aasland mentioned this pull request Aug 16, 2023
Merged
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 16, 2023

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 erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Aug 16, 2023
vstinner
vstinner reviewed Aug 16, 2023
View reviewed changes
Copy link
Member

@vstinner vstinner left a 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.

@vstinner
Copy link
Member

vstinner commented Aug 16, 2023

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

sobolevn
sobolevn reviewed Aug 16, 2023
View reviewed changes
@erlend-aasland
Pull in main
ce62430
@erlend-aasland
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
Copy link
Contributor Author

erlend-aasland commented Aug 16, 2023

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

vstinner
vstinner approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@vstinner vstinner left a 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.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 16, 2023

Thanks for the review, Victor and Nikita!

sobolevn
sobolevn approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a 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 ✅

@erlend-aasland 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 erlend-aasland merged commit 1344cfa into python:main Aug 17, 2023
@erlend-aasland erlend-aasland deleted the sqlite/explicit-resource-handling-in-tests branch August 17, 2023 06:46
@erlend-aasland 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@sobolevn sobolevn sobolevn approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

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

@erlend-aasland @vstinner @sobolevn @bedevere-bot