Patchwork testing: allow Hypothesis tests to disable extensions

login
register
mail settings
Submitter David MacIver
Date Feb. 26, 2016, 5:16 p.m.
Message ID <9a28035d39f02fc7e03d.1456507005@laser-shark>
Download mbox | patch
Permalink /patch/13421/
State Accepted
Commit c1fbc92d6238169fc5b1b8dfe4faa0ff0a72fc2d
Delegated to: Martin von Zweigbergk
Headers show

Comments

David MacIver - Feb. 26, 2016, 5:16 p.m.
# HG changeset patch
# User David R. MacIver <david@drmaciver.com>
# Date 1456506949 0
#      Fri Feb 26 17:15:49 2016 +0000
# Node ID 9a28035d39f02fc7e03d0d376ce66998628a6877
# Parent  18680fed30581e7286a72b93af97de980a178d96
testing: allow Hypothesis tests to disable extensions

Doing this required the introduction of a mechanism for keeping
track of more general config in the test. At present this is only
used for extensions but it could be used more widely (e.g. to
control specific extension behaviour)

This greatly simplifies the extension management logic by introducing
a general notion of config, which we maintain ourselves and pass to
HG on every invocation.

This results in significantly less error prone test generation, and
also allows us to turn extensions off as well as on.

The logic that used an environment variable to rerun the tests with
an extension disabled now just edits the test file (in a fresh copy)
to remove these --config command line flags.
Martin von Zweigbergk - Feb. 26, 2016, 6:51 p.m.
On Fri, Feb 26, 2016 at 9:16 AM, David R. MacIver <david@drmaciver.com> wrote:
> # HG changeset patch
> # User David R. MacIver <david@drmaciver.com>
> # Date 1456506949 0
> #      Fri Feb 26 17:15:49 2016 +0000
> # Node ID 9a28035d39f02fc7e03d0d376ce66998628a6877
> # Parent  18680fed30581e7286a72b93af97de980a178d96
> testing: allow Hypothesis tests to disable extensions
>
> Doing this required the introduction of a mechanism for keeping
> track of more general config in the test. At present this is only
> used for extensions but it could be used more widely (e.g. to
> control specific extension behaviour)

Do you know that you can disable extensions with "--config
extensions.foo=!"? I don't know if that would simplify this patch or
if there are other reasons to keep track of config ourselves, but I
wanted to check before I review it in detail.
David MacIver - Feb. 27, 2016, 1:14 p.m.
I do, but I don't think it would simplify this patch.

The major reason for keeping track of config ourselves is that making the
relevant changes to the hgrc from within the generated .t file ended up
being really annoying, and this simplifies it significantly.

On 26 February 2016 at 18:51, Martin von Zweigbergk <martinvonz@google.com>
wrote:

> On Fri, Feb 26, 2016 at 9:16 AM, David R. MacIver <david@drmaciver.com>
> wrote:
> > # HG changeset patch
> > # User David R. MacIver <david@drmaciver.com>
> > # Date 1456506949 0
> > #      Fri Feb 26 17:15:49 2016 +0000
> > # Node ID 9a28035d39f02fc7e03d0d376ce66998628a6877
> > # Parent  18680fed30581e7286a72b93af97de980a178d96
> > testing: allow Hypothesis tests to disable extensions
> >
> > Doing this required the introduction of a mechanism for keeping
> > track of more general config in the test. At present this is only
> > used for extensions but it could be used more widely (e.g. to
> > control specific extension behaviour)
>
> Do you know that you can disable extensions with "--config
> extensions.foo=!"? I don't know if that would simplify this patch or
> if there are other reasons to keep track of config ourselves, but I
> wanted to check before I review it in detail.
>
Martin von Zweigbergk - Feb. 29, 2016, 5:56 a.m.
Pushed to the clowncopter, thanks.

