Patchwork [3,of,4,V2] run-tests: support per-line conditional output in tests

login
register
mail settings
Submitter Matt Harbison
Date April 7, 2017, 1:11 a.m.
Message ID <8e3cf915084723551d0b.1491527502@Envy>
Download mbox | patch
Permalink /patch/19980/
State Accepted
Headers show

Comments

Matt Harbison - April 7, 2017, 1:11 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1491448647 14400
#      Wed Apr 05 23:17:27 2017 -0400
# Node ID 8e3cf915084723551d0b68094cede7e65f49cf39
# Parent  7c5e861f39fbffdfae0e31d1edba419a62391afe
run-tests: support per-line conditional output in tests

Duplicating entire tests just because the output is different is both error
prone and can make the tests harder to read.  This harnesses the existing '(?)'
infrastructure, both to improve readability, and because it seemed like the path
of least resistance.

The form is:

  $ test_cmd
  output (hghave-feature !) # required if hghave.has_feature(), else optional
  out2 (no-hghave-feature2 !) # req if not hghave.has_feature2(), else optional

I originally extended the '(?)' syntax.  For example, this:

  2 r4/.hg/cache/checkisexec (execbit ?)

pretty naturally reads as "checkisexec, if execbit".  In some ways though, this
inverts the meaning of '?'.  For '(?)', the line is purely optional.  In the
example, it is mandatory iff execbit.  Otherwise, it is carried forward as
optional, to preserve the test output.  I tried it the other way, (listing
'no-exec' in the example), but that is too confusing to read.  Kostia suggested
using '!', and that seems fine.
Ryan McElroy - April 7, 2017, 9:23 a.m.
On 4/7/17 2:11 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1491448647 14400
> #      Wed Apr 05 23:17:27 2017 -0400
> # Node ID 8e3cf915084723551d0b68094cede7e65f49cf39
> # Parent  7c5e861f39fbffdfae0e31d1edba419a62391afe
> run-tests: support per-line conditional output in tests

This series looks like a huge win for test de-duplication! Thanks for 
working on it.

>
> Duplicating entire tests just because the output is different is both error
> prone and can make the tests harder to read.  This harnesses the existing '(?)'
> infrastructure, both to improve readability, and because it seemed like the path
> of least resistance.
>
> The form is:
>
>    $ test_cmd
>    output (hghave-feature !) # required if hghave.has_feature(), else optional
>    out2 (no-hghave-feature2 !) # req if not hghave.has_feature2(), else optional

My only question is: why is it optional if the feature is not present? 
It seems like maybe we should have two things:

! = must be present iff feature
!? = must be present if feature

(NOTE: that the first is two-f "iff" -- as in "if and only if" -- so if 
the feature is not present, and the line is still there, that would be a 
failure. The second one is the one-f "if", as in, "optional if not true".)

Regardless, I think this could be done as a follow-up and I only if it's 
a good idea. I've marked this series as pre-reviewed in patchwork.

