| Message ID | 20191104151323.9883-7-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Acceptance test: Add "boot_linux" acceptance test | expand |
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? > + # 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 > self._vms = {} > > self.arch = self.params.get('arch',
On 11/4/19 4: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 > + # 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']) But qemu-img is compiled in BUILD_ROOT_DIR, isn't it? Maybe we should pass its path by argument, such --qemu-img /path/to/it. > self._vms = {} > > self.arch = self.params.get('arch', >
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? > > + # 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.
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. Thanks, Wainer > > - Cleber. > >
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. > > > > > >
On Fri, Nov 08, 2019 at 02:13:02PM +0100, Philippe Mathieu-Daudé wrote: > On 11/4/19 4: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 > > + # 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']) > > But qemu-img is compiled in BUILD_ROOT_DIR, isn't it? > > Maybe we should pass its path by argument, such --qemu-img /path/to/it. > Hi Philippe, On the next version we should see a properly named variable for the build directory, and (as explained in the previous response) also a more explicit setting of the qemu-img binary used (although not a parameter or command line argument at this point). Looking forward for your opinion on the next version, and thanks again! - Cleber. > > self._vms = {} > > self.arch = self.params.get('arch', > >
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 + # 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']) self._vms = {} self.arch = self.params.get('arch',
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(+)