Skip to content

bpo-44980: fix test_constructor to return None value #27898

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 1 commit into from
Aug 23, 2021
Merged

bpo-44980: fix test_constructor to return None value #27898

merged 1 commit into from
Aug 23, 2021

Conversation

akulakov
Copy link
Contributor

@akulakov akulakov commented Aug 22, 2021 •
edited
Loading

Update test_constructor() to conform to deprecation in #27748

https://bugs.python.org/issue44980

@akulakov
Copy link
Contributor Author

It might also be good to test contents of the null tuple?

@ambv
Copy link
Contributor

ambv commented Aug 22, 2021

You could but since we weren't looking at those before, this is new behavior that isn't necessarily the point of this PR.

@ambv ambv added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Aug 22, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ambv for commit 0d986418e50ab4f029845fa115976a5837a0a450 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 22, 2021
@ambv
Copy link
Contributor

ambv commented Aug 22, 2021

"test-with-buildbots" will test for refleaks for us.

serhiy-storchaka
serhiy-storchaka reviewed Aug 22, 2021
View reviewed changes
Modules/_testcapimodule.c Outdated
Comment on lines 2388 to 2389
Py_XDECREF(tuple);
Py_RETURN_NONE;
Copy link
Member

Choose a reason for hiding this comment

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

If tuple is NULL, the result should be NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should wait for buildbots to finish to push this change right?

Copy link
Member

Choose a reason for hiding this comment

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

No, you do not need to wait for buildbots.

@akulakov
Copy link
Contributor Author

Refleaks bot failure seems unrelated:

2:10:42 load avg: 5.18 [ 58/428/1] test_mailbox failed (1 failure) (2 hour 31 sec) -- running: test_gzip (1 hour 44 min), test_cmd_line (1 hour 39 min), test_argparse (1 hour 39 min)
beginning 6 repetitions
123456
..test test_mailbox failed -- Traceback (most recent call last):
File "D:\buildarea\pull_request.ware-win81-release.refleak\build\lib\test\test_mailbox.py", line 411, in test_set_item
self.assertEqual(len(self._box), 2)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 3 != 2

serhiy-storchaka
serhiy-storchaka reviewed Aug 23, 2021
View reviewed changes
Modules/_testcapimodule.c Outdated
@@ -2383,9 +2383,13 @@ test_null_strings(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyObject *o1 = PyObject_Str(NULL), *o2 = PyObject_Str(NULL);
PyObject *tuple = PyTuple_Pack(2, o1, o2);
if (tuple == NULL) {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

o1 and o2 may be leaked here.

Modules/_testcapimodule.c Outdated
Comment on lines 2388 to 2389
Py_XDECREF(tuple);
Py_RETURN_NONE;
Copy link
Member

Choose a reason for hiding this comment

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

No, you do not need to wait for buildbots.

serhiy-storchaka
serhiy-storchaka reviewed Aug 23, 2021
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Looking at the history of test_null_strings I decided that it should be completely rewritten. #27904.

serhiy-storchaka
serhiy-storchaka reviewed Aug 23, 2021
View reviewed changes
Modules/_testcapimodule.c Outdated
@@ -2385,7 +2385,11 @@ test_null_strings(PyObject *self, PyObject *Py_UNUSED(ignored))
PyObject *tuple = PyTuple_Pack(2, o1, o2);
Py_XDECREF(o1);
Py_XDECREF(o2);
return tuple;
Py_XDECREF(tuple);
if (tuple == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

BTW, comparing a pointer to freed memory with NULL is undefined behavior. Py_DECREF(tuple) should be after if (tuple == NULL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I've forgotten what I knew of C; thanks for reviewing and catching this!

@akulakov akulakov changed the title bpo-44980: fix two tests to return None rather than other values bpo-44980: fix test_constructor to return None value Aug 23, 2021
@ambv ambv force-pushed the 44980-Fix-two-tests-returning-nonnull-values branch from 74de888 to 87c0f83 Compare August 23, 2021 18:27
@ambv
Copy link
Contributor

ambv commented Aug 23, 2021

I rebased on main.

@ambv ambv added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Aug 23, 2021
@ambv ambv merged commit 27b761a into python:main Aug 23, 2021
@miss-islington
Copy link
Contributor

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 23, 2021
@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 23, 2021
@akulakov @miss-islington
bpo-44980: fix test_constructor to return None value (pythonGH-27898)
4cc8bdd 
(cherry picked from commit 27b761a)

Co-authored-by: andrei kulakov <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 23, 2021
@akulakov @miss-islington
bpo-44980: fix test_constructor to return None value (pythonGH-27898)
347be24 
(cherry picked from commit 27b761a)

Co-authored-by: andrei kulakov <[email protected]>
@bedevere-bot
Copy link

GH-27914 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Aug 23, 2021
@akulakov
Copy link
Contributor Author

I rebased on main.

@ambv thanks, I'll make sure to do that next time!

@akulakov akulakov deleted the 44980-Fix-two-tests-returning-nonnull-values branch August 23, 2021 19:01
ambv pushed a commit that referenced this pull request Aug 23, 2021
@miss-islington @akulakov
bpo-44980: fix test_constructor to return None value (GH-27898) (GH-2…
de49eac 
…7914)

(cherry picked from commit 27b761a)

Co-authored-by: andrei kulakov <[email protected]>
miss-islington added a commit that referenced this pull request Aug 23, 2021
@miss-islington @akulakov
bpo-44980: fix test_constructor to return None value (GH-27898)
1095535 
(cherry picked from commit 27b761a)

Co-authored-by: andrei kulakov <[email protected]>
@serhiy-storchaka serhiy-storchaka added the tests Tests in the Lib/test dir label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@akulakov @ambv @bedevere-bot @miss-islington @serhiy-storchaka @the-knights-who-say-ni