Cornelius Diekmann added the comment:

Thank you Martin very much for this very helpful review. I updated and 
simplified the tests and implemented your suggestions.

There are three open issues left.

> It looks like you depend on fixing Issue 26228, but the patch there will 
> conflict with your changes. Maybe merge with the other patch, or propose an 
> alternative fix.

I reverted my changes to, so the patches no longer conflict for this 
file. Currently, my test suite reveals the pty bug on FreeBSD but does not fix 
it. Since I'm not a FreeBSD expert and I don't want to steal Chris' patch 
[issue26228], I'd like to keep those issues separate. In other words, I'm 
proposing only a testsuite which introduces no behavioral changes and uncovers 
a bug.

This also means that this test suite is currently hanging on FreeBSD and OS X. 
The good news is, the patch in issue26228 fixes the problem.

> The documentation currently mentions the code is only tested on Linux, so it 
> would be nice to update that.
Is it okay to keep this hanging around until the pty module is no longer broken 
on FreeBSD?

> Why does the patch slow the tests down so much? Ideally, it is nice to keep 
> the tests as fast as possible.

The test suite does some heavy integration testing. The test in the class 
PtyWhiteBoxIntegrationReadSlaveTest are the slow ones. The test suite reads a 
large amount of data from the spawned child process. The amount of data was 
chosen to be a few kB in size to make sure that we won't lose data of the child 
and that --in case of a bug in the code-- we don't accidentally succeed due to 
race conditions.

Is 1-2 seconds really too slow? We can reduce the amount of data, but increase 
the risk that a future code change introduces a race condition which is not 
covered by the test suite.

About the system bell pretty printing test:
> This seems platform-dependent. Do you really need to test echoing of control
> characters? We only need to test Python, not the operating system.

Including these integration tests has advantages and disadvantages.

 * Integration tests which deliberately depend on the careful interplay of many 
modules make sure that the overall system is healthy.
 * Modules such as termios or terminal-specific parts of os seem not to be 
tested currently. This test suite depends on their correct functionality and is 
thus a high-level test for them. In addition, pty.spawn() is the perfect place 
to test them.
 * The tests test actual terminal features. Without, we could not uncover if 
e.g., a pty master/slave is replaced by a simple pipe.
 * Tests are set up by increasing order of complexity w.r.t. the underlying 
modules they depend on. If something breaks in the future, these tests are 
designed to ease debugging and directly point to the error.
 * They provide some documentation about the magic of terminals and examples.
 * Those tests are fast.

 * Control-Character pretty printing only works on Linux.
 * Relies on the (POSIX-compliant) internals of the operating system.

I wanted to include some tests which depend on some terminal-specific features 
to make sure that the complete setup works and is operational. For example, the 
test suite now documents the difference between ptys and pipes and would 
complain if any code change breaks terminal-specific features. I chose control 
character pretty printing in some tests because this is one of the simplest 
features of terminals to test, debug, and verify. In contrast, for example, it 
would be extremely fragile and probably complete overkill to test things such 
as terminal delays. 

My personal feeling says that the advantages outweigh the disadvantages. If you 
disagree, we can remove them :-)

Added file:

Python tracker <>
Python-bugs-list mailing list

Reply via email to