Skip to content

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 8, 2022

Source:

case 'p':
{
char number[MAX_LONG_LONG_CHARS];
len = sprintf(number, "%p", va_arg(*vargs, void*));
assert(len >= 0);
/* %p is ill-defined: ensure leading 0x. */
if (number[1] == 'X')
number[1] = 'x';
else if (number[1] != 'x') {
memmove(number + 2, number,
strlen(number) + 1);
number[0] = '0';
number[1] = 'x';
len += 2;
}
if (_PyUnicodeWriter_WriteASCIIString(writer, number, len) < 0)
return NULL;
break;
}

Internet says that some systems can return representation with 0x, some with 0X, and some with none. It is reflected in the implementation, but cannot be reflected in tests.

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Sep 8, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Sep 8, 2022

And we got a failure: MacOS passes locally and on CI, but other platforms do not.

Windows:

 ======================================================================
FAIL: test_from_format (test.test_unicode.CAPITest.test_from_format)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_unicode.py", line 2817, in test_from_format
    self.assertEqual(len(p_format1), 11)
AssertionError: 10 != 11

Ubuntu:

======================================================================
FAIL: test_from_format (test.test_unicode.CAPITest.test_from_format)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_unicode.py", line 2817, in test_from_format
    self.assertEqual(len(p_format1), 11)
AssertionError: 14 != 11

I think it is better to remove len assumptions.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

I think a regex test would be better here.

@sobolevn
Copy link
Member Author

sobolevn commented Sep 8, 2022

Good idea, thank you.

self.assertIsInstance(p_format1, str)
self.assertRegex(p_format1, p_format_regex)

p_format2 = PyUnicode_FromFormat(b'repr=%p', '123456', None, b'xyz')
Copy link
Member

Choose a reason for hiding this comment

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

What does this case test? I don't see what it adds over the previous case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It has:

  • Multiple arguments (while p_format1 only has one arg)
  • All arguments have different types
  • New types are tested: bytes and None

Copy link
Member

Choose a reason for hiding this comment

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

Aren't the additional arguments ignored though, because there is only one %p in the format string?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, tests can be better :)
Can you please take a look at the updated version?

It now has both:

  • Extra types
  • Ignored arguments

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@JelleZijlstra JelleZijlstra self-assigned this Oct 3, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Oct 6, 2022

From https://github.com/python/cpython/actions/runs/3200613350/jobs/5227726848

AssertionError: Regex didn't match: '0x[a-zA-Z0-9]{8,} 0x[a-zA-Z0-9]{1,} 0x[a-zA-Z0-9]{8,}' not found in '0x60400191c290 0x(nil) 0x60400191c230'

This is quite interesting. Is 0x(nil) valid?

@JelleZijlstra
Copy link
Member

Interesting! I think that comes from libc since I can't find code in CPython that would do this.

@sobolevn
Copy link
Member Author

sobolevn commented Oct 6, 2022

From https://pubs.opengroup.org/onlinepubs/009695399/functions/fprintf.html

p
The argument shall be a pointer to void. The value of the pointer is converted to a sequence of printable characters, in an implementation-defined manner.

I think it is better to just leave None out of this test case.
And possibly create a new discussion about this: aren't NULL and None confused here?

Related discussions:

@JelleZijlstra
Copy link
Member

I think ctypes translates None into NULL, which is reasonable.

@JelleZijlstra JelleZijlstra merged commit 72c166a into python:main Oct 7, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry @sobolevn and @JelleZijlstra, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 72c166add89a0cd992d66f75ce94eee5eb675a99 3.11

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 7, 2022
@bedevere-bot
Copy link

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

@JelleZijlstra JelleZijlstra removed the needs backport to 3.11 only security fixes label Oct 7, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2022
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 72c166a)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@JelleZijlstra JelleZijlstra added the needs backport to 3.11 only security fixes label Oct 7, 2022
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-98033 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 7, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2022
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 72c166a)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x SLES 3.x has failed when building commit 72c166a.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/540/builds/3747) and take a look at the build logs.
  4. Check if the failure is related to this commit (72c166a) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/540/builds/3747

Failed tests:

  • test_unicode

Failed subtests:

  • test_from_format - test.test_unicode.CAPITest.test_from_format

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

420 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 51 sec
  • test_tools: 2 min 34 sec
  • test_multiprocessing_spawn: 1 min 51 sec
  • test_asyncio: 1 min 17 sec
  • test_multiprocessing_forkserver: 1 min 14 sec
  • test_signal: 1 min 10 sec
  • test_multiprocessing_fork: 1 min 8 sec
  • test_nntplib: 1 min 5 sec
  • test_capi: 52.3 sec
  • test_tokenize: 48.6 sec

1 test failed:
test_unicode

16 tests skipped:
test_devpoll test_ioctl test_kqueue test_launcher test_msilib
test_nis test_perf_profiler test_startfile test_tix test_tkinter
test_ttk test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_unicode

Total duration: 5 min 53 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_unicode.py", line 2817, in test_from_format
    self.assertRegex(p_format1, p_format_regex)
AssertionError: Regex didn't match: '^0x[a-zA-Z0-9]{8,}$' not found in '0x153b8c0'

@JelleZijlstra
Copy link
Member

I guess the pointers are shorter on s390. I'll adjust the regex.

@JelleZijlstra
Copy link
Member

#98036

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD Non-Debug 3.x has failed when building commit 72c166a.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/172/builds/2913) and take a look at the build logs.
  4. Check if the failure is related to this commit (72c166a) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/172/builds/2913

Failed tests:

  • test_unicode

Failed subtests:

  • test_from_format - test.test_unicode.CAPITest.test_from_format

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

415 tests OK.

10 slowest tests:

  • test_multiprocessing_spawn: 4 min 3 sec
  • test_tokenize: 3 min 40 sec
  • test_asyncio: 3 min 18 sec
  • test_concurrent_futures: 3 min 9 sec
  • test_multiprocessing_forkserver: 2 min 19 sec
  • test_unparse: 2 min 10 sec
  • test_lib2to3: 2 min 3 sec
  • test_unicodedata: 1 min 48 sec
  • test_subprocess: 1 min 37 sec
  • test_multiprocessing_fork: 1 min 36 sec

1 test failed:
test_unicode

21 tests skipped:
test_dbm_gnu test_devpoll test_epoll test_idle test_ioctl
test_launcher test_msilib test_perf_profiler test_spwd
test_startfile test_tcl test_tix test_tkinter test_ttk
test_ttk_textonly test_turtle test_winconsoleio test_winreg
test_winsound test_wmi test_zipfile64

1 re-run test:
test_unicode

Total duration: 17 min

Click to see traceback logs
Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd-9e36.nondebug/build/Lib/test/test_unicode.py", line 2817, in test_from_format
    self.assertRegex(p_format1, p_format_regex)
AssertionError: Regex didn't match: '^0x[a-zA-Z0-9]{8,}$' not found in '0x612bd0'

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 72c166a.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/3172) and take a look at the build logs.
  4. Check if the failure is related to this commit (72c166a) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/3172

Failed tests:

  • test_unicode

Failed subtests:

  • test_from_format - test.test_unicode.CAPITest.test_from_format

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

417 tests OK.

1 test failed:
test_unicode

17 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_dbm_ndbm
test_devpoll test_gdb test_ioctl test_kqueue test_launcher
test_msilib test_perf_profiler test_startfile test_winconsoleio
test_winreg test_winsound test_wmi test_zipfile64

1 re-run test:
test_unicode

Total duration: 53 min 22 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.12/test/test_unicode.py", line 2817, in test_from_format
    self.assertRegex(p_format1, p_format_regex)
AssertionError: Regex didn't match: '^0x[a-zA-Z0-9]{8,}$' not found in '0x8bd9f0'


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.12/test/test_unicode.py", line 2817, in test_from_format
    self.assertRegex(p_format1, p_format_regex)
AssertionError: Regex didn't match: '^0x[a-zA-Z0-9]{8,}$' not found in '0x9809f0'

JelleZijlstra pushed a commit that referenced this pull request Oct 7, 2022
)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

(cherry picked from commit 72c166a)
JelleZijlstra pushed a commit that referenced this pull request Oct 8, 2022
)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

(cherry picked from commit 72c166a)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants