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

[Cleber Rosa]

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 <address@hidden>
> > ---
> >   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?

> > +        # 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.

- Cleber.