[PATCH 00/10] Revive ptest and add result to buildhistory

[PATCH 00/10] Revive ptest and add result to buildhistory

[Robert Yang]

We can get the regression result by:
$ git show HEAD ptest_log/pass.fail.skip.*

Can we add ptest to default TEST_SUITES, please ?

// Robert

The following changes since commit a2ab288bd002ebb6e64d46e941fb122e1157ff49:

  uninative: Update to 1.7 uninative release (2017-07-30 08:45:49 +0100)

are available in the git repository at:

  git://git.openembedded.org/openembedded-core-contrib rbt/ptest
  http://cgit.openembedded.org/openembedded-core-contrib/log/?h=rbt/ptest

Robert Yang (10):
  oeqa/targetcontrol.py: simplify checking for qemu_use_kvm
  runtime/cases/_ptest.py: revive it
  oeqa/utils/logparser.py: add skip status
  runtime/cases/_ptest.py: add skip status
  runtime/cases/_ptest.py: rename it to ptest.py
  utils/logparser.py: fix section check
  core/target/ssh.py: replace decode errors
  buildhistory.bbclass: print message when no commit
  buildhistory.bbclass: add ptest_log
  testimage.bbclass: update comments

 meta/classes/buildhistory.bbclass     |  30 ++++++++++
 meta/classes/testimage.bbclass        |   4 +-
 meta/lib/oeqa/core/target/ssh.py      |   4 +-
 meta/lib/oeqa/runtime/cases/_ptest.py | 103 ----------------------------------
 meta/lib/oeqa/runtime/cases/ptest.py  |  82 +++++++++++++++++++++++++++
 meta/lib/oeqa/targetcontrol.py        |   5 +-
 meta/lib/oeqa/utils/logparser.py      |   7 ++-
 7 files changed, 122 insertions(+), 113 deletions(-)
 delete mode 100644 meta/lib/oeqa/runtime/cases/_ptest.py
 create mode 100644 meta/lib/oeqa/runtime/cases/ptest.py

-- 
2.11.0

[PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Robert Yang]

The "if qemu_use_kvm" is not needed.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/targetcontrol.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/meta/lib/oeqa/targetcontrol.py b/meta/lib/oeqa/targetcontrol.py
index 3255e3a5c63..11e6c820e85 100644
--- a/meta/lib/oeqa/targetcontrol.py
+++ b/meta/lib/oeqa/targetcontrol.py
@@ -132,9 +132,8 @@ class QemuTarget(BaseTarget):
         dump_host_cmds = d.getVar("testimage_dump_host")
         dump_dir = d.getVar("TESTIMAGE_DUMP_DIR")
         qemu_use_kvm = d.getVar("QEMU_USE_KVM")
-        if qemu_use_kvm and \
-           (qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
-            d.getVar("MACHINE") in qemu_use_kvm.split()):
+        if qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
+            d.getVar("MACHINE") in qemu_use_kvm.split():
             use_kvm = True
         else:
             use_kvm = False
-- 
2.11.0

[PATCH 02/10] runtime/cases/_ptest.py: revive it

[Robert Yang]

* Make it work with current oeqa
* Skip the test if ptest is not in DISTRO_FEATURES
* Skip the test if ptest-pkgs is not in IMAGE_FEATURES
* The logs are saved to:
  testimage/ptest_log -> testimage/ptest_log.<datetime>
* Will write a tool to make regression check

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/runtime/cases/_ptest.py | 96 +++++++++++++----------------------
 1 file changed, 35 insertions(+), 61 deletions(-)

diff --git a/meta/lib/oeqa/runtime/cases/_ptest.py b/meta/lib/oeqa/runtime/cases/_ptest.py
index aaed9a5352f..6d239494b88 100644
--- a/meta/lib/oeqa/runtime/cases/_ptest.py
+++ b/meta/lib/oeqa/runtime/cases/_ptest.py
@@ -1,28 +1,10 @@
-import os
-import shutil
-import subprocess
-
 from oeqa.runtime.case import OERuntimeTestCase
 from oeqa.core.decorator.depends import OETestDepends
 from oeqa.core.decorator.oeid import OETestID
-from oeqa.core.decorator.data import skipIfNotDataVar, skipIfNotFeature
-from oeqa.runtime.decorator.package import OEHasPackage
-
-from oeqa.runtime.cases.dnf import DnfTest
-from oeqa.utils.logparser import *
-from oeqa.utils.httpserver import HTTPService
-
-class PtestRunnerTest(DnfTest):
+from oeqa.core.decorator.data import skipIfNotFeature
+from oeqa.utils.logparser import Lparser, Result
 
-    @classmethod
-    def setUpClass(cls):
-        rpm_deploy = os.path.join(cls.tc.td['DEPLOY_DIR'], 'rpm')
-        cls.repo_server = HTTPService(rpm_deploy, cls.tc.target.server_ip)
-        cls.repo_server.start()
-
-    @classmethod
-    def tearDownClass(cls):
-        cls.repo_server.stop()
+class PtestRunnerTest(OERuntimeTestCase):
 
     # a ptest log parser
     def parse_ptest(self, logfile):
@@ -59,45 +41,37 @@ class PtestRunnerTest(DnfTest):
         result.sort_tests()
         return result
 
-    def _install_ptest_packages(self):
-        # Get ptest packages that can be installed in the image.
-        packages_dir = os.path.join(self.tc.td['DEPLOY_DIR'], 'rpm')
-        ptest_pkgs = [pkg[:pkg.find('-ptest')+6]
-                          for _, _, filenames in os.walk(packages_dir)
-                          for pkg in filenames
-                          if 'ptest' in pkg
-                          and pkg[:pkg.find('-ptest')] in self.tc.image_packages]
-
-        repo_url = 'http://%s:%s' % (self.target.server_ip,
-                                     self.repo_server.port)
-        dnf_options = ('--repofrompath=oe-ptest-repo,%s '
-                       '--nogpgcheck '
-                       'install -y' % repo_url)
-        self.dnf('%s %s ptest-runner' % (dnf_options, ' '.join(ptest_pkgs)))
-
-    @skipIfNotFeature('package-management',
-                      'Test requires package-management to be in DISTRO_FEATURES')
-    @skipIfNotFeature('ptest',
-                      'Test requires package-management to be in DISTRO_FEATURES')
-    @skipIfNotDataVar('IMAGE_PKGTYPE', 'rpm',
-                      'RPM is not the primary package manager')
-    @OEHasPackage(['dnf'])
+    @OETestID(1600)
+    @skipIfNotFeature('ptest', 'Test requires ptest to be in DISTRO_FEATURES')
+    @skipIfNotFeature('ptest-pkgs', 'Test requires ptest-pkgs to be in IMAGE_FEATURES')
     @OETestDepends(['ssh.SSHTest.test_ssh'])
     def test_ptestrunner(self):
-        self.ptest_log = os.path.join(self.tc.td['TEST_LOG_DIR'],
-                                      'ptest-%s.log' % self.tc.td['DATETIME'])
-        self._install_ptest_packages()
-
-        (runnerstatus, result) = self.target.run('/usr/bin/ptest-runner > /tmp/ptest.log 2>&1', 0)
-        #exit code is !=0 even if ptest-runner executes because some ptest tests fail.
-        self.assertTrue(runnerstatus != 127, msg="Cannot execute ptest-runner!")
-        self.target.copyFrom('/tmp/ptest.log', self.ptest_log)
-        shutil.copyfile(self.ptest_log, "ptest.log")
-
-        result = self.parse_ptest("ptest.log")
-        log_results_to_location = "./results"
-        if os.path.exists(log_results_to_location):
-            shutil.rmtree(log_results_to_location)
-        os.makedirs(log_results_to_location)
-
-        result.log_as_files(log_results_to_location, test_status = ['pass','fail'])
+        import datetime
+
+        test_log_dir = self.td.get('TEST_LOG_DIR', '')
+        # The TEST_LOG_DIR maybe NULL when testimage is added after
+        # testdata.json is generated.
+        if not test_log_dir:
+            test_log_dir = os.path.join(self.td.get('WORKDIR', ''), 'testimage')
+        # Don't use self.td.get('DATETIME'), it's from testdata.json, not
+        # up-to-date, and may cause "File exists" when re-reun.
+        datetime = datetime.datetime.now().strftime('%Y%m%d%H%M%S')
+        ptest_log_dir_link = os.path.join(test_log_dir, 'ptest_log')
+        ptest_log_dir = '%s.%s' % (ptest_log_dir_link, datetime)
+        ptest_runner_log = os.path.join(ptest_log_dir, 'ptest-runner.log')
+
+        status, output = self.target.run('ptest-runner', 0)
+        os.makedirs(ptest_log_dir)
+        with open(ptest_runner_log, 'w') as f:
+            f.write(output)
+
+        # status != 0 is OK since some ptest tests may fail
+        self.assertTrue(status != 127, msg="Cannot execute ptest-runner!")
+
+        # Parse and save results
+        parse_result = self.parse_ptest(ptest_runner_log)
+        parse_result.log_as_files(ptest_log_dir, test_status = ['pass','fail'])
+        if os.path.exists(ptest_log_dir_link):
+            # Remove the old link to create a new one
+            os.remove(ptest_log_dir_link)
+        os.symlink(os.path.basename(ptest_log_dir), ptest_log_dir_link)
-- 
2.11.0

[PATCH 03/10] oeqa/utils/logparser.py: add skip status

[Robert Yang]

Some test cases maybe skipped, let's parse it.

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/utils/logparser.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/meta/lib/oeqa/utils/logparser.py b/meta/lib/oeqa/utils/logparser.py
index b377dcd2716..4355ce0b767 100644
--- a/meta/lib/oeqa/utils/logparser.py
+++ b/meta/lib/oeqa/utils/logparser.py
@@ -9,7 +9,7 @@ from . import ftools
 # A parser that can be used to identify weather a line is a test result or a section statement.
 class Lparser(object):
 
-    def __init__(self, test_0_pass_regex, test_0_fail_regex, section_0_begin_regex=None, section_0_end_regex=None, **kwargs):
+    def __init__(self, test_0_pass_regex, test_0_fail_regex, test_0_skip_regex, section_0_begin_regex=None, section_0_end_regex=None, **kwargs):
         # Initialize the arguments dictionary
         if kwargs:
             self.args = kwargs
@@ -19,12 +19,13 @@ class Lparser(object):
         # Add the default args to the dictionary
         self.args['test_0_pass_regex'] = test_0_pass_regex
         self.args['test_0_fail_regex'] = test_0_fail_regex
+        self.args['test_0_skip_regex'] = test_0_skip_regex
         if section_0_begin_regex:
             self.args['section_0_begin_regex'] = section_0_begin_regex
         if section_0_end_regex:
             self.args['section_0_end_regex'] = section_0_end_regex
 
-        self.test_possible_status = ['pass', 'fail', 'error']
+        self.test_possible_status = ['pass', 'fail', 'error', 'skip']
         self.section_possible_status = ['begin', 'end']
 
         self.initialized = False
-- 
2.11.0

[PATCH 04/10] runtime/cases/_ptest.py: add skip status

[Robert Yang]

The packages' test cases maybe skipped, check and save them.

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/runtime/cases/_ptest.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/meta/lib/oeqa/runtime/cases/_ptest.py b/meta/lib/oeqa/runtime/cases/_ptest.py
index 6d239494b88..ec8c038a566 100644
--- a/meta/lib/oeqa/runtime/cases/_ptest.py
+++ b/meta/lib/oeqa/runtime/cases/_ptest.py
@@ -10,6 +10,7 @@ class PtestRunnerTest(OERuntimeTestCase):
     def parse_ptest(self, logfile):
         parser = Lparser(test_0_pass_regex="^PASS:(.+)",
                          test_0_fail_regex="^FAIL:(.+)",
+                         test_0_skip_regex="^SKIP:(.+)",
                          section_0_begin_regex="^BEGIN: .*/(.+)/ptest",
                          section_0_end_regex="^END: .*/(.+)/ptest")
         parser.init()
@@ -38,6 +39,10 @@ class PtestRunnerTest(OERuntimeTestCase):
                     result.store(current_section, name, status)
                     continue
 
+                if line_type == 'test' and status == 'skip':
+                    result.store(current_section, name, status)
+                    continue
+
         result.sort_tests()
         return result
 
@@ -70,7 +75,7 @@ class PtestRunnerTest(OERuntimeTestCase):
 
         # Parse and save results
         parse_result = self.parse_ptest(ptest_runner_log)
-        parse_result.log_as_files(ptest_log_dir, test_status = ['pass','fail'])
+        parse_result.log_as_files(ptest_log_dir, test_status = ['pass','fail', 'skip'])
         if os.path.exists(ptest_log_dir_link):
             # Remove the old link to create a new one
             os.remove(ptest_log_dir_link)
-- 
2.11.0

[PATCH 05/10] runtime/cases/_ptest.py: rename it to ptest.py

[Robert Yang]

It works now.

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/runtime/cases/{_ptest.py => ptest.py} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename meta/lib/oeqa/runtime/cases/{_ptest.py => ptest.py} (100%)

diff --git a/meta/lib/oeqa/runtime/cases/_ptest.py b/meta/lib/oeqa/runtime/cases/ptest.py
similarity index 100%
rename from meta/lib/oeqa/runtime/cases/_ptest.py
rename to meta/lib/oeqa/runtime/cases/ptest.py
-- 
2.11.0

[PATCH 06/10] utils/logparser.py: fix section check

[Robert Yang]

The section might be None or '', so use "if section" for it.

Fixed:
File "/buildarea/lyang1/poky/meta/lib/oeqa/utils/logparser.py", line 113, in log_as_files
    prefix += section
TypeError: Can't convert 'NoneType' object to str implicitly

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/utils/logparser.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/lib/oeqa/utils/logparser.py b/meta/lib/oeqa/utils/logparser.py
index 4355ce0b767..0670627c3c0 100644
--- a/meta/lib/oeqa/utils/logparser.py
+++ b/meta/lib/oeqa/utils/logparser.py
@@ -109,7 +109,7 @@ class Result(object):
             prefix = ''
             for x in test_status:
                 prefix +=x+'.'
-            if (section != ''):
+            if section:
                 prefix += section
             section_file = os.path.join(target_dir, prefix)
             # purge the file contents if it exists
-- 
2.11.0

[PATCH 07/10] core/target/ssh.py: replace decode errors

[Robert Yang]

There might be wild strings when read from target (especially when
reading ptest results), replace the errors to avoid breaking the test.

Fixed: (Not always happen)
$ bitbake core-image-sato -ctestimage
[snip]
    status, output = self.target.run('ptest-runner', 0)
  File "/buildarea/lyang1/poky/meta/lib/oeqa/core/target/ssh.py", line 84, in run
    status, output = self._run(sshCmd, processTimeout, True)
  File "/buildarea/lyang1/poky/meta/lib/oeqa/core/target/ssh.py", line 55, in _run
    status, output = SSHCall(command, self.logger, timeout)
  File "/buildarea/lyang1/poky/meta/lib/oeqa/core/target/ssh.py", line 258, in SSHCall
    run()
  File "/buildarea/lyang1/poky/meta/lib/oeqa/core/target/ssh.py", line 236, in run
    output = process.communicate()[0].decode("utf-8")
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 4906: invalid continuation byte

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/lib/oeqa/core/target/ssh.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/lib/oeqa/core/target/ssh.py b/meta/lib/oeqa/core/target/ssh.py
index b80939c0e51..a2eafcd6f2a 100644
--- a/meta/lib/oeqa/core/target/ssh.py
+++ b/meta/lib/oeqa/core/target/ssh.py
@@ -211,7 +211,7 @@ def SSHCall(command, logger, timeout=None, **opts):
                             process.stdout.close()
                             eof = True
                         else:
-                            data = data.decode("utf-8")
+                            data = data.decode("utf-8", errors='replace')
                             output += data
                             logger.debug('Partial data from SSH call: %s' % data)
                             endtime = time.time() + timeout
@@ -233,7 +233,7 @@ def SSHCall(command, logger, timeout=None, **opts):
                 output += lastline
 
         else:
-            output = process.communicate()[0].decode("utf-8")
+            output = process.communicate()[0].decode("utf-8", errors='replace')
             logger.debug('Data from SSH call: %s' % output.rstrip())
 
     options = {
-- 
2.11.0

[PATCH 08/10] buildhistory.bbclass: print message when no commit

[Robert Yang]

This makes the user easier to know how to make commit in buildhistory.

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/classes/buildhistory.bbclass | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/buildhistory.bbclass
index 81784eef216..a3e4c7a734a 100644
--- a/meta/classes/buildhistory.bbclass
+++ b/meta/classes/buildhistory.bbclass
@@ -824,6 +824,8 @@ python buildhistory_eventhandler() {
                 interrupted = getattr(e, '_interrupted', 0)
                 localdata.setVar('BUILDHISTORY_BUILD_INTERRUPTED', str(interrupted))
                 bb.build.exec_func("buildhistory_commit", localdata)
+            else:
+                bb.note("No commit since BUILDHISTORY_COMMIT != '1'")
 }
 
 addhandler buildhistory_eventhandler
-- 
2.11.0

[PATCH 09/10] buildhistory.bbclass: add ptest_log

[Robert Yang]

The ptest log will be saved to buildhistory/ptest_log, we can easily get
the regression result between builds by:
$ git show HEAD ptest_log/pass.fail.skip.*

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/classes/buildhistory.bbclass | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/buildhistory.bbclass
index a3e4c7a734a..26430a63d31 100644
--- a/meta/classes/buildhistory.bbclass
+++ b/meta/classes/buildhistory.bbclass
@@ -912,3 +912,31 @@ def write_latest_srcrev(d, pkghistdir):
     else:
         if os.path.exists(srcrevfile):
             os.remove(srcrevfile)
+
+do_testimage[postfuncs] += "write_ptest_result"
+do_testimage[vardepsexclude] += "write_ptest_result"
+
+python write_ptest_result() {
+    write_latest_ptest_result(d, d.getVar('BUILDHISTORY_DIR'))
+}
+
+def write_latest_ptest_result(d, histdir):
+    import glob
+    import subprocess
+    test_log_dir = d.getVar('TEST_LOG_DIR')
+    input_ptest_log = os.path.join(test_log_dir, 'ptest_log')
+    output_ptest_log = os.path.join(histdir, 'ptest_log')
+    if os.path.exists(input_ptest_log):
+        # Lock it avoid race issue
+        lock = bb.utils.lockfile(output_ptest_log + "/ptest_log.lock")
+        bb.utils.mkdirhier(output_ptest_log)
+        oe.path.copytree(input_ptest_log, output_ptest_log)
+        # Sort test result
+        for result in glob.glob('%s/pass.fail.*' % output_ptest_log):
+            bb.debug(1, 'Processing %s' % result)
+            cmd = "sort %s -o %s" % (result, result)
+            bb.debug(1, 'Running %s' % cmd)
+            ret = subprocess.call(cmd, shell=True)
+            if ret != 0:
+                bb.error('Failed to run %s!' % cmd)
+        bb.utils.unlockfile(lock)
-- 
2.11.0

[PATCH 10/10] testimage.bbclass: update comments

[Robert Yang]

It's very important to add IMAGE_CLASSES += "testimage" in local.conf firstly,
otherwise the var like TEST_LOG_DIR (defined in testimage.bbclass) will not be
in testdata.json.

[YOCTO #11547]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/classes/testimage.bbclass | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/classes/testimage.bbclass b/meta/classes/testimage.bbclass
index 0c4a84e1119..6a435604222 100644
--- a/meta/classes/testimage.bbclass
+++ b/meta/classes/testimage.bbclass
@@ -7,8 +7,8 @@
 # Most of the tests are commands run on target image over ssh.
 # To use it add testimage to global inherit and call your target image with -c testimage
 # You can try it out like this:
-# - first build a qemu core-image-sato
-# - add IMAGE_CLASSES += "testimage" in local.conf
+# - first add IMAGE_CLASSES += "testimage" in local.conf
+# - build a qemu core-image-sato
 # - then bitbake core-image-sato -c testimage. That will run a standard suite of tests.
 
 # You can set (or append to) TEST_SUITES in local.conf to select the tests
-- 
2.11.0

Re: [PATCH 09/10] buildhistory.bbclass: add ptest_log

[Paul Eggleton]

Hi Robert,

A few minor comments below.

On Monday, 31 July 2017 11:50:09 AM CEST Robert Yang wrote:
> The ptest log will be saved to buildhistory/ptest_log, we can easily get
> the regression result between builds by:
> $ git show HEAD ptest_log/pass.fail.skip.*
> 
> [YOCTO #11547]
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  meta/classes/buildhistory.bbclass | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/
buildhistory.bbclass
> index a3e4c7a734a..26430a63d31 100644
> --- a/meta/classes/buildhistory.bbclass
> +++ b/meta/classes/buildhistory.bbclass
> @@ -912,3 +912,31 @@ def write_latest_srcrev(d, pkghistdir):
>      else:
>          if os.path.exists(srcrevfile):
>              os.remove(srcrevfile)
> +
> +do_testimage[postfuncs] += "write_ptest_result"
> +do_testimage[vardepsexclude] += "write_ptest_result"
> +
> +python write_ptest_result() {
> +    write_latest_ptest_result(d, d.getVar('BUILDHISTORY_DIR'))
> +}
> +
> +def write_latest_ptest_result(d, histdir):
> +    import glob
> +    import subprocess
> +    test_log_dir = d.getVar('TEST_LOG_DIR')
> +    input_ptest_log = os.path.join(test_log_dir, 'ptest_log')
> +    output_ptest_log = os.path.join(histdir, 'ptest_log')

Would it be reasonable for this to just be "ptest"? To ask the question a 
different way, would you expect to save other types of ptest information into 
buildhistory, or is this likely to be it?

> +    if os.path.exists(input_ptest_log):
> +        # Lock it avoid race issue
> +        lock = bb.utils.lockfile(output_ptest_log + "/ptest_log.lock")
> +        bb.utils.mkdirhier(output_ptest_log)
> +        oe.path.copytree(input_ptest_log, output_ptest_log)
> +        # Sort test result
> +        for result in glob.glob('%s/pass.fail.*' % output_ptest_log):
> +            bb.debug(1, 'Processing %s' % result)
> +            cmd = "sort %s -o %s" % (result, result)
> +            bb.debug(1, 'Running %s' % cmd)
> +            ret = subprocess.call(cmd, shell=True)

As a matter of good practice and since it's easy to do here I'd suggest not 
using shell=True and passing the command as a list instead, that way there 
can't be any issues with spaces or other shell interpretations.

> +            if ret != 0:
> +                bb.error('Failed to run %s!' % cmd)
> +        bb.utils.unlockfile(lock)

Shouldn't you be using try...finally here to ensure the lockfile gets unlocked 
in the case of exceptions?

Cheers,
Paul


-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 02/10] runtime/cases/_ptest.py: revive it

[Paul Eggleton]

On Monday, 31 July 2017 11:50:02 AM CEST Robert Yang wrote:
> * Will write a tool to make regression check

This commit doesn't do this and it's not completely covered by the rest of the 
series. I think you could say perhaps more appropriately "This provides data 
that could be used to detect regressions in ptest results" or similar assuming 
you need to mention it at all.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 00/10] Revive ptest and add result to buildhistory

[Paul Eggleton]

On Monday, 31 July 2017 11:50:00 AM CEST Robert Yang wrote:
> We can get the regression result by:
> $ git show HEAD ptest_log/pass.fail.skip.*
> 
> Can we add ptest to default TEST_SUITES, please ?

Is it possible to get an idea of the time impact if we did so?

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 09/10] buildhistory.bbclass: add ptest_log

