Merge lp:~gagern/bzr/bash_completion-ExecutableFeature into lp:bzr

[Martin von Gagern]

Generalise probing for executables used in selftest.

If the bash_completion plugin gets merged into the bzr source tree, here is a subsequent testing framework improvement that might benefit tests for other commands and plugins as well. The class ExecutableFeature checks for the availability of a named executable on the PATH.

[lp:~gagern/bzr/bash_completion-ExecutableFeature]

5169. By Martin von Gagern on 2010-05-18

Handle empty PATH elements in ExecutableFeature.

[John A Meinel (jameinel)]

Note that I think you could do this using a 'subprocess.Popen()' style feature, rather than iterating over environ['PATH'].

Does that seem reasonable to you?

review: Needs Information

[Robert Collins (lifeless)]

This feature seems to be written a little awkwardly to me - just defining _probe and the string should be enough, no ?

review: Needs Information

[Martin von Gagern (gagern)]

@jameinel: subprocess.Popen(cmd, shell=True) has three drawbacks:
1. additional shell invocation causes overhead for one more process
2. can't easily test for existence of command without actually executing it
3. might under some circumstances interfere with shell builtins
So iterating over PATH seems to me to be both cleaner and more performant

@lifeless: I want the path property available so I can execute the found executable by full path name without repeating the search, the way the bash_completion plugin tests do.

And I don't want to impose any restriction about in which order on feature check and access to the path property. Therefore both the feature check and access to the path both should essentially do the same: try to locate the executable upon first run, use cached result afterwards. Under these circumstances, _probe seems the wrong approach, as it is based on a different assumption, namely a boolean-only feature with no additional properties.

What might feel awkward as well is the fact that self._path is not initialized, but only set lazily at first access. Basically I have three possible states: _path is either unset, set to None, or set to some string. It would be easy to either have self._path initialized to a special value, denoting "no cached value", or to have a separate member track whether or not the result has been cached already. But as the simple try-except does the job well, and easily represents the three states named above, I went for that solution.

[Robert Collins (lifeless)]

>>> @lifeless: I want the path property available so I can execute the found executable by full path name without repeating the search, the way the bash_completion plugin tests do.

But if the feature isn't available, the path is useless. So the
feature *has* to be checked first.

I'm totally fine with the caching - thats standard for features. It
just seems a lot more awkward than simply having probe set the _path.

-Rob

[lp:~gagern/bzr/bash_completion-ExecutableFeature]

5170. By Martin von Gagern on 2010-05-25

Have ExecutableFeature implement and use _probe.

Setting _path now is a side-effect of _probe.
This makes Robert Collins happy.

[Robert Collins (lifeless)]

One last thing, rather than SedFeature, sed_feature please, and put them in features? [we've changed coding style in this regard].

Cheers,
Rob

review: Needs Fixing

[lp:~gagern/bzr/bash_completion-ExecutableFeature]

5171. By Martin von Gagern on 2010-05-25

Move ExecutableFeature instances to tests.features module.

[Robert Collins (lifeless)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-05-21 07:17:13 +0000
+++ NEWS	2010-05-25 11:21:39 +0000
@@ -244,6 +244,10 @@
   failures on Lucid, FreeBSD and gentoo.  
   (Vincent Ladeuil, #528436)
 
+* New class ``ExecutableFeature`` for checking the availability of
+  executables on the ``PATH``. Migrated from bash_completion plugin.
+  (Martin von Gagern)
+
 bzr 2.2b2
 #########
 

=== modified file 'bzrlib/plugins/bash_completion/tests/test_bashcomp.py'
--- bzrlib/plugins/bash_completion/tests/test_bashcomp.py	2010-05-04 19:30:44 +0000
+++ bzrlib/plugins/bash_completion/tests/test_bashcomp.py	2010-05-25 11:21:39 +0000
@@ -22,35 +22,10 @@
 import subprocess
 
 
-class _BashFeature(tests.Feature):
-    """Feature testing whether a bash executable is available."""
-
-    bash_paths = ['/bin/bash', '/usr/bin/bash']
-
-    def __init__(self):
-        super(_BashFeature, self).__init__()
-        self.bash_path = None
-
-    def available(self):
-        if self.bash_path is not None:
-            return self.bash_path is not False
-        for path in self.bash_paths:
-            if os.access(path, os.X_OK):
-                self.bash_path = path
-                return True
-        self.bash_path = False
-        return False
-
-    def feature_name(self):
-        return 'bash'
-
-BashFeature = _BashFeature()
-
-
 class BashCompletionMixin(object):
     """Component for testing execution of a bash completion script."""
 
-    _test_needs_features = [BashFeature]
+    _test_needs_features = [tests.features.bash_feature]
 
     def complete(self, words, cword=-1):
         """Perform a bash completion.
@@ -60,7 +35,8 @@
         """
         if self.script is None:
             self.script = self.get_script()
-        proc = subprocess.Popen([BashFeature.bash_path, '--noprofile'],
+        proc = subprocess.Popen([tests.features.bash_feature.path,
+                                 '--noprofile'],
                                 stdin=subprocess.PIPE,
                                 stdout=subprocess.PIPE,
                                 stderr=subprocess.PIPE)
@@ -187,6 +163,7 @@
         return s.replace("$(bzr ", "$('%s' " % self.get_bzr_path())
 
     def test_revspec_tag_all(self):
+        self.requireFeature(tests.features.sed_feature)
         wt = self.make_branch_and_tree('.', format='dirstate-tags')
         wt.branch.tags.set_tag('tag1', 'null:')
         wt.branch.tags.set_tag('tag2', 'null:')
@@ -195,6 +172,7 @@
         self.assertCompletionEquals('tag1', 'tag2', '3tag')
 
     def test_revspec_tag_prefix(self):
+        self.requireFeature(tests.features.sed_feature)
         wt = self.make_branch_and_tree('.', format='dirstate-tags')
         wt.branch.tags.set_tag('tag1', 'null:')
         wt.branch.tags.set_tag('tag2', 'null:')

=== modified file 'bzrlib/tests/features.py'
--- bzrlib/tests/features.py	2010-05-21 06:14:50 +0000
+++ bzrlib/tests/features.py	2010-05-25 11:21:39 +0000
@@ -80,3 +80,36 @@
 
 chown_feature = _ChownFeature()
 
+
+class ExecutableFeature(tests.Feature):
+    """Feature testing whether an executable of a given name is on the PATH."""
+
+    def __init__(self, name):
+        super(ExecutableFeature, self).__init__()
+        self.name = name
+        self._path = None
+
+    @property
+    def path(self):
+        # This is a property, so accessing path ensures _probe was called
+        self.available()
+        return self._path
+
+    def _probe(self):
+        path = os.environ.get('PATH')
+        if path is None:
+            return False
+        for d in path.split(os.pathsep):
+            if d:
+                f = os.path.join(d, self.name)
+                if os.access(f, os.X_OK):
+                    self._path = f
+                    return True
+        return False
+
+    def feature_name(self):
+        return '%s executable' % self.name
+
+
+bash_feature = ExecutableFeature('bash')
+sed_feature = ExecutableFeature('sed')