Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

erlend-aasland
Copy link
Contributor

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

@erlend-aasland
Copy link
ContributorAuthor

This PR either uses thememory_database() resource management helper fromLib/test/test_sqlite3/test_dbapi.py, or introduces explicit resource management viasetUp()/tearDown() test case methods.

Copy link
Member

@vstinnervstinner 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 newutils.py file. For me, it's surprising that tests import other tests. See for exampleLib/test/test_asyncio/utils.py.

@vstinner
Copy link
Member

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

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

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

Copy link
Member

@vstinnervstinner 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 replacefrom test.test_sqlite3.util import withfrom .util import.

erlend-aasland reacted with rocket emoji
@erlend-aasland
Copy link
ContributorAuthor

Thanks for the review, Victor and Nikita!

vstinner reacted with thumbs up emoji

Copy link
Member

@sobolevnsobolevn left a comment

Choose a reason for hiding this comment

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

My firstsqlite3 PR review ✅

erlend-aasland reacted with rocket emoji
@erlend-aaslanderlend-aasland removed needs backport to 3.11only security fixes needs backport to 3.12only security fixes labelsAug 17, 2023
@erlend-aaslanderlend-aasland merged commit1344cfa intopython:mainAug 17, 2023
@erlend-aaslanderlend-aasland deleted the sqlite/explicit-resource-handling-in-tests branchAugust 17, 2023 06:46
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 berkerpeksagberkerpeksag is a code owner

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
[8]ページ先頭
©2009-2025 Movatter.jp