[Robert Yang]

On 07/31/2017 09:30 PM, Paul Eggleton wrote:
> Hi Robert,
>
> A few minor comments below.
>
> On Monday, 31 July 2017 11:50:09 AM CEST Robert Yang wrote:
>> The ptest log will be saved to buildhistory/ptest_log, we can easily get
>> the regression result between builds by:
>> $ git show HEAD ptest_log/pass.fail.skip.*
>>
>> [YOCTO #11547]
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> ---
>>  meta/classes/buildhistory.bbclass | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/
> buildhistory.bbclass
>> index a3e4c7a734a..26430a63d31 100644
>> --- a/meta/classes/buildhistory.bbclass
>> +++ b/meta/classes/buildhistory.bbclass
>> @@ -912,3 +912,31 @@ def write_latest_srcrev(d, pkghistdir):
>>      else:
>>          if os.path.exists(srcrevfile):
>>              os.remove(srcrevfile)
>> +
>> +do_testimage[postfuncs] += "write_ptest_result"
>> +do_testimage[vardepsexclude] += "write_ptest_result"
>> +
>> +python write_ptest_result() {
>> +    write_latest_ptest_result(d, d.getVar('BUILDHISTORY_DIR'))
>> +}
>> +
>> +def write_latest_ptest_result(d, histdir):
>> +    import glob
>> +    import subprocess
>> +    test_log_dir = d.getVar('TEST_LOG_DIR')
>> +    input_ptest_log = os.path.join(test_log_dir, 'ptest_log')
>> +    output_ptest_log = os.path.join(histdir, 'ptest_log')
>
> Would it be reasonable for this to just be "ptest"? To ask the question a
> different way, would you expect to save other types of ptest information into
> buildhistory, or is this likely to be it?

I'm not sure atm, so use "ptest" sounds reasonable. I will update it.

>
>> +    if os.path.exists(input_ptest_log):
>> +        # Lock it avoid race issue
>> +        lock = bb.utils.lockfile(output_ptest_log + "/ptest_log.lock")
>> +        bb.utils.mkdirhier(output_ptest_log)
>> +        oe.path.copytree(input_ptest_log, output_ptest_log)
>> +        # Sort test result
>> +        for result in glob.glob('%s/pass.fail.*' % output_ptest_log):
>> +            bb.debug(1, 'Processing %s' % result)
>> +            cmd = "sort %s -o %s" % (result, result)
>> +            bb.debug(1, 'Running %s' % cmd)
>> +            ret = subprocess.call(cmd, shell=True)
>
> As a matter of good practice and since it's easy to do here I'd suggest not
> using shell=True and passing the command as a list instead, that way there
> can't be any issues with spaces or other shell interpretations.

Thanks, I will update it.

>
>> +            if ret != 0:
>> +                bb.error('Failed to run %s!' % cmd)
>> +        bb.utils.unlockfile(lock)
>
> Shouldn't you be using try...finally here to ensure the lockfile gets unlocked
> in the case of exceptions?

Yes, good idea.

// Robert

>
> Cheers,
> Paul
>
>

Re: [PATCH 02/10] runtime/cases/_ptest.py: revive it

[Robert Yang]

On 07/31/2017 09:38 PM, Paul Eggleton wrote:
> On Monday, 31 July 2017 11:50:02 AM CEST Robert Yang wrote:
>> * Will write a tool to make regression check
>
> This commit doesn't do this and it's not completely covered by the rest of the
> series. I think you could say perhaps more appropriately "This provides data
> that could be used to detect regressions in ptest results" or similar assuming
> you need to mention it at all.

Sorry, I will update it, it was written when I didn't plan to use buildhistory,
now I save the data into buildhistory, so the tool is not needed any more.

// Robert

>
> Cheers,
> Paul
>

Re: [PATCH 00/10] Revive ptest and add result to buildhistory

[Robert Yang]

On 07/31/2017 09:41 PM, Paul Eggleton wrote:
> On Monday, 31 July 2017 11:50:00 AM CEST Robert Yang wrote:
>> We can get the regression result by:
>> $ git show HEAD ptest_log/pass.fail.skip.*
>>
>> Can we add ptest to default TEST_SUITES, please ?
>
> Is it possible to get an idea of the time impact if we did so?

The ptest cost about 18 minutes (qemux86 +QEMU_USE_KVM) when test
core-image-sato. My host is 32 core * 2.6GHz, and 128G MEM. I didn't
customize qemu settings, just used the default.


I fixed all your comments and put them into the repo:

   git://git.openembedded.org/openembedded-core-contrib rbt/ptest
   http://cgit.openembedded.org/openembedded-core-contrib/log/?h=rbt/ptest

Robert Yang (10):
   oeqa/targetcontrol.py: simplify checking for qemu_use_kvm
   runtime/cases/_ptest.py: revive it
   oeqa/utils/logparser.py: add skip status
   runtime/cases/_ptest.py: add skip status
   runtime/cases/_ptest.py: rename it to ptest.py
   utils/logparser.py: fix section check
   core/target/ssh.py: replace decode errors
   buildhistory.bbclass: print message when no commit
   buildhistory.bbclass: add ptest
   testimage.bbclass: update comments

>
> Cheers,
> Paul
>

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Richard Purdie]

