[PATCH 2 of 5] tests: terminate subprocess in test-stdio.py in case of exception
Manuel Jacob
me at manueljacob.de
Fri Jul 10 12:19:59 UTC 2020
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().
More information about the Mercurial-devel
mailing list