[Qemu-devel] [RFC PATCH] tests/acceptance: Handle machine type for ARM target

[Qemu-devel] [RFC PATCH] tests/acceptance: Handle machine type for ARM target

[Wainer dos Santos Moschetta]

Hi all,

I'm still unsure this is the best solution. I tend to think that
any arch-independent test case (i.e. not tagged 'arch') should
be skipped on all arches except for x86_64. Opening up for
discussion though.

Note: It was decided that ARM targets should not default to any
machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html

-- 8< --
Some tests are meant arch-independent and as such they don't set
the machine type (i.e. relying to defaults) on launched VMs. The arm
targets, however, don't provide any default machine so tests fail.

This patch adds a logic on the base Test class so that machine type
is set to 'virt' when:
   a) The test case doesn't have arch:aarch64 or arch:arm tag. Here
      I assume that if the test was tagged for a specific arch then
      the writer took care of setting a machine type.
   b) The target binary arch is any of aarch64 or arm. Note:
      self.target_arch can end up None if qemu_bin is passed by
      Avocado parameter and the filename doesn't match expected
      format. In this case the test will fail.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 2b236a1cf0..fb3e0dc2bc 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -9,6 +9,7 @@
 # later.  See the COPYING file in the top-level directory.
 
 import os
+import re
 import sys
 import uuid
 
@@ -65,10 +66,21 @@ class Test(avocado.Test):
         if self.qemu_bin is None:
             self.cancel("No QEMU binary defined or found in the source tree")
 
+        m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop())
+        if m:
+            self.target_arch = m.group(1)
+        else:
+            self.target_arch = None
+
     def _new_vm(self, *args):
         vm = QEMUMachine(self.qemu_bin)
         if args:
             vm.add_args(*args)
+        # Handle lack of default machine type on some targets.
+        # Assume that arch tagged tests have machine type set properly.
+        if self.tags.get('arch') is None and \
+           self.target_arch in ('aarch64', 'arm'):
+            vm.set_machine('virt')
         return vm
 
     @property
-- 
2.18.1

Re: [Qemu-devel] [RFC PATCH] tests/acceptance: Handle machine type for ARM target

[Cleber Rosa]

On Fri, Jun 21, 2019 at 11:38:06AM -0400, Wainer dos Santos Moschetta wrote:
> Hi all,
> 
> I'm still unsure this is the best solution. I tend to think that
> any arch-independent test case (i.e. not tagged 'arch') should
> be skipped on all arches except for x86_64. Opening up for
> discussion though.
>

I'm confused... if you're calling a test case "arch-independent", why
should it be skipped on all but one arch?  Anyway, I don't think we
should define such a broad policy... This line of thought is very
x86_64 centric, and quite honestly, doesn't map to QEMU's goals.

I agree that we're being a bit "disonest" by not assuring that tests
we send will work on all targets... but at least we're having that
discussion.  The next step would be to start triaging and discussing
wether it's worth running those against other targets, considering
the cost and benefits.

> Note: It was decided that ARM targets should not default to any
> machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html
> 
> -- 8< --
> Some tests are meant arch-independent and as such they don't set
> the machine type (i.e. relying to defaults) on launched VMs. The arm
> targets, however, don't provide any default machine so tests fail.
> 
> This patch adds a logic on the base Test class so that machine type
> is set to 'virt' when:
>    a) The test case doesn't have arch:aarch64 or arch:arm tag. Here
>       I assume that if the test was tagged for a specific arch then
>       the writer took care of setting a machine type.
>    b) The target binary arch is any of aarch64 or arm. Note:
>       self.target_arch can end up None if qemu_bin is passed by
>       Avocado parameter and the filename doesn't match expected
>       format. In this case the test will fail.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 2b236a1cf0..fb3e0dc2bc 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -9,6 +9,7 @@
>  # later.  See the COPYING file in the top-level directory.
>  
>  import os
> +import re
>  import sys
>  import uuid
>  
> @@ -65,10 +66,21 @@ class Test(avocado.Test):
>          if self.qemu_bin is None:
>              self.cancel("No QEMU binary defined or found in the source tree")
>  
> +        m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop())
> +        if m:
> +            self.target_arch = m.group(1)
> +        else:
> +            self.target_arch = None
> +

The "arch" tag and parameter are actually related to the target that
should be used.  I don't see the need for a "target_arch" based on
that.

>      def _new_vm(self, *args):
>          vm = QEMUMachine(self.qemu_bin)
>          if args:
>              vm.add_args(*args)
> +        # Handle lack of default machine type on some targets.
> +        # Assume that arch tagged tests have machine type set properly.
> +        if self.tags.get('arch') is None and \
> +           self.target_arch in ('aarch64', 'arm'):
> +            vm.set_machine('virt')

