Patchwork [5,of,5,pure-fix] tests: fix test-bdiff to handle variance between pure and c bdiff code

login
register
mail settings
Submitter Augie Fackler
Date Dec. 15, 2016, 4:32 p.m.
Message ID <774639bc892b7576a53a.1481819557@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/17925/
State Accepted
Headers show

Comments

Augie Fackler - Dec. 15, 2016, 4:32 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1481818440 18000
#      Thu Dec 15 11:14:00 2016 -0500
# Node ID 774639bc892b7576a53a672dd4a4492a536f8f0f
# Parent  afa152bbdf097667c095f054d78463bf6d8a666a
tests: fix test-bdiff to handle variance between pure and c bdiff code

Obviously we'd rather patch pure to have the same algorithmic win as
the C code, but this is a quick fix for the pure build since pure
isn't wrong, just not as fast as it could be.
Pierre-Yves David - Dec. 15, 2016, 8:38 p.m.
On 12/15/2016 05:32 PM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1481818440 18000
> #      Thu Dec 15 11:14:00 2016 -0500
> # Node ID 774639bc892b7576a53a672dd4a4492a536f8f0f
> # Parent  afa152bbdf097667c095f054d78463bf6d8a666a
> tests: fix test-bdiff to handle variance between pure and c bdiff code
>
> Obviously we'd rather patch pure to have the same algorithmic win as
> the C code, but this is a quick fix for the pure build since pure
> isn't wrong, just not as fast as it could be.

Okay, the test upgrade looks good², and I would take it if except for 
the import checker complaining about the first patch. I've sent a fix 
for the import checker¹:

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-December/091525.html

I'll push this series once that checker fix is in, unless someone else 
does it before me.

