Skip to content
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 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

bpo-43853: Expand test suite for SQLite UDF's #27642

Merged
merged 16 commits into from Jan 26, 2022
Merged

bpo-43853: Expand test suite for SQLite UDF's #27642

merged 16 commits into from Jan 26, 2022

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 6, 2021 •
edited

  • Test that strings that contain embedded NULL characters are passed to UDF's
  • Consolidate "test param" tests to reduce the amount of boilerplate code
  • Test more error conditions when building UDF arguments, and returning UDF values
  • Handle PyFloat_AsDouble() errors

https://bugs.python.org/issue43853
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 6, 2021

FYI: Tests for aggregate functions are already in place (implemented in GH-27588).

@serhiy-storchaka, is the refactor commit acceptable? Code coverage is preserved.

serhiy-storchaka
serhiy-storchaka reviewed Aug 6, 2021
View changes
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 7, 2021

Ignore my comments about values which cause errors. I covered them in #27654. But please add more test cases for valid values of different types.

@erlend-aasland erlend-aasland mentioned this pull request Aug 7, 2021
Draft
@erlend-aasland erlend-aasland mentioned this pull request Aug 7, 2021
Merged
serhiy-storchaka
serhiy-storchaka reviewed Aug 8, 2021
View changes
Modules/_sqlite/connection.c Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/userfunctions.py Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 12, 2021

I've updated the PR. PTAL, @serhiy-storchaka.

@erlend-aasland erlend-aasland marked this pull request as draft Sep 14, 2021
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Sep 14, 2021 •
edited

Marked as draft, as I need to resolve some potential issues created by GH-27884.

Resolved: only test arguments passed to UDF's

@erlend-aasland erlend-aasland marked this pull request as ready for review Sep 15, 2021
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Sep 15, 2021

@serhiy-storchaka, please take a new look.

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 25, 2022

Thanks, @JelleZijlstra!

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Jan 26, 2022

@gvanrossum I think this one can be merged. (Though maybe Serhyi should review it again? It's been a few months and the change seems low risk.)

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 26, 2022

@erlend-aasland Do you want to land it?

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 26, 2022

@erlend-aasland Do you want to land it?

Yes, it would be nice to land this.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 26, 2022

Go ahead and land it!

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 26, 2022

Go ahead and land it!

I can't; I don't have the commit bit :)

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 26, 2022

What?! I thought you did. You should. Who's mentoring you?

@gvanrossum gvanrossum merged commit 3eb3b4f into python:main Jan 26, 2022
10 of 11 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Thanks @erlend-aasland for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Sorry, @erlend-aasland and @gvanrossum, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3eb3b4f270757f66c7fb6dcf5afa416ee1582a4b 3.10

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jan 26, 2022

Sorry @erlend-aasland and @gvanrossum, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 3eb3b4f270757f66c7fb6dcf5afa416ee1582a4b 3.9

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Jan 26, 2022

I can't; I don't have the commit bit :)

I was surprised when I learned that too, I feel like I've seen your name around CPython a lot :)

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 26, 2022

What?! I thought you did. You should. Who's mentoring you?

First @corona10, now @pablogsal 🙂

(BTW, I'll fix the backports.)

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 26, 2022

I was surprised when I learned that too, I feel like I've seen your name around CPython a lot :)

I've been around a year or two, IIRC :)

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 26, 2022

What?! I thought you did. You should. Who's mentoring you?

We have already offer the promotion a couple of times, we are working on it 😉

@erlend-aasland erlend-aasland deleted the sqlite-test-udf-str-with-null branch Jan 26, 2022
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jan 26, 2022
@erlend-aasland
[3.10] bpo-43853: Expand test suite for SQLite UDF's (pythonGH-27642).
f0fd0bd 
(cherry picked from commit 3eb3b4f)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 30, 2022

GH-31030 is a backport of this pull request to the 3.10 branch.

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jan 30, 2022

Backporting to 3.9 from GH-31030.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@berkerpeksag berkerpeksag

@serhiy-storchaka serhiy-storchaka

Assignees

@gvanrossum gvanrossum

Labels
CLA signed skip news
Projects
None yet
Milestone
No milestone
8 participants
@erlend-aasland @serhiy-storchaka @JelleZijlstra @gvanrossum @miss-islington @pablogsal @bedevere-bot @the-knights-who-say-ni