This (considering it deals with "arch" instead of "target_arch") is
one of the very important points to be determined.  How much wrapping
around different QEMU behavior on different targets/machines/devices
should we do?  This will possibly be case-by-case discussions with
different outcomes, but hopefully we can come up with a general
direction.

Thanks,
- Cleber.

>          return vm
>  
>      @property
> -- 
> 2.18.1
> 

Re: [Qemu-devel] [RFC PATCH] tests/acceptance: Handle machine type for ARM target

[Wainer dos Santos Moschetta]

On 06/21/2019 07:07 PM, Cleber Rosa wrote:
> On Fri, Jun 21, 2019 at 11:38:06AM -0400, Wainer dos Santos Moschetta wrote:
>> Hi all,
>>
>> I'm still unsure this is the best solution. I tend to think that
>> any arch-independent test case (i.e. not tagged 'arch') should
>> be skipped on all arches except for x86_64. Opening up for
>> discussion though.
>>
> I'm confused... if you're calling a test case "arch-independent", why
> should it be skipped on all but one arch?  Anyway, I don't think we
> should define such a broad policy... This line of thought is very
> x86_64 centric, and quite honestly, doesn't map to QEMU's goals.

What I meant for "arch-independent" is a test which does not have "arch" 
tag. You call it "generic test". We can agree on use "generic test" for 
better communication.

The absence of "arch" tag can means either 1) "run on all built targets" 
or 2) "pick one target and run it".  I suggested x86_64 thinking on case 
2) because it provides a default machine type that I supposed (I might 
be indeed wrong) just works on most cases. Your follow up patch [1] 
addresses the scenario 1) but allows the user to pick that one target of 2).

All in all, we should agree on the semantic of "arch" when it is not 
present. Should it be "any" or "all"? Configurable or not? Let's discuss 
this on [1].

So NACK this patch.

[1] [RFC PATCH] Acceptance tests: run generic tests on all built targets


>
> I agree that we're being a bit "disonest" by not assuring that tests
> we send will work on all targets... but at least we're having that
> discussion.  The next step would be to start triaging and discussing
> wether it's worth running those against other targets, considering
> the cost and benefits.
>
>> Note: It was decided that ARM targets should not default to any
>> machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html
>>
>> -- 8< --
>> Some tests are meant arch-independent and as such they don't set
>> the machine type (i.e. relying to defaults) on launched VMs. The arm
>> targets, however, don't provide any default machine so tests fail.
>>
>> This patch adds a logic on the base Test class so that machine type
>> is set to 'virt' when:
>>     a) The test case doesn't have arch:aarch64 or arch:arm tag. Here
>>        I assume that if the test was tagged for a specific arch then
>>        the writer took care of setting a machine type.
>>     b) The target binary arch is any of aarch64 or arm. Note:
>>        self.target_arch can end up None if qemu_bin is passed by
>>        Avocado parameter and the filename doesn't match expected
>>        format. In this case the test will fail.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>> index 2b236a1cf0..fb3e0dc2bc 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -9,6 +9,7 @@
>>   # later.  See the COPYING file in the top-level directory.
>>   
>>   import os
>> +import re
>>   import sys
>>   import uuid
>>   
>> @@ -65,10 +66,21 @@ class Test(avocado.Test):
>>           if self.qemu_bin is None:
>>               self.cancel("No QEMU binary defined or found in the source tree")
>>   
>> +        m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop())
>> +        if m:
>> +            self.target_arch = m.group(1)
>> +        else:
>> +            self.target_arch = None
>> +
> The "arch" tag and parameter are actually related to the target that
> should be used.  I don't see the need for a "target_arch" based on
> that.

I could have used self.arch indeed. But notice that self.arch is None 
when "arch" tag does not exist. So I still need to guess the 
architecture from the binary.

>
>>       def _new_vm(self, *args):
>>           vm = QEMUMachine(self.qemu_bin)
>>           if args:
>>               vm.add_args(*args)
>> +        # Handle lack of default machine type on some targets.
>> +        # Assume that arch tagged tests have machine type set properly.
>> +        if self.tags.get('arch') is None and \
>> +           self.target_arch in ('aarch64', 'arm'):
>> +            vm.set_machine('virt')
> This (considering it deals with "arch" instead of "target_arch") is
> one of the very important points to be determined.  How much wrapping
> around different QEMU behavior on different targets/machines/devices
> should we do?  This will possibly be case-by-case discussions with
> different outcomes, but hopefully we can come up with a general
> direction.

Yep. Let's continue that discussion on your patch.

Thanks for the comments!

- Wainer


>
> Thanks,
> - Cleber.
>
>>           return vm
>>   
>>       @property
>> -- 
>> 2.18.1
>>

[Qemu-devel] [RFC PATCH] Acceptance tests: run generic tests on all built targets

