bpo-13886: Fix test_builtin.PtyTests tests when readline is loaded by xdegaye · Pull Request #7133 · python/cpython
| [master, slave] = pty.openpty() | ||
| proc = subprocess.Popen(cmd, stdin=slave, stdout=slave, stderr=slave) | ||
| os.close(slave) | ||
| output = "could not read process output" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this string is always overwritten below (or if there is an exception, it is never used)
| '# Check we exercise the PyOS_Readline() path\n' | ||
| 'if not sys.stdin.isatty() or not sys.stdout.isatty():\n' | ||
| ' raise AssertionError("standard IO should be a tty")\n' | ||
| 'print("{sentinel}", end="")\n' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the end="" bit? Add a comment saying you are testing that input flushes stdout if that is your intention, because it’s not obvious.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is expected that input() flushes stdout, not sure why you think this would need to be tested here. Avoiding the \n makes it clear that the length of sentinel on input is not tied to platform new line conversions.
| output = "could not read process output" | ||
| with proc, os.fdopen(master, "wb", closefd=False) as writer: | ||
| # Wait for the process to be fully started. | ||
| sentinel = os.read(master, len(expected)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m curious why you need to synchronize the parent with the child. What happens if the process isn’t fully started? Does OS X throw away the input if the child isn’t ready or something?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have an OS X platform and have been merely guessing at solutions to fix the patch from issue 13886 on this platform. Synchronizing the parent with the child makes the test more robust (and easier to debug).
| 'if result != {expected!a}:\n' | ||
| ' raise AssertionError("unexpected input " + ascii(result))\n' | ||
| ) | ||
| sentinel = 'Hé, this is before calling input()' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Python still run in environments where stdout or the command line cannot encode e-acute? I haven’t kept up with the new Python encoding changes, but this used to be the case. You can avoid the CLI problem by using the ascii function, or “!a” in the format string.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What CLI problem ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, got it now.
Simplest is to set sentinel to 123 then.