[PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception

[Yuya Nishihara]

On Fri, 10 Jul 2020 23:39:16 +0200, Manuel Jacob wrote:
> On 2020-07-10 15:17, Yuya Nishihara wrote:
> > 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 <me at 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.
> 
> I’m not sure why we should make a difference between AssertionError and 
> other errors. If there’s any error (expected or unexpected), we should 
> try to terminate hanging subprocesses.

Because AssertionError isn't an error in unittest context, I feel it's weird
to catch it as an exception and kill the process. But yeah, it's just a
cosmetic issue.