>
> I originally extended the '(?)' syntax.  For example, this:
>
>    2 r4/.hg/cache/checkisexec (execbit ?)
>
> pretty naturally reads as "checkisexec, if execbit".  In some ways though, this
> inverts the meaning of '?'.  For '(?)', the line is purely optional.  In the
> example, it is mandatory iff execbit.  Otherwise, it is carried forward as
> optional, to preserve the test output.  I tried it the other way, (listing
> 'no-exec' in the example), but that is too confusing to read.  Kostia suggested
> using '!', and that seems fine.
>
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -497,6 +497,12 @@
>   # sans \t, \n and \r
>   CDATA_EVIL = re.compile(br"[\000-\010\013\014\016-\037]")
>   
> +# Match feature conditionalized output lines in the form, capturing the feature
> +# list in group 2, and the preceeding line output in group 1:
> +#
> +#   output..output (feature !)\n
> +optline = re.compile(b'(.+) \((.+?) !\)\n$')
> +
>   def cdatasafe(data):
>       """Make a string safe to include in a CDATA block.
>   
> @@ -1271,8 +1277,19 @@
>                       if r:
>                           els.pop(i)
>                           break
> -                    if el and el.endswith(b" (?)\n"):
> -                        optional.append(i)
> +                    if el:
> +                        if el.endswith(b" (?)\n"):
> +                            optional.append(i)
> +                        else:
> +                            m = optline.match(el)
> +                            if m:
> +                                conditions = [c for c in m.group(2).split(' ')]
> +
> +                                if self._hghave(conditions)[0]:
> +                                    lout = el
> +                                else:
> +                                    optional.append(i)
> +
>                       i += 1
>   
>                   if r:
> @@ -1298,8 +1315,10 @@
>                   # clean up any optional leftovers
>                   while expected.get(pos, None):
>                       el = expected[pos].pop(0)
> -                    if el and not el.endswith(b" (?)\n"):
> -                        break
> +                    if el:
> +                        if (not optline.match(el)
> +                            and not el.endswith(b" (?)\n")):
> +                            break
>                       postout.append(b'  ' + el)
>   
>               if lcmd:
> @@ -1371,6 +1390,12 @@
>               if el.endswith(b" (?)\n"):
>                   retry = "retry"
>                   el = el[:-5] + b"\n"
> +            else:
> +                m = optline.match(el)
> +                if m:
> +                    el = m.group(1) + b"\n"
> +                    retry = "retry"
> +
>               if el.endswith(b" (esc)\n"):
>                   if PYTHON3:
>                       el = el[:-7].decode('unicode_escape') + '\n'
> diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
> --- a/tests/test-run-tests.t
> +++ b/tests/test-run-tests.t
> @@ -39,6 +39,19 @@
>     $ rm hg
>   #endif
>   
> +Features for testing optional lines
> +===================================
> +
> +  $ cat > hghaveaddon.py <<EOF
> +  > import hghave
> +  > @hghave.check("custom", "custom hghave feature")
> +  > def has_custom():
> +  >     return True
> +  > @hghave.check("missing", "missing hghave feature")
> +  > def has_missing():
> +  >     return False
> +  > EOF
> +
>   an empty test
>   =======================
>   
> @@ -67,6 +80,13 @@
>     >   def (?)
>     >   456 (?)
>     >   xyz
> +  >   $ printf 'zyx\nwvu\ntsr\n'
> +  >   abc (?)
> +  >   zyx (custom !)
> +  >   wvu
> +  >   no_print (no-custom !)
> +  >   tsr (no-missing !)
> +  >   missing (missing !)
>     > EOF
>   
>     $ rt
> @@ -341,6 +361,12 @@
>     xyz
>     + echo *SALT* 15 0 (glob)
>     *SALT* 15 0 (glob)
> +  + printf 'zyx\nwvu\ntsr\n'
> +  zyx
> +  wvu
> +  tsr
> +  + echo *SALT* 22 0 (glob)
> +  *SALT* 22 0 (glob)
>     .
>     # Ran 2 tests, 0 skipped, 0 warned, 0 failed.
>   
>
Matt Harbison - April 7, 2017, 11:46 a.m.
> On Apr 7, 2017, at 5:23 AM, Ryan McElroy <rm@fb.com> wrote:
> 
>> On 4/7/17 2:11 AM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1491448647 14400
>> #      Wed Apr 05 23:17:27 2017 -0400
>> # Node ID 8e3cf915084723551d0b68094cede7e65f49cf39
>> # Parent  7c5e861f39fbffdfae0e31d1edba419a62391afe
>> run-tests: support per-line conditional output in tests
> 
> This series looks like a huge win for test de-duplication! Thanks for working on it.
> 
>> 
>> Duplicating entire tests just because the output is different is both error
>> prone and can make the tests harder to read.  This harnesses the existing '(?)'
>> infrastructure, both to improve readability, and because it seemed like the path
>> of least resistance.
>> 
>> The form is:
>> 
>>   $ test_cmd
>>   output (hghave-feature !) # required if hghave.has_feature(), else optional
>>   out2 (no-hghave-feature2 !) # req if not hghave.has_feature2(), else optional
> 
> My only question is: why is it optional if the feature is not present? It seems like maybe we should have two things:
> 
> ! = must be present iff feature
> !? = must be present if feature
> 
> (NOTE: that the first is two-f "iff" -- as in "if and only if" -- so if the feature is not present, and the line is still there, that would be a failure. The second one is the one-f "if", as in, "optional if not true".)

It's an inherent property of the (?) infrastructure that was used, rather than a design goal.  I have no problem with someone refining it like you propose.  But I think that splitting will only prevent cruft from accumulating (which isn't a bad thing). I'm struggling to come up with a scenario where we want to fail the iff case other than because the condition is unnecessary.

> Regardless, I think this could be done as a follow-up and I only if it's a good idea. I've marked this series as pre-reviewed in patchwork.
> 
>> 
>> I originally extended the '(?)' syntax.  For example, this:
>> 
>>   2 r4/.hg/cache/checkisexec (execbit ?)
>> 
>> pretty naturally reads as "checkisexec, if execbit".  In some ways though, this
>> inverts the meaning of '?'.  For '(?)', the line is purely optional.  In the
>> example, it is mandatory iff execbit.  Otherwise, it is carried forward as
>> optional, to preserve the test output.  I tried it the other way, (listing
>> 'no-exec' in the example), but that is too confusing to read.  Kostia suggested
>> using '!', and that seems fine.
>> 
>> diff --git a/tests/run-tests.py b/tests/run-tests.py
>> --- a/tests/run-tests.py
>> +++ b/tests/run-tests.py
>> @@ -497,6 +497,12 @@
>>  # sans \t, \n and \r
>>  CDATA_EVIL = re.compile(br"[\000-\010\013\014\016-\037]")
>>  +# Match feature conditionalized output lines in the form, capturing the feature
>> +# list in group 2, and the preceeding line output in group 1:
>> +#
>> +#   output..output (feature !)\n
>> +optline = re.compile(b'(.+) \((.+?) !\)\n$')
>> +
>>  def cdatasafe(data):
>>      """Make a string safe to include in a CDATA block.
>>  @@ -1271,8 +1277,19 @@
>>                      if r:
>>                          els.pop(i)
>>                          break
>> -                    if el and el.endswith(b" (?)\n"):
>> -                        optional.append(i)
>> +                    if el:
>> +                        if el.endswith(b" (?)\n"):
>> +                            optional.append(i)
>> +                        else:
>> +                            m = optline.match(el)
>> +                            if m:
>> +                                conditions = [c for c in m.group(2).split(' ')]
>> +
>> +                                if self._hghave(conditions)[0]:
>> +                                    lout = el
>> +                                else:
>> +                                    optional.append(i)
>> +
>>                      i += 1
>>                    if r:
>> @@ -1298,8 +1315,10 @@
>>                  # clean up any optional leftovers
>>                  while expected.get(pos, None):
>>                      el = expected[pos].pop(0)
>> -                    if el and not el.endswith(b" (?)\n"):
>> -                        break
>> +                    if el:
>> +                        if (not optline.match(el)
>> +                            and not el.endswith(b" (?)\n")):
>> +                            break
>>                      postout.append(b'  ' + el)
>>                if lcmd:
>> @@ -1371,6 +1390,12 @@
>>              if el.endswith(b" (?)\n"):
>>                  retry = "retry"
>>                  el = el[:-5] + b"\n"
>> +            else:
>> +                m = optline.match(el)
>> +                if m:
>> +                    el = m.group(1) + b"\n"
>> +                    retry = "retry"
>> +
>>              if el.endswith(b" (esc)\n"):
>>                  if PYTHON3:
>>                      el = el[:-7].decode('unicode_escape') + '\n'
>> diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
>> --- a/tests/test-run-tests.t
>> +++ b/tests/test-run-tests.t
>> @@ -39,6 +39,19 @@
>>    $ rm hg
>>  #endif
>>  +Features for testing optional lines
>> +===================================
>> +
>> +  $ cat > hghaveaddon.py <<EOF
>> +  > import hghave
>> +  > @hghave.check("custom", "custom hghave feature")
>> +  > def has_custom():
>> +  >     return True
>> +  > @hghave.check("missing", "missing hghave feature")
>> +  > def has_missing():
>> +  >     return False
>> +  > EOF
>> +
>>  an empty test
>>  =======================
>>  @@ -67,6 +80,13 @@
>>    >   def (?)
>>    >   456 (?)
>>    >   xyz
>> +  >   $ printf 'zyx\nwvu\ntsr\n'
>> +  >   abc (?)
>> +  >   zyx (custom !)
>> +  >   wvu
>> +  >   no_print (no-custom !)
>> +  >   tsr (no-missing !)
>> +  >   missing (missing !)
>>    > EOF
>>      $ rt
>> @@ -341,6 +361,12 @@
>>    xyz
>>    + echo *SALT* 15 0 (glob)
>>    *SALT* 15 0 (glob)
>> +  + printf 'zyx\nwvu\ntsr\n'
>> +  zyx
>> +  wvu
>> +  tsr
>> +  + echo *SALT* 22 0 (glob)
>> +  *SALT* 22 0 (glob)
>>    .
>>    # Ran 2 tests, 0 skipped, 0 warned, 0 failed.
>>  
>
Yuya Nishihara - April 7, 2017, 12:42 p.m.
On Thu, 06 Apr 2017 21:11:42 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1491448647 14400
> #      Wed Apr 05 23:17:27 2017 -0400
> # Node ID 8e3cf915084723551d0b68094cede7e65f49cf39
> # Parent  7c5e861f39fbffdfae0e31d1edba419a62391afe
> run-tests: support per-line conditional output in tests

