[v3,4/4] support/testing: test_optee.py: test optee boot and testsuite

Run a Qemu emulation over qemu_armv7a_tz_virt_defconfig and
run the embedded OP-TEE regression test suite (xtest).

Tool xtest dumps traces that contain '# ' (hash + space) which
corrupts infra/emulator.py sequence which use such traces to
find shell prompt when command is completed. To overcome the issue
the xtest traces are shown only if the test failed.

One can run the test from something like:

  $> support/testing/run-tests \
         -o output/optee-runtest -d output/dwl \
         tests.package.test_optee

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes v2 -> v3:
  - Test provides a config_emulator attribute and mandates use of
    the locally built emulator.
  - Update qemu defconfig filename.
  - Remove postprocessing of test image file since driven from the
    selected defconfig

Changes v1 -> v2:
  - Add argument local=True to test emulator to use the qemu host
    built from test configuration.
  - Fix typo in trace "Silent test takes a while, be patient..."
---
 support/testing/tests/package/test_optee.py | 42 +++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 support/testing/tests/package/test_optee.py

Hello,

Looks good, except for the chdir stuff.
Also a lot of nits below.

On Fri, Mar 22, 2019 at 06:58 AM, Etienne Carriere wrote:

[snip]
> ---
>  support/testing/tests/package/test_optee.py | 42 +++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 support/testing/tests/package/test_optee.py

Please run flake8 and fix the warning it generates.
test_optee.py:9:1: E302 expected 2 blank lines, found 1

Please run 'make .gitlab-ci.yml'. BTW the same is also missing in the defconfig
patch 2.

> 
> diff --git a/support/testing/tests/package/test_optee.py b/support/testing/tests/package/test_optee.py
> new file mode 100644
> index 0000000000..44cee74fd8
> --- /dev/null
> +++ b/support/testing/tests/package/test_optee.py
> @@ -0,0 +1,42 @@
> +import os
> +
> +import infra.basetest
> +
> +# This test enforces locally built emulator to prevent old Qemu to
> +# dump secure trace to stdio and corrupting trace synchro expected
> +# through pexpect.
> +
> +class TestOptee(infra.basetest.BRTest):
> +
> +    with open(os.path.join(os.getcwd(), 'configs',
> +                           'qemu_arm_vexpress_tz_defconfig'),
> +              'r') as config_file:

Regarding the use of open(), I see no better option here.

Regarding the use of os.getcwd(), I would prefer to have my patch applied
before this one:
http://patchwork.ozlabs.org/patch/992697/
So here you could use:
    with open(infra.basepath('configs/qemu_armv7a_tz_virt_defconfig'), 'r') as config_file:
This way we would keep the logic to get any path in a single point in the test
infra.
But this suggestion depends on what the maintainers think about my patch.
If you like the idea you can add my patch to your series.

> +        config = "".join(line for line in config_file if line[:1] != '#') + \
> +                 """

nit: for consistence with all other test cases, please use one less indent:
        config = "".join(line for line in config_file if line[:1] != '#') + \
            """

> +                 BR2_PACKAGE_HOST_QEMU=y
> +                 BR2_PACKAGE_HOST_QEMU_SYSTEM_MODE=y

Shouldn't these 2 lines be in the config_emulator below ...

