-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-140287: Handle PYTHONSTARTUP script exceptions in the asyncio REPL
#140288
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
faa22f4
5b3ad2c
00edac4
e396622
4079074
0440d0e
af6f657
848638d
d158dbf
9355da7
3dc4cac
c786584
b76db67
cad1748
d114ed5
e6e10ad
ac6cd83
149740a
bffac1c
a4c307e
a774da5
b3ed3d4
df6dfd8
b004839
ce03cce
9e92510
0a50a50
f8b8d53
5701fef
393aaad
875fd2a
4377d82
c4e488e
b9ffea7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import subprocess | ||
| import sys | ||
| import unittest | ||
| from contextlib import contextmanager | ||
| from functools import partial | ||
| from textwrap import dedent | ||
| from test import support | ||
|
|
@@ -28,7 +29,7 @@ | |
| raise unittest.SkipTest("test module requires subprocess") | ||
|
|
||
|
|
||
| def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, custom=False, **kw): | ||
| def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, custom=False, isolated=True, **kw): | ||
| """Run the Python REPL with the given arguments. | ||
|
|
||
| kw is extra keyword args to pass to subprocess.Popen. Returns a Popen | ||
|
|
@@ -42,7 +43,10 @@ def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, custom=F | |
| # path may be used by PyConfig_Get("module_search_paths") to build the | ||
| # default module search path. | ||
| stdin_fname = os.path.join(os.path.dirname(sys.executable), "<stdin>") | ||
| cmd_line = [stdin_fname, '-I'] | ||
| cmd_line = [stdin_fname] | ||
| # Isolated mode implies -EPs and ignores PYTHON* variables. | ||
| if isolated: | ||
| cmd_line.append('-I') | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reuse this routine and The asyncio REPL currently doesn't comply to that, which is a separate issue I'll report having learnt this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracking the |
||
| # Don't re-run the built-in REPL from interactive mode | ||
| # if we're testing a custom REPL (such as the asyncio REPL). | ||
| if not custom: | ||
|
|
@@ -64,6 +68,16 @@ def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, custom=F | |
| spawn_asyncio_repl = partial(spawn_repl, "-m", "asyncio", custom=True) | ||
|
|
||
|
|
||
| @contextmanager | ||
| def new_startup_env(*, code: str, histfile: str = ".pythonhist"): | ||
| """Create environment variables for a PYTHONSTARTUP script in a temporary directory.""" | ||
| with os_helper.temp_dir() as tmpdir: | ||
| filename = os.path.join(tmpdir, "pythonstartup.py") | ||
| with open(filename, "w") as f: | ||
| f.write('\n'.join(code.splitlines())) | ||
| yield {"PYTHONSTARTUP": filename, "PYTHON_HISTORY": os.path.join(tmpdir, histfile)} | ||
|
|
||
|
|
||
| def run_on_interactive_mode(source): | ||
| """Spawn a new Python interpreter, pass the given | ||
| input source code from the stdin and return the | ||
|
|
@@ -197,68 +211,6 @@ def foo(x): | |
| ] | ||
| self.assertEqual(traceback_lines, expected_lines) | ||
|
|
||
| def test_pythonstartup_error_reporting(self): | ||
| # errors based on https://github.com/python/cpython/issues/137576 | ||
|
|
||
| def make_repl(env): | ||
| return subprocess.Popen( | ||
| [os.path.join(os.path.dirname(sys.executable), '<stdin>'), "-i"], | ||
| executable=sys.executable, | ||
| text=True, | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| env=env, | ||
| ) | ||
|
|
||
| # case 1: error in user input, but PYTHONSTARTUP is fine | ||
| with os_helper.temp_dir() as tmpdir: | ||
| script = os.path.join(tmpdir, "pythonstartup.py") | ||
| with open(script, "w") as f: | ||
| f.write("print('from pythonstartup')" + os.linesep) | ||
|
|
||
| env = os.environ.copy() | ||
| env['PYTHONSTARTUP'] = script | ||
| env["PYTHON_HISTORY"] = os.path.join(tmpdir, ".pythonhist") | ||
| p = make_repl(env) | ||
| p.stdin.write("1/0") | ||
| output = kill_python(p) | ||
| expected = dedent(""" | ||
| Traceback (most recent call last): | ||
| File "<stdin>", line 1, in <module> | ||
| 1/0 | ||
| ~^~ | ||
| ZeroDivisionError: division by zero | ||
| """) | ||
| self.assertIn("from pythonstartup", output) | ||
| self.assertIn(expected, output) | ||
|
|
||
| # case 2: error in PYTHONSTARTUP triggered by user input | ||
| with os_helper.temp_dir() as tmpdir: | ||
| script = os.path.join(tmpdir, "pythonstartup.py") | ||
| with open(script, "w") as f: | ||
| f.write("def foo():\n 1/0\n") | ||
|
|
||
| env = os.environ.copy() | ||
| env['PYTHONSTARTUP'] = script | ||
| env["PYTHON_HISTORY"] = os.path.join(tmpdir, ".pythonhist") | ||
| p = make_repl(env) | ||
| p.stdin.write('foo()') | ||
| output = kill_python(p) | ||
| expected = dedent(""" | ||
| Traceback (most recent call last): | ||
| File "<stdin>", line 1, in <module> | ||
| foo() | ||
| ~~~^^ | ||
| File "%s", line 2, in foo | ||
| 1/0 | ||
| ~^~ | ||
| ZeroDivisionError: division by zero | ||
| """) % script | ||
| self.assertIn(expected, output) | ||
|
|
||
|
|
||
|
|
||
| def test_runsource_show_syntax_error_location(self): | ||
| user_input = dedent("""def f(x, x): ... | ||
| """) | ||
|
|
@@ -292,23 +244,46 @@ def bar(x): | |
| expected = "(30, None, [\'def foo(x):\\n\', \' return x + 1\\n\', \'\\n\'], \'<stdin>\')" | ||
| self.assertIn(expected, output, expected) | ||
|
|
||
| def test_asyncio_repl_reaches_python_startup_script(self): | ||
| with os_helper.temp_dir() as tmpdir: | ||
| script = os.path.join(tmpdir, "pythonstartup.py") | ||
| with open(script, "w") as f: | ||
| f.write("print('pythonstartup done!')" + os.linesep) | ||
| f.write("exit(0)" + os.linesep) | ||
|
|
||
| env = os.environ.copy() | ||
| env["PYTHON_HISTORY"] = os.path.join(tmpdir, ".asyncio_history") | ||
| env["PYTHONSTARTUP"] = script | ||
| subprocess.check_call( | ||
| [sys.executable, "-m", "asyncio"], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| env=env, | ||
| timeout=SHORT_TIMEOUT, | ||
| ) | ||
|
Comment on lines
-295
to
-311
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Covered by |
||
| def test_pythonstartup_success(self): | ||
| # errors based on https://github.com/python/cpython/issues/137576 | ||
| # case 1: error in user input, but PYTHONSTARTUP is fine | ||
| startup_code = "print('notice from pythonstartup')" | ||
| startup_env = self.enterContext(new_startup_env(code=startup_code)) | ||
| # -q to suppress noise | ||
| p = spawn_repl("-q", env=os.environ | startup_env, isolated=False) | ||
| p.stdin.write("1/0") | ||
| output_lines = kill_python(p).splitlines() | ||
| self.assertEqual(output_lines[0], 'notice from pythonstartup') | ||
|
|
||
| traceback_lines = output_lines[2:-1] | ||
| expected_lines = [ | ||
| 'Traceback (most recent call last):', | ||
| ' File "<stdin>", line 1, in <module>', | ||
| ' 1/0', | ||
| ' ~^~', | ||
| 'ZeroDivisionError: division by zero', | ||
| ] | ||
| self.assertEqual(traceback_lines, expected_lines) | ||
|
|
||
| def test_pythonstartup_failure(self): | ||
| # case 2: error in PYTHONSTARTUP triggered by user input | ||
| startup_code = "def foo():\n 1/0\n" | ||
| startup_env = self.enterContext(new_startup_env(code=startup_code)) | ||
| # -q to suppress noise | ||
| p = spawn_repl("-q", env=os.environ | startup_env, isolated=False) | ||
| p.stdin.write("foo()") | ||
| traceback_lines = kill_python(p).splitlines()[1:-1] | ||
| expected_lines = [ | ||
| 'Traceback (most recent call last):', | ||
| ' File "<stdin>", line 1, in <module>', | ||
| ' foo()', | ||
| ' ~~~^^', | ||
| f' File "{startup_env['PYTHONSTARTUP']}", line 2, in foo', | ||
| ' 1/0', | ||
| ' ~^~', | ||
| 'ZeroDivisionError: division by zero', | ||
| ] | ||
| self.assertEqual(traceback_lines, expected_lines) | ||
|
|
||
| @unittest.skipUnless(pty, "requires pty") | ||
| def test_asyncio_repl_is_ok(self): | ||
|
|
@@ -365,6 +340,7 @@ def f(): | |
| self.assertEqual(traceback_lines, expected_lines) | ||
|
|
||
|
|
||
| @support.force_not_colorized_test_class | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is needed, but I think it's OK to keep it as is. |
||
| class TestAsyncioREPL(unittest.TestCase): | ||
| def test_multiple_statements_fail_early(self): | ||
| user_input = "1 / 0; print(f'afterwards: {1+1}')" | ||
|
|
@@ -409,6 +385,55 @@ def test_toplevel_contextvars_async(self): | |
| expected = "toplevel contextvar test: ok" | ||
| self.assertIn(expected, output, expected) | ||
|
|
||
| def test_pythonstartup_success(self): | ||
| startup_code = "import sys\nprint('notice from pythonstartup in asyncio repl', file=sys.stderr)" | ||
| startup_env = self.enterContext(new_startup_env(code=startup_code, histfile=".asyncio_history")) | ||
| p = spawn_asyncio_repl(env=os.environ | startup_env, stderr=subprocess.PIPE, isolated=False) | ||
| p.stdin.write("1/0") | ||
| kill_python(p) | ||
| output_lines = p.stderr.read().splitlines() | ||
| p.stderr.close() | ||
|
|
||
| self.assertEqual(output_lines[3], 'notice from pythonstartup in asyncio repl') | ||
|
|
||
| tb_start_lines = output_lines[5:6] | ||
| tb_final_lines = output_lines[13:] | ||
| expected_lines = [ | ||
| 'Traceback (most recent call last):', | ||
| ' File "<stdin>", line 1, in <module>', | ||
| ' 1/0', | ||
| ' ~^~', | ||
| 'ZeroDivisionError: division by zero', | ||
| '', | ||
| 'exiting asyncio REPL...', | ||
| ] | ||
| self.assertEqual(tb_start_lines + tb_final_lines, expected_lines) | ||
|
|
||
| def test_pythonstartup_failure(self): | ||
| startup_code = "def foo():\n 1/0\n" | ||
| startup_env = self.enterContext(new_startup_env(code=startup_code, histfile=".asyncio_history")) | ||
| p = spawn_asyncio_repl(env=os.environ | startup_env, stderr=subprocess.PIPE, isolated=False) | ||
| p.stdin.write("foo()") | ||
| kill_python(p) | ||
| output_lines = p.stderr.read().splitlines() | ||
| p.stderr.close() | ||
|
|
||
| tb_start_lines = output_lines[4:5] | ||
| tb_final_lines = output_lines[12:] | ||
| expected_lines = [ | ||
| 'Traceback (most recent call last):', | ||
| ' File "<stdin>", line 1, in <module>', | ||
| ' foo()', | ||
| ' ~~~^^', | ||
| f' File "{startup_env['PYTHONSTARTUP']}", line 2, in foo', | ||
| ' 1/0', | ||
| ' ~^~', | ||
| 'ZeroDivisionError: division by zero', | ||
| '', | ||
| 'exiting asyncio REPL...', | ||
| ] | ||
| self.assertEqual(tb_start_lines + tb_final_lines, expected_lines) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| The asyncio REPL now properly handles exceptions in ``PYTHONSTARTUP`` | ||
| scripts. Patch by Bartosz Sławecki in :gh:`140287`. |
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.
This resembles the original behavior of the REPLs -- see GH-143023.
I don't think this is correct though. We can remove this as part of GH-143023.
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.
This is also why I didn't add a test for this.