Patchwork [3,of,3] run-tests: accept '\' vs '/' path differences without '(glob)'

login
register
mail settings
Submitter Matt Harbison
Date Dec. 10, 2017, 8:47 p.m.
Message ID <ebc7a05d9389989fc2e8.1512938865@Envy>
Download mbox | patch
Permalink /patch/26204/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 10, 2017, 8:47 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1512882971 18000
#      Sun Dec 10 00:16:11 2017 -0500
# Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
# Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
run-tests: accept '\' vs '/' path differences without '(glob)'

Having to constantly adjust these is a hassle.  It's easy for this to slip by
when not testing on Windows, and then when it happens on stable, the tests fail
for the next 3 months if we follow the rules for stable.

This works the same way the EOL differences are ignored, namely to adjust on the
fly and recheck on Windows.  I can't think of any situation where there would be
a '\' on Windows, a '/' elsewhere, and the '/' should be considered a failure on
Windows.

This fixes the obvious output problems where (glob) is missing.  Without this,
test-alias.t, test-remotenames.t and test-largefiles-misc.t are failing.  The
flip side (not handled by this) is the case where an unnecessary glob is
present.  There seems to be two separate behaviors.  cf300c1ad7bf is an example
of where the test has been autocorrecting (with output differences), and
d4ec69ff652a is an example where the test fails and reports 'no result code from
test'.  Hopefully those cases will become even more rare if people don't need to
guess at when a glob is needed for a Windows path.

It's probably unreasonable to submit a single patch that wipes out all of the
(glob) instances that were only used to hide path differences, given the churn
from other contributors.  Since their presence isn't harming the tests, these
can be removed through attrition.
Augie Fackler - Dec. 10, 2017, 9:48 p.m.
> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1512882971 18000
> #      Sun Dec 10 00:16:11 2017 -0500
> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
> run-tests: accept '\' vs '/' path differences without '(glob)’

series queued, many thanks

[…]

> It's probably unreasonable to submit a single patch that wipes out all of the
> (glob) instances that were only used to hide path differences, given the churn
> from other contributors.  Since their presence isn't harming the tests, these
> can be removed through attrition.

You could do it if you wanted with a skip-blame annotation as was done in d92dc725223b. Then we can pass the right filter to `hg annotate` and not see the noop changes. Not a requirement though, don’t feel obligated to do so.
Matt Harbison - Dec. 10, 2017, 10:39 p.m.
On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:

>
>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com>  
>> wrote:
>>
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1512882971 18000
>> #      Sun Dec 10 00:16:11 2017 -0500
>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>> run-tests: accept '\' vs '/' path differences without '(glob)’
>
> series queued, many thanks
>
> […]
>
>> It's probably unreasonable to submit a single patch that wipes out all  
>> of the
>> (glob) instances that were only used to hide path differences, given  
>> the churn
>> from other contributors.  Since their presence isn't harming the tests,  
>> these
>> can be removed through attrition.
>
> You could do it if you wanted with a skip-blame annotation as was done  
> in d92dc725223b. Then we can pass the right filter to `hg annotate` and  
> not see the noop changes. Not a requirement though, don’t feel obligated  
> to do so.

I hadn't thought of blame- I was more worried about import conflicts.   
Does skip-blame somehow help the import tools?

Probably a rough approximation:

$ grep '(glob)' *.t | grep -v '[*?]' | wc -l
1125

Without the negative filter, it's 3216.

(If I knew the equivalent regex, it should be trivial to pipe it through  
sed, and see what blows up.)
Augie Fackler - Dec. 10, 2017, 11:28 p.m.
> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:
> 
>> 
>>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1512882971 18000
>>> #      Sun Dec 10 00:16:11 2017 -0500
>>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>>> run-tests: accept '\' vs '/' path differences without '(glob)’
>> 
>> series queued, many thanks
>> 
>> […]
>> 
>>> It's probably unreasonable to submit a single patch that wipes out all of the
>>> (glob) instances that were only used to hide path differences, given the churn
>>> from other contributors.  Since their presence isn't harming the tests, these
>>> can be removed through attrition.
>> 
>> You could do it if you wanted with a skip-blame annotation as was done in d92dc725223b. Then we can pass the right filter to `hg annotate` and not see the noop changes. Not a requirement though, don’t feel obligated to do so.
> 
> I hadn't thought of blame- I was more worried about import conflicts.  Does skip-blame somehow help the import tools?

Sadly no, but I’ve often worked around issues like that by importing at the parent revision listed in the patch header and then using rebase so I get a proper merge tool. And we’d likely only get patches with such conflicts for a week or two...