On Mon, 2017-07-31 at 02:50 -0700, Robert Yang wrote:
> The "if qemu_use_kvm" is not needed.
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  meta/lib/oeqa/targetcontrol.py | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/lib/oeqa/targetcontrol.py
> b/meta/lib/oeqa/targetcontrol.py
> index 3255e3a5c63..11e6c820e85 100644
> --- a/meta/lib/oeqa/targetcontrol.py
> +++ b/meta/lib/oeqa/targetcontrol.py
> @@ -132,9 +132,8 @@ class QemuTarget(BaseTarget):
>          dump_host_cmds = d.getVar("testimage_dump_host")
>          dump_dir = d.getVar("TESTIMAGE_DUMP_DIR")
>          qemu_use_kvm = d.getVar("QEMU_USE_KVM")
> -        if qemu_use_kvm and \
> -           (qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
> -            d.getVar("MACHINE") in qemu_use_kvm.split()):
> +        if qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
> +            d.getVar("MACHINE") in qemu_use_kvm.split():
>              use_kvm = True
>          else:
>              use_kvm = False
> 

It is needed since qemu_use_kvm could be None and None.split() will
cause an error.

Cheers,

Richard

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Robert Yang]

On 08/08/2017 11:30 PM, Richard Purdie wrote:
> On Mon, 2017-07-31 at 02:50 -0700, Robert Yang wrote:
>> The "if qemu_use_kvm" is not needed.
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> ---
>>  meta/lib/oeqa/targetcontrol.py | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/meta/lib/oeqa/targetcontrol.py
>> b/meta/lib/oeqa/targetcontrol.py
>> index 3255e3a5c63..11e6c820e85 100644
>> --- a/meta/lib/oeqa/targetcontrol.py
>> +++ b/meta/lib/oeqa/targetcontrol.py
>> @@ -132,9 +132,8 @@ class QemuTarget(BaseTarget):
>>          dump_host_cmds = d.getVar("testimage_dump_host")
>>          dump_dir = d.getVar("TESTIMAGE_DUMP_DIR")
>>          qemu_use_kvm = d.getVar("QEMU_USE_KVM")
>> -        if qemu_use_kvm and \
>> -           (qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>> -            d.getVar("MACHINE") in qemu_use_kvm.split()):
>> +        if qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>> +            d.getVar("MACHINE") in qemu_use_kvm.split():
>>              use_kvm = True
>>          else:
>>              use_kvm = False
>>
>
> It is needed since qemu_use_kvm could be None and None.split() will
> cause an error.

Thanks, I removed from the repo:

   git://git.openembedded.org/openembedded-core-contrib rbt/ptest
   http://cgit.openembedded.org/openembedded-core-contrib/log/?h=rbt/ptest

