GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111495: Add tests for PyList C API by rawwar · Pull Request #111562 · python/cpython

Conversation

@rawwar

@rawwar rawwar commented Oct 31, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Adding tests for PyList C API - Issue #111495

  • Issue: Add more C API tests #111495

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
rawwar added 2 commits October 31, 2023 21:37
@rawwar
init test c file
d43d9b2
@rawwar
single test added
119cd0e
@bedevere-app bedevere-app Bot mentioned this pull request Oct 31, 2023
Add more C API tests #111495
Closed
10 tasks done
rawwar added 5 commits October 31, 2023 22:43
@rawwar
fix lint
ccf1bff
@rawwar
fix list check tests
8d17ca6
@rawwar
lint
bd9415d
@rawwar
add 2 tests
17d60d1
@rawwar
add test
322eab9
@Eclips4
Eclips4 requested a review from serhiy-storchaka October 31, 2023 19:15
rawwar added 5 commits November 1, 2023 02:06
@rawwar
add getitem test
5c58022
@rawwar
test list_get_item
6b12f1f
@rawwar
add test for setitem
70e385f
@rawwar
add test for setitem
3166ba4
Signed-off-by: kalyanr <[email protected]>
@rawwar
Merge branch 'kalyan/test-capi-list' of github.com:rawwar/cpython int…
2a50fb8
…o kalyan/test-capi-list

Signed-off-by: kalyanr <[email protected]>
serhiy-storchaka
serhiy-storchaka reviewed Oct 31, 2023
View reviewed changes
Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/list.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/list.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev skirpichev mentioned this pull request Nov 1, 2023
gh-111495: add stub files for C API test modules #111586
Merged
@rawwar
Merge branch 'main' of github.com:rawwar/cpython into kalyan/test-cap…
1c30f3b
…i-list

Signed-off-by: kalyanr <[email protected]>
@rawwar

rawwar commented Nov 1, 2023

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka , Is adding {NULL}, to test_methods[] necessary? yesterday, i was able to run make without any issues. Today, after taking the pull from main, its been failing. below are the details

static PyMethodDef test_methods[] = {
    {"list_check", list_check, METH_O},
    {"list_check_exact", list_check_exact, METH_O},
    {"list_new", list_new, METH_O},
    {"list_size", list_size, METH_O},
};

Fails with

(base) kalyan@rawwar:~/oss/python-projects/cpython$ make
gcc  -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall    -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -fPIC -c ./Modules/_testcapi/list.c -o Modules/_testcapi/list.o
gcc -shared      Modules/_testcapimodule.o Modules/_testcapi/vectorcall.o Modules/_testcapi/vectorcall_limited.o Modules/_testcapi/heaptype.o Modules/_testcapi/abstract.o Modules/_testcapi/bytearray.o Modules/_testcapi/bytes.o Modules/_testcapi/unicode.o Modules/_testcapi/dict.o Modules/_testcapi/set.o Modules/_testcapi/list.o Modules/_testcapi/tuple.o Modules/_testcapi/getargs.o Modules/_testcapi/datetime.o Modules/_testcapi/docstring.o Modules/_testcapi/mem.o Modules/_testcapi/watchers.o Modules/_testcapi/long.o Modules/_testcapi/float.o Modules/_testcapi/complex.o Modules/_testcapi/numbers.o Modules/_testcapi/structmember.o Modules/_testcapi/exceptions.o Modules/_testcapi/code.o Modules/_testcapi/buffer.o Modules/_testcapi/pyatomic.o Modules/_testcapi/pyos.o Modules/_testcapi/file.o Modules/_testcapi/codec.o Modules/_testcapi/immortal.o Modules/_testcapi/heaptype_relative.o Modules/_testcapi/gc.o Modules/_testcapi/sys.o   -o Modules/_testcapi.cpython-313-x86_64-linux-gnu.so
[ERROR] Importing extension '_testcapi' failed!
Traceback (most recent call last):
  File "/home/kalyan/oss/python-projects/cpython/./Tools/build/check_extension_modules.py", line 412, in check_module_import
    bootstrap_load(spec)
  File "<frozen importlib._bootstrap>", line 960, in _load
  File "<frozen importlib._bootstrap>", line 915, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 813, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 1302, in create_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
