Message ID | 20190322095818.19914-4-etienne.carriere@linaro.org |
---|---|
State | Changes Requested |
Headers | show |
Series | [v3,1/4] boot/arm-trusted-firmware: support alternate image files | expand |
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
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: + config = "".join(line for line in config_file if line[:1] != '#') + \ + """ + BR2_PACKAGE_HOST_QEMU=y + BR2_PACKAGE_HOST_QEMU_SYSTEM_MODE=y + BR2_TOOLCHAIN_EXTERNAL=y + """ + config_emulator = '' + + 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 + os.chdir(os.path.join(self.builddir, 'images')) + + 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) + self.assertEqual(exit_code, 0)
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