On Fri, 10 Jul 2020 14:19:59 +0200, Manuel Jacob wrote: > On 2020-07-10 13:14, Yuya Nishihara wrote: > > On Fri, 10 Jul 2020 06:46:39 +0200, Manuel Jacob wrote: > >> # HG changeset patch > >> # User Manuel Jacob <m...@manueljacob.de> > >> # Date 1594118129 -7200 > >> # Tue Jul 07 12:35:29 2020 +0200 > >> # Node ID 0bcb1c7fc0885319f9628de2cb6ea0cca3bf2d7b > >> # Parent 7c5896871c7e9e8e510c0436f5c2bcc6018e8177 > >> # EXP-Topic stdio > >> tests: terminate subprocess in test-stdio.py in case of exception > >> > >> If an error happened while reading the output of the subprocess, the > >> pipe / TTY > >> buffer can fill up and prevent that the subprocess ends. Therefore we > >> should > >> terminate the subprocess in case of an exception. > >> > >> diff --git a/tests/test-stdio.py b/tests/test-stdio.py > >> --- a/tests/test-stdio.py > >> +++ b/tests/test-stdio.py > >> @@ -99,6 +99,9 @@ > >> self.assertEqual( > >> _readall(stream_receiver, 1024), expected_output > >> ) > >> + except: # re-raises > >> + proc.terminate() > >> + raise > > > > I think assertEqual() should be moved out of the try block. > > AssertionError > > shouldn't be an unexpected error. > > I’m generally in favor of making try blocks as small as possible. > However, this particular try block is more like a finally block, except > that it should only be executed if there was an error. > > > I've queued the patch 1-3, thanks. The patch 4 might need some rework > > in > > order to move assertEqual() out of the try block. > > Upcoming patches will add tests with check_output() that does more than > simply reading the output once. In one particular case, it reads one > byte from the stream, sends a signal, and reads the rest of the bytes. > Moving the assert out of the try block would be possible (although the > code will become less clear), but I wonder if it’s really worth it to > separate interaction and verification if we anyway have more complicated > logic in check_output().
Okay. Adding "except AssertionError: _readall(...); ...; raise" should also be fine. _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