ValueError: module functions cannot set METH_CLASS or METH_STATIC
Traceback (most recent call last):
  File "/home/kalyan/oss/python-projects/cpython/./Tools/build/check_extension_modules.py", line 483, in <module>
    main()
  File "/home/kalyan/oss/python-projects/cpython/./Tools/build/check_extension_modules.py", line 474, in main
    checker.check()
  File "/home/kalyan/oss/python-projects/cpython/./Tools/build/check_extension_modules.py", line 171, in check
    self.check_module_import(modinfo)
  File "/home/kalyan/oss/python-projects/cpython/./Tools/build/check_extension_modules.py", line 412, in check_module_import
    bootstrap_load(spec)
  File "<frozen importlib._bootstrap>", line 960, in _load
  File "<frozen importlib._bootstrap>", line 915, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 813, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 1302, in create_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
ValueError: module functions cannot set METH_CLASS or METH_STATIC
make: *** [Makefile:1177: checksharedmods] Error 1

But, when I include {NULL} like below, I am able to run make without any issues:

static PyMethodDef test_methods[] = {
    {"list_check", list_check, METH_O},
    {"list_check_exact", list_check_exact, METH_O},
    {"list_new", list_new, METH_O},
    {"list_size", list_size, METH_O},
    {NULL},
};
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 1, 2023
gh-111138: Test the public PyList C API #111582
Closed
rawwar added 2 commits November 1, 2023 14:28
@rawwar
add tests
392b3c6
Signed-off-by: kalyanr <[email protected]>
@rawwar
lint fix
eac8c67
Signed-off-by: kalyanr <[email protected]>
vstinner
vstinner reviewed Nov 1, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh! I wrote a very similar PR yesterday! I have a few more tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rawwar reacted with eyes emoji
All reactions
  • 👀 1 reaction
serhiy-storchaka
serhiy-storchaka reviewed Nov 1, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See also #111582. It would be nice if you with @vstinner unite your works.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Outdated
lst = [1, 2, NULL]
self.assertEqual(getitem(lst, 0), 1)
self.assertRaises(IndexError, getitem, lst, -1)
self.assertRaises(IndexError, getitem, lst, 10)

@serhiy-storchaka serhiy-storchaka Nov 1, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Add tests for index equal to the size of the list, PY_SSIZE_T_MIN, PY_SSIZE_T_MAX (imported from _testcapi).

All functions that have Py_ssize_t parameter should be tested with the following values: 0, size-1, -1, size, PY_SSIZE_T_MIN, PY_SSIZE_T_MAX.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rawwar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@rawwar rawwar Nov 1, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

For methods like slice, they accept multiple Py_ssize_t parameters. Should each of these parameters be tested with the values you mentioned?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_capi/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/list.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

rawwar and others added 5 commits November 1, 2023 15:36
@rawwar
add tests
0f40f6d
Signed-off-by: kalyanr <[email protected]>
@rawwar @serhiy-storchaka
Update Lib/test/test_capi/test_list.py
40e4022
Co-authored-by: Serhiy Storchaka <[email protected]>
@rawwar @serhiy-storchaka
Update Modules/_testcapi/list.c
d31fa57
Co-authored-by: Serhiy Storchaka <[email protected]>
@rawwar @serhiy-storchaka
Update Lib/test/test_capi/test_list.py
433351d
Co-authored-by: Serhiy Storchaka <[email protected]>
@rawwar @serhiy-storchaka
Update Lib/test/test_capi/test_list.py
34e915b
Co-authored-by: Serhiy Storchaka <[email protected]>
@rawwar

