Skip to content

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

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

Copy link
Copy Markdown
Contributor

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

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
ContributorAuthor

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-aaslanderlend-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

@vstinnervstinner 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.

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 threadLib/test/test_sqlite3/test_dbapi.py Outdated
@vstinner

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

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
ContributorAuthor

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

vstinner
vstinner approved these changes Aug 16, 2023
View reviewed changes

@vstinnervstinner 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.

LGTM.

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

@erlend-aasland

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review, Victor and Nikita!

sobolevn
sobolevn approved these changes Aug 17, 2023
View reviewed changes

@sobolevnsobolevn 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.

My first sqlite3 PR review ✅

@erlend-aaslanderlend-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:mainAug 17, 2023
@erlend-aasland
erlend-aasland deleted the sqlite/explicit-resource-handling-in-tests branch August 17, 2023 06:46
@erlend-aaslanderlend-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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes
@sobolevnsobolevnsobolevn approved these changes
@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag

Assignees

No one assigned

Labels

skip newstestsTests 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