[PATCH 0/3] Make CI targets be configured

[PATCH 0/3] Make CI targets be configured

[Anton Mikanovich]

This patchset removes hardcoded KFAIL protection from targets and makes
any target be KFAILed or SKIPed based on execution environment.
ISAR_CI_KFAIL and ISAR_CI_SKIP variables are used to store those lists.

Anton Mikanovich (3):
  ci: Implement dynamic KFAIL checking
  ci: Implement dynamic tests skipping
  ci: Correct container test case name

 testsuite/cibuilder.py | 56 ++++++++++++++++++++++++++++--------------
 testsuite/citest.py    | 32 ++++++------------------
 2 files changed, 45 insertions(+), 43 deletions(-)

-- 
2.17.1

[PATCH 1/3] ci: Implement dynamic KFAIL checking

[Anton Mikanovich]

Remove KFAIL protection hardcoding and inctroduce ISAR_CI_KFAIL
environment variable instead.

Example:
ISAR_CI_KFAIL="cross_rpi cross_ubuntu nocross_rpi nocross_sidports"

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 testsuite/cibuilder.py | 54 ++++++++++++++++++++++++++++--------------
 testsuite/citest.py    | 30 +++++------------------
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py
index 7ce13dc..98b8933 100755
--- a/testsuite/cibuilder.py
+++ b/testsuite/cibuilder.py
@@ -10,6 +10,7 @@ import tempfile
 
 import start_vm
 
+from avocado.core import exceptions
 from avocado import Test
 from avocado.utils import path
 from avocado.utils import process
@@ -124,6 +125,15 @@ class CIBuilder(Test):
         self.check_init()
         process.run('rm -rf ' + self.build_dir + '/' + path, sudo=True)
 
+    def is_in_list(self, envname):
+        test_prefix = 'test_'
+        caller = self._testMethodName
+        tests_list = (os.getenv(envname) or '').split()
+        if caller.startswith(test_prefix):
+            if caller[len(test_prefix):] in tests_list:
+                return True
+        return False
+
     def bitbake(self, target, bitbake_cmd=None, **kwargs):
         self.check_init()
         self.log.info('===================================================')
@@ -141,24 +151,30 @@ class CIBuilder(Test):
         else:
             cmdline.append(target)
 
-        with subprocess.Popen(" ".join(cmdline), stdout=subprocess.PIPE,
-                              stderr=subprocess.PIPE, universal_newlines=True,
-                              shell=True) as p1:
-            poller = select.poll()
-            poller.register(p1.stdout, select.POLLIN)
-            poller.register(p1.stderr, select.POLLIN)
-            while p1.poll() is None:
-                events = poller.poll(1000)
-                for fd, event in events:
-                    if event != select.POLLIN:
-                        continue
-                    if fd == p1.stdout.fileno():
-                        self.log.info(p1.stdout.readline().rstrip())
-                    if fd == p1.stderr.fileno():
-                        app_log.error(p1.stderr.readline().rstrip())
-            p1.wait()
-            if p1.returncode:
-                self.fail('Bitbake failed')
+        try:
+            with subprocess.Popen(" ".join(cmdline), stdout=subprocess.PIPE,
+                                  stderr=subprocess.PIPE, universal_newlines=True,
+                                  shell=True) as p1:
+                poller = select.poll()
+                poller.register(p1.stdout, select.POLLIN)
+                poller.register(p1.stderr, select.POLLIN)
+                while p1.poll() is None:
+                    events = poller.poll(1000)
+                    for fd, event in events:
+                        if event != select.POLLIN:
+                            continue
+                        if fd == p1.stdout.fileno():
+                            self.log.info(p1.stdout.readline().rstrip())
+                        if fd == p1.stderr.fileno():
+                            app_log.error(p1.stderr.readline().rstrip())
+                p1.wait()
+                if p1.returncode:
+                    self.fail('Bitbake failed')
+        except exceptions.TestFail:
+            if self.is_in_list('ISAR_CI_KFAIL'):
+                self.cancel('KFAIL')
+            else:
+                raise
 
     def backupfile(self, path):
         self.check_init()
@@ -258,4 +274,6 @@ class CIBuilder(Test):
                 else:
                     app_log.error(data.decode(errors='replace'))
 
+        if self.is_in_list('ISAR_CI_KFAIL'):
+            self.cancel('KFAIL: log ' + output_file)
         self.fail('Log ' + output_file)
diff --git a/testsuite/citest.py b/testsuite/citest.py
index 994c130..879c8d2 100755
--- a/testsuite/citest.py
+++ b/testsuite/citest.py
@@ -79,10 +79,7 @@ class CrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=True, debsrc_cache=True)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=True, debsrc_cache=True)
 
     def test_cross_ubuntu(self):
         targets = [
@@ -90,10 +87,7 @@ class CrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=True)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=True)
 
     def test_cross_bookworm(self):
         targets = [
@@ -101,10 +95,7 @@ class CrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=True)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=True)
 
 class SdkTest(CIBaseTest):
 
@@ -165,10 +156,7 @@ class NoCrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=False, debsrc_cache=True)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=False, debsrc_cache=True)
 
     def test_nocross_bookworm(self):
         targets = [
@@ -180,10 +168,7 @@ class NoCrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=False)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=False)
 
     def test_nocross_sidports(self):
         targets = [
@@ -192,10 +177,7 @@ class NoCrossTest(CIBaseTest):
                   ]
 
         self.init()
-        try:
-            self.perform_build_test(targets, cross=False)
-        except:
-            self.cancel('KFAIL')
+        self.perform_build_test(targets, cross=False)
 
 class RebuildTest(CIBaseTest):
 
-- 
2.17.1

[PATCH 2/3] ci: Implement dynamic tests skipping

[Anton Mikanovich]

Instroduce ISAR_CI_SKIP environment variable to skip any single test.

Example:
ISAR_CI_SKIP="repro_unsigned nocross_rpi"

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 testsuite/cibuilder.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py
index 98b8933..59d25f3 100755
--- a/testsuite/cibuilder.py
+++ b/testsuite/cibuilder.py
@@ -11,6 +11,7 @@ import tempfile
 import start_vm
 
 from avocado.core import exceptions
+from avocado import skipIf
 from avocado import Test
 from avocado.utils import path
 from avocado.utils import process
@@ -24,6 +25,7 @@ class CanBeFinished(Exception):
     pass
 
 class CIBuilder(Test):
+    @skipIf(lambda x: x.is_in_list('ISAR_CI_SKIP'))
     def setUp(self):
         super(CIBuilder, self).setUp()
         job_log = os.path.join(os.path.dirname(self.logdir), '..', 'job.log')
-- 
2.17.1

[PATCH 3/3] ci: Correct container test case name

[Anton Mikanovich]

Make all test case names unique to be controlled.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 testsuite/citest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/citest.py b/testsuite/citest.py
index 879c8d2..d962597 100755
--- a/testsuite/citest.py
+++ b/testsuite/citest.py
@@ -209,7 +209,7 @@ class ContainerImageTest(CIBaseTest):
     :avocado: tags=containerbuild,fast,full,container
     """
     @skipUnless(UMOCI_AVAILABLE and SKOPEO_AVAILABLE, 'umoci/skopeo not found')
-    def test_nocross(self):
+    def test_container_nocross(self):
         targets = [
             'mc:container-amd64-stretch:isar-image-base',
             'mc:container-amd64-buster:isar-image-base',
-- 
2.17.1

Re: [PATCH 0/3] Make CI targets be configured

[Henning Schild]

The test suite already has too many knobs, now including env variables
would be the number one cause of "works for me" ... does not work in
CI-A and even different in CI-B.

I think if i pushed this series to our CI it would simply fail, because
i would not have any of the variables set there ... and no clue how you
did set them on your jenkins.

Tests that are known to not work should probably be skipped in general,
not kfailed because that would just waste time.

If we ever see certain tests not working in certain setups we can see
how we make skipping configurable, but it is imho a very bad idea to
introduce that without a need.

So i would say let us get rid of some of the KFAILs and turn the still
valid ones into SKIPs ... and see about the rest laster.

Henning

Am Wed, 27 Apr 2022 15:32:02 +0300
schrieb Anton Mikanovich <amikan@ilbers.de>:

> This patchset removes hardcoded KFAIL protection from targets and
> makes any target be KFAILed or SKIPed based on execution environment.
> ISAR_CI_KFAIL and ISAR_CI_SKIP variables are used to store those
> lists.
> 
> Anton Mikanovich (3):
>   ci: Implement dynamic KFAIL checking
>   ci: Implement dynamic tests skipping
>   ci: Correct container test case name
> 
>  testsuite/cibuilder.py | 56
> ++++++++++++++++++++++++++++-------------- testsuite/citest.py    |
> 32 ++++++------------------ 2 files changed, 45 insertions(+), 43
> deletions(-)
> 

Re: [PATCH 0/3] Make CI targets be configured

[Anton Mikanovich]

27.04.2022 18:59, Henning Schild wrote:
> The test suite already has too many knobs, now including env variables
> would be the number one cause of "works for me" ... does not work in
> CI-A and even different in CI-B.
>
> I think if i pushed this series to our CI it would simply fail, because
> i would not have any of the variables set there ... and no clue how you
> did set them on your jenkins.
>
> Tests that are known to not work should probably be skipped in general,
> not kfailed because that would just waste time.
>
> If we ever see certain tests not working in certain setups we can see
> how we make skipping configurable, but it is imho a very bad idea to
> introduce that without a need.
>
> So i would say let us get rid of some of the KFAILs and turn the still
> valid ones into SKIPs ... and see about the rest laster.
>
> Henning
>
> Am Wed, 27 Apr 2022 15:32:02 +0300
> schrieb Anton Mikanovich <amikan@ilbers.de>:
>
>> This patchset removes hardcoded KFAIL protection from targets and
>> makes any target be KFAILed or SKIPed based on execution environment.
>> ISAR_CI_KFAIL and ISAR_CI_SKIP variables are used to store those
>> lists.
>>
>> Anton Mikanovich (3):
>>    ci: Implement dynamic KFAIL checking
>>    ci: Implement dynamic tests skipping
>>    ci: Correct container test case name
>>
>>   testsuite/cibuilder.py | 56
>> ++++++++++++++++++++++++++++-------------- testsuite/citest.py    |
>> 32 ++++++------------------ 2 files changed, 45 insertions(+), 43
>> deletions(-)

Using env variables is the only suitable way for Gitlab, that's why it 
was done
that way. Env is quite hard to use for Jenkins CI, so any better 
solutions are
welcome.

Applying those patches without setting any env variables will just make 
CI to
run without any KFAILs, so it's not a problem.

We need to have KFAIL mostly for upstream related issues as temporary 
hotfix,
so we can't maintain this via commits. Removing KFAILs will make us to 
freeze
all merging until any single upstream issue on unstable target will be 
fixed.
Otherwise if we keep static KFAILs we will always have 'hidden' issues just
like we already had with python update on bookworm.

SKIPs are implemented for testing purposes to make CI run faster then all
targets check during single user-specific testing cases.

Re: [PATCH 0/3] Make CI targets be configured

[Henning Schild]

Am Wed, 27 Apr 2022 20:50:32 +0300
schrieb Anton Mikanovich <amikan@ilbers.de>:

> 27.04.2022 18:59, Henning Schild wrote:
> > The test suite already has too many knobs, now including env
> > variables would be the number one cause of "works for me" ... does
> > not work in CI-A and even different in CI-B.
> >
> > I think if i pushed this series to our CI it would simply fail,
> > because i would not have any of the variables set there ... and no
> > clue how you did set them on your jenkins.
> >
> > Tests that are known to not work should probably be skipped in
> > general, not kfailed because that would just waste time.
> >
> > If we ever see certain tests not working in certain setups we can
> > see how we make skipping configurable, but it is imho a very bad
> > idea to introduce that without a need.
> >
> > So i would say let us get rid of some of the KFAILs and turn the
> > still valid ones into SKIPs ... and see about the rest laster.
> >
> > Henning
> >
> > Am Wed, 27 Apr 2022 15:32:02 +0300
> > schrieb Anton Mikanovich <amikan@ilbers.de>:
> >  
> >> This patchset removes hardcoded KFAIL protection from targets and
> >> makes any target be KFAILed or SKIPed based on execution
> >> environment. ISAR_CI_KFAIL and ISAR_CI_SKIP variables are used to
> >> store those lists.
> >>
> >> Anton Mikanovich (3):
> >>    ci: Implement dynamic KFAIL checking
> >>    ci: Implement dynamic tests skipping
> >>    ci: Correct container test case name
> >>
> >>   testsuite/cibuilder.py | 56
> >> ++++++++++++++++++++++++++++-------------- testsuite/citest.py    |
> >> 32 ++++++------------------ 2 files changed, 45 insertions(+), 43
> >> deletions(-)  
> 
> Using env variables is the only suitable way for Gitlab, that's why
> it was done
> that way. Env is quite hard to use for Jenkins CI, so any better 
> solutions are
> welcome.

Simply not making that configurable, use code as we used to.

> Applying those patches without setting any env variables will just
> make CI to
> run without any KFAILs, so it's not a problem.

The pipeline will turn red on known issues. And always red would render
the pipeline less meaningful, the overall result would even be
meaningless.

> We need to have KFAIL mostly for upstream related issues as temporary 
> hotfix,
> so we can't maintain this via commits. Removing KFAILs will make us

Yes i suggest to use code, that will magically spread to all local and
CI builds. And all the known upstream issues will be dealt with,
without everybody really knowing why.

> to freeze
> all merging until any single upstream issue on unstable target will
> be fixed.

I guess the merge policy is up to the maintainer. In certain cases it
might sure be fine to merge a red pipeline.

regards,
Henning

> Otherwise if we keep static KFAILs we will always have 'hidden'
> issues just like we already had with python update on bookworm.
> 
> SKIPs are implemented for testing purposes to make CI run faster then
> all targets check during single user-specific testing cases.
>