Patchwork [1,of,4] tests: add a wrapper to run fsmonitor tests

login
register
mail settings
Submitter Siddharth Agarwal
Date June 10, 2017, 9:10 p.m.
Message ID <af12ded5040ae8b5becf.1497129019@devvm31800.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21319/
State Accepted
Headers show

Comments

Siddharth Agarwal - June 10, 2017, 9:10 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1497128850 25200
#      Sat Jun 10 14:07:30 2017 -0700
# Node ID af12ded5040ae8b5becf5c354aafe19949d93444
# Parent  776d077eb4ef815e08631fb1e7b33375adca3ef1
tests: add a wrapper to run fsmonitor tests

This script does a bunch of non-trivial configuration work: in particular, it
sets up an isolated instance of Watchman which isn't affected by global state
and can be torn down on completion.

This script also sets the HGFSMONITOR_TESTS environment variable, which hghave
will use in the next patch to allow gating on whether fsmonitor is enabled.

With fsmonitor enabled, there appear to be a number of failures in the test
suite. It's not yet clear to me why they're happening, but if someone would
like to jump in and fix some of them I hope this will be helpful for that.
Yuya Nishihara - June 11, 2017, 2:49 a.m.
On Sat, 10 Jun 2017 14:10:19 -0700, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1497128850 25200
> #      Sat Jun 10 14:07:30 2017 -0700
> # Node ID af12ded5040ae8b5becf5c354aafe19949d93444
> # Parent  776d077eb4ef815e08631fb1e7b33375adca3ef1
> tests: add a wrapper to run fsmonitor tests

Great. Queued these, thanks.

> With fsmonitor enabled, there appear to be a number of failures in the test
> suite. It's not yet clear to me why they're happening, but if someone would
> like to jump in and fix some of them I hope this will be helpful for that.

Maybe we'll need an option to run fsmonitor-run-tests.py without the
blacklist.

> +@contextlib.contextmanager
> +def watchman(args):
> +    basedir = tempfile.mkdtemp(prefix='hg-fsmonitor')
> +    try:
> +        # Much of this configuration is borrowed from Watchman's test harness.
> +        cfgfile = os.path.join(basedir, 'config.json')
> +        # TODO: allow setting a config
> +        with open(cfgfile, 'w') as f:
> +            f.write(json.dumps({}))
> +
> +        logfile = os.path.join(basedir, 'log')
> +        clilogfile = os.path.join(basedir, 'cli-log')
> +        if os.name == 'nt':
> +            sockfile = '\\\\.\\pipe\\watchman-test-%s' % uuid.uuid4().hex
> +        else:
> +            sockfile = os.path.join(basedir, 'sock')
> +        pidfile = os.path.join(basedir, 'pid')
> +        statefile = os.path.join(basedir, 'state')
> +
> +        argv = [
> +            args.watchman,
> +            '--sockname', sockfile,
> +            '--logfile', logfile,
> +            '--pidfile', pidfile,
> +            '--statefile', statefile,
> +            '--foreground',
> +            '--log-level=2', # debug logging for watchman
> +        ]
> +
> +        envb = osenvironb.copy()
> +        envb[b'WATCHMAN_CONFIG_FILE'] = _bytespath(cfgfile)
> +        with open(clilogfile, 'wb') as f:
> +            proc = subprocess.Popen(
> +                argv, env=envb, stdin=None, stdout=f, stderr=f)
> +            try:
> +                yield sockfile
> +            finally:
> +                proc.terminate()
> +                proc.kill()

Perhaps it's better to wait the process termination by proc.communicate()
or proc.wait().
Siddharth Agarwal - June 12, 2017, 3:11 a.m.
On 6/10/17 7:49 PM, Yuya Nishihara wrote:
> Perhaps it's better to wait the process termination by proc.communicate()
> or proc.wait().


Unfortunately, in this case we can't really communicate() to it since 
it'll keep running unless it is told to shutdown or terminated.

One interesting option would be to send a shutdown-server command over 
the domain socket.
Yuya Nishihara - June 12, 2017, 12:37 p.m.
On Sun, 11 Jun 2017 20:11:31 -0700, Siddharth Agarwal wrote:
> On 6/10/17 7:49 PM, Yuya Nishihara wrote:
> > Perhaps it's better to wait the process termination by proc.communicate()
> > or proc.wait().
> 
> 
> Unfortunately, in this case we can't really communicate() to it since 
> it'll keep running unless it is told to shutdown or terminated.

I meant it would be slightly better to collect the child process after the
kill.

  proc.kill()
  proc.communicate()  # close() pipe and waitpid()

> One interesting option would be to send a shutdown-server command over
> the domain socket.

That sounds nicer, but I guess SIGTERM would be good enough.

Patch

diff --git a/tests/blacklists/fsmonitor b/tests/blacklists/fsmonitor
--- a/tests/blacklists/fsmonitor
+++ b/tests/blacklists/fsmonitor
@@ -1,7 +1,5 @@ 
 # Blacklist for a full testsuite run with fsmonitor enabled.
