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. 1) > 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 pty.py, 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? 2) > 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. 3) 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. Advantages: * 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. Disadvantages: * 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: http://bugs.python.org/file46177/pty_tests.patch _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue29070> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
