Patchwork D7652: test: extract some generic data and utility from test-rust-ancestor.py

login
register
mail settings
Submitter phabricator
Date Dec. 13, 2019, 8:11 p.m.
Message ID <differential-rev-PHID-DREV-vz6pmtploofxdi5zethh-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43807/
State Superseded
Headers show

Comments

phabricator - Dec. 13, 2019, 8:11 p.m.
marmoute created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We will reuse this for more tests related to revlog index. In pratice this
  series of changesets add an index implementation provided from Rust and we want
  to be able to test it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7652

AFFECTED FILES
  mercurial/testing/revlog.py
  tests/test-rust-ancestor.py

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 17, 2019, 11:56 a.m.
pulkit added inline comments.

INLINE COMMENTS

> revlog.py:34
> +    cparsers is None,
> +    "The C Extension parsers module tests relies on is not available",
> +)

this and next message requires an oxford comma to help understand better.

> test-rust-ancestor.py:34
>  @unittest.skipIf(
> -    rustext is None or cparsers is None,
> +    rustext is None, "rustext module ancestor relies on is not available",
> +)

this is the next one.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7652/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7652

To: marmoute, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Dec. 17, 2019, 12:59 p.m.
marmoute added inline comments.

INLINE COMMENTS

> pulkit wrote in revlog.py:34
> this and next message requires an oxford comma to help understand better.

Can you clarify where you want a comma added ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7652/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7652

To: marmoute, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Dec. 18, 2019, 6:12 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> marmoute wrote in revlog.py:34
> Can you clarify where you want a comma added ?

"C", "Extension", "parsers", "module", and maybe also "tests" are all nouns and I had trouble figuring out if any of them were used as verbs here (maybe "parsers" should be "parses", I speculated). I think this is much clearer (assuming it's still accurate):

`The C version of the "parsers" module is not available. It is needed for this test.`

> pulkit wrote in test-rust-ancestor.py:34
> this is the next one.

`The Rust version of the "ancestor" module is not available. It is needed for this test.`

> test-rust-ancestor.py:38-39
> +    rustext is None,
>      "rustext or the C Extension parsers module "
>      "ancestor relies on is not available",
>  )

`The Rust or C version of the "parsers" module, which the "ancestor" module relies on, is not available.`

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7652/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7652

To: marmoute, #hg-reviewers
Cc: martinvonz, pulkit, mercurial-devel
phabricator - Dec. 20, 2019, 3:25 p.m.
pulkit added a comment.


  Amended the following in flight to make `test-check-code.t` happy.
  
    diff --git a/tests/test-rust-ancestor.py b/tests/test-rust-ancestor.py
    --- a/tests/test-rust-ancestor.py
    +++ b/tests/test-rust-ancestor.py
    @@ -33,12 +33,12 @@ except ImportError:
     @unittest.skipIf(
         rustext is None,
         'The Rust version of the "ancestor" module is not available. It is needed'
    -    'for this test.',
    +    ' for this test.',
     )
     @unittest.skipIf(
         rustext is None,
         'The Rust or C version of the "parsers" module, which the "ancestor" module'
    -    'relies on, is not available.',
    +    ' relies on, is not available.',
     )
     class rustancestorstest(revlogtesting.RevlogBasedTestBase):
         """Test the correctness of binding to Rust code.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7652/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7652

To: marmoute, #hg-reviewers, pulkit
Cc: martinvonz, pulkit, mercurial-devel

Patch

diff --git a/tests/test-rust-ancestor.py b/tests/test-rust-ancestor.py
--- a/tests/test-rust-ancestor.py
+++ b/tests/test-rust-ancestor.py
@@ -7,6 +7,8 @@ 
     node,
 )
 
+from mercurial.testing import revlog as revlogtesting
+
 try:
     from mercurial import rustext
 
@@ -27,34 +29,16 @@ 
 except ImportError:
     cparsers = None
 
-# picked from test-parse-index2, copied rather than imported
-# so that it stays stable even if test-parse-index2 changes or disappears.
-data_non_inlined = (
-    b'\x00\x00\x00\x01\x00\x00\x00\x00\x00\x01D\x19'
-    b'\x00\x07e\x12\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff'
-    b'\xff\xff\xff\xff\xd1\xf4\xbb\xb0\xbe\xfc\x13\xbd\x8c\xd3\x9d'
-    b'\x0f\xcd\xd9;\x8c\x07\x8cJ/\x00\x00\x00\x00\x00\x00\x00\x00\x00'
-    b'\x00\x00\x00\x00\x00\x00\x01D\x19\x00\x00\x00\x00\x00\xdf\x00'
-    b'\x00\x01q\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x00\xff'
-    b'\xff\xff\xff\xc1\x12\xb9\x04\x96\xa4Z1t\x91\xdfsJ\x90\xf0\x9bh'
-    b'\x07l&\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
-    b'\x00\x01D\xf8\x00\x00\x00\x00\x01\x1b\x00\x00\x01\xb8\x00\x00'
-    b'\x00\x01\x00\x00\x00\x02\x00\x00\x00\x01\xff\xff\xff\xff\x02\n'
-    b'\x0e\xc6&\xa1\x92\xae6\x0b\x02i\xfe-\xe5\xbao\x05\xd1\xe7\x00'
-    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01F'
-    b'\x13\x00\x00\x00\x00\x01\xec\x00\x00\x03\x06\x00\x00\x00\x01'
-    b'\x00\x00\x00\x03\x00\x00\x00\x02\xff\xff\xff\xff\x12\xcb\xeby1'
-    b'\xb6\r\x98B\xcb\x07\xbd`\x8f\x92\xd9\xc4\x84\xbdK\x00\x00\x00'
-    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00'
-)
-
 
 @unittest.skipIf(
-    rustext is None or cparsers is None,
+    rustext is None, "rustext module ancestor relies on is not available",
+)
+@unittest.skipIf(
+    rustext is None,
     "rustext or the C Extension parsers module "
     "ancestor relies on is not available",
 )
-class rustancestorstest(unittest.TestCase):
+class rustancestorstest(revlogtesting.RevlogBasedTestBase):
     """Test the correctness of binding to Rust code.
 
     This test is merely for the binding to Rust itself: extraction of
@@ -67,9 +51,6 @@ 
     Algorithmic correctness is asserted by the Rust unit tests.
     """
 
-    def parseindex(self):
-        return cparsers.parse_index2(data_non_inlined, False)[0]
-
     def testiteratorrevlist(self):
         idx = self.parseindex()
         # checking test assumption about the index binary data:
@@ -150,7 +131,9 @@ 
 
     def testgrapherror(self):
         data = (
-            data_non_inlined[: 64 + 27] + b'\xf2' + data_non_inlined[64 + 28 :]
+            revlogtesting.data_non_inlined[: 64 + 27]
+            + b'\xf2'
+            + revlogtesting.data_non_inlined[64 + 28 :]
         )
         idx = cparsers.parse_index2(data, False)[0]
         with self.assertRaises(rustext.GraphError) as arc:
diff --git a/mercurial/testing/revlog.py b/mercurial/testing/revlog.py
new file mode 100644
--- /dev/null
+++ b/mercurial/testing/revlog.py
@@ -0,0 +1,38 @@ 
+from __future__ import absolute_import
+import unittest
+
+# picked from test-parse-index2, copied rather than imported
+# so that it stays stable even if test-parse-index2 changes or disappears.
+data_non_inlined = (
+    b'\x00\x00\x00\x01\x00\x00\x00\x00\x00\x01D\x19'
+    b'\x00\x07e\x12\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff'
+    b'\xff\xff\xff\xff\xd1\xf4\xbb\xb0\xbe\xfc\x13\xbd\x8c\xd3\x9d'
+    b'\x0f\xcd\xd9;\x8c\x07\x8cJ/\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+    b'\x00\x00\x00\x00\x00\x00\x01D\x19\x00\x00\x00\x00\x00\xdf\x00'
+    b'\x00\x01q\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x00\xff'
+    b'\xff\xff\xff\xc1\x12\xb9\x04\x96\xa4Z1t\x91\xdfsJ\x90\xf0\x9bh'
+    b'\x07l&\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+    b'\x00\x01D\xf8\x00\x00\x00\x00\x01\x1b\x00\x00\x01\xb8\x00\x00'
+    b'\x00\x01\x00\x00\x00\x02\x00\x00\x00\x01\xff\xff\xff\xff\x02\n'
+    b'\x0e\xc6&\xa1\x92\xae6\x0b\x02i\xfe-\xe5\xbao\x05\xd1\xe7\x00'
+    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01F'
+    b'\x13\x00\x00\x00\x00\x01\xec\x00\x00\x03\x06\x00\x00\x00\x01'
+    b'\x00\x00\x00\x03\x00\x00\x00\x02\xff\xff\xff\xff\x12\xcb\xeby1'
+    b'\xb6\r\x98B\xcb\x07\xbd`\x8f\x92\xd9\xc4\x84\xbdK\x00\x00\x00'
+    b'\x00\x00\x00\x00\x00\x00\x00\x00\x00'
+)
+
+
+try:
+    from ..cext import parsers as cparsers
+except ImportError:
+    cparsers = None
+
+
+@unittest.skipIf(
+    cparsers is None,
+    "The C Extension parsers module tests relies on is not available",
+)
+class RevlogBasedTestBase(unittest.TestCase):
+    def parseindex(self):
+        return cparsers.parse_index2(data_non_inlined, False)[0]