> @@ -341,6 +361,12 @@
>    xyz
>    + echo *SALT* 15 0 (glob)
>    *SALT* 15 0 (glob)
> +  + printf 'zyx\nwvu\ntsr\n'

Globbed out quotes. IIRC, it is shell dependent.

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -497,6 +497,12 @@ 
 # sans \t, \n and \r
 CDATA_EVIL = re.compile(br"[\000-\010\013\014\016-\037]")
 
+# Match feature conditionalized output lines in the form, capturing the feature
+# list in group 2, and the preceeding line output in group 1:
+#
+#   output..output (feature !)\n
+optline = re.compile(b'(.+) \((.+?) !\)\n$')
+
 def cdatasafe(data):
     """Make a string safe to include in a CDATA block.
 
@@ -1271,8 +1277,19 @@ 
                     if r:
                         els.pop(i)
                         break
-                    if el and el.endswith(b" (?)\n"):
-                        optional.append(i)
+                    if el:
+                        if el.endswith(b" (?)\n"):
+                            optional.append(i)
+                        else:
+                            m = optline.match(el)
+                            if m:
+                                conditions = [c for c in m.group(2).split(' ')]
+
+                                if self._hghave(conditions)[0]:
+                                    lout = el
+                                else:
+                                    optional.append(i)
+
                     i += 1
 
                 if r:
@@ -1298,8 +1315,10 @@ 
                 # clean up any optional leftovers
                 while expected.get(pos, None):
                     el = expected[pos].pop(0)
-                    if el and not el.endswith(b" (?)\n"):
-                        break
+                    if el:
+                        if (not optline.match(el)
+                            and not el.endswith(b" (?)\n")):
+                            break
                     postout.append(b'  ' + el)
 
             if lcmd:
@@ -1371,6 +1390,12 @@ 
             if el.endswith(b" (?)\n"):
                 retry = "retry"
                 el = el[:-5] + b"\n"
+            else:
+                m = optline.match(el)
+                if m:
+                    el = m.group(1) + b"\n"
+                    retry = "retry"
+
             if el.endswith(b" (esc)\n"):
                 if PYTHON3:
                     el = el[:-7].decode('unicode_escape') + '\n'
diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
--- a/tests/test-run-tests.t
+++ b/tests/test-run-tests.t
@@ -39,6 +39,19 @@ 
   $ rm hg
 #endif
 
+Features for testing optional lines
+===================================
+
+  $ cat > hghaveaddon.py <<EOF
+  > import hghave
+  > @hghave.check("custom", "custom hghave feature")
+  > def has_custom():
+  >     return True
+  > @hghave.check("missing", "missing hghave feature")
+  > def has_missing():
+  >     return False
+  > EOF
+
 an empty test
 =======================
 
@@ -67,6 +80,13 @@ 
   >   def (?)
   >   456 (?)
   >   xyz
+  >   $ printf 'zyx\nwvu\ntsr\n'
+  >   abc (?)
+  >   zyx (custom !)
+  >   wvu
+  >   no_print (no-custom !)
+  >   tsr (no-missing !)
+  >   missing (missing !)
   > EOF
 
   $ rt
@@ -341,6 +361,12 @@ 
   xyz
   + echo *SALT* 15 0 (glob)
   *SALT* 15 0 (glob)
+  + printf 'zyx\nwvu\ntsr\n'
+  zyx
+  wvu
+  tsr
+  + echo *SALT* 22 0 (glob)
+  *SALT* 22 0 (glob)
   .
   # Ran 2 tests, 0 skipped, 0 warned, 0 failed.