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.

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

gh-92886: fixed tests that were breaking while running with basic optimizations enabled (-O, assertions off) #93174

Closed
wants to merge 14 commits into from
Closed

gh-92886: fixed tests that were breaking while running with basic optimizations enabled (-O, assertions off) #93174

wants to merge 14 commits into from

Conversation

jackh-ncl
Copy link
Contributor

@jackh-ncl jackh-ncl commented May 24, 2022 •
edited
Loading

Resolves #92886

$ uname -a
Darwin ... 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:47:26 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T8101 arm64

$ ./python.exe -Om test -j8
...
== Tests result: SUCCESS ==

407 tests OK.

29 tests skipped:
    test_curses test_dbm_gnu test_devpoll test_epoll test_gdb
    test_idle test_ioctl test_launcher test_msilib
    test_multiprocessing_fork test_ossaudiodev test_smtpnet
    test_socketserver test_spwd test_ssl test_startfile test_tcl
    test_tix test_tk test_ttk_guionly test_ttk_textonly test_turtle
    test_urllib2net test_urllibnet test_winconsoleio test_winreg
    test_winsound test_xmlrpc_net test_zipfile64

Total duration: 3 min 49 sec
Tests result: SUCCESS

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented May 24, 2022 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@jackh-ncl jackh-ncl mentioned this pull request May 24, 2022
Closed
10 tasks
jackh-ncl
jackh-ncl commented May 24, 2022
View reviewed changes
Lib/test/_test_multiprocessing.py
@@ -5690,45 +5690,48 @@ def test_joinable_queue(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be worth fixing the rest of the assertions in this file?

jackh-ncl
jackh-ncl commented May 24, 2022
View reviewed changes
Lib/test/test_dis.py
code_info_consts = "0: None"
else:
code_info_consts = "0: 'Formatted details of methods, functions, or code.'"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failing tests were:

image

It seemed as though this condition was not necessary anymore, though I'm unsure if it is the expected behaviour.

jackh-ncl
jackh-ncl commented May 24, 2022
View reviewed changes
Lib/test/test_imaplib.py
@@ -939,6 +939,7 @@ def test_with_statement_logout(self):

@threading_helper.reap_threads
@cpython_only
@unittest.skipUnless(__debug__, "Won't work if __debug__ is False")
Copy link
Contributor Author

@jackh-ncl jackh-ncl May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

_mesg() @ Lib/imaplib.py:1247 is defined within a if __debug__ block, so I assume this test can be skipped when running with optimizations.

jackh-ncl
jackh-ncl commented May 24, 2022
View reviewed changes
Lib/test/test_py_compile.py
@@ -235,11 +235,12 @@ def pycompilecmd(self, *args, **kwargs):
# assert_python_* helpers don't return proc object. We'll just use
# subprocess.run() instead of spawn_python() and its friends to test
# stdin support of the CLI.
opts = '-m' if __debug__ else '-Om'
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'm not sure how this actually fixed things, but it seems to have done the trick 🤷‍♂️

jackh-ncl
jackh-ncl commented May 24, 2022
View reviewed changes
Lib/test/test_sys_settrace.py
(13, 'line'),
(13, 'return')])
(10, 'line')] +
([(13, 'line'), (13, 'return')] if __debug__ else [(10, 'return')]))
Copy link
Contributor Author

@jackh-ncl jackh-ncl May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The assertions on line 819 affect the line numbers we are testing here when running with optimizations.
An alternative could be for these test functions to do something other than an assert.

@jackh-ncl jackh-ncl marked this pull request as ready for review May 24, 2022 14:59
@AlexWaygood AlexWaygood added the tests Tests in the Lib/test dir label May 24, 2022
@erlend-aasland
Copy link
Contributor

Please limit this PR to tests only (files in the Lib/test directory). It may make sense to break this up in multiple PRs.

@jackh-ncl
Copy link
Contributor Author

@erlend-aasland I've split it out into several PRs, which hopefully doesn't seem too overkill. I can see there being some level of back and forth for a number of them.

@erlend-aasland
Copy link
Contributor

@erlend-aasland I've split it out into several PRs, which hopefully doesn't seem too overkill. I can see there being some level of back and forth for a number of them.

Thanks. One PR per file was perhaps a little bit too much, but IMO it is better than one PR for everything.

IIUC, this PR is now superfluous and can be closed?

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label May 26, 2022
@jackh-ncl jackh-ncl closed this May 26, 2022
@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner

@encukou encukou Awaiting requested review from encukou

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

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

Successfully merging this pull request may close these issues.

some tests fail with -O (assertions off)
4 participants
@jackh-ncl @bedevere-bot @erlend-aasland @AlexWaygood