> +                 BR2_TOOLCHAIN_EXTERNAL=y
> +                 """
> +    config_emulator = ''

... like this?
     config_emulator = \
         """
         BR2_PACKAGE_HOST_QEMU=y
         BR2_PACKAGE_HOST_QEMU_SYSTEM_MODE=y
         """

But see my question on patch 3 before changing this.

> +
> +    def test_run(self):
> +
> +        qemu_options = ['-machine', 'virt,secure=on']
> +        qemu_options.extend(['-cpu', 'cortex-a15'])
> +        qemu_options.extend(['-m', '1024'])
> +        qemu_options.extend(['-semihosting-config', 'enable,target=native'])
> +        qemu_options.extend(['-bios', 'bl1.bin'])
> +
> +        # This test expects Qemu is run from the image direcotry

s/direcotry/directory/

> +        os.chdir(os.path.join(self.builddir, 'images'))

This is not good.
When running multiple test cases, it will make any test case that runs after
this one to fail, see:

$ ./support/testing/run-tests -o o -k \
  tests.core.test_post_scripts.TestPostScripts \
  tests.package.test_optee.TestOptee
20:14:02 TestOptee                                Starting
20:17:01 TestOptee                                Cleaning up
.20:17:01 TestPostScripts                          Starting
E
======================================================================
ERROR: test_run (tests.core.test_post_scripts.TestPostScripts)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ricardo/src/buildroot/support/testing/infra/basetest.py", line 76, in setUp
    super(BRTest, self).setUp()
  File "/home/ricardo/src/buildroot/support/testing/infra/basetest.py", line 62, in setUp
    self.b.configure(make_extra_opts=["BR2_EXTERNAL={}".format(":".join(self.br2_external))])
  File "/home/ricardo/src/buildroot/support/testing/infra/builder.py", line 48, in configure
    raise SystemError("Cannot olddefconfig")
SystemError: Cannot olddefconfig

A better solution is to add a new optional parameter to Emulator.boot method,
perhaps named "cwd" that defaults to None and is passed to pexpect.spawn(...,
cwd=cwd) (not tested).
This way the Emulator class is aware that qemu must run from a directory, only
for the test cases that pass this.

> +
> +        self.emulator.boot(arch='armv7', options=qemu_options, local=True)
> +        self.emulator.login()
> +
> +        # Trick traces since xtest prints "# " which corrupts emulator run
> +        # method. Tests are dumped only if test fails.
> +        cmd = 'echo "Silent test takes a while, be patient..."; ' + \
> +              'xtest -t regression > /tmp/xtest.log ||' + \
> +              '(cat /tmp/xtest.log && false)'
> +        output, exit_code = self.emulator.run(cmd, timeout=240)

'output' is currently not tested.
It's better to use the convention:
        _, exit_code = self.emulator.run(cmd, timeout=240)

> +        self.assertEqual(exit_code, 0)
> -- 
> 2.17.1


Regards,
Ricardo

Hello Ricardo,

On Sat, 30 Mar 2019 at 04:33, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
>
> Hello,
>
> Looks good, except for the chdir stuff.
> Also a lot of nits below.
>
> On Fri, Mar 22, 2019 at 06:58 AM, Etienne Carriere wrote:
>
> [snip]
> > ---
> >  support/testing/tests/package/test_optee.py | 42 +++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 support/testing/tests/package/test_optee.py
>
> Please run flake8 and fix the warning it generates.
> test_optee.py:9:1: E302 expected 2 blank lines, found 1

Sure, sorry. Python newbie bad habit.

>
> Please run 'make .gitlab-ci.yml'. BTW the same is also missing in the defconfig
> patch 2.

Ok.

>
> >
> > diff --git a/support/testing/tests/package/test_optee.py b/support/testing/tests/package/test_optee.py
> > new file mode 100644
> > index 0000000000..44cee74fd8
> > --- /dev/null
> > +++ b/support/testing/tests/package/test_optee.py
> > @@ -0,0 +1,42 @@
> > +import os
> > +
> > +import infra.basetest
> > +
> > +# This test enforces locally built emulator to prevent old Qemu to
> > +# dump secure trace to stdio and corrupting trace synchro expected
> > +# through pexpect.
> > +
> > +class TestOptee(infra.basetest.BRTest):
> > +
> > +    with open(os.path.join(os.getcwd(), 'configs',
> > +                           'qemu_arm_vexpress_tz_defconfig'),
> > +              'r') as config_file:
>
> Regarding the use of open(), I see no better option here.
>
> Regarding the use of os.getcwd(), I would prefer to have my patch applied
> before this one:
> http://patchwork.ozlabs.org/patch/992697/
> So here you could use:
>     with open(infra.basepath('configs/qemu_armv7a_tz_virt_defconfig'), 'r') as config_file:
> This way we would keep the logic to get any path in a single point in the test
> infra.
> But this suggestion depends on what the maintainers think about my patch.
> If you like the idea you can add my patch to your series.

Ok, thanks, it will be more consistent with the runtime env.
Maybe I could simply merge your proposal in my change (and append your
s-o-b tag to it), if you agree.

>
> > +        config = "".join(line for line in config_file if line[:1] != '#') + \
> > +                 """
>
> nit: for consistence with all other test cases, please use one less indent:
>         config = "".join(line for line in config_file if line[:1] != '#') + \
>             """

ok.

>
> > +                 BR2_PACKAGE_HOST_QEMU=y
> > +                 BR2_PACKAGE_HOST_QEMU_SYSTEM_MODE=y
>
> Shouldn't these 2 lines be in the config_emulator below ...
>
> > +                 BR2_TOOLCHAIN_EXTERNAL=y
> > +                 """
> > +    config_emulator = ''
>
> ... like this?
>      config_emulator = \
>          """
>          BR2_PACKAGE_HOST_QEMU=y
>          BR2_PACKAGE_HOST_QEMU_SYSTEM_MODE=y
>          """