-# Use with
-#     run-tests --blacklist=blacklists/fsmonitor \
-#         --extra-config="extensions.fsmonitor="
+# Used by fsmonitor-run-tests.
 # The following tests all fail because they either use extensions that conflict
 # with fsmonitor, use subrepositories, or don't anticipate the extra file in
 # the .hg directory that fsmonitor adds.
diff --git a/tests/fsmonitor-run-tests.py b/tests/fsmonitor-run-tests.py
new file mode 100755
--- /dev/null
+++ b/tests/fsmonitor-run-tests.py
@@ -0,0 +1,133 @@ 
+#!/usr/bin/env python
+
+# fsmonitor-run-tests.py - Run Mercurial tests with fsmonitor enabled
+#
+# Copyright 2017 Facebook, Inc.
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2 or any later version.
+#
+# This is a wrapper around run-tests.py that spins up an isolated instance of
+# Watchman and runs the Mercurial tests against it. This ensures that the global
+# version of Watchman isn't affected by anything this test does.
+
+from __future__ import absolute_import
+from __future__ import print_function
+
+import argparse
+import contextlib
+import json
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+import uuid
+
+osenvironb = getattr(os, 'environb', os.environ)
+
+if sys.version_info > (3, 5, 0):
+    PYTHON3 = True
+    xrange = range # we use xrange in one place, and we'd rather not use range
+    def _bytespath(p):
+        return p.encode('utf-8')
+
+elif sys.version_info >= (3, 0, 0):
+    print('%s is only supported on Python 3.5+ and 2.7, not %s' %
+          (sys.argv[0], '.'.join(str(v) for v in sys.version_info[:3])))
+    sys.exit(70) # EX_SOFTWARE from `man 3 sysexit`
+else:
+    PYTHON3 = False
+
+    # In python 2.x, path operations are generally done using
+    # bytestrings by default, so we don't have to do any extra
+    # fiddling there. We define the wrapper functions anyway just to
+    # help keep code consistent between platforms.
+    def _bytespath(p):
+        return p
+
+def getparser():
+    """Obtain the argument parser used by the CLI."""
+    parser = argparse.ArgumentParser(
+        description='Run tests with fsmonitor enabled.',
+        epilog='Unrecognized options are passed to run-tests.py.')
+    # - keep these sorted
+    # - none of these options should conflict with any in run-tests.py
+    parser.add_argument('--keep-fsmonitor-tmpdir', action='store_true',
+        help='keep temporary directory with fsmonitor state')
+    parser.add_argument('--watchman',
+        help='location of watchman binary (default: watchman in PATH)',
+        default='watchman')
+
+    return parser
+
+@contextlib.contextmanager
+def watchman(args):
+    basedir = tempfile.mkdtemp(prefix='hg-fsmonitor')
+    try:
+        # Much of this configuration is borrowed from Watchman's test harness.
+        cfgfile = os.path.join(basedir, 'config.json')
+        # TODO: allow setting a config
+        with open(cfgfile, 'w') as f:
+            f.write(json.dumps({}))
+
+        logfile = os.path.join(basedir, 'log')
+        clilogfile = os.path.join(basedir, 'cli-log')
+        if os.name == 'nt':
+            sockfile = '\\\\.\\pipe\\watchman-test-%s' % uuid.uuid4().hex
+        else:
+            sockfile = os.path.join(basedir, 'sock')
+        pidfile = os.path.join(basedir, 'pid')
+        statefile = os.path.join(basedir, 'state')
+
+        argv = [
+            args.watchman,
+            '--sockname', sockfile,
+            '--logfile', logfile,
+            '--pidfile', pidfile,
+            '--statefile', statefile,
+            '--foreground',
+            '--log-level=2', # debug logging for watchman
+        ]
+
+        envb = osenvironb.copy()
+        envb[b'WATCHMAN_CONFIG_FILE'] = _bytespath(cfgfile)
+        with open(clilogfile, 'wb') as f:
+            proc = subprocess.Popen(
+                argv, env=envb, stdin=None, stdout=f, stderr=f)
+            try:
+                yield sockfile
+            finally:
+                proc.terminate()
+                proc.kill()
+    finally:
+        if args.keep_fsmonitor_tmpdir:
+            print('fsmonitor dir available at %s' % basedir)
+        else:
+            shutil.rmtree(basedir, ignore_errors=True)
+
+def run():
+    parser = getparser()
+    args, runtestsargv = parser.parse_known_args()
+
+    with watchman(args) as sockfile:
+        osenvironb[b'WATCHMAN_SOCK'] = _bytespath(sockfile)
+        # Indicate to hghave that we're running with fsmonitor enabled.
+        osenvironb[b'HGFSMONITOR_TESTS'] = b'1'
+
+        runtestdir = os.path.dirname(__file__)
+        runtests = os.path.join(runtestdir, 'run-tests.py')
+        blacklist = os.path.join(runtestdir, 'blacklists', 'fsmonitor')
+
+        runtestsargv.insert(0, runtests)
+        runtestsargv.extend([
+            '--extra-config',
+            'extensions.fsmonitor=',
+            '--blacklist',
+            blacklist,
+        ])
+
+        return subprocess.call(runtestsargv)
+
+if __name__ == '__main__':
+    sys.exit(run())