> diff --git a/tests/test-bdiff.py b/tests/test-bdiff.py
> --- a/tests/test-bdiff.py
> +++ b/tests/test-bdiff.py
> @@ -81,18 +81,28 @@ class BdiffTests(unittest.TestCase):
>                'x\n\n',
>                diffreplace(9, 9, '', 'y\n\n'),
>                'x\n\nz\n']),
> -            # we should pick up abbbc. rather than bc.de as the longest match
> -            ("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
> -             "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n",
> -             ['a\nb\nb\n',
> -              diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
> -              'b\nc\n.\nd\ne\n',
> -              diffreplace(16, 18, '.\n', ''),
> -              'f\n']),
>          ]
>          for old, new, want in cases:
>              self.assertEqual(self.showdiff(old, new), want)
>
> +    def test_issue1295_varies_on_pure(self):
> +            # we should pick up abbbc. rather than bc.de as the longest match
> +        got = self.showdiff("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
> +                            "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n")
> +        want_c = ['a\nb\nb\n',
> +                  diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
> +                  'b\nc\n.\nd\ne\n',
> +                  diffreplace(16, 18, '.\n', ''),
> +                  'f\n']
> +        want_pure = [diffreplace(0, 0, '', 'a\nb\nb\n'),
> +                     'a\nb\nb\nb\nc\n.\n',
> +                     diffreplace(12, 12, '', 'b\nc\n.\n'),
> +                     'd\ne\n',
> +                     diffreplace(16, 18, '.\n', ''), 'f\n']
> +        self.assert_(got in (want_c, want_pure),
> +                     'got: %r, wanted either %r or %r' % (
> +                         got, want_c, want_pure))
> +
>      def test_fixws(self):
>          cases = [
>              (" \ta\r b\t\n", "ab\n", 1),
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Dec. 15, 2016, 9:36 p.m.
> On Dec 15, 2016, at 3:38 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 12/15/2016 05:32 PM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1481818440 18000
>> #      Thu Dec 15 11:14:00 2016 -0500
>> # Node ID 774639bc892b7576a53a672dd4a4492a536f8f0f
>> # Parent  afa152bbdf097667c095f054d78463bf6d8a666a
>> tests: fix test-bdiff to handle variance between pure and c bdiff code
>> 
>> Obviously we'd rather patch pure to have the same algorithmic win as
>> the C code, but this is a quick fix for the pure build since pure
>> isn't wrong, just not as fast as it could be.
> 
> Okay, the test upgrade looks good², and I would take it if except for the import checker complaining about the first patch. I've sent a fix for the import checker¹:
> 
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-December/091525.html
> 
> I'll push this series once that checker fix is in, unless someone else does it before me.
> 
>> diff --git a/tests/test-bdiff.py b/tests/test-bdiff.py
>> --- a/tests/test-bdiff.py
>> +++ b/tests/test-bdiff.py
>> @@ -81,18 +81,28 @@ class BdiffTests(unittest.TestCase):
>>               'x\n\n',
>>               diffreplace(9, 9, '', 'y\n\n'),
>>               'x\n\nz\n']),
>> -            # we should pick up abbbc. rather than bc.de as the longest match
>> -            ("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
>> -             "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n",
>> -             ['a\nb\nb\n',
>> -              diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
>> -              'b\nc\n.\nd\ne\n',
>> -              diffreplace(16, 18, '.\n', ''),
>> -              'f\n']),
>>         ]
>>         for old, new, want in cases:
>>             self.assertEqual(self.showdiff(old, new), want)
>> 
>> +    def test_issue1295_varies_on_pure(self):
>> +            # we should pick up abbbc. rather than bc.de as the longest match
>> +        got = self.showdiff("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
>> +                            "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n")
>> +        want_c = ['a\nb\nb\n',
>> +                  diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
>> +                  'b\nc\n.\nd\ne\n',
>> +                  diffreplace(16, 18, '.\n', ''),
>> +                  'f\n']
>> +        want_pure = [diffreplace(0, 0, '', 'a\nb\nb\n'),
>> +                     'a\nb\nb\nb\nc\n.\n',
>> +                     diffreplace(12, 12, '', 'b\nc\n.\n'),
>> +                     'd\ne\n',
>> +                     diffreplace(16, 18, '.\n', ''), 'f\n']
>> +        self.assert_(got in (want_c, want_pure),
>> +                     'got: %r, wanted either %r or %r' % (
>> +                         got, want_c, want_pure))
>> +
>>     def test_fixws(self):
>>         cases = [
>>             (" \ta\r b\t\n", "ab\n", 1),
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
> 
> -- 
> Pierre-Yves David
> 
> [1] You don't seems to be in front a screen anymore so falling back to the normal process.
> 
> [2] (If I had done it, would have probably tested for "pure" and pinned the result, but this is good enough and fixes the test.)

I wasn’t sure offhand how to test for pure-mode, and I finished the series while in meetings, so I just did something that I knew would work. It’d be fine with me to push an alternate version of patch 5 with the refactoring in 1-4 done. Up to you though!

Patch

diff --git a/tests/test-bdiff.py b/tests/test-bdiff.py
--- a/tests/test-bdiff.py
+++ b/tests/test-bdiff.py
@@ -81,18 +81,28 @@  class BdiffTests(unittest.TestCase):
               'x\n\n',
               diffreplace(9, 9, '', 'y\n\n'),
               'x\n\nz\n']),
-            # we should pick up abbbc. rather than bc.de as the longest match
-            ("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
-             "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n",
-             ['a\nb\nb\n',
-              diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
-              'b\nc\n.\nd\ne\n',
-              diffreplace(16, 18, '.\n', ''),
-              'f\n']),
         ]
         for old, new, want in cases:
             self.assertEqual(self.showdiff(old, new), want)
 
+    def test_issue1295_varies_on_pure(self):
+            # we should pick up abbbc. rather than bc.de as the longest match
+        got = self.showdiff("a\nb\nb\nb\nc\n.\nd\ne\n.\nf\n",
+                            "a\nb\nb\na\nb\nb\nb\nc\n.\nb\nc\n.\nd\ne\nf\n")
+        want_c = ['a\nb\nb\n',
+                  diffreplace(6, 6, '', 'a\nb\nb\nb\nc\n.\n'),
+                  'b\nc\n.\nd\ne\n',
+                  diffreplace(16, 18, '.\n', ''),
+                  'f\n']
+        want_pure = [diffreplace(0, 0, '', 'a\nb\nb\n'),
+                     'a\nb\nb\nb\nc\n.\n',
+                     diffreplace(12, 12, '', 'b\nc\n.\n'),
+                     'd\ne\n',
+                     diffreplace(16, 18, '.\n', ''), 'f\n']
+        self.assert_(got in (want_c, want_pure),
+                     'got: %r, wanted either %r or %r' % (
+                         got, want_c, want_pure))
+
     def test_fixws(self):
         cases = [
             (" \ta\r b\t\n", "ab\n", 1),