// Robert

>
> Cheers,
>
> Richard
>

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Robert Yang]

On 08/08/2017 11:30 PM, Richard Purdie wrote:
> On Mon, 2017-07-31 at 02:50 -0700, Robert Yang wrote:
>> The "if qemu_use_kvm" is not needed.
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> ---
>>  meta/lib/oeqa/targetcontrol.py | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/meta/lib/oeqa/targetcontrol.py
>> b/meta/lib/oeqa/targetcontrol.py
>> index 3255e3a5c63..11e6c820e85 100644
>> --- a/meta/lib/oeqa/targetcontrol.py
>> +++ b/meta/lib/oeqa/targetcontrol.py
>> @@ -132,9 +132,8 @@ class QemuTarget(BaseTarget):
>>          dump_host_cmds = d.getVar("testimage_dump_host")
>>          dump_dir = d.getVar("TESTIMAGE_DUMP_DIR")
>>          qemu_use_kvm = d.getVar("QEMU_USE_KVM")
>> -        if qemu_use_kvm and \
>> -           (qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>> -            d.getVar("MACHINE") in qemu_use_kvm.split()):
>> +        if qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>> +            d.getVar("MACHINE") in qemu_use_kvm.split():
>>              use_kvm = True
>>          else:
>>              use_kvm = False
>>
>
> It is needed since qemu_use_kvm could be None and None.split() will
> cause an error.

Thanks, I removed it from the repo:

   git://git.openembedded.org/openembedded-core-contrib rbt/ptest
   http://cgit.openembedded.org/openembedded-core-contrib/log/?h=rbt/ptest

// Robert

>
> Cheers,
>
> Richard
>

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Robert Yang]

Ping.

// Robert

On 08/09/2017 05:46 PM, Robert Yang wrote:
>
>
> On 08/08/2017 11:30 PM, Richard Purdie wrote:
>> On Mon, 2017-07-31 at 02:50 -0700, Robert Yang wrote:
>>> The "if qemu_use_kvm" is not needed.
>>>
>>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>>> ---
>>>  meta/lib/oeqa/targetcontrol.py | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/meta/lib/oeqa/targetcontrol.py
>>> b/meta/lib/oeqa/targetcontrol.py
>>> index 3255e3a5c63..11e6c820e85 100644
>>> --- a/meta/lib/oeqa/targetcontrol.py
>>> +++ b/meta/lib/oeqa/targetcontrol.py
>>> @@ -132,9 +132,8 @@ class QemuTarget(BaseTarget):
>>>          dump_host_cmds = d.getVar("testimage_dump_host")
>>>          dump_dir = d.getVar("TESTIMAGE_DUMP_DIR")
>>>          qemu_use_kvm = d.getVar("QEMU_USE_KVM")
>>> -        if qemu_use_kvm and \
>>> -           (qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>>> -            d.getVar("MACHINE") in qemu_use_kvm.split()):
>>> +        if qemu_use_kvm == "True" and "x86" in d.getVar("MACHINE") or \
>>> +            d.getVar("MACHINE") in qemu_use_kvm.split():
>>>              use_kvm = True
>>>          else:
>>>              use_kvm = False
>>>
>>
>> It is needed since qemu_use_kvm could be None and None.split() will
>> cause an error.
>
> Thanks, I removed from the repo:
>
>   git://git.openembedded.org/openembedded-core-contrib rbt/ptest
>   http://cgit.openembedded.org/openembedded-core-contrib/log/?h=rbt/ptest
>
> // Robert
>
>>
>> Cheers,
>>
>> Richard
>>

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Richard Purdie]

On Mon, 2017-08-21 at 16:02 +0800, Robert Yang wrote:
> Ping.

Can you repost this series please? I suspect you've changed the patches
as per the feedback from Paul but I'd like to see another round of
review before we consider this.

Just repushing is fine for minor changes but more significant things
should be resubmitted.

Cheers,

Richard

Re: [PATCH 01/10] oeqa/targetcontrol.py: simplify checking for qemu_use_kvm

[Robert Yang]

On 08/22/2017 04:35 AM, Richard Purdie wrote:
> On Mon, 2017-08-21 at 16:02 +0800, Robert Yang wrote:
>> Ping.
>
> Can you repost this series please? I suspect you've changed the patches

Thanks, I will repost them. And Yes, I had fixed Paul's comments.

// Robert

> as per the feedback from Paul but I'd like to see another round of
> review before we consider this.
>
> Just repushing is fine for minor changes but more significant things
> should be resubmitted.
>
> Cheers,
>
> Richard
>