Re: [PATCH v7 6/8] Acceptance tests: add the build directory to the system PATH

[Cleber Rosa <crosa@redhat.com>]

On Tue, Nov 12, 2019 at 12:00:20PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/11/19 8:49 PM, Cleber Rosa wrote:
> > On Thu, Nov 07, 2019 at 05:46:13PM -0200, Wainer dos Santos Moschetta wrote:
> > > On 11/4/19 1:13 PM, Cleber Rosa wrote:
> > > > So that when binaries such as qemu-img are searched for, those in the
> > > > build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> > > > dependent on the location from where tests are executed, so they are
> > > > equal to the build directory if one is being used.
> > > > 
> > > > The original motivation is that Avocado libraries such as
> > > > avocado.utils.vmimage.get() may use the matching binaries, but it may
> > > > also apply to any other binary that test code may eventually attempt
> > > > to execute.
> > > > 
> > > > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > > > ---
> > > >    tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
> > > >    1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > > > index 17ce583c87..a4bb796a47 100644
> > > > --- a/tests/acceptance/avocado_qemu/__init__.py
> > > > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > > > @@ -110,6 +110,12 @@ class Test(avocado.Test):
> > > >            return None
> > > >        def setUp(self):
> > > > +        # Some utility code uses binaries from the system's PATH.  For
> > > > +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> > > > +        # create a snapshot image.  This is a transparent way of
> > > Because PATH is changed in a transparent way, wouldn't be better to also
> > > self.log.info() that fact?
> > > 
> > I don't have a problem with logging it, but because it will happen for
> > *every single* test, it seems like it will become noise.  I think it's
> > better to properly document this aspect of "avocado_qemu.Test" instead
> > (which is currently missing here).  Something like:
> > 
> > "Tests based on avocado_qemu.Test will have, as a convenience, the
> > QEMU build directory added to their PATH environment variable.  The goal
> > is to allow tests to seamless use matching built binaries, instead of
> > binaries installed elsewhere in the system".
> > 
> > How does it sound?
> 
> 
> It does.
> 
> 
> > 
> > > > +        # making sure those utilities find and use binaries on the
> > > > +        # build tree by default.
> > > > +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
> > > I think PATH should be set only once at class initialization. Perhaps in
> > > setUpClass()?
> > > 
> > > - Wainer
> > > 
> > The Avocado test isolation model makes setUpClass() unnecessary,
> > unsupported and pointless, so we only support setUp().
> > 
> > Every test already runs on its own process, and with the nrunner
> > model, should be able to run on completely different systems.  That's
> > why we don't want to support a setUpClass() like approach.
> 
> Okay, thanks for the explanation.
>

And thanks for the review.  Given the level of controversy here, I've
decided to take a different approach on v8.  Basically, I'm adding an
interface to avocado.utils.vmimage[1], so that we can explicitly
control the qemu-img binary used.

Looking forward to your opinion on v8.

Thanks,
- Cleber.

[1] - https://github.com/avocado-framework/avocado/pull/3374

> Thanks,
> 
> Wainer
> 
> > 
> > - Cleber.
> > 
> > 
> 
>