Re: [PATCH] tests/acceptance: Test powernv machines

[Philippe Mathieu-Daudé]

On 8/12/21 3:40 AM, David Gibson wrote:
> On Wed, Aug 11, 2021 at 11:07:31AM +0200, Thomas Huth wrote:
>> On 10/08/2021 11.09, Cédric Le Goater wrote:
>>> On 8/10/21 10:36 AM, Joel Stanley wrote:
>>>> On Tue, 10 Aug 2021 at 08:34, Cédric Le Goater <clg@kaod.org> wrote:
>>>>>
>>>>> Fetch the OpenPOWER images to boot the powernv8 and powernv9 machines
>>>>> with a simple PCI layout.
>>>>>
>>>>> Cc: Cleber Rosa <crosa@redhat.com>
>>>>> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>   tests/acceptance/boot_linux_console.py | 42 ++++++++++++++++++++++++++
>>>>>   1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>>>> b/tests/acceptance/boot_linux_console.py
>>>>> index 5248c8097df9..da93a475ca87 100644
>>>>> --- a/tests/acceptance/boot_linux_console.py
>>>>> +++ b/tests/acceptance/boot_linux_console.py
>>>>> @@ -1176,6 +1176,48 @@ def test_ppc64_e500(self):
>>>>>           tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
>>>>>           self.do_test_advcal_2018('19', tar_hash, 'uImage')
>>>>>
>>>>> +    def do_test_ppc64_powernv(self, proc):
>>>>> +
>>>>> +        images_url = 
>>>>> ('https://github.com/open-power/op-build/releases/download/v2.7/')
>>>>> +
>>>>> +        skiboot_url = images_url + 'skiboot.lid'
>>>>> +        skiboot_hash = 
>>>>> 'a9ffcddbf238f86cda4b2cae2882d6bd13cff8489109758a4980efaf154f4a29'
>>>>> +        skiboot_path = self.fetch_asset(skiboot_url, 
>>>>> asset_hash=skiboot_hash,
>>>>> +                                       algorithm='sha256')
>>>>
>>>> What's the thought that led you to using this instead of the one that
>>>> gets packaged with qemu?
>>>
>>> Good question.
>>>
>>> I considered that the skiboot.lid shipped with QEMU was somewhat a default
>>> to make things work. The official released versions are the ones used by
>>> the outside world on real systems and were a better target for tests.
>>>
>>> That said, using the default version might be enough. Maintainers, please
>>> advise !
>>
>> IMHO:
>>
>> - We want to test the things that *we* ship

I'd clarify here:

- We want to test "in the default pipeline" the things that *we* ship

>>
>> - We want to download as few things as possible, since downloads
>>   often slow down the tests and break CI runs if the network to
>>   the download site is not available
>>
>>  ==> I'd prefer to use the internal skiboot.lid unless there is
>>      really a compelling reason to use the external one.
>>
>> Just my 0.02 € though.
> 
> I agree 100%.  If the internal skiboot isn't the "real" one, then we
> should update it to something that is.

Here I'm in favor in adding the external skiboot as a non-default
test. So tests are shared in mainstream, and forks willing to run
them can opt for it. The current simplest way to do that is using
environment variables [*], and tag your tests with @skipUnless.
I.e.:

  def do_test_ppc64_powernv(self, proc, use_internal_skiboot_lid):
      ....

  def test_ppc_powernv8(self):
      self.do_test_ppc64_powernv('P8', true)

  @skipUnless(os.getenv('TEST_BLEEDING_EDGE_SKIBOOT'),
              'Restricted to internal skiboot.lid')
  def test_ppc_powernv8_bleeding_edge(self):
      self.do_test_ppc64_powernv('P8', false)

[*] https://qemu-project.gitlab.io/qemu/devel/ci.html#custom-ci-cd-variables