Re: [PATCH v2] tests/functional: Convert most Aspeed machine tests

[Thomas Huth <thuth@redhat.com>]

On 17/10/2024 09.09, Cédric Le Goater wrote:
> On 10/17/24 08:06, Thomas Huth wrote:
>> On 16/10/2024 22.38, Cédric Le Goater wrote:
>>> This is a simple conversion of the tests with some cleanups and
>>> adjustments to match the new test framework. Replace the zephyr image
>>> MD5 hashes with SHA256 hashes while at it.
>>>
>>> The SDK tests depend on a ssh class from avocado.utils which is
>>> difficult to replace. To be addressed separately.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
...
>>> -    @skipUnless(*has_cmd('swtpm'))
>>> -    def test_arm_ast2600_evb_buildroot_tpm(self):
>>> -        """
>>> -        :avocado: tags=arch:arm
>>> -        :avocado: tags=machine:ast2600-evb
>>> -        """
>>> -
>>> -        image_url = ('https://github.com/legoater/qemu-aspeed-boot/raw/ 
>>> master/'
>>> -                     'images/ast2600-evb/buildroot-2023.02-tpm/flash.img')
>>> -        image_hash = 
>>> ('a46009ae8a5403a0826d607215e731a8c68d27c14c41e55331706b8f9c7bd997')
>>> -        image_path = self.fetch_asset(image_url, asset_hash=image_hash,
>>> -                                      algorithm='sha256')
>>> -
>>> -        # force creation of VM object, which also defines self._sd
>>> -        vm = self.vm
>>> -
>>> -        socket = os.path.join(self._sd.name, 'swtpm-socket')
>>> -
>>> -        subprocess.run(['swtpm', 'socket', '-d', '--tpm2',
>>> -                        '--tpmstate', f'dir={self.vm.temp_dir}',
>>> -                        '--ctrl', f'type=unixio,path={socket}'])
>>> -
>>> -        self.vm.add_args('-chardev', f'socket,id=chrtpm,path={socket}')
>>> -        self.vm.add_args('-tpmdev', 'emulator,id=tpm0,chardev=chrtpm')
>>> -        self.vm.add_args('-device',
>>> -                         'tpm-tis- 
>>> i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e')
>>> -        self.do_test_arm_aspeed_buildroot_start(image_path, '0xf00', 
>>> 'Aspeed AST2600 EVB')
>>> -
>>> -        exec_command_and_wait_for_pattern(self,
>>> -            'echo tpm_tis_i2c 0x2e > /sys/bus/i2c/devices/i2c-12/ 
>>> new_device',
>>> -            'tpm_tis_i2c 12-002e: 2.0 TPM (device-id 0x1, rev-id 1)');
>>> -        exec_command_and_wait_for_pattern(self,
>>> -            'cat /sys/class/tpm/tpm0/pcr-sha256/0',
>>> -            
>>> 'B804724EA13F52A9072BA87FE8FDCC497DFC9DF9AA15B9088694639C431688E0');
>>> -
>>> -        self.do_test_arm_aspeed_buildroot_poweroff()
>>>   class AST2x00MachineSDK(QemuSystemTest, LinuxSSHMixIn):
>>
>> You also copied AST2x00MachineMMC but you didn't remove it from the 
>> avocado test yet, so there is a hunk missing here, I think?
> 
> oh. yes, I should remove it from the avocado test.
> 
> I was initially hesitating on having multiple test_arm_aspeed.py files:
> one per machine or one per fw image type. This to set the timeout more
> precisely in the meson.build file, since the SDK FW takes longer to
> boot, and also to handle more complex test scenarios, like the SDK.
> 
> What would be your recommandation ?

If the tests do not share anything at all (neither common python code, nor 
the same firmware binary asset), I think it's maybe better to put them into 
separate files, indeed. That way we can also run the tests in parallel when 
you add "-j$(nproc)" to your "make check-functional" line.

> Btw, I have been trying to rewrite the LinuxSSHMixIn class with a
> minimal ssh Session class. It is not trivial :/

:-(

Did you peek at tests/vm/basevm.py ? I was hoping we could lent some code 
there...

...
>>> +    def test_ast1030_zephyros_1_04(self):
>>> +        kernel_name = "ast1030-evb-demo/zephyr.elf"
>>> +        zip_file = self.ASSET_ZEPHYR_1_04.fetch()
>>> +        with ZipFile(zip_file, 'r') as zf:
>>> +                     zf.extract(kernel_name, path=self.workdir)
>>> +        kernel_file = os.path.join(self.workdir, kernel_name)
>>> +
>>> +        self.set_machine('ast1030-evb')
>>
>> It's slightly better to do the set_machine() at the very top of the 
>> function. set_machine() can cancel the test in case the machine is not 
>> available in the QEMU binary, so this should best be done before fetching 
>> and extracting any assets.
> 
> Should set_machine() be the first call of a test function ?

Yes, either set_machine() or require_netdev() should be at the very top, 
since they could cancel the test.

  Thomas