8000 gh-121245: a regression test for site.register_readline() by skirpichev · Pull Request #121259 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-121245: a regression test for site.register_readline() #121259

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

Merged
merged 13 commits into from
Jul 3, 2024
Merged

gh-121245: a regression test for site.register_readline() #121259

Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8195067
Refactor site.register_readline()
skirpichev Jul 2, 2024
8cbf9e5
+ news entry
skirpichev Jul 2, 2024
9dd6305
+ add cleanup for test file
skirpichev Jul 2, 2024
4c57916
Merge branch 'main' into fix-repl-121245-v2
skirpichev Jul 2, 2024
41d0f3d
Update Lib/site.py (drop history length check)
skirpichev Jul 2, 2024
595e5da
+ test exit_code
skirpichev Jul 2, 2024
e358963
Update Misc/NEWS.d/next/Library/2024-07-02-11-34-06.gh-issue-121245.s…
skirpichev Jul 3, 2024
77a7baa
XXX skip test_not_wiping_history_file() if pyrepl can't be used
skirpichev Jul 3, 2024
8ed572d
Revert "XXX skip test_not_wiping_history_file() if pyrepl can't be used"
skirpichev Jul 3, 2024
753ef78
Merge branch 'master' into fix-repl-121245-v2
skirpichev Jul 3, 2024
f625be8
XXX keep only test
skirpichev Jul 3, 2024
d3dbbfb
+ test with/without basic repl
skirpichev Jul 3, 2024
d4b0f10
Revert news; lets just add a test
skirpichev Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions Lib/test/test_pyrepl/test_pyrepl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
import io
import itertools
import os
import pathlib
import rlcompleter
import select
import subprocess
import sys
import tempfile
from unittest import TestCase, skipUnless
from unittest.mock import patch
from test.support import force_not_colorized
from test.support import SHORT_TIMEOUT
from test.support.os_helper import unlink

from .support import (
FakeConsole,
Expand Down Expand Up @@ -898,6 +901,30 @@ def test_python_basic_repl(self):
self.assertNotIn("Exception", output)
self.assertNotIn("Traceback", output)

def test_not_wiping_history_file(self):
Copy link
Member

Choose a reason for hiding this comment

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

I have tried and reverting the changes you did in site.py and running this test still succeeds

Copy link
Member

Choose a reason for hiding this comment

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

Example:

❯ git --no-pager diff --cached
diff --git a/Lib/site.py b/Lib/site.py
index fcfd2fe7bbe..9381f6f510e 100644
--- a/Lib/site.py
+++ b/Lib/site.py
@@ -509,10 +509,6 @@ def register_readline():
         pass

     if readline.get_current_history_length() == 0:
-        try:
-            from _pyrepl.main import CAN_USE_PYREPL
-        except ImportError:
-            CAN_USE_PYREPL = False
         # If no history was loaded, default to .python_history,
         # or PYTHON_HISTORY.
         # The guard is necessary to avoid doubling history size at
@@ -520,23 +516,29 @@ def register_readline():
         # through a PYTHONSTARTUP hook, see:
         # http://bugs.python.org/issue5845#msg198636
         history = gethistoryfile()
-        if os.getenv("PYTHON_BASIC_REPL") or not CAN_USE_PYREPL:
-            my_readline = readline
-        else:
-            my_readline = _pyrepl.readline
         try:
-            my_readline.read_history_file(history)
+            if os.getenv("PYTHON_BASIC_REPL"):
+                readline.read_history_file(history)
+            else:
+                _pyrepl.readline.read_history_file(history)
         except (OSError,* _pyrepl.unix_console._error):
             pass

         def write_history():
             try:
-                my_readline.write_history_file(history)
-            except (FileNotFoundError, PermissionError,
-                    _pyrepl.unix_console.InvalidTerminal):
+                # _pyrepl.__main__ is executed as the __main__ module
+                from __main__ import CAN_USE_PYREPL
+            except ImportError:
+                CAN_USE_PYREPL = False
+
+            try:
+                if os.getenv("PYTHON_BASIC_REPL") or not CAN_USE_PYREPL:
+                    readline.write_history_file(history)
+                else:
+                    _pyrepl.readline.write_history_file(history)
+            except (FileNotFoundError, PermissionError):
                 # home directory does not exist or is not writable
                 # https://bugs.python.org/issue19891
-                # or terminal doesn't have the required capability
                 pass

         atexit.register(write_history)

~/github/python/main fix-repl-121245-v2*
❯ ./python.exe -m test test_pyrepl -uall
Using random seed: 2158058370
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 3.76 Run 1 test sequentially in a single process
0:00:00 load avg: 3.76 [1/1] test_pyrepl

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3.8 sec
Total tests: run=118 skipped=1
Total test files: run=1/1
Result: SUCCESS

Copy link
Contributor

Choose a reason for hiding this comment

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

The test should fail when you revert #121255

Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks for pointing that out! I was confused by the fact that we have a NEWS entry in this PR claiming that it fixes the bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR amends that news entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for a confusion. I was thinking that this pr doesn't deserve a separate news entry. Just a minor cleanup and adds a test. #121255 was a quick fix for annoying issue.

FYI: news entry slightly corrected as I've reverted a guard that prevents wiping history file (e.g. if user run readline.clear_history()).

Unfortunately, I can't reproduce build failure on "Address sanitizer" job. It looks as PYTHON_HISTORY either doesn't affect the interpreter in run_repl() (but it should, there is no -E option!) or write_history_file() just fails.

hfile = tempfile.NamedTemporaryFile(delete=False)
self.addCleanup(unlink, hfile.name)
env = os.environ.copy()
env["PYTHON_HISTORY"] = hfile.name
commands = "123\nspam\nexit()\n"

env.pop("PYTHON_BASIC_REPL", None)
output, exit_code = self.run_repl(commands, env=env)
self.assertEqual(exit_code, 0)
self.assertIn("123", output)
self.assertIn("spam", output)
self.assertNotEqual(pathlib.Path(hfile.name).stat().st_size, 0)

hfile.file.truncate()
hfile.close()

env["PYTHON_BASIC_REPL"] = "1"
output, exit_code = self.run_repl(commands, env=env)
self.assertEqual(exit_code, 0)
self.assertIn("123", output)
self.assertIn("spam", output)
self.assertNotEqual(pathlib.Path(hfile.name).stat().st_size, 0)

def run_repl(self, repl_input: str | list[str], env: dict | None = None) -> tuple[str, int]:
master_fd, slave_fd = pty.openpty()
process = subprocess.Popen(
Expand Down
Loading
0