On Fri, Feb 26, 2016 at 9:16 AM, David R. MacIver <david@drmaciver.com> wrote:
> diff -r 18680fed3058 -r 9a28035d39f0 tests/test-verify-repo-operations.py
> --- a/tests/test-verify-repo-operations.py      Wed Feb 24 13:20:06 2016 +0000
> +++ b/tests/test-verify-repo-operations.py      Fri Feb 26 17:15:49 2016 +0000
> @@ -190,29 +192,32 @@
>          e = None
>          if not self.failed:
>              try:
> -                for ext in (
> -                    self.all_extensions - self.non_skippable_extensions
> -                ):
> -                    try:
> -                        os.environ["SKIP_EXTENSION"] = ext
> -                        output = subprocess.check_output([
> -                            runtests, path, "--local",
> -                        ], stderr=subprocess.STDOUT)
> -                        assert "Ran 1 test" in output, output
> -                    finally:
> -                        del os.environ["SKIP_EXTENSION"]
>                  output = subprocess.check_output([
>                      runtests, path, "--local", "--pure"
>                  ], stderr=subprocess.STDOUT)
>                  assert "Ran 1 test" in output, output
> +                for ext in (
> +                    self.all_extensions - self.non_skippable_extensions
> +                ):
> +                    tf = os.path.join(testtmp, "test-generated-no-%s.t" % (
> +                        ext,
> +                    ))
> +                    with open(tf, 'w') as o:
> +                        for l in ttest.splitlines():
> +                            if l.startswith("  $ hg"):
> +                                l = l.replace(
> +                                    "--config %s=" % (
> +                                        extensionconfigkey(ext),), "")
> +                            o.write(l + os.linesep)
> +                    with open(tf, 'r') as r:
> +                        t = r.read()
> +                        assert ext not in t, t
> +                    output = subprocess.check_output([
> +                        runtests, tf, "--local",
> +                    ], stderr=subprocess.STDOUT)
> +                    assert "Ran 1 test" in output, output
>              except subprocess.CalledProcessError as e:

I guess I didn't think of asking on an earlier patch, but why is this
rerunning needed? I read something about Hypothesis trying to minimize
found examples. I would have thought that that would ideally produce a
test case without the call to addextension("unrelatedextension").
Simon Farnsworth - Feb. 29, 2016, 9 a.m.
On 29/02/2016, 05:56, "Mercurial-devel on behalf of Martin von Zweigbergk" <mercurial-devel-bounces@mercurial-scm.org on behalf of martinvonz@google.com> wrote:



>Pushed to the clowncopter, thanks.

>

>On Fri, Feb 26, 2016 at 9:16 AM, David R. MacIver <david@drmaciver.com> wrote:

>> diff -r 18680fed3058 -r 9a28035d39f0 tests/test-verify-repo-operations.py

>> --- a/tests/test-verify-repo-operations.py      Wed Feb 24 13:20:06 2016 +0000

>> +++ b/tests/test-verify-repo-operations.py      Fri Feb 26 17:15:49 2016 +0000

>> @@ -190,29 +192,32 @@

>>          e = None

>>          if not self.failed:

>>              try:

>> -                for ext in (

>> -                    self.all_extensions - self.non_skippable_extensions

>> -                ):

>> -                    try:

>> -                        os.environ["SKIP_EXTENSION"] = ext

>> -                        output = subprocess.check_output([

>> -                            runtests, path, "--local",

>> -                        ], stderr=subprocess.STDOUT)

>> -                        assert "Ran 1 test" in output, output

>> -                    finally:

>> -                        del os.environ["SKIP_EXTENSION"]

>>                  output = subprocess.check_output([

>>                      runtests, path, "--local", "--pure"

>>                  ], stderr=subprocess.STDOUT)

>>                  assert "Ran 1 test" in output, output

>> +                for ext in (

>> +                    self.all_extensions - self.non_skippable_extensions

>> +                ):

>> +                    tf = os.path.join(testtmp, "test-generated-no-%s.t" % (

>> +                        ext,

>> +                    ))

>> +                    with open(tf, 'w') as o:

>> +                        for l in ttest.splitlines():

>> +                            if l.startswith("  $ hg"):

>> +                                l = l.replace(

>> +                                    "--config %s=" % (

>> +                                        extensionconfigkey(ext),), "")

>> +                            o.write(l + os.linesep)

>> +                    with open(tf, 'r') as r:

>> +                        t = r.read()

>> +                        assert ext not in t, t

>> +                    output = subprocess.check_output([

>> +                        runtests, tf, "--local",

>> +                    ], stderr=subprocess.STDOUT)

>> +                    assert "Ran 1 test" in output, output

>>              except subprocess.CalledProcessError as e:

>

>I guess I didn't think of asking on an earlier patch, but why is this

>rerunning needed? I read something about Hypothesis trying to minimize

>found examples. I would have thought that that would ideally produce a

>test case without the call to addextension("unrelatedextension").


The intention here is to check whether an enabled extension changes behaviour unexpectedly. For example, we expect that a test case that uses watchman to speed up hg status will generate exactly the same results (albeit slower) if we run it without watchman enabled. If we see different results with watchman enabled, then we've found a bug. This is also why we don't disable all extensions; in theory, non_skippable_extensions contains the list of extensions that we know are required for this test to run.