> Probably a rough approximation:
> 
> $ grep '(glob)' *.t | grep -v '[*?]' | wc -l
> 1125
> 
> Without the negative filter, it's 3216.
> 
> (If I knew the equivalent regex, it should be trivial to pipe it through sed, and see what blows up.)

I’d probably write a tool in Python to do this replacement, because I’m lazy and it’d be less work for my brain.
Matt Harbison - Dec. 11, 2017, 5:24 a.m.
On Sun, 10 Dec 2017 18:28:03 -0500, Augie Fackler <raf@durin42.com> wrote:

>
>> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com>  
>> wrote:
>>
>> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com>  
>> wrote:
>>
>>>
>>>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com>  
>>>> wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1512882971 18000
>>>> #      Sun Dec 10 00:16:11 2017 -0500
>>>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>>>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>>>> run-tests: accept '\' vs '/' path differences without '(glob)’
>>>
>>> series queued, many thanks
>>>
>>> […]
>>>
>>>> It's probably unreasonable to submit a single patch that wipes out  
>>>> all of the
>>>> (glob) instances that were only used to hide path differences, given  
>>>> the churn
>>>> from other contributors.  Since their presence isn't harming the  
>>>> tests, these
>>>> can be removed through attrition.
>>>
>>> You could do it if you wanted with a skip-blame annotation as was done  
>>> in d92dc725223b. Then we can pass the right filter to `hg annotate`  
>>> and not see the noop changes. Not a requirement though, don’t feel  
>>> obligated to do so.

OK, done.  But 3 of 3 isn't showing up in the mail archive, and at ~290K  
for that patch, I would have expected a digest email too.  So something  
choked.  I'll try to check tomorrow around 8:30 EST and resend if needed,  
before too much more activity occurs.

Random thought: should these # skip-blame markers also imply a default  
skip in bisect?  It's probably too late to change the name now...
Augie Fackler - Dec. 11, 2017, 1:48 p.m.
> On Dec 11, 2017, at 00:24, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Sun, 10 Dec 2017 18:28:03 -0500, Augie Fackler <raf@durin42.com> wrote:
> 
>> 
>>> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:
>>> 
>>>> 
>>>>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>> 
>>>>> # HG changeset patch
>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>> # Date 1512882971 18000
>>>>> #      Sun Dec 10 00:16:11 2017 -0500
>>>>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>>>>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>>>>> run-tests: accept '\' vs '/' path differences without '(glob)’
>>>> 
>>>> series queued, many thanks
>>>> 
>>>> […]
>>>> 
>>>>> It's probably unreasonable to submit a single patch that wipes out all of the
>>>>> (glob) instances that were only used to hide path differences, given the churn
>>>>> from other contributors.  Since their presence isn't harming the tests, these
>>>>> can be removed through attrition.
>>>> 
>>>> You could do it if you wanted with a skip-blame annotation as was done in d92dc725223b. Then we can pass the right filter to `hg annotate` and not see the noop changes. Not a requirement though, don’t feel obligated to do so.
> 
> OK, done.  But 3 of 3 isn't showing up in the mail archive, and at ~290K for that patch, I would have expected a digest email too.  So something choked.  I'll try to check tomorrow around 8:30 EST and resend if needed, before too much more activity occurs.

It probably got stuck in moderation. I'll mod it through.

> 
> Random thought: should these # skip-blame markers also imply a default skip in bisect?  It's probably too late to change the name now...

probably not? It's not a default skip in blame, you have to specify the flag anyway...
via Mercurial-devel - Dec. 13, 2017, 5:40 a.m.
Is it this patch that broke test-run-tests.t? See
https://buildbot.mercurial-scm.org/builders/hg%20tests/builds/1357/steps/run-tests.py%20%28python%202.7.10%29/logs/stdio.
I haven't even tried to verify, but Matt and I were both on the blame
list for the broken build and it didn't seem related to anything I
did.

