Patchwork [6,of,9] run-tests: extract a `process_cmd_line` from the main function

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1560529827 -3600
#      Fri Jun 14 17:30:27 2019 +0100
# Node ID e9d8154ab03d353fcc5284a10d943be0129e4f4e
# Parent  a8b6d502b98abcb4853d062d06cbb9d8168c8a32
# EXP-Topic test-match
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e9d8154ab03d
run-tests: extract a `process_cmd_line` from the main function

The main function doing line comparison is quite complex. Slicing it in smaller
piece should clarify it.

(This is a gratuitous cleanup that I made while investigating a bug).

On Sat, Sep 07, 2019 at 02:16:45PM +0200, Pierre-Yves David wrote:
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -1619,6 +1619,7 @@ class TTest(Test):
>              if salt in out_rawline:
>                  out_line, cmd_line = out_rawline.split(salt, 1)
>  
> +
>              while out_line:
>                  if not out_line.endswith(b'\n'):
>                      out_line += b' (no-eol)\n'

Extra newline?

> @@ -1699,15 +1700,8 @@ class TTest(Test):
>                                  continue
>                      postout.append(b'  ' + el)
>  
> -            if cmd_line:
> -                # Add on last return code.
> -                ret = int(cmd_line.split()[1])
> -                if ret != 0:
> -                    postout.append(b'  [%d]\n' % ret)
> -                if pos in after:
> -                    # Merge in non-active test bits.
> -                    postout += after.pop(pos)
> -                pos = int(cmd_line.split()[0])
> +            pos, postout = self._process_cmd_line(cmd_line, pos, postout, after)
> +

I find the behavior with postout here a bit surprising. It is modified
in place, but also returned as value. IMO it should be either one or the
other? Also extra newline.

> @@ -1717,6 +1711,19 @@ class TTest(Test):
>  
>          return exitcode, postout
>  
> +    def _process_cmd_line(self, cmd_line, pos, postout, after):
> +        """process a "command" part of a line from unified test output"""
> +        if cmd_line:
> +            # Add on last return code.
> +            ret = int(cmd_line.split()[1])
> +            if ret != 0:
> +                postout.append(b'  [%d]\n' % ret)
> +            if pos in after:
> +                # Merge in non-active test bits.
> +                postout += after.pop(pos)
> +            pos = int(cmd_line.split()[0])
> +        return pos, postout
> +

Does it make sense to mark it as staticmethod?

Joerg

On 9/7/19 3:03 PM, Joerg Sonnenberger wrote:
> On Sat, Sep 07, 2019 at 02:16:45PM +0200, Pierre-Yves David wrote:
>> diff --git a/tests/run-tests.py b/tests/run-tests.py
>> --- a/tests/run-tests.py
>> +++ b/tests/run-tests.py
>> @@ -1619,6 +1619,7 @@ class TTest(Test):
>>               if salt in out_rawline:
>>                   out_line, cmd_line = out_rawline.split(salt, 1)
>>   
>> +
>>               while out_line:
>>                   if not out_line.endswith(b'\n'):
>>                       out_line += b' (no-eol)\n'
> 
> Extra newline?
> 
>> @@ -1699,15 +1700,8 @@ class TTest(Test):
>>                                   continue
>>                       postout.append(b'  ' + el)
>>   
>> -            if cmd_line:
>> -                # Add on last return code.
>> -                ret = int(cmd_line.split()[1])
>> -                if ret != 0:
>> -                    postout.append(b'  [%d]\n' % ret)
>> -                if pos in after:
>> -                    # Merge in non-active test bits.
>> -                    postout += after.pop(pos)
>> -                pos = int(cmd_line.split()[0])
>> +            pos, postout = self._process_cmd_line(cmd_line, pos, postout, after)
>> +
> 
> I find the behavior with postout here a bit surprising. It is modified
> in place, but also returned as value. IMO it should be either one or the
> other? Also extra newline.

This is mostly how the current code flow. I am mostly slicing it up, not 
doing a deep refactoring.