Simon
David MacIver - Feb. 29, 2016, 9:48 a.m.
Right, what Simon said.

There is something of a question of how useful this is, as it does add a
fair bit of time to the test (your test runner is not fast). I'd probably
advocate for removing the rerunning functionality entirely if after a month
of heavy usage it hasn't found any bugs even though it was sortof the
original point of the exercise.

Having said that, I think the timing issue in shelve was found through
rerunning the final .t test more than once and getting non-deterministic
results, so there's at least some value to it right there.

On 29 February 2016 at 09:00, Simon Farnsworth <simonfar@fb.com> wrote:

> On 29/02/2016, 05:56, "Mercurial-devel on behalf of Martin von Zweigbergk"
> <mercurial-devel-bounces@mercurial-scm.org on behalf of
> martinvonz@google.com> wrote:
>
>
>
> >Pushed to the clowncopter, thanks.
> >
> >On Fri, Feb 26, 2016 at 9:16 AM, David R. MacIver <david@drmaciver.com>
> wrote:
> >> diff -r 18680fed3058 -r 9a28035d39f0
> tests/test-verify-repo-operations.py
> >> --- a/tests/test-verify-repo-operations.py      Wed Feb 24 13:20:06
> 2016 +0000
> >> +++ b/tests/test-verify-repo-operations.py      Fri Feb 26 17:15:49
> 2016 +0000
> >> @@ -190,29 +192,32 @@
> >>          e = None
> >>          if not self.failed:
> >>              try:
> >> -                for ext in (
> >> -                    self.all_extensions - self.non_skippable_extensions
> >> -                ):
> >> -                    try:
> >> -                        os.environ["SKIP_EXTENSION"] = ext
> >> -                        output = subprocess.check_output([
> >> -                            runtests, path, "--local",
> >> -                        ], stderr=subprocess.STDOUT)
> >> -                        assert "Ran 1 test" in output, output
> >> -                    finally:
> >> -                        del os.environ["SKIP_EXTENSION"]
> >>                  output = subprocess.check_output([
> >>                      runtests, path, "--local", "--pure"
> >>                  ], stderr=subprocess.STDOUT)
> >>                  assert "Ran 1 test" in output, output
> >> +                for ext in (
> >> +                    self.all_extensions - self.non_skippable_extensions
> >> +                ):
> >> +                    tf = os.path.join(testtmp,
> "test-generated-no-%s.t" % (
> >> +                        ext,
> >> +                    ))
> >> +                    with open(tf, 'w') as o:
> >> +                        for l in ttest.splitlines():
> >> +                            if l.startswith("  $ hg"):
> >> +                                l = l.replace(
> >> +                                    "--config %s=" % (
> >> +                                        extensionconfigkey(ext),), "")
> >> +                            o.write(l + os.linesep)
> >> +                    with open(tf, 'r') as r:
> >> +                        t = r.read()
> >> +                        assert ext not in t, t
> >> +                    output = subprocess.check_output([
> >> +                        runtests, tf, "--local",
> >> +                    ], stderr=subprocess.STDOUT)
> >> +                    assert "Ran 1 test" in output, output
> >>              except subprocess.CalledProcessError as e:
> >
> >I guess I didn't think of asking on an earlier patch, but why is this
> >rerunning needed? I read something about Hypothesis trying to minimize
> >found examples. I would have thought that that would ideally produce a
> >test case without the call to addextension("unrelatedextension").
>
> The intention here is to check whether an enabled extension changes
> behaviour unexpectedly. For example, we expect that a test case that uses
> watchman to speed up hg status will generate exactly the same results
> (albeit slower) if we run it without watchman enabled. If we see different
> results with watchman enabled, then we've found a bug. This is also why we
> don't disable all extensions; in theory, non_skippable_extensions contains
> the list of extensions that we know are required for this test to run.
>
> Simon
>

Patch

diff -r 18680fed3058 -r 9a28035d39f0 tests/test-verify-repo-operations.py
--- a/tests/test-verify-repo-operations.py	Wed Feb 24 13:20:06 2016 +0000
+++ b/tests/test-verify-repo-operations.py	Fri Feb 26 17:15:49 2016 +0000
@@ -97,6 +97,8 @@ 
     lambda s: s.encode('utf-8')
 )
 