rawwar commented Nov 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@vstinner , What can we do now? I'm not sure If I should continue working on this PR? Although, I also added tests for Py_LIMITED_API ones as well.
I am also planning on contributing tests for PyTuple. let me know if I can go ahead with that

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rawwar
pr feedback fixes
0e340a1
Signed-off-by: kalyanr <[email protected]>
@bedevere-app bedevere-app Bot added the awaiting review label Nov 1, 2023
@rawwar
Update list.c
e486451
@rawwar
rawwar requested review from serhiy-storchaka and vstinner November 1, 2023 17:13
rawwar added 3 commits November 1, 2023 22:43
@rawwar
Merge branch 'main' into kalyan/test-capi-list
633951b
@rawwar
Merge branch 'main' into kalyan/test-capi-list
9afa50c
@rawwar
Merge branch 'main' into kalyan/test-capi-list
361b045
vstinner
vstinner reviewed Nov 8, 2023
View reviewed changes
Comment thread Modules/_testcapi/list.c Outdated
}
NULLABLE(obj);
NULLABLE(value);
RETURN_INT(PyList_SetSlice(obj, ilow, ihigh, Py_XNewRef(value)));

@vstinner vstinner Nov 8, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Py_XNewRef() is wrong and causes a reference leak.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_testcapi/list.c Outdated
}
NULLABLE(obj);
NULLABLE(value);
RETURN_INT(PyList_Append(obj, Py_XNewRef(value)));

@vstinner vstinner Nov 8, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Py_XNewRef() is wrong and causes a reference leak.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vstinner
vstinner reviewed Nov 8, 2023
View reviewed changes
Comment thread Modules/_testcapi/list.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
Update Modules/_testcapi/list.c
ca07ac9
vstinner
vstinner reviewed Nov 8, 2023
View reviewed changes
Comment thread Modules/_testcapi/list.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner and others added 3 commits November 8, 2023 15:57
@vstinner
Update Modules/_testcapi/list.c
dc039f1
@serhiy-storchaka
Merge branch 'main' into kalyan/test-capi-list
5ecfa40
@serhiy-storchaka
Minor fixes.
42a2a54
serhiy-storchaka
serhiy-storchaka approved these changes Nov 8, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you @rawwar.

I had too much minor suggestions, so I simply applied them myself.

Now it LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_capi/test_list.py Outdated
Comment on lines +116 to +117
self.assertRaises(TypeError, setitem, lst, 1.5, 10)
self.assertRaises(TypeError, setitem, 23, 'a', 5)

@serhiy-storchaka serhiy-storchaka Nov 8, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It only tests the wrapper, not the C API function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Nov 8, 2023
@vstinner vstinner added the skip news label Nov 8, 2023
@vstinner
vstinner merged commit a3903c8 into python:main Nov 8, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 8, 2023
@vstinner

vstinner commented Nov 8, 2023

Copy link
Copy Markdown
Member

I merged your PR @rawwar, thanks for adding tests for the PyList API!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka added the needs backport to 3.12 only security fixes label Nov 8, 2023
@miss-islington-app

miss-islington-app Bot commented Nov 8, 2023

Copy link
Copy Markdown

Thanks @rawwar for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 8, 2023
@rawwar @serhiy-storchaka
@vstinner @miss-islington
pythongh-111495: Add tests for PyList C API (pythonGH-111562)
3b384e7
(cherry picked from commit a3903c8)

Co-authored-by: Kalyan <[email protected]>
Signed-off-by: kalyanr <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@bedevere-app

bedevere-app Bot commented Nov 8, 2023

Copy link
Copy Markdown

GH-111861 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Nov 8, 2023
@vstinner vstinner mentioned this pull request Nov 8, 2023
C API: Consider adding a public PyList_Extend() function #111138
Closed
serhiy-storchaka added a commit that referenced this pull request Nov 8, 2023
@miss-islington @rawwar
@serhiy-storchaka @vstinner
[3.12] gh-111495: Add tests for PyList C API (GH-111562) (GH-111861)
759168a
(cherry picked from commit a3903c8)

Signed-off-by: kalyanr <[email protected]>
Co-authored-by: Kalyan <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@rawwar @serhiy-storchaka
@vstinner @aisk
pythongh-111495: Add tests for PyList C API (python#111562)
69c3ae3
Signed-off-by: kalyanr <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@rawwar @serhiy-storchaka
@vstinner @Glyphack
pythongh-111495: Add tests for PyList C API (python#111562)
e6ce4f2
Signed-off-by: kalyanr <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@rawwar @vstinner @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.