[pacman-dev] [PATCH] Simplify pmtest.generate and .run API (removed pacman parameter).

[Dave Reisner]

On Mon, Nov 11, 2013 at 09:16:42AM -0500, Jeremy Heiner wrote:
> Previously pmtest.__init__ received and cached the pmenv.root, and
> generate and run received the pmenv.pacman. Now __init__ receives and
> caches the pmenv, allowing any method to fetch root, pacman, or other
> pmenv goodies directly instead of needing the extra parameter.
> 
> Moved the "which fakeroot" and "which fakechroot" logic from pmtest up
> into pmenv. Repeating the probes and warnings for each test was a bit
> redundant.
> 
> Signed-off-by: Jeremy Heiner <ScalaProtractor at gmail.com>
> ---
>  test/pacman/pmenv.py  | 13 ++++++++++---
>  test/pacman/pmtest.py | 30 +++++++++++++-----------------
>  2 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/test/pacman/pmenv.py b/test/pacman/pmenv.py
> index f358285..2da6566 100644
> --- a/test/pacman/pmenv.py
> +++ b/test/pacman/pmenv.py
> @@ -21,6 +21,7 @@
>  
>  import pmtest
>  import tap
> +import util
>  
>  
>  class pmenv(object):
> @@ -42,6 +43,12 @@ def __init__(self, root = "root"):
>              "valgrind": 0,
>              "nolog": 0
>          }
> +        self.fakecmds = [] if os.geteuid() == 0 else [
> +            cmd
> +            for cmd in ["fakeroot", "fakechroot"]
> +            for found in [util.which(cmd) or # diag returns None...
> +                          tap.diag("WARNING: %s not found!" % cmd)]
> +            if found]

Nested list comprehensions are completely unreadable at a glance.
There's no gain in doing this. Can we get rid of the some of the clever
for the sake of readability? Something like...

  self.fakecmds = []
  if os.geteuid() != 0:
    for cmd in ("fakeroot", "fakechroot"):
      resolved_path = util.which(cmd)
      if resolved_path:
        self.fakecmds.append(resolved)
      else:
        tap.diag("WARNING: %s not found!" % cmd)


>  
>      def __str__(self):
>          return "root = %s\n" \
> @@ -53,7 +60,7 @@ def addtest(self, testcase):
>          """
>          if not os.path.isfile(testcase):
>              raise IOError("test file %s not found" % testcase)
> -        test = pmtest.pmtest(testcase, self.root)
> +        test = pmtest.pmtest(testcase, self)
>          self.testcases.append(test)
>  
>      def run(self):
> @@ -64,8 +71,8 @@ def run(self):
>              tap.diag("Running '%s'" % t.testname)
>  
>              t.load()
> -            t.generate(self.pacman)
> -            t.run(self.pacman)
> +            t.generate()
> +            t.run()
>  
>              tap.diag("==> Checking rules")
>              tap.todo = t.expectfailure
> diff --git a/test/pacman/pmtest.py b/test/pacman/pmtest.py
> index a1f3645d..6c02ac5 100644
> --- a/test/pacman/pmtest.py
> +++ b/test/pacman/pmtest.py
> @@ -35,12 +35,16 @@ class pmtest(object):
>      """Test object
>      """
>  
> -    def __init__(self, name, root):
> +    def __init__(self, name, env):
>          self.name = name
>          self.testname = os.path.basename(name).replace('.py', '')
> -        self.root = root
> +        self.env = env
>          self.cachepkgs = True
>  
> +    @property
> +    def root(self):
> +        return self.env.root
> +
>      def __str__(self):
>          return "name = %s\n" \
>                 "testname = %s\n" \
> @@ -105,7 +109,9 @@ def load(self):
>          else:
>              raise IOError("file %s does not exist!" % self.name)
>  
> -    def generate(self, pacman):
> +    def generate(self):
> +        pacman = self.env.pacman
> +
>          tap.diag("==> Generating test environment")
>  
>          # Cleanup leftover files from a previous test session
> @@ -192,7 +198,9 @@ def generate(self, pacman):
>                  self.files.append(f)
>                  vprint("\t%s" % f.name)
>  
> -    def run(self, pacman):
> +    def run(self):
> +        pacman = self.env.pacman
> +
>          if os.path.isfile(util.PM_LOCK):
>              tap.bail("\tERROR: another pacman session is on-going -- skipping")
>              return
> @@ -200,19 +208,7 @@ def run(self, pacman):
>          tap.diag("==> Running test")
>          vprint("\tpacman %s" % self.args)
>  
> -        cmd = []
> -        if os.geteuid() != 0:
> -            fakeroot = util.which("fakeroot")
> -            if not fakeroot:
> -                tap.diag("WARNING: fakeroot not found!")
> -            else:
> -                cmd.append("fakeroot")
> -
> -            fakechroot = util.which("fakechroot")
> -            if not fakechroot:
> -                tap.diag("WARNING: fakechroot not found!")
> -            else:
> -                cmd.append("fakechroot")
> +        cmd = self.env.fakecmds[:]
>  
>          if pacman["gdb"]:
>              cmd.extend(["libtool", "execute", "gdb", "--args"])
> -- 
> 1.8.4.2
> 
>