[Cleber Rosa]

The intention here is to discuss the validity of running the the
acceptance tests are not depedent on target specific functionality on
all built targets.

Subtle but important questions that this topic brings:

 1) Should the QEMU binary target provide, as much as possible,
    a similar experience across targets, or should upper layer
    code deal with it?

 2) Are those tests exercising the same exact features and
    implementation, which just happen to be linked to various
    different binaries?  Or is the simple fact that they are
    integrated into different code worth testing?

 3) Should the default target match the host target?  Or the
    first binary found in the build tree?  Or something else?

An example of a Travis CI job based on this can be seen here:

 https://travis-ci.org/clebergnu/qemu/jobs/548905146

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 .travis.yml                               |  2 +-
 tests/Makefile.include                    | 18 +++++++++++++++++-
 tests/acceptance/avocado_qemu/__init__.py |  9 +++++++++
 tests/acceptance/cpu_queries.py           |  3 +++
 tests/acceptance/migration.py             |  4 +++-
 5 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index aeb9b211cd..310febb866 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -232,7 +232,7 @@ matrix:
     # Acceptance (Functional) tests
     - env:
         - CONFIG="--python=/usr/bin/python3 --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu"
-        - TEST_CMD="make check-acceptance"
+        - TEST_CMD="make check-acceptance-all"
       after_failure:
         - cat tests/results/latest/job.log
       addons:
diff --git a/tests/Makefile.include b/tests/Makefile.include
index db750dd6d0..34126167a5 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -1154,7 +1154,23 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR)
             --filter-by-tags-include-empty --filter-by-tags-include-empty-key \
             $(AVOCADO_TAGS) \
             --failfast=on $(SRC_PATH)/tests/acceptance, \
-            "AVOCADO", "tests/acceptance")
+            "AVOCADO", "tests/acceptance (target arch based on test)")
+
+# Allows one to run tests that are generic (independent of target)
+# using a given target
+check-acceptance-generic-on-%: check-venv $(TESTS_RESULTS_DIR)
+	$(call quiet-command, \
+            $(TESTS_VENV_DIR)/bin/python -m avocado \
+            --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) \
+            --filter-by-tags-include-empty --filter-by-tags-include-empty-key \
+            --filter-by-tags=-arch -p arch=$* \
+            -p add-qtest=yes -p set-arm-aarch64-machine=yes \
+            --failfast=on $(SRC_PATH)/tests/acceptance, \
+            "AVOCADO", "tests/acceptance (target arch set to $*)")
+
+check-acceptance-generic: $(patsubst %-softmmu,check-acceptance-generic-on-%, $(filter %-softmmu,$(TARGET_DIRS)))
+
+check-acceptance-all: check-acceptance check-acceptance-generic
 
 # Consolidated targets
 
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 2b236a1cf0..7a47f0d514 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -69,6 +69,15 @@ class Test(avocado.Test):
         vm = QEMUMachine(self.qemu_bin)
         if args:
             vm.add_args(*args)
+        if self.params.get('add-qtest', default=False):
+            # mips and sh4 targets require either a bios or a kernel or
+            # qtest enabled to not abort right at the commad line
+            if self.arch in ('mips', 'mipsel', 'mips64', 'mips64el', 'sh4'):
+                vm.add_args('-accel', 'qtest')
+        if self.params.get('set-arm-aarch64-machine', default=False):
+            # arm and aarch64 require a machine type to be set
+            if self.arch in ('arm', 'aarch64'):
+                vm.set_machine('virt')
         return vm
 
     @property
diff --git a/tests/acceptance/cpu_queries.py b/tests/acceptance/cpu_queries.py
index e71edec39f..af47d2795a 100644
--- a/tests/acceptance/cpu_queries.py
+++ b/tests/acceptance/cpu_queries.py
@@ -18,6 +18,9 @@ class QueryCPUModelExpansion(Test):
     """
 
     def test(self):
+        """
+        :avocado: tags=arch:x86_64
+        """
         self.vm.set_machine('none')
         self.vm.add_args('-S')
         self.vm.launch()
diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
index 6115cf6c24..c4ed87cd98 100644
--- a/tests/acceptance/migration.py
+++ b/tests/acceptance/migration.py
@@ -33,8 +33,10 @@ class Migration(Test):
             self.cancel('Failed to find a free port')
         return port
 
-
     def test_migration_with_tcp_localhost(self):
+        blacklist_targets = ["arm", "ppc64", "sh4", "s390x"]
+        if self.arch in blacklist_targets:
+            self.cancel("Test failing on targets: %s" % ", ".join(blacklist_targets))
         source_vm = self.get_vm()
         dest_uri = 'tcp:localhost:%u' % self._get_free_port()
         dest_vm = self.get_vm('-incoming', dest_uri)
-- 
2.21.0