[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