8000 bpo-40823: Use loadTestsFromTestCase() iso. makeSuite() in sqlite3 tests by erlend-aasland · Pull Request #20538 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-40823: Use loadTestsFromTestCase() iso. makeSuite() in sqlite3 tests #20538

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
merged 6 commits into from
Jan 7, 2021
Merged

bpo-40823: Use loadTestsFromTestCase() iso. makeSuite() in sqlite3 tests #20538

merged 6 commits into from
Jan 7, 2021

Conversation

erlend-aasland
Copy link
Contributor
@erlend-aasland erlend-aasland commented May 30, 2020 •
edited by bedevere-bot
Loading

@erlend-aasland erlend-aasland force-pushed the fix-issue-40823 branch 2 times, most recently from 5742333 to aaf8ec3 Compare November 17, 2020 22:24
berkerpeksag
berkerpeksag requested changes Jan 6, 2021
View reviewed changes
Copy link
Member
@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

I don't have a strong objection to this, but if we are going to drop Check prefix, I'd prefer using test_* instead of test*.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@erlend-aasland
Copy link
Contributor Author

I don't have a strong objection to this, but if we are going to drop Check prefix, I'd prefer using test_* instead of test*.

No problem. I'll rebase onto master and fix the naming.

Erlend E. Aasland added 2 commits January 6, 2021 20:34
Use loadTestsFromTestCase iso. makeSuite in Lib/sqlite3/test/
412506a 
Ref. issue 5846: unittest.makeSuite is deprecated
Add NEWS
0f3b5fd
erlend-aasland
erlend-aasland commented Jan 6, 2021
View reviewed changes
@erlend-aasland
8000 Copy link
Contributor Author

PTAL, @berkerpeksag. Rebased onto master with snake case method naming. IMHO, it looks really nice now.

@erlend-aasland
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

berkerpeksag
berkerpeksag requested changes Jan 6, 2021
View reviewed changes
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Erlend Egeberg Aasland added 3 commits January 6, 2021 22:19
Fix naming
fd4f651 
test_cursor_description_ctes_multiple_columns => test_cursor_description_cte_multiple_columns
Sort userfunctions.py suites and remove extra newline
1340b89
Fix NEWS entry
7208fc5
@erlend-aasland
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

@erlend-aasland
Copy link
Contributor Author
erlend-aasland commented Jan 6, 2021 •
edited
Loading

GitHub messed up the encoding again! See #20538 (comment). I must remember no to use the suggestion feature for these files. We should normalise the encodings:

$ file Lib/sqlite3/test/*.py
Lib/sqlite3/test/__init__.py:      empty
Lib/sqlite3/test/backup.py:        Python script text executable, ASCII text
Lib/sqlite3/test/dbapi.py:         Python script text executable, ISO-8859 text
Lib/sqlite3/test/dump.py:          Python script text executable, ASCII text
Lib/sqlite3/test/factory.py:       Python script text executable, ISO-8859 text
Lib/sqlite3/test/hooks.py:         Python script text executable, ISO-8859 text
Lib/sqlite3/test/regression.py:    Python script text executable, ISO-8859 text
Lib/sqlite3/test/transactions.py:  Python script text executable, ISO-8859 text
Lib/sqlite3/test/types.py:         Python script text executable, UTF-8 Unicode text
Lib/sqlite3/test/userfunctions.py: Python script text executable, UTF-8 Unicode text

@erlend-aasland
Copy link
Contributor Author
erlend-aasland commented Jan 6, 2021 •
edited
Loading

The same thing happened in #20448, and we ended up just converting Lib/sqlite3/test/userfunctions.py to UTF-8 in the same PR. I'd say we convert the rest right away. Agreed, @berkerpeksag?

@berkerpeksag
Copy link
Member

The same thing happened in #20448, and we ended up just converting Lib/sqlite3/test/userfunctions.py to UTF-8 in the same PR. I'd say we convert the rest right away. Agreed, @berkerpeksag?

I failed to see what's wrong here and #20448 has lots of comments. Why do you want to convert encodings of these files?

@erlend-aasland
Copy link
Contributor Author
erlend-aasland commented Jan 6, 2021 •
edited
Loading

I failed to see what's wrong here and #20448 has lots of comments. Why do you want to convert encodings of these files?

Commit fd4f651 was auto-generated by GitHub from the "suggestion" I set up in #20538 (comment). As you notice from the diff, GitHub silently converted the file from ISO-8859 encoding to UTF-8 encoding (ignoring the #-*- coding: iso-8859-1 -*- header), in addition to applying the "suggestion" I set up. Over at #20448 the same thing happened; Victor Stinner set up a "suggestion", which I applied, and then GitHub silently converted Lib/sqlite3/test/userfunctions.py from ISO-8859 to UTF-8. Instead of reverting the encoding change GitHub made, we decided to just remove the #-*- coding: iso-8859-1 -*- header, since GitHub obviously dislikes ISO-8859.

@berkerpeksag
Copy link
Member

Ah, I see now, thanks for the explanation! Personally, I prefer reverting the commit generated by GitHub and apply the suggested change manually. I don't like when companies try to impose their own practices (it doesn't mean they are bad practices) on a perfectly fine piece of code :)

@erlend-aasland
Copy link
Contributor Author

… or we can revert the encoding change and create an issue for normalising the encodings.

Ah, I see now, thanks for the explanation! Personally, I prefer reverting the commit generated by GitHub and apply the suggested change manually. I don't like when companies try to impose their own practices (it doesn't mean they are bad practices) on a perfectly fine piece of code :)

No problem. I'll open an issue for normalising the file encodings. Consistence FTW :)

@erlend-aasland
Copy link
Contributor Author

Reverted in f93a63b. PTAL, @berkerpeksag

berkerpeksag
berkerpeksag approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member
@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

LGTM!

@berkerpeksag berkerpeksag merged commit 849e339 into python:master Jan 7, 2021
@erlend-aasland erlend-aasland deleted the fix-issue-40823 branch January 7, 2021 00:05
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@adorilson
bpo-40823: Use loadTestsFromTestCase() iso. makeSuite() in sqlite3 te…
54608cc 
…sts (pythonGH-20538)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erlend-aasland @bedevere-bot @berkerpeksag @the-knights-who-say-ni
0