yes. true.

> But see my question on patch 3 before changing this.
>

true again, I think I will remove this config_emulator from the test
case and move it to a generic config in basetest.py.


>
> > +
> > +    def test_run(self):
> > +
> > +        qemu_options = ['-machine', 'virt,secure=on']
> > +        qemu_options.extend(['-cpu', 'cortex-a15'])
> > +        qemu_options.extend(['-m', '1024'])
> > +        qemu_options.extend(['-semihosting-config', 'enable,target=native'])
> > +        qemu_options.extend(['-bios', 'bl1.bin'])
> > +
> > +        # This test expects Qemu is run from the image direcotry
>
> s/direcotry/directory/

thanks.

>
> > +        os.chdir(os.path.join(self.builddir, 'images'))
>
> This is not good.
> When running multiple test cases, it will make any test case that runs after
> this one to fail, see:
>
> $ ./support/testing/run-tests -o o -k \
>   tests.core.test_post_scripts.TestPostScripts \
>   tests.package.test_optee.TestOptee
> 20:14:02 TestOptee                                Starting
> 20:17:01 TestOptee                                Cleaning up
> .20:17:01 TestPostScripts                          Starting
> E
> ======================================================================
> ERROR: test_run (tests.core.test_post_scripts.TestPostScripts)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/ricardo/src/buildroot/support/testing/infra/basetest.py", line 76, in setUp
>     super(BRTest, self).setUp()
>   File "/home/ricardo/src/buildroot/support/testing/infra/basetest.py", line 62, in setUp
>     self.b.configure(make_extra_opts=["BR2_EXTERNAL={}".format(":".join(self.br2_external))])
>   File "/home/ricardo/src/buildroot/support/testing/infra/builder.py", line 48, in configure
>     raise SystemError("Cannot olddefconfig")
> SystemError: Cannot olddefconfig
>
> A better solution is to add a new optional parameter to Emulator.boot method,
> perhaps named "cwd" that defaults to None and is passed to pexpect.spawn(...,
> cwd=cwd) (not tested).
> This way the Emulator class is aware that qemu must run from a directory, only
> for the test cases that pass this.

Ok. Indeed my proposal is bad.
I will go this way, and try to come up with something not too ugly.

>
> > +
> > +        self.emulator.boot(arch='armv7', options=qemu_options, local=True)
> > +        self.emulator.login()
> > +
> > +        # Trick traces since xtest prints "# " which corrupts emulator run
> > +        # method. Tests are dumped only if test fails.
> > +        cmd = 'echo "Silent test takes a while, be patient..."; ' + \
> > +              'xtest -t regression > /tmp/xtest.log ||' + \
> > +              '(cat /tmp/xtest.log && false)'
> > +        output, exit_code = self.emulator.run(cmd, timeout=240)
>
> 'output' is currently not tested.
> It's better to use the convention:
>         _, exit_code = self.emulator.run(cmd, timeout=240)

Ok thanks, will fix.

>
> > +        self.assertEqual(exit_code, 0)
> > --
> > 2.17.1
>
>
> Regards,
> Ricardo

Thanks again for your feedback.

Regards,
etienne

Hello,

On Wed, Apr 03, 2019 at 07:19 AM, Etienne Carriere wrote:

[snip]
>> > +class TestOptee(infra.basetest.BRTest):
>> > +
>> > +    with open(os.path.join(os.getcwd(), 'configs',
>> > +                           'qemu_arm_vexpress_tz_defconfig'),
>> > +              'r') as config_file:
>>
>> Regarding the use of open(), I see no better option here.
>>
>> Regarding the use of os.getcwd(), I would prefer to have my patch applied
>> before this one:
>> http://patchwork.ozlabs.org/patch/992697/
>> So here you could use:
>>     with open(infra.basepath('configs/qemu_armv7a_tz_virt_defconfig'), 'r') as config_file:
>> This way we would keep the logic to get any path in a single point in the test
>> infra.
>> But this suggestion depends on what the maintainers think about my patch.
>> If you like the idea you can add my patch to your series.
> 
> Ok, thanks, it will be more consistent with the runtime env.
> Maybe I could simply merge your proposal in my change (and append your
> s-o-b tag to it), if you agree.

I prefer my patch to stay as a separate commit because it changes how run-tests
can be called. Currently we can chdir to one buildroot tree and call run-tests
from another buildroot tree. Currently we also need to be in the buildroot top
dir when we call run-tests. After that patch when run-tests is called it will
execute in (and consequently test) the buildroot tree it belongs to, no matter
which is the current directory when run-tests is called.
That patch can still be applied with 'git am -3'.

But feel free to just grab only part of the code from there if you want to. No
need for my SoB if you use only part of it. Maybe something like this is enough:

def basepath(relpath=""):
    return os.path.join(os.getcwd(), relpath)

This way we have all calls to os.getcwd() in support/testing/infra/__init__.py
and I resend my patch later.

But also feel free to not do this right now. Up to you.


Regards,
Ricardo

On Thu, 4 Apr 2019 at 05:30, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
>
> Hello,
>
> On Wed, Apr 03, 2019 at 07:19 AM, Etienne Carriere wrote:
>
> [snip]
> >> > +class TestOptee(infra.basetest.BRTest):
> >> > +
> >> > +    with open(os.path.join(os.getcwd(), 'configs',
> >> > +                           'qemu_arm_vexpress_tz_defconfig'),
> >> > +              'r') as config_file:
> >>
> >> Regarding the use of open(), I see no better option here.
> >>
> >> Regarding the use of os.getcwd(), I would prefer to have my patch applied
> >> before this one:
> >> http://patchwork.ozlabs.org/patch/992697/
> >> So here you could use:
> >>     with open(infra.basepath('configs/qemu_armv7a_tz_virt_defconfig'), 'r') as config_file:
> >> This way we would keep the logic to get any path in a single point in the test
> >> infra.
> >> But this suggestion depends on what the maintainers think about my patch.
> >> If you like the idea you can add my patch to your series.
> >
> > Ok, thanks, it will be more consistent with the runtime env.
> > Maybe I could simply merge your proposal in my change (and append your
> > s-o-b tag to it), if you agree.
>
> I prefer my patch to stay as a separate commit because it changes how run-tests
> can be called. (...)

Oh, yes, sorry! I misunderstood your previous comment.
Sure, I won't merge your change and mine :)
Thanks for the clarification. I'll focus on this cwd() issue you
suggest and o which extend I need to take your proposal into account
in my change.

Regards,
etienne

> (...) Currently we can chdir to one buildroot tree and call run-tests
> from another buildroot tree. Currently we also need to be in the buildroot top
> dir when we call run-tests. After that patch when run-tests is called it will
> execute in (and consequently test) the buildroot tree it belongs to, no matter
> which is the current directory when run-tests is called.
> That patch can still be applied with 'git am -3'.
>
> But feel free to just grab only part of the code from there if you want to. No
> need for my SoB if you use only part of it. Maybe something like this is enough:
>
> def basepath(relpath=""):
>     return os.path.join(os.getcwd(), relpath)
>
> This way we have all calls to os.getcwd() in support/testing/infra/__init__.py
> and I resend my patch later.
>
> But also feel free to not do this right now. Up to you.
>
>
> Regards,
> Ricardo