Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-116303: Skip test module dependent tests if test modules are disabled#116307

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedMar 4, 2024•
edited by bedevere-appbot
Loading

@encukou
Copy link
Member

Would it be worth it to skip only the affected assertions, rather than the whole test method?

@erlend-aasland
Copy link
ContributorAuthor

Would it be worth it to skip only the affected assertions, rather than the whole test method?

Yes, I think so. I'll split them up.

@erlend-aaslanderlend-aasland changed the titlegh-116303: Skip some sqlite3 tests if testcapi is unavailablegh-116303: Skip test module dependent tests if test modules are disabledMar 4, 2024
@erlend-aaslanderlend-aasland marked this pull request as draftMarch 4, 2024 13:54
- Lib/test/support/__init__.py- Lib/test/test_audit.py- Lib/test/test_gdb/test_misc.py- Lib/test/test_inspect/test_inspect.py- Lib/test/test_pydoc/test_pydoc.py
@erlend-aasland
Copy link
ContributorAuthor

@encukou: I started fixing some of the remaining tests, but I won't be back at my laptop until tonight. Feel free to pick this up in the mean time.

@erlend-aasland
Copy link
ContributorAuthor

@serhiy-storchaka, would you like to join in on finishing this PR? There are still a lot of test failures1, but I'm slowly getting there. For example,test_importlib still fails; it might be because I introduced a new bug, it might be because we need more skips, or it might be because we uncovered latent test bugs. It is hard to tell right now2.

Footnotes

  1. when testing with--disable-test-modules only; unfortunately, they do not show up in CI nor in buildbots, since we do not test with this configure option (yet)↩

  2. the amount of tests annotated with docstrings and/or sprinkled withprint()s do not make this job easier; ideally we should clean up the whole test suite so runningpython -m test -v generated usable output.test_threading, for example, is sometimes so verbose that it is hard to tell signal from noise.↩

Comment on lines +511 to 513
returnunittest.skip("_testinternalcapi required")
config=_testinternalcapi.get_config()
returnnotbool(config['code_debug_ranges'])

Choose a reason for hiding this comment

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

Why does it return a boolean (which is not callable) or a function (which always true)? How this helper is supposed to be used?

finder=self.machinery.FileFinder(None)
self.spec=importlib.util.find_spec(self.name)
self.assertIsNotNone(self.spec)
assertself.spec

Choose a reason for hiding this comment

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

Why skip this in optimized mode?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@jaraco requested to leave the assert's for importlib tests, so I reverted the unittest assert style changes. The code is shared with an external repo.

Copy link
Member

Choose a reason for hiding this comment

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

FYI - I only care about the retention of bare asserts in the tests for importlib.metadata and importlib.resources. Unfortunately, the tests for importlib.metadata are interspersed with other importlib tests, so it's not easy to know which are which without looking at thecpython branch of importlib_metadata. I do have plans to move importlib.metadata tests into their own package so they're clearly separated.

erlend-aasland reacted with thumbs up emoji
@vstinner
Copy link
Member

About the test_embed bug, it seems like importing_ctypes is enough to trigger the bug which looks like a memory corruption:

vstinner@WIN C:\victor\python\main>type bug.pyimport subprocessprogram = r"PCbuild\amd64\_testembed_d.exe"cmd = [program, "test_repeated_init_exec", "import _ctypes"]subprocess.run(cmd)vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ------ Loop #4 ---vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ------ Loop #4 ---vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ------ Loop #4 ---vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ------ Loop #4 ---vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ---Assertion failed: PyUnicode_CheckExact(ep->me_key), file C:\victor\python\main\Objects\dictobject.c, line 1101

@vstinner
Copy link
Member

About the test_embed bug, it seems like importing _ctypes is enough to trigger the bug which looks like a memory corruption

I can reproduce the bug on the main branch, without these changes. Use this patch to make the bug more likely:

diff --git a/Programs/_testembed.c b/Programs/_testembed.cindex 30998bf80f..e8f36ae442 100644--- a/Programs/_testembed.c+++ b/Programs/_testembed.c@@ -38,7 +38,7 @@ char **main_argv; /* Use path starting with "./" avoids a search along the PATH */ #define PROGRAM_NAME L"./_testembed"-#define INIT_LOOPS 4+#define INIT_LOOPS 12 // Ignore Py_DEPRECATED() compiler warnings: deprecated functions are // tested on purpose here.

Example of crash:

vstinner@WIN C:\victor\python\main>python bug.py    Running Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ------ Loop #4 ------ Loop #5 ------ Loop #6 ------ Loop #7 ------ Loop #8 ------ Loop #9 ------ Loop #10 ------ Loop #11 ------ Loop #12 ---vstinner@WIN C:\victor\python\main>python bug.pyRunning Debug|x64 interpreter...--- Loop #1 ------ Loop #2 ------ Loop #3 ---Assertion failed: PyUnicode_CheckExact(ep->me_key), file C:\victor\python\main\Objects\dictobject.c, line 1101

@vstinner
Copy link
Member

I can reproduce the bug on the main branch, without these changes.

I tried to dig into Python history: the bug exists since July 1st, 2023 at least. I failed to go further in the past withgit bisect, around June 1st, 2023, the test fails at importing_ctypes at the second loop iteration!

vstinner@WIN C:\victor\python\main>python bug.py  Running Debug|x64 interpreter...   == Process #1 ===--- Loop #1 ------ Loop #2 ---Traceback (most recent call last):  File "<string>", line 1, in <module>ModuleNotFoundError: No module named '_ctypes'=> exitcode 1

Latest version of mybug.py:

importsubprocessprogram=r"PCbuild\amd64\_testembed_d.exe"cmd= [program,"test_repeated_init_exec","import _ctypes"]foriinrange(1,11):print(f"   == Process #{i} ===")proc=subprocess.run(cmd)exitcode=proc.returncodeprint(f"=> exitcode{exitcode}")ifexitcode:break

@vstinner
Copy link
Member

I created#116467 for the test_embed crash: Initialize/Finalize Python multiple time and import _ctypes each time lead to a memory corruption.

erlend-aasland reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

@vstinner, could you please open a separate issue for this and copy the results of researches there?

vstinner and erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

Thanks for your help, everyone. I'm going to close this PR and split it up in multiple parts.

@erlend-aaslanderlend-aasland deleted the sqlite/testcapi branchMarch 7, 2024 22:41
jaraco added a commit that referenced this pull requestMar 12, 2024
gh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMar 12, 2024
…ythonGH-116680)(cherry picked from commita254807)Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
pablogsal pushed a commit to pablogsal/cpython that referenced this pull requestMar 12, 2024
… tests (pythonGH-116680) (python#116684)pythongh-116307: Proper fix for 'mod' leaking across importlib tests (pythonGH-116680)(cherry picked from commita254807)pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
jaraco added a commit that referenced this pull requestMar 13, 2024
#116694)[3.11]gh-116307: Proper fix for 'mod' leaking across importlib tests (GH-116680)(cherry picked from commita254807)gh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
jaraco added a commit to jaraco/cpython that referenced this pull requestMar 13, 2024
… tests (pythonGH-116680)(cherry picked from commita254807)Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
vstinner pushed a commit to vstinner/cpython that referenced this pull requestMar 20, 2024
…ython#116680)pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
…ython#116680)pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
…ython#116680)pythongh-116307: Create a new import helper 'isolated modules' and use that instead of 'Clean Import' to ensure that tests from importlib_resources don't leave modules in sys.modules.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jaracojaracojaraco approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

@encukouencukouAwaiting requested review from encukouencukou is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

@FFY00FFY00Awaiting requested review from FFY00FFY00 is a code owner

@brettcannonbrettcannonAwaiting requested review from brettcannonbrettcannon is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

@corona10corona10Awaiting requested review from corona10corona10 is a code owner

Assignees

No one assigned

Labels

awaiting mergeneeds backport to 3.11only security fixesneeds backport to 3.12only security fixesskip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland@encukou@jaraco@vstinner@serhiy-storchaka
[8]ページ先頭
©2009-2025 Movatter.jp