On Mon, Dec 11, 2017 at 5:48 AM, Augie Fackler <raf@durin42.com> wrote:
>
>> On Dec 11, 2017, at 00:24, Matt Harbison <mharbison72@gmail.com> wrote:
>>
>> On Sun, 10 Dec 2017 18:28:03 -0500, Augie Fackler <raf@durin42.com> wrote:
>>
>>>
>>>> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>
>>>> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:
>>>>
>>>>>
>>>>>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>>> # Date 1512882971 18000
>>>>>> #      Sun Dec 10 00:16:11 2017 -0500
>>>>>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>>>>>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>>>>>> run-tests: accept '\' vs '/' path differences without '(glob)’
>>>>>
>>>>> series queued, many thanks
>>>>>
>>>>> […]
>>>>>
>>>>>> It's probably unreasonable to submit a single patch that wipes out all of the
>>>>>> (glob) instances that were only used to hide path differences, given the churn
>>>>>> from other contributors.  Since their presence isn't harming the tests, these
>>>>>> can be removed through attrition.
>>>>>
>>>>> You could do it if you wanted with a skip-blame annotation as was done in d92dc725223b. Then we can pass the right filter to `hg annotate` and not see the noop changes. Not a requirement though, don’t feel obligated to do so.
>>
>> OK, done.  But 3 of 3 isn't showing up in the mail archive, and at ~290K for that patch, I would have expected a digest email too.  So something choked.  I'll try to check tomorrow around 8:30 EST and resend if needed, before too much more activity occurs.
>
> It probably got stuck in moderation. I'll mod it through.
>
>>
>> Random thought: should these # skip-blame markers also imply a default skip in bisect?  It's probably too late to change the name now...
>
> probably not? It's not a default skip in blame, you have to specify the flag anyway...
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Harbison - Dec. 13, 2017, 6:06 a.m.
> On Dec 13, 2017, at 12:40 AM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> Is it this patch that broke test-run-tests.t? See
> https://buildbot.mercurial-scm.org/builders/hg%20tests/builds/1357/steps/run-tests.py%20%28python%202.7.10%29/logs/stdio.
> I haven't even tried to verify, but Matt and I were both on the blame
> list for the broken build and it didn't seem related to anything I
> did.

My guess is that it’s this one:

https://www.mercurial-scm.org/repo/hg-all/rev/fd61bad1a338

I noticed the .replace() wasn’t doing anything useful when I tried adding the other patterns to common-patterns.py last night. Somehow, it not only passed on Windows and Linux for me, it provided the correct output when I eliminated the existing output.

Maybe try dropping that tuple in run-tests.py, since I don’t think it’s needed any more.  Sorry about that.


>> On Mon, Dec 11, 2017 at 5:48 AM, Augie Fackler <raf@durin42.com> wrote:
>> 
>>> On Dec 11, 2017, at 00:24, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> On Sun, 10 Dec 2017 18:28:03 -0500, Augie Fackler <raf@durin42.com> wrote:
>>> 
>>>> 
>>>>> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>> 
>>>>> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:
>>>>> 
>>>>>> 
>>>>>>> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>>>> 
>>>>>>> # HG changeset patch
>>>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>>>> # Date 1512882971 18000
>>>>>>> #      Sun Dec 10 00:16:11 2017 -0500
>>>>>>> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>>>>>>> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>>>>>>> run-tests: accept '\' vs '/' path differences without '(glob)’
>>>>>> 
>>>>>> series queued, many thanks
>>>>>> 
>>>>>> […]
>>>>>> 
>>>>>>> It's probably unreasonable to submit a single patch that wipes out all of the
>>>>>>> (glob) instances that were only used to hide path differences, given the churn
>>>>>>> from other contributors.  Since their presence isn't harming the tests, these
>>>>>>> can be removed through attrition.
>>>>>> 
>>>>>> You could do it if you wanted with a skip-blame annotation as was done in d92dc725223b. Then we can pass the right filter to `hg annotate` and not see the noop changes. Not a requirement though, don’t feel obligated to do so.
>>> 
>>> OK, done.  But 3 of 3 isn't showing up in the mail archive, and at ~290K for that patch, I would have expected a digest email too.  So something choked.  I'll try to check tomorrow around 8:30 EST and resend if needed, before too much more activity occurs.
>> 
>> It probably got stuck in moderation. I'll mod it through.
>> 
>>> 
>>> Random thought: should these # skip-blame markers also imply a default skip in bisect?  It's probably too late to change the name now...
>> 
>> probably not? It's not a default skip in blame, you have to specify the flag anyway...
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - Dec. 13, 2017, 6:28 p.m.
I think the problem is in this line from the test (that you changed):

  $ echo "  saved backup bundle to \$TESTTMP\foo.hg" >> test-failure.t

The '\' there probably just needs to be escaped. It seems like at
least dash and tcsh otherwise interpret it (but bash treats it
literally). Proof:

$ dash -c 'echo "  saved backup bundle to \$TESTTMP\foo.hg"'
  saved backup bundle to $TESTTMP
                                 oo.hg

I'll just change the '\f' to '\\f' in flight and we'll see if tests
pass when they run again.

