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

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 7, 2019, 12:16 p.m.
Message ID <e9d8154ab03d353fcc52.1567858605@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/41537/
State Accepted
Headers show

Comments

Pierre-Yves David - Sept. 7, 2019, 12:16 p.m.
# 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).
Joerg Sonnenberger - Sept. 7, 2019, 1:03 p.m.
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
Pierre-Yves David - Sept. 8, 2019, 8:23 a.m.
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.

Patch

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'
@@ -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)
+
 
         if pos in after:
             postout += after.pop(pos)
@@ -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
+
     @staticmethod
     def rematch(el, l):
         try: