Patchwork [6,of,6] run-tests: use a file for capturing replacements

login
register
mail settings
Submitter Gregory Szorc
Date March 29, 2015, 3:15 a.m.
Message ID <03d1333662fb83144a7d.1427598911@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/8341/
State Accepted
Headers show

Comments

Gregory Szorc - March 29, 2015, 3:15 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1427575392 25200
#      Sat Mar 28 13:43:12 2015 -0700
# Node ID 03d1333662fb83144a7dac9de798c82902ceb30b
# Parent  f62a75b235644da799d0f43e492e55aca2e66206
run-tests: use a file for capturing replacements

Currently, the test runner defines a strict set of regular expression
substitutions that will be performed on the test output. Up to this
point, if we needed a new substitution in a test, we would modify the
test harness to add a global substitution. If you did not want to add
a global substitution, you would need to add a glob or regular
expression match to each line containing variable output.

As part of developing 3rd party Mercurial extensions, I semi-frequently
found myself having to add glob and regular expression matching on a
large number of lines to handle variable output. For example, I have
a number of tests that run Docker containers and the hostname of the
started container is variable between test runs. I'd strongly prefer
to set the hostname to a variable and use that variable as a
placeholder in the test output. This results in more accurate testing
than globs and regular expressions, which match potentially more things
than they should.

This patch changes the replacements mechanism so it is file based.
Tests can write new replacements to the file defined by the
$REPLACEMENTS environment variable.

The format of this file is a bit wonky. Entries span 3 lines. The
reason for this is that other delimiters could appear in regular
expressions. Newlines are a relatively safe delimiter compared to
spaces, commas, etc and are easier to write than tabs (escaping
issues). Still, I'm not convinced this is the best solution. This
implementation detail should be bikeshedded.

I'm sure there is a use case for this feature in an existing test. But
I was lazy and just implemented a basic test in test-run-tests.t (which
needs to be done anyway).
Augie Fackler - March 30, 2015, 6:45 p.m.
On Sat, Mar 28, 2015 at 08:15:11PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1427575392 25200
> #      Sat Mar 28 13:43:12 2015 -0700
> # Node ID 03d1333662fb83144a7dac9de798c82902ceb30b
> # Parent  f62a75b235644da799d0f43e492e55aca2e66206
> run-tests: use a file for capturing replacements
>

[...] Overall, I like this, but...

>  #endif
> +
> +Replacements written to replacements file are substituted
> +
> +  $ cat > test-replacement.t << EOF
> +  >   $ cat >> \$REPLACEMENTS << EOF
> +  >   > re
> +  >   > REPLACE_ME
> +  >   > REPLACED
> +  >   > EOF
> +  >   $ echo REPLACE_ME
> +  >   REPLACED
> +  > EOF
> +
> +  $ $TESTDIR/run-tests.py --with-hg=`which hg` test-replacement.t
> +  .
> +  # Ran 1 tests, 0 skipped, 0 warned, 0 failed.

I'm not in love with the way tests can define replacements and then they're:

1) permanent and
2) global to all tests

2 is the worrying part, really, because I can see it leading to some
very kooky test failures that would depend on the order in which tests
execute. Could I persuade you to have a --replacements-file= flag to
run-tests instead?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - March 30, 2015, 6:46 p.m.
On 03/28/2015 08:15 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1427575392 25200
> #      Sat Mar 28 13:43:12 2015 -0700
> # Node ID 03d1333662fb83144a7dac9de798c82902ceb30b
> # Parent  f62a75b235644da799d0f43e492e55aca2e66206
> run-tests: use a file for capturing replacements
>
> Currently, the test runner defines a strict set of regular expression
> substitutions that will be performed on the test output. Up to this
> point, if we needed a new substitution in a test, we would modify the
> test harness to add a global substitution. If you did not want to add
> a global substitution, you would need to add a glob or regular
> expression match to each line containing variable output.
>
> As part of developing 3rd party Mercurial extensions, I semi-frequently
> found myself having to add glob and regular expression matching on a
> large number of lines to handle variable output. For example, I have
> a number of tests that run Docker containers and the hostname of the
> started container is variable between test runs. I'd strongly prefer
> to set the hostname to a variable and use that variable as a
> placeholder in the test output. This results in more accurate testing
> than globs and regular expressions, which match potentially more things
> than they should.
>
> This patch changes the replacements mechanism so it is file based.
> Tests can write new replacements to the file defined by the
> $REPLACEMENTS environment variable.

I'm not sure about the variable name. I'm not fan of it, but I've not 
better idea.

> The format of this file is a bit wonky. Entries span 3 lines. The
> reason for this is that other delimiters could appear in regular
> expressions. Newlines are a relatively safe delimiter compared to
> spaces, commas, etc and are easier to write than tabs (escaping
> issues). Still, I'm not convinced this is the best solution. This
> implementation detail should be bikeshedded.

Not super fan either, is there a lightweight standard format we could 
use? (.ini file?)

> I'm sure there is a use case for this feature in an existing test. But
> I was lazy and just implemented a basic test in test-run-tests.t (which
> needs to be done anyway).

I like the overall idea especially the ability to alter it from the 
tests (but I'm not sure how it get reload).

Reading the description it was unclear to me how the existing 
replacement were preserved. But I see that the test runner is 
bootstrapping the file with them.

+1 for going in such direct
Gregory Szorc - March 30, 2015, 6:49 p.m.
On Mon, Mar 30, 2015 at 11:45 AM, Augie Fackler <raf@durin42.com> wrote:

> On Sat, Mar 28, 2015 at 08:15:11PM -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1427575392 25200
> > #      Sat Mar 28 13:43:12 2015 -0700
> > # Node ID 03d1333662fb83144a7dac9de798c82902ceb30b
> > # Parent  f62a75b235644da799d0f43e492e55aca2e66206
> > run-tests: use a file for capturing replacements
> >
>
> [...] Overall, I like this, but...
>
> >  #endif
> > +
> > +Replacements written to replacements file are substituted
> > +
> > +  $ cat > test-replacement.t << EOF
> > +  >   $ cat >> \$REPLACEMENTS << EOF
> > +  >   > re
> > +  >   > REPLACE_ME
> > +  >   > REPLACED
> > +  >   > EOF
> > +  >   $ echo REPLACE_ME
> > +  >   REPLACED
> > +  > EOF
> > +
> > +  $ $TESTDIR/run-tests.py --with-hg=`which hg` test-replacement.t
> > +  .
> > +  # Ran 1 tests, 0 skipped, 0 warned, 0 failed.
>
> I'm not in love with the way tests can define replacements and then
> they're:
>
> 1) permanent and
> 2) global to all tests
>
> 2 is the worrying part, really, because I can see it leading to some
> very kooky test failures that would depend on the order in which tests
> execute. Could I persuade you to have a --replacements-file= flag to
> run-tests instead?
>
>
Oh, making it global was definitely not my intent. v2 will definitely have
per-test replacements, like how DAEMON_PIDS works.

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -429,8 +429,9 @@  class Test(unittest.TestCase):
         self._ret = None
         self._out = None
         self._skipped = None
         self._testtmp = None
+        self._replacementspath = os.path.join(self._threadtmp, 'replacements')
 
         # If we're not in --debug mode and reference output file exists,
         # check test output against it.
         if debug:
@@ -475,8 +476,32 @@  class Test(unittest.TestCase):
                 # file.
                 if e.errno != errno.ENOENT:
                     raise
 
+        # Write built-in replacements to replacements file.
+        fh = open(self._replacementspath, 'wb')
+        def writeregex(search, replace):
+            fh.write('re\n')
+            fh.write('%s\n' % search)
+            fh.write('%s\n' % replace)
+
+        writeregex(r':%s\b' % self._startport, ':$HGPORT')
+        writeregex(r':%s\b' % (self._startport + 1), ':$HGPORT1')
+        writeregex(r':%s\b' % (self._startport + 2), ':$HGPORT2')
+        writeregex(r'(?m)^(saved backup bundle to .*\.hg)( \(glob\))?$',
+                   r'\1 (glob)')
+
+        if os.name == 'nt':
+            s = ''.join(
+                c.isalpha() and '[%s%s]' % (c.lower(), c.upper())
+                or c in '/\\' and r'[/\\]' or c.isdigit() and c or '\\' + c
+                for c in self._testtmp)
+            writeregex(s, '$TESTTMP')
+        else:
+            writeregex(re.escape(self._testtmp), '$TESTTMP')
+
+        fh.close()
+
     def run(self, result):
         """Run this test and report results against a TestResult instance."""
         # This function is extremely similar to unittest.TestCase.run(). Once
         # we require Python 2.7 (or at least its version of unittest), this
@@ -633,26 +658,23 @@  class Test(unittest.TestCase):
     def _getreplacements(self):
         """Obtain a mapping of text replacements to apply to test output.
 
         Test output needs to be normalized so it can be compared to expected
-        output. This function defines how some of that normalization will
-        occur.
+        output. This function retrieves a list of search and replacement
+        patterns and strings.
         """
-        r = [
-            (r':%s\b' % self._startport, ':$HGPORT'),
-            (r':%s\b' % (self._startport + 1), ':$HGPORT1'),
-            (r':%s\b' % (self._startport + 2), ':$HGPORT2'),
-            (r'(?m)^(saved backup bundle to .*\.hg)( \(glob\))?$',
-             r'\1 (glob)'),
-            ]
+        r = []
 
-        if os.name == 'nt':
-            r.append(
-                (''.join(c.isalpha() and '[%s%s]' % (c.lower(), c.upper()) or
-                    c in '/\\' and r'[/\\]' or c.isdigit() and c or '\\' + c
-                    for c in self._testtmp), '$TESTTMP'))
-        else:
-            r.append((re.escape(self._testtmp), '$TESTTMP'))
+        lines = open(self._replacementspath, 'rb').read().splitlines()
+        i = 0
+        while i < len(lines):
+            line = lines[i]
+            if line == 're':
+                r.append((lines[i + 1], lines[i + 2]))
+                i += 3
+            else:
+                raise ValueError('invalid line in replacements file: %s' %
+                                 line)
 
         return r
 
     def _getenv(self):
@@ -664,8 +686,9 @@  class Test(unittest.TestCase):
         env["HGPORT1"] = str(self._startport + 1)
         env["HGPORT2"] = str(self._startport + 2)
         env["HGRCPATH"] = os.path.join(self._threadtmp, '.hgrc')
         env["DAEMON_PIDS"] = os.path.join(self._threadtmp, 'daemon.pids')
+        env["REPLACEMENTS"] = self._replacementspath
         env["HGEDITOR"] = ('"' + sys.executable + '"'
                            + ' -c "import sys; sys.exit(0)"')
         env["HGMERGE"] = "internal:merge"
         env["HGUSER"]   = "test"
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
@@ -455,4 +455,20 @@  test for --json
       }
   } (no-eol)
 
 #endif
+
+Replacements written to replacements file are substituted
+
+  $ cat > test-replacement.t << EOF
+  >   $ cat >> \$REPLACEMENTS << EOF
+  >   > re
+  >   > REPLACE_ME
+  >   > REPLACED
+  >   > EOF
+  >   $ echo REPLACE_ME
+  >   REPLACED
+  > EOF
+
+  $ $TESTDIR/run-tests.py --with-hg=`which hg` test-replacement.t
+  .
+  # Ran 1 tests, 0 skipped, 0 warned, 0 failed.