On Tue, Dec 12, 2017 at 10:06 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>
> On Dec 13, 2017, at 12:40 AM, Martin von Zweigbergk <martinvonz@google.com>
> wrote:
>
> Is it this patch that broke test-run-tests.t? See
> https://buildbot.mercurial-scm.org/builders/hg%20tests/builds/1357/steps/run-tests.py%20%28python%202.7.10%29/logs/stdio.
> I haven't even tried to verify, but Matt and I were both on the blame
> list for the broken build and it didn't seem related to anything I
> did.
>
>
> My guess is that it’s this one:
>
> https://www.mercurial-scm.org/repo/hg-all/rev/fd61bad1a338
>
>
> I noticed the .replace() wasn’t doing anything useful when I tried adding
> the other patterns to common-patterns.py last night. Somehow, it not only
> passed on Windows and Linux for me, it provided the correct output when I
> eliminated the existing output.
>
>
> Maybe try dropping that tuple in run-tests.py, since I don’t think it’s
> needed any more.  Sorry about that.
>
>
>
> On Mon, Dec 11, 2017 at 5:48 AM, Augie Fackler <raf@durin42.com> wrote:
>
>
> On Dec 11, 2017, at 00:24, Matt Harbison <mharbison72@gmail.com> wrote:
>
>
> On Sun, 10 Dec 2017 18:28:03 -0500, Augie Fackler <raf@durin42.com> wrote:
>
>
>
> On Dec 10, 2017, at 5:39 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>
>
> On Sun, 10 Dec 2017 16:48:26 -0500, Augie Fackler <raf@durin42.com> wrote:
>
>
>
> On Dec 10, 2017, at 3:47 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>
>
> # HG changeset patch
>
> # User Matt Harbison <matt_harbison@yahoo.com>
>
> # Date 1512882971 18000
>
> #      Sun Dec 10 00:16:11 2017 -0500
>
> # Node ID ebc7a05d9389989fc2e81d30fa1c10dd01be9a76
>
> # Parent  e1119050f15e2169ac766e6a69eeab70729e9e40
>
> run-tests: accept '\' vs '/' path differences without '(glob)’
>
>
> series queued, many thanks
>
>
> […]
>
>
> It's probably unreasonable to submit a single patch that wipes out all of
> the
>
> (glob) instances that were only used to hide path differences, given the
> churn
>
> from other contributors.  Since their presence isn't harming the tests,
> these
>
> can be removed through attrition.
>
>
> You could do it if you wanted with a skip-blame annotation as was done in
> d92dc725223b. Then we can pass the right filter to `hg annotate` and not see
> the noop changes. Not a requirement though, don’t feel obligated to do so.
>
>
> OK, done.  But 3 of 3 isn't showing up in the mail archive, and at ~290K for
> that patch, I would have expected a digest email too.  So something choked.
> I'll try to check tomorrow around 8:30 EST and resend if needed, before too
> much more activity occurs.
>
>
> It probably got stuck in moderation. I'll mod it through.
>
>
>
> Random thought: should these # skip-blame markers also imply a default skip
> in bisect?  It's probably too late to change the name now...
>
>
> probably not? It's not a default skip in blame, you have to specify the flag
> anyway...
>
> _______________________________________________
>
> Mercurial-devel mailing list
>
> Mercurial-devel@mercurial-scm.org
>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -1451,10 +1451,7 @@ 
 
                     r = self.linematch(el, lout)
                     if isinstance(r, str):
-                        if r == '+glob':
-                            lout = el[:-1] + ' (glob)\n'
-                            r = '' # Warn only this line.
-                        elif r == '-glob':
+                        if r == '-glob':
                             lout = ''.join(el.rsplit(' (glob)', 1))
                             r = '' # Warn only this line.
                         elif r == "retry":
@@ -1613,7 +1610,7 @@ 
             if os.altsep:
                 _l = l.replace(b'\\', b'/')
                 if el == _l or os.name == 'nt' and el[:-1] + b'\r\n' == _l:
-                    return b'+glob'
+                    return True
         return retry
 
     @staticmethod
diff --git a/tests/test-run-tests.py b/tests/test-run-tests.py
--- a/tests/test-run-tests.py
+++ b/tests/test-run-tests.py
@@ -67,9 +67,9 @@ 
 
     missing glob
         >>> lm(b'/g/c/d/fg\n', b'\\g\\c\\d/fg\n')
-        'special: +glob'
+        True
         >>> lm(b'/g/c/d/fg\n', b'\\g\\c\\d\\fg\r\n')
-        'special: +glob'
+        True
 
     restore os.altsep
         >>> os.altsep = _osaltsep