+extensions = st.sampled_from(('shelve', 'mq', 'blackbox',))
+
 @contextmanager
 def acceptableerrors(*args):
     """Sometimes we know an operation we're about to perform might fail, and
@@ -151,15 +153,15 @@ 
         os.chdir(testtmp)
         self.log = []
         self.failed = False
+        self.configperrepo = {}
+        self.all_extensions = set()
+        self.non_skippable_extensions = set()
 
         self.mkdirp("repos")
         self.cd("repos")
         self.mkdirp("repo1")
         self.cd("repo1")
         self.hg("init")
-        self.extensions = {}
-        self.all_extensions = set()
-        self.non_skippable_extensions = set()
 
     def teardown(self):
         """On teardown we clean up after ourselves as usual, but we also
@@ -190,29 +192,32 @@ 
         e = None
         if not self.failed:
             try:
-                for ext in (
-                    self.all_extensions - self.non_skippable_extensions
-                ):
-                    try:
-                        os.environ["SKIP_EXTENSION"] = ext
-                        output = subprocess.check_output([
-                            runtests, path, "--local",
-                        ], stderr=subprocess.STDOUT)
-                        assert "Ran 1 test" in output, output
-                    finally:
-                        del os.environ["SKIP_EXTENSION"]
                 output = subprocess.check_output([
                     runtests, path, "--local", "--pure"
                 ], stderr=subprocess.STDOUT)
                 assert "Ran 1 test" in output, output
+                for ext in (
+                    self.all_extensions - self.non_skippable_extensions
+                ):
+                    tf = os.path.join(testtmp, "test-generated-no-%s.t" % (
+                        ext,
+                    ))
+                    with open(tf, 'w') as o:
+                        for l in ttest.splitlines():
+                            if l.startswith("  $ hg"):
+                                l = l.replace(
+                                    "--config %s=" % (
+                                        extensionconfigkey(ext),), "")
+                            o.write(l + os.linesep)
+                    with open(tf, 'r') as r:
+                        t = r.read()
+                        assert ext not in t, t
+                    output = subprocess.check_output([
+                        runtests, tf, "--local",
+                    ], stderr=subprocess.STDOUT)
+                    assert "Ran 1 test" in output, output
             except subprocess.CalledProcessError as e:
                 note(e.output)
-            finally:
-                os.unlink(path)
-                try:
-                    os.unlink(path + ".err")
-                except OSError:
-                    pass
         if self.failed or e is not None:
             with open(savefile, "wb") as o:
                 o.write(ttest)
@@ -244,7 +249,11 @@ 
         self.log.append("$ cd -- %s" % (pipes.quote(path),))
 
     def hg(self, *args):
-        self.command("hg", *args)
+        extra_flags = []
+        for key, value in self.config.items():
+            extra_flags.append("--config")
+            extra_flags.append("%s=%s" % (key, value))
+        self.command("hg", *(tuple(extra_flags) + args))
 
     def command(self, *args):
         self.log.append("$ " + ' '.join(map(pipes.quote, args)))
@@ -384,6 +393,10 @@ 
     def currentrepo(self):
         return os.path.basename(os.getcwd())
 
+    @property
+    def config(self):
+        return self.configperrepo.setdefault(self.currentrepo, {})
+
     @rule(
         target=repos,
         source=repos,
@@ -486,32 +499,20 @@ 
 
     # Section: Extension management
     def hasextension(self, extension):
-        repo = self.currentrepo
-        return repo in self.extensions and extension in self.extensions[repo]
+        return extensionconfigkey(extension) in self.config
 
     def commandused(self, extension):
         assert extension in self.all_extensions
         self.non_skippable_extensions.add(extension)
 
-    @rule(extension=st.sampled_from((
-        'shelve', 'mq', 'blackbox',
-    )))
+    @rule(extension=extensions)
     def addextension(self, extension):
         self.all_extensions.add(extension)
-        extensions = self.extensions.setdefault(self.currentrepo, set())
-        if extension in extensions:
-            return
-        extensions.add(extension)
-        if not os.path.exists(hgrc):
-            self.command("touch", hgrc)
-        with open(hgrc, 'a') as o:
-            line = "[extensions]\n%s=\n" % (extension,)
-            o.write(line)
-        for l in line.splitlines():
-            self.log.append((
-                '$ if test "$SKIP_EXTENSION" != "%s" ; '
-                'then echo %r >> %s; fi') % (
-                    extension, l, hgrc,))
+        self.config[extensionconfigkey(extension)] = ""
+
+    @rule(extension=extensions)
+    def removeextension(self, extension):
+        self.config.pop(extensionconfigkey(extension), None)
 
     # Section: Commands from the shelve extension
     @rule()
@@ -545,6 +546,9 @@ 
     def close(self):
         self.underlying.close()
 
+def extensionconfigkey(extension):
+    return "extensions." + extension
+
 settings.register_profile(
     'default',  settings(
         timeout=300,